drm/i915/gvt: use the correct length of child_device_config

Submitted by Weinan Li on Aug. 29, 2018, 4:17 a.m.

Details

Message ID 1535516276-28364-1-git-send-email-weinan.z.li@intel.com
State New
Headers show
Series "drm/i915/gvt: use the correct length of child_device_config" ( rev: 1 ) in Intel GVT devel

Not browsing as part of any series.

Commit Message

Weinan Li Aug. 29, 2018, 4:17 a.m.
GVT-g emualte the opregion for guest with bdb version as '186' which
child_device_config length should be '33'.

CC: Xiaolin Zhang <xiaolin.zhang@intel.com>
Signed-off-by: Weinan Li <weinan.z.li@intel.com>
---
 drivers/gpu/drm/i915/gvt/opregion.c | 142 ++++++++++++++++++++++--------------
 1 file changed, 88 insertions(+), 54 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
index 82586c8..5895869 100644
--- a/drivers/gpu/drm/i915/gvt/opregion.c
+++ b/drivers/gpu/drm/i915/gvt/opregion.c
@@ -42,8 +42,6 @@ 
 #define DEVICE_TYPE_EFP3   0x20
 #define DEVICE_TYPE_EFP4   0x10
 
-#define DEV_SIZE	38
-
 struct opregion_header {
 	u8 signature[16];
 	u32 size;
@@ -63,58 +61,92 @@  struct bdb_data_header {
 	u16 size; /* data size */
 } __packed;
 
+/* For supporting windows guest with opregion, here hardcode the emulated
+ * bdb header version as '186', and the corresponding child_device_config
+ * length should be '33' but not '38'. Define one new struct named as
+ * efp_child_device_config which is one copy of child_device_config but
+ * without fields which added from version '195'.
+ */
 struct efp_child_device_config {
 	u16 handle;
 	u16 device_type;
-	u16 device_class;
-	u8 i2c_speed;
-	u8 dp_onboard_redriver; /* 158 */
-	u8 dp_ondock_redriver; /* 158 */
-	u8 hdmi_level_shifter_value:4; /* 169 */
-	u8 hdmi_max_data_rate:4; /* 204 */
-	u16 dtd_buf_ptr; /* 161 */
-	u8 edidless_efp:1; /* 161 */
-	u8 compression_enable:1; /* 198 */
-	u8 compression_method:1; /* 198 */
-	u8 ganged_edp:1; /* 202 */
-	u8 skip0:4;
-	u8 compression_structure_index:4; /* 198 */
-	u8 skip1:4;
-	u8 slave_port; /*  202 */
-	u8 skip2;
+
+	union {
+		u8  device_id[10]; /* ascii string */
+		struct {
+			u8 i2c_speed;
+			u8 dp_onboard_redriver;			/* 158 */
+			u8 dp_ondock_redriver;			/* 158 */
+			u8 hdmi_level_shifter_value:4;		/* 169 */
+			u8 hdmi_max_data_rate:4;		/* 204 */
+			u16 dtd_buf_ptr;			/* 161 */
+			u8 edidless_efp:1;			/* 161 */
+			u8 compression_enable:1;		/* 198 */
+			u8 compression_method:1;		/* 198 */
+			u8 ganged_edp:1;			/* 202 */
+			u8 reserved0:4;
+			u8 compression_structure_index:4;	/* 198 */
+			u8 reserved1:4;
+			u8 slave_port;				/* 202 */
+			u8 reserved2;
+		} __packed;
+	} __packed;
+
+	u16 addin_offset;
 	u8 dvo_port;
-	u8 i2c_pin; /* for add-in card */
-	u8 slave_addr; /* for add-in card */
+	u8 i2c_pin;
+	u8 slave_addr;
 	u8 ddc_pin;
 	u16 edid_ptr;
-	u8 dvo_config;
-	u8 efp_docked_port:1; /* 158 */
-	u8 lane_reversal:1; /* 184 */
-	u8 onboard_lspcon:1; /* 192 */
-	u8 iboost_enable:1; /* 196 */
-	u8 hpd_invert:1; /* BXT 196 */
-	u8 slip3:3;
-	u8 hdmi_compat:1;
-	u8 dp_compat:1;
-	u8 tmds_compat:1;
-	u8 skip4:5;
-	u8 aux_channel;
-	u8 dongle_detect;
-	u8 pipe_cap:2;
-	u8 sdvo_stall:1; /* 158 */
-	u8 hpd_status:2;
-	u8 integrated_encoder:1;
-	u8 skip5:2;
-	u8 dvo_wiring;
-	u8 mipi_bridge_type; /* 171 */
-	u16 device_class_ext;
+	u8 dvo_cfg; /* See DEVICE_CFG_* above */
+
+	union {
+		struct {
+			u8 dvo2_port;
+			u8 i2c2_pin;
+			u8 slave2_addr;
+			u8 ddc2_pin;
+		} __packed;
+		struct {
+			u8 efp_routed:1;			/* 158 */
+			u8 lane_reversal:1;			/* 184 */
+			u8 lspcon:1;				/* 192 */
+			u8 iboost:1;				/* 196 */
+			u8 hpd_invert:1;			/* 196 */
+			u8 flag_reserved:3;
+			u8 hdmi_support:1;			/* 158 */
+			u8 dp_support:1;			/* 158 */
+			u8 tmds_support:1;			/* 158 */
+			u8 support_reserved:5;
+			u8 aux_channel;
+			u8 dongle_detect;
+		} __packed;
+	} __packed;
+
+	union {
+		u8 capabilities;
+		struct {
+			u8 pipe_cap:2;
+			u8 sdvo_stall:1;
+			u8 hpd_status:2;
+			u8 integrated_encoder:1;		/* 158 */
+			u8 skip5:2;
+		} __packed;
+	} __packed;
+
+	u8 dvo_wiring; /* See DEVICE_WIRE_* above */
+
+	union {
+		u8 dvo2_wiring;
+		u8 mipi_bridge_type;				/* 171 */
+	} __packed;
+
+	u16 extended_type;
 	u8 dvo_function;
-	u8 dp_usb_type_c:1; /* 195 */
-	u8 skip6:7;
-	u8 dp_usb_type_c_2x_gpio_index; /* 195 */
-	u16 dp_usb_type_c_2x_gpio_pin; /* 195 */
-	u8 iboost_dp:4; /* 196 */
-	u8 iboost_hdmi:4; /* 196 */
+	/* u8 flags2;						 195 */
+	/* u8 dp_gpio_index;					 195 */
+	/* u16 dp_gpio_pin_num;					 195 */
+	/* u8 iboost_level;					 196 */
 } __packed;
 
 struct vbt {
@@ -155,7 +187,7 @@  static void virt_vbt_generation(struct vbt *v)
 	v->header.bdb_offset = offsetof(struct vbt, bdb_header);
 
 	strcpy(&v->bdb_header.signature[0], "BIOS_DATA_BLOCK");
-	v->bdb_header.version = 186; /* child_dev_size = 38 */
+	v->bdb_header.version = 186; /* child_dev_size = 33 */
 	v->bdb_header.header_size = sizeof(v->bdb_header);
 
 	v->bdb_header.bdb_size = sizeof(struct vbt) - sizeof(struct vbt_header)
@@ -169,18 +201,20 @@  static void virt_vbt_generation(struct vbt *v)
 
 	/* child device */
 	num_child = 4; /* each port has one child */
+	v->general_definitions.child_dev_size =
+		sizeof(struct efp_child_device_config);
 	v->general_definitions_header.id = BDB_GENERAL_DEFINITIONS;
 	/* size will include child devices */
 	v->general_definitions_header.size =
-		sizeof(struct bdb_general_definitions) + num_child * DEV_SIZE;
-	v->general_definitions.child_dev_size = DEV_SIZE;
+		sizeof(struct bdb_general_definitions) +
+			num_child * v->general_definitions.child_dev_size;
 
 	/* portA */
 	v->child0.handle = DEVICE_TYPE_EFP1;
 	v->child0.device_type = DEVICE_TYPE_DP;
 	v->child0.dvo_port = DVO_PORT_DPA;
 	v->child0.aux_channel = DP_AUX_A;
-	v->child0.dp_compat = true;
+	v->child0.dp_support = true;
 	v->child0.integrated_encoder = true;
 
 	/* portB */
@@ -188,7 +222,7 @@  static void virt_vbt_generation(struct vbt *v)
 	v->child1.device_type = DEVICE_TYPE_DP;
 	v->child1.dvo_port = DVO_PORT_DPB;
 	v->child1.aux_channel = DP_AUX_B;
-	v->child1.dp_compat = true;
+	v->child1.dp_support = true;
 	v->child1.integrated_encoder = true;
 
 	/* portC */
@@ -196,7 +230,7 @@  static void virt_vbt_generation(struct vbt *v)
 	v->child2.device_type = DEVICE_TYPE_DP;
 	v->child2.dvo_port = DVO_PORT_DPC;
 	v->child2.aux_channel = DP_AUX_C;
-	v->child2.dp_compat = true;
+	v->child2.dp_support = true;
 	v->child2.integrated_encoder = true;
 
 	/* portD */
@@ -204,7 +238,7 @@  static void virt_vbt_generation(struct vbt *v)
 	v->child3.device_type = DEVICE_TYPE_DP;
 	v->child3.dvo_port = DVO_PORT_DPD;
 	v->child3.aux_channel = DP_AUX_D;
-	v->child3.dp_compat = true;
+	v->child3.dp_support = true;
 	v->child3.integrated_encoder = true;
 
 	/* driver features */

Comments


On 2018.08.29 12:17:56 +0800, Weinan Li wrote:
> GVT-g emualte the opregion for guest with bdb version as '186' which
> child_device_config length should be '33'.
> 
> CC: Xiaolin Zhang <xiaolin.zhang@intel.com>
> Signed-off-by: Weinan Li <weinan.z.li@intel.com>
> ---
>  drivers/gpu/drm/i915/gvt/opregion.c | 142 ++++++++++++++++++++++--------------
>  1 file changed, 88 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gvt/opregion.c b/drivers/gpu/drm/i915/gvt/opregion.c
> index 82586c8..5895869 100644
> --- a/drivers/gpu/drm/i915/gvt/opregion.c
> +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> @@ -42,8 +42,6 @@
>  #define DEVICE_TYPE_EFP3   0x20
>  #define DEVICE_TYPE_EFP4   0x10
>  
> -#define DEV_SIZE	38
> -

Looks what this patch requires is just to fix DEV_SIZE as 33, then other
change seems not necessary? Or we have to remove extra fields in struct
child_dev_config?

As new added config fields seem not to be used, why we have to add that?

>  struct opregion_header {
>  	u8 signature[16];
>  	u32 size;
> @@ -63,58 +61,92 @@ struct bdb_data_header {
>  	u16 size; /* data size */
>  } __packed;
>  
> +/* For supporting windows guest with opregion, here hardcode the emulated
> + * bdb header version as '186', and the corresponding child_device_config
> + * length should be '33' but not '38'. Define one new struct named as
> + * efp_child_device_config which is one copy of child_device_config but
> + * without fields which added from version '195'.
> + */
>  struct efp_child_device_config {
>  	u16 handle;
>  	u16 device_type;
> -	u16 device_class;
> -	u8 i2c_speed;
> -	u8 dp_onboard_redriver; /* 158 */
> -	u8 dp_ondock_redriver; /* 158 */
> -	u8 hdmi_level_shifter_value:4; /* 169 */
> -	u8 hdmi_max_data_rate:4; /* 204 */
> -	u16 dtd_buf_ptr; /* 161 */
> -	u8 edidless_efp:1; /* 161 */
> -	u8 compression_enable:1; /* 198 */
> -	u8 compression_method:1; /* 198 */
> -	u8 ganged_edp:1; /* 202 */
> -	u8 skip0:4;
> -	u8 compression_structure_index:4; /* 198 */
> -	u8 skip1:4;
> -	u8 slave_port; /*  202 */
> -	u8 skip2;
> +
> +	union {
> +		u8  device_id[10]; /* ascii string */
> +		struct {
> +			u8 i2c_speed;
> +			u8 dp_onboard_redriver;			/* 158 */
> +			u8 dp_ondock_redriver;			/* 158 */
> +			u8 hdmi_level_shifter_value:4;		/* 169 */
> +			u8 hdmi_max_data_rate:4;		/* 204 */
> +			u16 dtd_buf_ptr;			/* 161 */
> +			u8 edidless_efp:1;			/* 161 */
> +			u8 compression_enable:1;		/* 198 */
> +			u8 compression_method:1;		/* 198 */
> +			u8 ganged_edp:1;			/* 202 */
> +			u8 reserved0:4;
> +			u8 compression_structure_index:4;	/* 198 */
> +			u8 reserved1:4;
> +			u8 slave_port;				/* 202 */
> +			u8 reserved2;
> +		} __packed;
> +	} __packed;
> +
> +	u16 addin_offset;
>  	u8 dvo_port;
> -	u8 i2c_pin; /* for add-in card */
> -	u8 slave_addr; /* for add-in card */
> +	u8 i2c_pin;
> +	u8 slave_addr;
>  	u8 ddc_pin;
>  	u16 edid_ptr;
> -	u8 dvo_config;
> -	u8 efp_docked_port:1; /* 158 */
> -	u8 lane_reversal:1; /* 184 */
> -	u8 onboard_lspcon:1; /* 192 */
> -	u8 iboost_enable:1; /* 196 */
> -	u8 hpd_invert:1; /* BXT 196 */
> -	u8 slip3:3;
> -	u8 hdmi_compat:1;
> -	u8 dp_compat:1;
> -	u8 tmds_compat:1;
> -	u8 skip4:5;
> -	u8 aux_channel;
> -	u8 dongle_detect;
> -	u8 pipe_cap:2;
> -	u8 sdvo_stall:1; /* 158 */
> -	u8 hpd_status:2;
> -	u8 integrated_encoder:1;
> -	u8 skip5:2;
> -	u8 dvo_wiring;
> -	u8 mipi_bridge_type; /* 171 */
> -	u16 device_class_ext;
> +	u8 dvo_cfg; /* See DEVICE_CFG_* above */
> +
> +	union {
> +		struct {
> +			u8 dvo2_port;
> +			u8 i2c2_pin;
> +			u8 slave2_addr;
> +			u8 ddc2_pin;
> +		} __packed;
> +		struct {
> +			u8 efp_routed:1;			/* 158 */
> +			u8 lane_reversal:1;			/* 184 */
> +			u8 lspcon:1;				/* 192 */
> +			u8 iboost:1;				/* 196 */
> +			u8 hpd_invert:1;			/* 196 */
> +			u8 flag_reserved:3;
> +			u8 hdmi_support:1;			/* 158 */
> +			u8 dp_support:1;			/* 158 */
> +			u8 tmds_support:1;			/* 158 */
> +			u8 support_reserved:5;
> +			u8 aux_channel;
> +			u8 dongle_detect;
> +		} __packed;
> +	} __packed;
> +
> +	union {
> +		u8 capabilities;
> +		struct {
> +			u8 pipe_cap:2;
> +			u8 sdvo_stall:1;
> +			u8 hpd_status:2;
> +			u8 integrated_encoder:1;		/* 158 */
> +			u8 skip5:2;
> +		} __packed;
> +	} __packed;
> +
> +	u8 dvo_wiring; /* See DEVICE_WIRE_* above */
> +
> +	union {
> +		u8 dvo2_wiring;
> +		u8 mipi_bridge_type;				/* 171 */
> +	} __packed;
> +
> +	u16 extended_type;
>  	u8 dvo_function;
> -	u8 dp_usb_type_c:1; /* 195 */
> -	u8 skip6:7;
> -	u8 dp_usb_type_c_2x_gpio_index; /* 195 */
> -	u16 dp_usb_type_c_2x_gpio_pin; /* 195 */
> -	u8 iboost_dp:4; /* 196 */
> -	u8 iboost_hdmi:4; /* 196 */
> +	/* u8 flags2;						 195 */
> +	/* u8 dp_gpio_index;					 195 */
> +	/* u16 dp_gpio_pin_num;					 195 */
> +	/* u8 iboost_level;					 196 */
>  } __packed;
>  
>  struct vbt {
> @@ -155,7 +187,7 @@ static void virt_vbt_generation(struct vbt *v)
>  	v->header.bdb_offset = offsetof(struct vbt, bdb_header);
>  
>  	strcpy(&v->bdb_header.signature[0], "BIOS_DATA_BLOCK");
> -	v->bdb_header.version = 186; /* child_dev_size = 38 */
> +	v->bdb_header.version = 186; /* child_dev_size = 33 */
>  	v->bdb_header.header_size = sizeof(v->bdb_header);
>  
>  	v->bdb_header.bdb_size = sizeof(struct vbt) - sizeof(struct vbt_header)
> @@ -169,18 +201,20 @@ static void virt_vbt_generation(struct vbt *v)
>  
>  	/* child device */
>  	num_child = 4; /* each port has one child */
> +	v->general_definitions.child_dev_size =
> +		sizeof(struct efp_child_device_config);
>  	v->general_definitions_header.id = BDB_GENERAL_DEFINITIONS;
>  	/* size will include child devices */
>  	v->general_definitions_header.size =
> -		sizeof(struct bdb_general_definitions) + num_child * DEV_SIZE;
> -	v->general_definitions.child_dev_size = DEV_SIZE;
> +		sizeof(struct bdb_general_definitions) +
> +			num_child * v->general_definitions.child_dev_size;
>  
>  	/* portA */
>  	v->child0.handle = DEVICE_TYPE_EFP1;
>  	v->child0.device_type = DEVICE_TYPE_DP;
>  	v->child0.dvo_port = DVO_PORT_DPA;
>  	v->child0.aux_channel = DP_AUX_A;
> -	v->child0.dp_compat = true;
> +	v->child0.dp_support = true;
>  	v->child0.integrated_encoder = true;
>  
>  	/* portB */
> @@ -188,7 +222,7 @@ static void virt_vbt_generation(struct vbt *v)
>  	v->child1.device_type = DEVICE_TYPE_DP;
>  	v->child1.dvo_port = DVO_PORT_DPB;
>  	v->child1.aux_channel = DP_AUX_B;
> -	v->child1.dp_compat = true;
> +	v->child1.dp_support = true;
>  	v->child1.integrated_encoder = true;
>  
>  	/* portC */
> @@ -196,7 +230,7 @@ static void virt_vbt_generation(struct vbt *v)
>  	v->child2.device_type = DEVICE_TYPE_DP;
>  	v->child2.dvo_port = DVO_PORT_DPC;
>  	v->child2.aux_channel = DP_AUX_C;
> -	v->child2.dp_compat = true;
> +	v->child2.dp_support = true;
>  	v->child2.integrated_encoder = true;
>  
>  	/* portD */
> @@ -204,7 +238,7 @@ static void virt_vbt_generation(struct vbt *v)
>  	v->child3.device_type = DEVICE_TYPE_DP;
>  	v->child3.dvo_port = DVO_PORT_DPD;
>  	v->child3.aux_channel = DP_AUX_D;
> -	v->child3.dp_compat = true;
> +	v->child3.dp_support = true;
>  	v->child3.integrated_encoder = true;
>  
>  	/* driver features */
> -- 
> 1.9.1
> 
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Friday, August 31, 2018 10:46 AM
> To: Li, Weinan Z <weinan.z.li@intel.com>
> Cc: intel-gvt-dev@lists.freedesktop.org; Zhang, Xiaolin
> <xiaolin.zhang@intel.com>
> Subject: Re: [PATCH] drm/i915/gvt: use the correct length of
> child_device_config
> 
> On 2018.08.29 12:17:56 +0800, Weinan Li wrote:
> > GVT-g emualte the opregion for guest with bdb version as '186' which
> > child_device_config length should be '33'.
> >
> > CC: Xiaolin Zhang <xiaolin.zhang@intel.com>
> > Signed-off-by: Weinan Li <weinan.z.li@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gvt/opregion.c | 142
> > ++++++++++++++++++++++--------------
> >  1 file changed, 88 insertions(+), 54 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gvt/opregion.c
> > b/drivers/gpu/drm/i915/gvt/opregion.c
> > index 82586c8..5895869 100644
> > --- a/drivers/gpu/drm/i915/gvt/opregion.c
> > +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> > @@ -42,8 +42,6 @@
> >  #define DEVICE_TYPE_EFP3   0x20
> >  #define DEVICE_TYPE_EFP4   0x10
> >
> > -#define DEV_SIZE	38
> > -
> 
> Looks what this patch requires is just to fix DEV_SIZE as 33, then other change
> seems not necessary? Or we have to remove extra fields in struct
> child_dev_config?
> 
The size should be the length of child_device_config, otherwise it will impact the guest opregion parser to parse the child_device_config, then may get unexpected data. So remove the extra fields is necessary I think.

> As new added config fields seem not to be used, why we have to add that?
> 
> >  struct opregion_header {
> >  	u8 signature[16];
> >  	u32 size;
> > @@ -63,58 +61,92 @@ struct bdb_data_header {
> >  	u16 size; /* data size */
> >  } __packed;
> >
> > +/* For supporting windows guest with opregion, here hardcode the
> > +emulated
> > + * bdb header version as '186', and the corresponding
> > +child_device_config
> > + * length should be '33' but not '38'. Define one new struct named as
> > + * efp_child_device_config which is one copy of child_device_config
> > +but
> > + * without fields which added from version '195'.
> > + */
> >  struct efp_child_device_config {
> >  	u16 handle;
> >  	u16 device_type;
> > -	u16 device_class;
> > -	u8 i2c_speed;
> > -	u8 dp_onboard_redriver; /* 158 */
> > -	u8 dp_ondock_redriver; /* 158 */
> > -	u8 hdmi_level_shifter_value:4; /* 169 */
> > -	u8 hdmi_max_data_rate:4; /* 204 */
> > -	u16 dtd_buf_ptr; /* 161 */
> > -	u8 edidless_efp:1; /* 161 */
> > -	u8 compression_enable:1; /* 198 */
> > -	u8 compression_method:1; /* 198 */
> > -	u8 ganged_edp:1; /* 202 */
> > -	u8 skip0:4;
> > -	u8 compression_structure_index:4; /* 198 */
> > -	u8 skip1:4;
> > -	u8 slave_port; /*  202 */
> > -	u8 skip2;
> > +
> > +	union {
> > +		u8  device_id[10]; /* ascii string */
> > +		struct {
> > +			u8 i2c_speed;
> > +			u8 dp_onboard_redriver;			/* 158 */
> > +			u8 dp_ondock_redriver;			/* 158 */
> > +			u8 hdmi_level_shifter_value:4;		/* 169 */
> > +			u8 hdmi_max_data_rate:4;		/* 204 */
> > +			u16 dtd_buf_ptr;			/* 161 */
> > +			u8 edidless_efp:1;			/* 161 */
> > +			u8 compression_enable:1;		/* 198 */
> > +			u8 compression_method:1;		/* 198 */
> > +			u8 ganged_edp:1;			/* 202 */
> > +			u8 reserved0:4;
> > +			u8 compression_structure_index:4;	/* 198 */
> > +			u8 reserved1:4;
> > +			u8 slave_port;				/* 202 */
> > +			u8 reserved2;
> > +		} __packed;
> > +	} __packed;
> > +
> > +	u16 addin_offset;
> >  	u8 dvo_port;
> > -	u8 i2c_pin; /* for add-in card */
> > -	u8 slave_addr; /* for add-in card */
> > +	u8 i2c_pin;
> > +	u8 slave_addr;
> >  	u8 ddc_pin;
> >  	u16 edid_ptr;
> > -	u8 dvo_config;
> > -	u8 efp_docked_port:1; /* 158 */
> > -	u8 lane_reversal:1; /* 184 */
> > -	u8 onboard_lspcon:1; /* 192 */
> > -	u8 iboost_enable:1; /* 196 */
> > -	u8 hpd_invert:1; /* BXT 196 */
> > -	u8 slip3:3;
> > -	u8 hdmi_compat:1;
> > -	u8 dp_compat:1;
> > -	u8 tmds_compat:1;
> > -	u8 skip4:5;
> > -	u8 aux_channel;
> > -	u8 dongle_detect;
> > -	u8 pipe_cap:2;
> > -	u8 sdvo_stall:1; /* 158 */
> > -	u8 hpd_status:2;
> > -	u8 integrated_encoder:1;
> > -	u8 skip5:2;
> > -	u8 dvo_wiring;
> > -	u8 mipi_bridge_type; /* 171 */
> > -	u16 device_class_ext;
> > +	u8 dvo_cfg; /* See DEVICE_CFG_* above */
> > +
> > +	union {
> > +		struct {
> > +			u8 dvo2_port;
> > +			u8 i2c2_pin;
> > +			u8 slave2_addr;
> > +			u8 ddc2_pin;
> > +		} __packed;
> > +		struct {
> > +			u8 efp_routed:1;			/* 158 */
> > +			u8 lane_reversal:1;			/* 184 */
> > +			u8 lspcon:1;				/* 192 */
> > +			u8 iboost:1;				/* 196 */
> > +			u8 hpd_invert:1;			/* 196 */
> > +			u8 flag_reserved:3;
> > +			u8 hdmi_support:1;			/* 158 */
> > +			u8 dp_support:1;			/* 158 */
> > +			u8 tmds_support:1;			/* 158 */
> > +			u8 support_reserved:5;
> > +			u8 aux_channel;
> > +			u8 dongle_detect;
> > +		} __packed;
> > +	} __packed;
> > +
> > +	union {
> > +		u8 capabilities;
> > +		struct {
> > +			u8 pipe_cap:2;
> > +			u8 sdvo_stall:1;
> > +			u8 hpd_status:2;
> > +			u8 integrated_encoder:1;		/* 158 */
> > +			u8 skip5:2;
> > +		} __packed;
> > +	} __packed;
> > +
> > +	u8 dvo_wiring; /* See DEVICE_WIRE_* above */
> > +
> > +	union {
> > +		u8 dvo2_wiring;
> > +		u8 mipi_bridge_type;				/* 171 */
> > +	} __packed;
> > +
> > +	u16 extended_type;
> >  	u8 dvo_function;
> > -	u8 dp_usb_type_c:1; /* 195 */
> > -	u8 skip6:7;
> > -	u8 dp_usb_type_c_2x_gpio_index; /* 195 */
> > -	u16 dp_usb_type_c_2x_gpio_pin; /* 195 */
> > -	u8 iboost_dp:4; /* 196 */
> > -	u8 iboost_hdmi:4; /* 196 */
> > +	/* u8 flags2;						 195 */
> > +	/* u8 dp_gpio_index;					 195 */
> > +	/* u16 dp_gpio_pin_num;					 195 */
> > +	/* u8 iboost_level;					 196 */
> >  } __packed;
> >
> >  struct vbt {
> > @@ -155,7 +187,7 @@ static void virt_vbt_generation(struct vbt *v)
> >  	v->header.bdb_offset = offsetof(struct vbt, bdb_header);
> >
> >  	strcpy(&v->bdb_header.signature[0], "BIOS_DATA_BLOCK");
> > -	v->bdb_header.version = 186; /* child_dev_size = 38 */
> > +	v->bdb_header.version = 186; /* child_dev_size = 33 */
> >  	v->bdb_header.header_size = sizeof(v->bdb_header);
> >
> >  	v->bdb_header.bdb_size = sizeof(struct vbt) - sizeof(struct
> > vbt_header) @@ -169,18 +201,20 @@ static void
> > virt_vbt_generation(struct vbt *v)
> >
> >  	/* child device */
> >  	num_child = 4; /* each port has one child */
> > +	v->general_definitions.child_dev_size =
> > +		sizeof(struct efp_child_device_config);
> >  	v->general_definitions_header.id = BDB_GENERAL_DEFINITIONS;
> >  	/* size will include child devices */
> >  	v->general_definitions_header.size =
> > -		sizeof(struct bdb_general_definitions) + num_child * DEV_SIZE;
> > -	v->general_definitions.child_dev_size = DEV_SIZE;
> > +		sizeof(struct bdb_general_definitions) +
> > +			num_child * v->general_definitions.child_dev_size;
> >
> >  	/* portA */
> >  	v->child0.handle = DEVICE_TYPE_EFP1;
> >  	v->child0.device_type = DEVICE_TYPE_DP;
> >  	v->child0.dvo_port = DVO_PORT_DPA;
> >  	v->child0.aux_channel = DP_AUX_A;
> > -	v->child0.dp_compat = true;
> > +	v->child0.dp_support = true;
> >  	v->child0.integrated_encoder = true;
> >
> >  	/* portB */
> > @@ -188,7 +222,7 @@ static void virt_vbt_generation(struct vbt *v)
> >  	v->child1.device_type = DEVICE_TYPE_DP;
> >  	v->child1.dvo_port = DVO_PORT_DPB;
> >  	v->child1.aux_channel = DP_AUX_B;
> > -	v->child1.dp_compat = true;
> > +	v->child1.dp_support = true;
> >  	v->child1.integrated_encoder = true;
> >
> >  	/* portC */
> > @@ -196,7 +230,7 @@ static void virt_vbt_generation(struct vbt *v)
> >  	v->child2.device_type = DEVICE_TYPE_DP;
> >  	v->child2.dvo_port = DVO_PORT_DPC;
> >  	v->child2.aux_channel = DP_AUX_C;
> > -	v->child2.dp_compat = true;
> > +	v->child2.dp_support = true;
> >  	v->child2.integrated_encoder = true;
> >
> >  	/* portD */
> > @@ -204,7 +238,7 @@ static void virt_vbt_generation(struct vbt *v)
> >  	v->child3.device_type = DEVICE_TYPE_DP;
> >  	v->child3.dvo_port = DVO_PORT_DPD;
> >  	v->child3.aux_channel = DP_AUX_D;
> > -	v->child3.dp_compat = true;
> > +	v->child3.dp_support = true;
> >  	v->child3.integrated_encoder = true;
> >
> >  	/* driver features */
> > --
> > 1.9.1
> >
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
On 2018.08.31 03:25:53 +0000, Li, Weinan Z wrote:
> > -----Original Message-----
> > From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> > Sent: Friday, August 31, 2018 10:46 AM
> > To: Li, Weinan Z <weinan.z.li@intel.com>
> > Cc: intel-gvt-dev@lists.freedesktop.org; Zhang, Xiaolin
> > <xiaolin.zhang@intel.com>
> > Subject: Re: [PATCH] drm/i915/gvt: use the correct length of
> > child_device_config
> > 
> > On 2018.08.29 12:17:56 +0800, Weinan Li wrote:
> > > GVT-g emualte the opregion for guest with bdb version as '186' which
> > > child_device_config length should be '33'.
> > >
> > > CC: Xiaolin Zhang <xiaolin.zhang@intel.com>
> > > Signed-off-by: Weinan Li <weinan.z.li@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/gvt/opregion.c | 142
> > > ++++++++++++++++++++++--------------
> > >  1 file changed, 88 insertions(+), 54 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gvt/opregion.c
> > > b/drivers/gpu/drm/i915/gvt/opregion.c
> > > index 82586c8..5895869 100644
> > > --- a/drivers/gpu/drm/i915/gvt/opregion.c
> > > +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> > > @@ -42,8 +42,6 @@
> > >  #define DEVICE_TYPE_EFP3   0x20
> > >  #define DEVICE_TYPE_EFP4   0x10
> > >
> > > -#define DEV_SIZE	38
> > > -
> > 
> > Looks what this patch requires is just to fix DEV_SIZE as 33, then other change
> > seems not necessary? Or we have to remove extra fields in struct
> > child_dev_config?
> > 
> The size should be the length of child_device_config, otherwise it will impact the guest opregion parser to parse the child_device_config, then may get unexpected data. So remove the extra fields is necessary I think.

yeah, we allocate them as packed so need to match struct size vs. general block definition.
So fix DEV_SIZE and remove extra field, maybe add some build-time assert for that, then
seems to be enough. Other changes required?

> 
> > As new added config fields seem not to be used, why we have to add that?
> > 
> > >  struct opregion_header {
> > >  	u8 signature[16];
> > >  	u32 size;
> > > @@ -63,58 +61,92 @@ struct bdb_data_header {
> > >  	u16 size; /* data size */
> > >  } __packed;
> > >
> > > +/* For supporting windows guest with opregion, here hardcode the
> > > +emulated
> > > + * bdb header version as '186', and the corresponding
> > > +child_device_config
> > > + * length should be '33' but not '38'. Define one new struct named as
> > > + * efp_child_device_config which is one copy of child_device_config
> > > +but
> > > + * without fields which added from version '195'.
> > > + */
> > >  struct efp_child_device_config {
> > >  	u16 handle;
> > >  	u16 device_type;
> > > -	u16 device_class;
> > > -	u8 i2c_speed;
> > > -	u8 dp_onboard_redriver; /* 158 */
> > > -	u8 dp_ondock_redriver; /* 158 */
> > > -	u8 hdmi_level_shifter_value:4; /* 169 */
> > > -	u8 hdmi_max_data_rate:4; /* 204 */
> > > -	u16 dtd_buf_ptr; /* 161 */
> > > -	u8 edidless_efp:1; /* 161 */
> > > -	u8 compression_enable:1; /* 198 */
> > > -	u8 compression_method:1; /* 198 */
> > > -	u8 ganged_edp:1; /* 202 */
> > > -	u8 skip0:4;
> > > -	u8 compression_structure_index:4; /* 198 */
> > > -	u8 skip1:4;
> > > -	u8 slave_port; /*  202 */
> > > -	u8 skip2;
> > > +
> > > +	union {
> > > +		u8  device_id[10]; /* ascii string */
> > > +		struct {
> > > +			u8 i2c_speed;
> > > +			u8 dp_onboard_redriver;			/* 158 */
> > > +			u8 dp_ondock_redriver;			/* 158 */
> > > +			u8 hdmi_level_shifter_value:4;		/* 169 */
> > > +			u8 hdmi_max_data_rate:4;		/* 204 */
> > > +			u16 dtd_buf_ptr;			/* 161 */
> > > +			u8 edidless_efp:1;			/* 161 */
> > > +			u8 compression_enable:1;		/* 198 */
> > > +			u8 compression_method:1;		/* 198 */
> > > +			u8 ganged_edp:1;			/* 202 */
> > > +			u8 reserved0:4;
> > > +			u8 compression_structure_index:4;	/* 198 */
> > > +			u8 reserved1:4;
> > > +			u8 slave_port;				/* 202 */
> > > +			u8 reserved2;
> > > +		} __packed;
> > > +	} __packed;
> > > +
> > > +	u16 addin_offset;
> > >  	u8 dvo_port;
> > > -	u8 i2c_pin; /* for add-in card */
> > > -	u8 slave_addr; /* for add-in card */
> > > +	u8 i2c_pin;
> > > +	u8 slave_addr;
> > >  	u8 ddc_pin;
> > >  	u16 edid_ptr;
> > > -	u8 dvo_config;
> > > -	u8 efp_docked_port:1; /* 158 */
> > > -	u8 lane_reversal:1; /* 184 */
> > > -	u8 onboard_lspcon:1; /* 192 */
> > > -	u8 iboost_enable:1; /* 196 */
> > > -	u8 hpd_invert:1; /* BXT 196 */
> > > -	u8 slip3:3;
> > > -	u8 hdmi_compat:1;
> > > -	u8 dp_compat:1;
> > > -	u8 tmds_compat:1;
> > > -	u8 skip4:5;
> > > -	u8 aux_channel;
> > > -	u8 dongle_detect;
> > > -	u8 pipe_cap:2;
> > > -	u8 sdvo_stall:1; /* 158 */
> > > -	u8 hpd_status:2;
> > > -	u8 integrated_encoder:1;
> > > -	u8 skip5:2;
> > > -	u8 dvo_wiring;
> > > -	u8 mipi_bridge_type; /* 171 */
> > > -	u16 device_class_ext;
> > > +	u8 dvo_cfg; /* See DEVICE_CFG_* above */
> > > +
> > > +	union {
> > > +		struct {
> > > +			u8 dvo2_port;
> > > +			u8 i2c2_pin;
> > > +			u8 slave2_addr;
> > > +			u8 ddc2_pin;
> > > +		} __packed;
> > > +		struct {
> > > +			u8 efp_routed:1;			/* 158 */
> > > +			u8 lane_reversal:1;			/* 184 */
> > > +			u8 lspcon:1;				/* 192 */
> > > +			u8 iboost:1;				/* 196 */
> > > +			u8 hpd_invert:1;			/* 196 */
> > > +			u8 flag_reserved:3;
> > > +			u8 hdmi_support:1;			/* 158 */
> > > +			u8 dp_support:1;			/* 158 */
> > > +			u8 tmds_support:1;			/* 158 */
> > > +			u8 support_reserved:5;
> > > +			u8 aux_channel;
> > > +			u8 dongle_detect;
> > > +		} __packed;
> > > +	} __packed;
> > > +
> > > +	union {
> > > +		u8 capabilities;
> > > +		struct {
> > > +			u8 pipe_cap:2;
> > > +			u8 sdvo_stall:1;
> > > +			u8 hpd_status:2;
> > > +			u8 integrated_encoder:1;		/* 158 */
> > > +			u8 skip5:2;
> > > +		} __packed;
> > > +	} __packed;
> > > +
> > > +	u8 dvo_wiring; /* See DEVICE_WIRE_* above */
> > > +
> > > +	union {
> > > +		u8 dvo2_wiring;
> > > +		u8 mipi_bridge_type;				/* 171 */
> > > +	} __packed;
> > > +
> > > +	u16 extended_type;
> > >  	u8 dvo_function;
> > > -	u8 dp_usb_type_c:1; /* 195 */
> > > -	u8 skip6:7;
> > > -	u8 dp_usb_type_c_2x_gpio_index; /* 195 */
> > > -	u16 dp_usb_type_c_2x_gpio_pin; /* 195 */
> > > -	u8 iboost_dp:4; /* 196 */
> > > -	u8 iboost_hdmi:4; /* 196 */
> > > +	/* u8 flags2;						 195 */
> > > +	/* u8 dp_gpio_index;					 195 */
> > > +	/* u16 dp_gpio_pin_num;					 195 */
> > > +	/* u8 iboost_level;					 196 */
> > >  } __packed;
> > >
> > >  struct vbt {
> > > @@ -155,7 +187,7 @@ static void virt_vbt_generation(struct vbt *v)
> > >  	v->header.bdb_offset = offsetof(struct vbt, bdb_header);
> > >
> > >  	strcpy(&v->bdb_header.signature[0], "BIOS_DATA_BLOCK");
> > > -	v->bdb_header.version = 186; /* child_dev_size = 38 */
> > > +	v->bdb_header.version = 186; /* child_dev_size = 33 */
> > >  	v->bdb_header.header_size = sizeof(v->bdb_header);
> > >
> > >  	v->bdb_header.bdb_size = sizeof(struct vbt) - sizeof(struct
> > > vbt_header) @@ -169,18 +201,20 @@ static void
> > > virt_vbt_generation(struct vbt *v)
> > >
> > >  	/* child device */
> > >  	num_child = 4; /* each port has one child */
> > > +	v->general_definitions.child_dev_size =
> > > +		sizeof(struct efp_child_device_config);
> > >  	v->general_definitions_header.id = BDB_GENERAL_DEFINITIONS;
> > >  	/* size will include child devices */
> > >  	v->general_definitions_header.size =
> > > -		sizeof(struct bdb_general_definitions) + num_child * DEV_SIZE;
> > > -	v->general_definitions.child_dev_size = DEV_SIZE;
> > > +		sizeof(struct bdb_general_definitions) +
> > > +			num_child * v->general_definitions.child_dev_size;
> > >
> > >  	/* portA */
> > >  	v->child0.handle = DEVICE_TYPE_EFP1;
> > >  	v->child0.device_type = DEVICE_TYPE_DP;
> > >  	v->child0.dvo_port = DVO_PORT_DPA;
> > >  	v->child0.aux_channel = DP_AUX_A;
> > > -	v->child0.dp_compat = true;
> > > +	v->child0.dp_support = true;
> > >  	v->child0.integrated_encoder = true;
> > >
> > >  	/* portB */
> > > @@ -188,7 +222,7 @@ static void virt_vbt_generation(struct vbt *v)
> > >  	v->child1.device_type = DEVICE_TYPE_DP;
> > >  	v->child1.dvo_port = DVO_PORT_DPB;
> > >  	v->child1.aux_channel = DP_AUX_B;
> > > -	v->child1.dp_compat = true;
> > > +	v->child1.dp_support = true;
> > >  	v->child1.integrated_encoder = true;
> > >
> > >  	/* portC */
> > > @@ -196,7 +230,7 @@ static void virt_vbt_generation(struct vbt *v)
> > >  	v->child2.device_type = DEVICE_TYPE_DP;
> > >  	v->child2.dvo_port = DVO_PORT_DPC;
> > >  	v->child2.aux_channel = DP_AUX_C;
> > > -	v->child2.dp_compat = true;
> > > +	v->child2.dp_support = true;
> > >  	v->child2.integrated_encoder = true;
> > >
> > >  	/* portD */
> > > @@ -204,7 +238,7 @@ static void virt_vbt_generation(struct vbt *v)
> > >  	v->child3.device_type = DEVICE_TYPE_DP;
> > >  	v->child3.dvo_port = DVO_PORT_DPD;
> > >  	v->child3.aux_channel = DP_AUX_D;
> > > -	v->child3.dp_compat = true;
> > > +	v->child3.dp_support = true;
> > >  	v->child3.integrated_encoder = true;
> > >
> > >  	/* driver features */
> > > --
> > > 1.9.1
> > >
> > > _______________________________________________
> > > intel-gvt-dev mailing list
> > > intel-gvt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> > 
> > --
> > Open Source Technology Center, Intel ltd.
> > 
> > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Friday, August 31, 2018 11:41 AM
> To: Li, Weinan Z <weinan.z.li@intel.com>
> Cc: Zhang, Xiaolin <xiaolin.zhang@intel.com>;
> intel-gvt-dev@lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/gvt: use the correct length of
> child_device_config
> 
> On 2018.08.31 03:25:53 +0000, Li, Weinan Z wrote:
> > > -----Original Message-----
> > > From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> > > Sent: Friday, August 31, 2018 10:46 AM
> > > To: Li, Weinan Z <weinan.z.li@intel.com>
> > > Cc: intel-gvt-dev@lists.freedesktop.org; Zhang, Xiaolin
> > > <xiaolin.zhang@intel.com>
> > > Subject: Re: [PATCH] drm/i915/gvt: use the correct length of
> > > child_device_config
> > >
> > > On 2018.08.29 12:17:56 +0800, Weinan Li wrote:
> > > > GVT-g emualte the opregion for guest with bdb version as '186'
> > > > which child_device_config length should be '33'.
> > > >
> > > > CC: Xiaolin Zhang <xiaolin.zhang@intel.com>
> > > > Signed-off-by: Weinan Li <weinan.z.li@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/gvt/opregion.c | 142
> > > > ++++++++++++++++++++++--------------
> > > >  1 file changed, 88 insertions(+), 54 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/gvt/opregion.c
> > > > b/drivers/gpu/drm/i915/gvt/opregion.c
> > > > index 82586c8..5895869 100644
> > > > --- a/drivers/gpu/drm/i915/gvt/opregion.c
> > > > +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> > > > @@ -42,8 +42,6 @@
> > > >  #define DEVICE_TYPE_EFP3   0x20
> > > >  #define DEVICE_TYPE_EFP4   0x10
> > > >
> > > > -#define DEV_SIZE	38
> > > > -
> > >
> > > Looks what this patch requires is just to fix DEV_SIZE as 33, then
> > > other change seems not necessary? Or we have to remove extra fields
> > > in struct child_dev_config?
> > >
> > The size should be the length of child_device_config, otherwise it will impact
> the guest opregion parser to parse the child_device_config, then may get
> unexpected data. So remove the extra fields is necessary I think.
> 
> yeah, we allocate them as packed so need to match struct size vs. general
> block definition.
> So fix DEV_SIZE and remove extra field, maybe add some build-time assert for
> that, then seems to be enough. Other changes required?
> 
Yes, I remove the DEV_SIZE definition and use sizeof(struct) instead. And change the fields which align with the "child_device_config" in i915
> >
> > > As new added config fields seem not to be used, why we have to add that?
> > >
> > > >  struct opregion_header {
> > > >  	u8 signature[16];
> > > >  	u32 size;
> > > > @@ -63,58 +61,92 @@ struct bdb_data_header {
> > > >  	u16 size; /* data size */
> > > >  } __packed;
> > > >
> > > > +/* For supporting windows guest with opregion, here hardcode the
> > > > +emulated
> > > > + * bdb header version as '186', and the corresponding
> > > > +child_device_config
> > > > + * length should be '33' but not '38'. Define one new struct
> > > > +named as
> > > > + * efp_child_device_config which is one copy of
> > > > +child_device_config but
> > > > + * without fields which added from version '195'.
> > > > + */
> > > >  struct efp_child_device_config {
> > > >  	u16 handle;
> > > >  	u16 device_type;
> > > > -	u16 device_class;

device_class remove.
> > > > -	u8 i2c_speed;
> > > > -	u8 dp_onboard_redriver; /* 158 */
> > > > -	u8 dp_ondock_redriver; /* 158 */
> > > > -	u8 hdmi_level_shifter_value:4; /* 169 */
> > > > -	u8 hdmi_max_data_rate:4; /* 204 */
> > > > -	u16 dtd_buf_ptr; /* 161 */
> > > > -	u8 edidless_efp:1; /* 161 */
> > > > -	u8 compression_enable:1; /* 198 */
> > > > -	u8 compression_method:1; /* 198 */
> > > > -	u8 ganged_edp:1; /* 202 */
> > > > -	u8 skip0:4;
> > > > -	u8 compression_structure_index:4; /* 198 */
> > > > -	u8 skip1:4;
> > > > -	u8 slave_port; /*  202 */
> > > > -	u8 skip2;
> > > > +
> > > > +	union {
> > > > +		u8  device_id[10]; /* ascii string */
> > > > +		struct {
> > > > +			u8 i2c_speed;
> > > > +			u8 dp_onboard_redriver;			/* 158 */
> > > > +			u8 dp_ondock_redriver;			/* 158 */
> > > > +			u8 hdmi_level_shifter_value:4;		/* 169 */
> > > > +			u8 hdmi_max_data_rate:4;		/* 204 */
> > > > +			u16 dtd_buf_ptr;			/* 161 */
> > > > +			u8 edidless_efp:1;			/* 161 */
> > > > +			u8 compression_enable:1;		/* 198 */
> > > > +			u8 compression_method:1;		/* 198 */
> > > > +			u8 ganged_edp:1;			/* 202 */
> > > > +			u8 reserved0:4;
> > > > +			u8 compression_structure_index:4;	/* 198 */
> > > > +			u8 reserved1:4;
> > > > +			u8 slave_port;				/* 202 */
> > > > +			u8 reserved2;
> > > > +		} __packed;
> > > > +	} __packed;
> > > > +
> > > > +	u16 addin_offset;

add addin_offset.
> > > >  	u8 dvo_port;
> > > > -	u8 i2c_pin; /* for add-in card */
> > > > -	u8 slave_addr; /* for add-in card */
> > > > +	u8 i2c_pin;
> > > > +	u8 slave_addr;
> > > >  	u8 ddc_pin;
> > > >  	u16 edid_ptr;
> > > > -	u8 dvo_config;
> > > > -	u8 efp_docked_port:1; /* 158 */
> > > > -	u8 lane_reversal:1; /* 184 */
> > > > -	u8 onboard_lspcon:1; /* 192 */
> > > > -	u8 iboost_enable:1; /* 196 */
> > > > -	u8 hpd_invert:1; /* BXT 196 */
> > > > -	u8 slip3:3;
> > > > -	u8 hdmi_compat:1;
> > > > -	u8 dp_compat:1;
> > > > -	u8 tmds_compat:1;
> > > > -	u8 skip4:5;
> > > > -	u8 aux_channel;
> > > > -	u8 dongle_detect;
> > > > -	u8 pipe_cap:2;
> > > > -	u8 sdvo_stall:1; /* 158 */
> > > > -	u8 hpd_status:2;
> > > > -	u8 integrated_encoder:1;
> > > > -	u8 skip5:2;
> > > > -	u8 dvo_wiring;
> > > > -	u8 mipi_bridge_type; /* 171 */
> > > > -	u16 device_class_ext;
> > > > +	u8 dvo_cfg; /* See DEVICE_CFG_* above */
> > > > +
> > > > +	union {
> > > > +		struct {
> > > > +			u8 dvo2_port;
> > > > +			u8 i2c2_pin;
> > > > +			u8 slave2_addr;
> > > > +			u8 ddc2_pin;
> > > > +		} __packed;
> > > > +		struct {
> > > > +			u8 efp_routed:1;			/* 158 */
> > > > +			u8 lane_reversal:1;			/* 184 */
> > > > +			u8 lspcon:1;				/* 192 */
> > > > +			u8 iboost:1;				/* 196 */
> > > > +			u8 hpd_invert:1;			/* 196 */
> > > > +			u8 flag_reserved:3;
> > > > +			u8 hdmi_support:1;			/* 158 */
> > > > +			u8 dp_support:1;			/* 158 */
> > > > +			u8 tmds_support:1;			/* 158 */
> > > > +			u8 support_reserved:5;
> > > > +			u8 aux_channel;
> > > > +			u8 dongle_detect;
> > > > +		} __packed;
> > > > +	} __packed;
> > > > +
> > > > +	union {
> > > > +		u8 capabilities;
> > > > +		struct {
> > > > +			u8 pipe_cap:2;
> > > > +			u8 sdvo_stall:1;
> > > > +			u8 hpd_status:2;
> > > > +			u8 integrated_encoder:1;		/* 158 */
> > > > +			u8 skip5:2;
> > > > +		} __packed;
> > > > +	} __packed;
> > > > +
> > > > +	u8 dvo_wiring; /* See DEVICE_WIRE_* above */
> > > > +
> > > > +	union {
> > > > +		u8 dvo2_wiring;
> > > > +		u8 mipi_bridge_type;				/* 171 */
> > > > +	} __packed;
> > > > +
> > > > +	u16 extended_type;
> > > >  	u8 dvo_function;
> > > > -	u8 dp_usb_type_c:1; /* 195 */
> > > > -	u8 skip6:7;
> > > > -	u8 dp_usb_type_c_2x_gpio_index; /* 195 */
> > > > -	u16 dp_usb_type_c_2x_gpio_pin; /* 195 */
> > > > -	u8 iboost_dp:4; /* 196 */
> > > > -	u8 iboost_hdmi:4; /* 196 */
> > > > +	/* u8 flags2;						 195 */
> > > > +	/* u8 dp_gpio_index;					 195 */
> > > > +	/* u16 dp_gpio_pin_num;					 195 */
> > > > +	/* u8 iboost_level;					 196 */
> > > >  } __packed;
> > > >
> > > >  struct vbt {
> > > > @@ -155,7 +187,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > >  	v->header.bdb_offset = offsetof(struct vbt, bdb_header);
> > > >
> > > >  	strcpy(&v->bdb_header.signature[0], "BIOS_DATA_BLOCK");
> > > > -	v->bdb_header.version = 186; /* child_dev_size = 38 */
> > > > +	v->bdb_header.version = 186; /* child_dev_size = 33 */
> > > >  	v->bdb_header.header_size = sizeof(v->bdb_header);
> > > >
> > > >  	v->bdb_header.bdb_size = sizeof(struct vbt) - sizeof(struct
> > > > vbt_header) @@ -169,18 +201,20 @@ static void
> > > > virt_vbt_generation(struct vbt *v)
> > > >
> > > >  	/* child device */
> > > >  	num_child = 4; /* each port has one child */
> > > > +	v->general_definitions.child_dev_size =
> > > > +		sizeof(struct efp_child_device_config);
> > > >  	v->general_definitions_header.id = BDB_GENERAL_DEFINITIONS;
> > > >  	/* size will include child devices */
> > > >  	v->general_definitions_header.size =
> > > > -		sizeof(struct bdb_general_definitions) + num_child * DEV_SIZE;
> > > > -	v->general_definitions.child_dev_size = DEV_SIZE;
> > > > +		sizeof(struct bdb_general_definitions) +
> > > > +			num_child * v->general_definitions.child_dev_size;
> > > >
> > > >  	/* portA */
> > > >  	v->child0.handle = DEVICE_TYPE_EFP1;
> > > >  	v->child0.device_type = DEVICE_TYPE_DP;
> > > >  	v->child0.dvo_port = DVO_PORT_DPA;
> > > >  	v->child0.aux_channel = DP_AUX_A;
> > > > -	v->child0.dp_compat = true;
> > > > +	v->child0.dp_support = true;
> > > >  	v->child0.integrated_encoder = true;
> > > >
> > > >  	/* portB */
> > > > @@ -188,7 +222,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > >  	v->child1.device_type = DEVICE_TYPE_DP;
> > > >  	v->child1.dvo_port = DVO_PORT_DPB;
> > > >  	v->child1.aux_channel = DP_AUX_B;
> > > > -	v->child1.dp_compat = true;
> > > > +	v->child1.dp_support = true;
> > > >  	v->child1.integrated_encoder = true;
> > > >
> > > >  	/* portC */
> > > > @@ -196,7 +230,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > >  	v->child2.device_type = DEVICE_TYPE_DP;
> > > >  	v->child2.dvo_port = DVO_PORT_DPC;
> > > >  	v->child2.aux_channel = DP_AUX_C;
> > > > -	v->child2.dp_compat = true;
> > > > +	v->child2.dp_support = true;
> > > >  	v->child2.integrated_encoder = true;
> > > >
> > > >  	/* portD */
> > > > @@ -204,7 +238,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > >  	v->child3.device_type = DEVICE_TYPE_DP;
> > > >  	v->child3.dvo_port = DVO_PORT_DPD;
> > > >  	v->child3.aux_channel = DP_AUX_D;
> > > > -	v->child3.dp_compat = true;
> > > > +	v->child3.dp_support = true;
> > > >  	v->child3.integrated_encoder = true;
> > > >
> > > >  	/* driver features */
> > > > --
> > > > 1.9.1
> > > >
> > > > _______________________________________________
> > > > intel-gvt-dev mailing list
> > > > intel-gvt-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> > >
> > > --
> > > Open Source Technology Center, Intel ltd.
> > >
> > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> > _______________________________________________
> > intel-gvt-dev mailing list
> > intel-gvt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
On 2018.08.31 05:06:38 +0000, Li, Weinan Z wrote:
> > On 2018.08.31 03:25:53 +0000, Li, Weinan Z wrote:
> > > > -----Original Message-----
> > > > From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> > > > Sent: Friday, August 31, 2018 10:46 AM
> > > > To: Li, Weinan Z <weinan.z.li@intel.com>
> > > > Cc: intel-gvt-dev@lists.freedesktop.org; Zhang, Xiaolin
> > > > <xiaolin.zhang@intel.com>
> > > > Subject: Re: [PATCH] drm/i915/gvt: use the correct length of
> > > > child_device_config
> > > >
> > > > On 2018.08.29 12:17:56 +0800, Weinan Li wrote:
> > > > > GVT-g emualte the opregion for guest with bdb version as '186'
> > > > > which child_device_config length should be '33'.
> > > > >
> > > > > CC: Xiaolin Zhang <xiaolin.zhang@intel.com>
> > > > > Signed-off-by: Weinan Li <weinan.z.li@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/gvt/opregion.c | 142
> > > > > ++++++++++++++++++++++--------------
> > > > >  1 file changed, 88 insertions(+), 54 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/gvt/opregion.c
> > > > > b/drivers/gpu/drm/i915/gvt/opregion.c
> > > > > index 82586c8..5895869 100644
> > > > > --- a/drivers/gpu/drm/i915/gvt/opregion.c
> > > > > +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> > > > > @@ -42,8 +42,6 @@
> > > > >  #define DEVICE_TYPE_EFP3   0x20
> > > > >  #define DEVICE_TYPE_EFP4   0x10
> > > > >
> > > > > -#define DEV_SIZE	38
> > > > > -
> > > >
> > > > Looks what this patch requires is just to fix DEV_SIZE as 33, then
> > > > other change seems not necessary? Or we have to remove extra fields
> > > > in struct child_dev_config?
> > > >
> > > The size should be the length of child_device_config, otherwise it will impact
> > the guest opregion parser to parse the child_device_config, then may get
> > unexpected data. So remove the extra fields is necessary I think.
> > 
> > yeah, we allocate them as packed so need to match struct size vs. general
> > block definition.
> > So fix DEV_SIZE and remove extra field, maybe add some build-time assert for
> > that, then seems to be enough. Other changes required?
> > 
> Yes, I remove the DEV_SIZE definition and use sizeof(struct) instead. And change the fields which align with the "child_device_config" in i915

yeah, my point is to see if you can seperate real fix vs. other cleanup
e.g align with i915 definition, the fix can goto upstream more quickly.

> > >
> > > > As new added config fields seem not to be used, why we have to add that?
> > > >
> > > > >  struct opregion_header {
> > > > >  	u8 signature[16];
> > > > >  	u32 size;
> > > > > @@ -63,58 +61,92 @@ struct bdb_data_header {
> > > > >  	u16 size; /* data size */
> > > > >  } __packed;
> > > > >
> > > > > +/* For supporting windows guest with opregion, here hardcode the
> > > > > +emulated
> > > > > + * bdb header version as '186', and the corresponding
> > > > > +child_device_config
> > > > > + * length should be '33' but not '38'. Define one new struct
> > > > > +named as
> > > > > + * efp_child_device_config which is one copy of
> > > > > +child_device_config but
> > > > > + * without fields which added from version '195'.
> > > > > + */
> > > > >  struct efp_child_device_config {
> > > > >  	u16 handle;
> > > > >  	u16 device_type;
> > > > > -	u16 device_class;
> 
> device_class remove.

Is this a difference error? Then should be splitted.

> > > > > -	u8 i2c_speed;
> > > > > -	u8 dp_onboard_redriver; /* 158 */
> > > > > -	u8 dp_ondock_redriver; /* 158 */
> > > > > -	u8 hdmi_level_shifter_value:4; /* 169 */
> > > > > -	u8 hdmi_max_data_rate:4; /* 204 */
> > > > > -	u16 dtd_buf_ptr; /* 161 */
> > > > > -	u8 edidless_efp:1; /* 161 */
> > > > > -	u8 compression_enable:1; /* 198 */
> > > > > -	u8 compression_method:1; /* 198 */
> > > > > -	u8 ganged_edp:1; /* 202 */
> > > > > -	u8 skip0:4;
> > > > > -	u8 compression_structure_index:4; /* 198 */
> > > > > -	u8 skip1:4;
> > > > > -	u8 slave_port; /*  202 */
> > > > > -	u8 skip2;
> > > > > +
> > > > > +	union {
> > > > > +		u8  device_id[10]; /* ascii string */
> > > > > +		struct {
> > > > > +			u8 i2c_speed;
> > > > > +			u8 dp_onboard_redriver;			/* 158 */
> > > > > +			u8 dp_ondock_redriver;			/* 158 */
> > > > > +			u8 hdmi_level_shifter_value:4;		/* 169 */
> > > > > +			u8 hdmi_max_data_rate:4;		/* 204 */
> > > > > +			u16 dtd_buf_ptr;			/* 161 */
> > > > > +			u8 edidless_efp:1;			/* 161 */
> > > > > +			u8 compression_enable:1;		/* 198 */
> > > > > +			u8 compression_method:1;		/* 198 */
> > > > > +			u8 ganged_edp:1;			/* 202 */
> > > > > +			u8 reserved0:4;
> > > > > +			u8 compression_structure_index:4;	/* 198 */
> > > > > +			u8 reserved1:4;
> > > > > +			u8 slave_port;				/* 202 */
> > > > > +			u8 reserved2;
> > > > > +		} __packed;
> > > > > +	} __packed;
> > > > > +
> > > > > +	u16 addin_offset;
> 
> add addin_offset.
> > > > >  	u8 dvo_port;
> > > > > -	u8 i2c_pin; /* for add-in card */
> > > > > -	u8 slave_addr; /* for add-in card */
> > > > > +	u8 i2c_pin;
> > > > > +	u8 slave_addr;
> > > > >  	u8 ddc_pin;
> > > > >  	u16 edid_ptr;
> > > > > -	u8 dvo_config;
> > > > > -	u8 efp_docked_port:1; /* 158 */
> > > > > -	u8 lane_reversal:1; /* 184 */
> > > > > -	u8 onboard_lspcon:1; /* 192 */
> > > > > -	u8 iboost_enable:1; /* 196 */
> > > > > -	u8 hpd_invert:1; /* BXT 196 */
> > > > > -	u8 slip3:3;
> > > > > -	u8 hdmi_compat:1;
> > > > > -	u8 dp_compat:1;
> > > > > -	u8 tmds_compat:1;
> > > > > -	u8 skip4:5;
> > > > > -	u8 aux_channel;
> > > > > -	u8 dongle_detect;
> > > > > -	u8 pipe_cap:2;
> > > > > -	u8 sdvo_stall:1; /* 158 */
> > > > > -	u8 hpd_status:2;
> > > > > -	u8 integrated_encoder:1;
> > > > > -	u8 skip5:2;
> > > > > -	u8 dvo_wiring;
> > > > > -	u8 mipi_bridge_type; /* 171 */
> > > > > -	u16 device_class_ext;
> > > > > +	u8 dvo_cfg; /* See DEVICE_CFG_* above */
> > > > > +
> > > > > +	union {
> > > > > +		struct {
> > > > > +			u8 dvo2_port;
> > > > > +			u8 i2c2_pin;
> > > > > +			u8 slave2_addr;
> > > > > +			u8 ddc2_pin;
> > > > > +		} __packed;
> > > > > +		struct {
> > > > > +			u8 efp_routed:1;			/* 158 */
> > > > > +			u8 lane_reversal:1;			/* 184 */
> > > > > +			u8 lspcon:1;				/* 192 */
> > > > > +			u8 iboost:1;				/* 196 */
> > > > > +			u8 hpd_invert:1;			/* 196 */
> > > > > +			u8 flag_reserved:3;
> > > > > +			u8 hdmi_support:1;			/* 158 */
> > > > > +			u8 dp_support:1;			/* 158 */
> > > > > +			u8 tmds_support:1;			/* 158 */
> > > > > +			u8 support_reserved:5;
> > > > > +			u8 aux_channel;
> > > > > +			u8 dongle_detect;
> > > > > +		} __packed;
> > > > > +	} __packed;
> > > > > +
> > > > > +	union {
> > > > > +		u8 capabilities;
> > > > > +		struct {
> > > > > +			u8 pipe_cap:2;
> > > > > +			u8 sdvo_stall:1;
> > > > > +			u8 hpd_status:2;
> > > > > +			u8 integrated_encoder:1;		/* 158 */
> > > > > +			u8 skip5:2;
> > > > > +		} __packed;
> > > > > +	} __packed;
> > > > > +
> > > > > +	u8 dvo_wiring; /* See DEVICE_WIRE_* above */
> > > > > +
> > > > > +	union {
> > > > > +		u8 dvo2_wiring;
> > > > > +		u8 mipi_bridge_type;				/* 171 */
> > > > > +	} __packed;
> > > > > +
> > > > > +	u16 extended_type;
> > > > >  	u8 dvo_function;
> > > > > -	u8 dp_usb_type_c:1; /* 195 */
> > > > > -	u8 skip6:7;
> > > > > -	u8 dp_usb_type_c_2x_gpio_index; /* 195 */
> > > > > -	u16 dp_usb_type_c_2x_gpio_pin; /* 195 */
> > > > > -	u8 iboost_dp:4; /* 196 */
> > > > > -	u8 iboost_hdmi:4; /* 196 */
> > > > > +	/* u8 flags2;						 195 */
> > > > > +	/* u8 dp_gpio_index;					 195 */
> > > > > +	/* u16 dp_gpio_pin_num;					 195 */
> > > > > +	/* u8 iboost_level;					 196 */
> > > > >  } __packed;
> > > > >
> > > > >  struct vbt {
> > > > > @@ -155,7 +187,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > > >  	v->header.bdb_offset = offsetof(struct vbt, bdb_header);
> > > > >
> > > > >  	strcpy(&v->bdb_header.signature[0], "BIOS_DATA_BLOCK");
> > > > > -	v->bdb_header.version = 186; /* child_dev_size = 38 */
> > > > > +	v->bdb_header.version = 186; /* child_dev_size = 33 */
> > > > >  	v->bdb_header.header_size = sizeof(v->bdb_header);
> > > > >
> > > > >  	v->bdb_header.bdb_size = sizeof(struct vbt) - sizeof(struct
> > > > > vbt_header) @@ -169,18 +201,20 @@ static void
> > > > > virt_vbt_generation(struct vbt *v)
> > > > >
> > > > >  	/* child device */
> > > > >  	num_child = 4; /* each port has one child */
> > > > > +	v->general_definitions.child_dev_size =
> > > > > +		sizeof(struct efp_child_device_config);
> > > > >  	v->general_definitions_header.id = BDB_GENERAL_DEFINITIONS;
> > > > >  	/* size will include child devices */
> > > > >  	v->general_definitions_header.size =
> > > > > -		sizeof(struct bdb_general_definitions) + num_child * DEV_SIZE;
> > > > > -	v->general_definitions.child_dev_size = DEV_SIZE;
> > > > > +		sizeof(struct bdb_general_definitions) +
> > > > > +			num_child * v->general_definitions.child_dev_size;
> > > > >
> > > > >  	/* portA */
> > > > >  	v->child0.handle = DEVICE_TYPE_EFP1;
> > > > >  	v->child0.device_type = DEVICE_TYPE_DP;
> > > > >  	v->child0.dvo_port = DVO_PORT_DPA;
> > > > >  	v->child0.aux_channel = DP_AUX_A;
> > > > > -	v->child0.dp_compat = true;
> > > > > +	v->child0.dp_support = true;
> > > > >  	v->child0.integrated_encoder = true;
> > > > >
> > > > >  	/* portB */
> > > > > @@ -188,7 +222,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > > >  	v->child1.device_type = DEVICE_TYPE_DP;
> > > > >  	v->child1.dvo_port = DVO_PORT_DPB;
> > > > >  	v->child1.aux_channel = DP_AUX_B;
> > > > > -	v->child1.dp_compat = true;
> > > > > +	v->child1.dp_support = true;
> > > > >  	v->child1.integrated_encoder = true;
> > > > >
> > > > >  	/* portC */
> > > > > @@ -196,7 +230,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > > >  	v->child2.device_type = DEVICE_TYPE_DP;
> > > > >  	v->child2.dvo_port = DVO_PORT_DPC;
> > > > >  	v->child2.aux_channel = DP_AUX_C;
> > > > > -	v->child2.dp_compat = true;
> > > > > +	v->child2.dp_support = true;
> > > > >  	v->child2.integrated_encoder = true;
> > > > >
> > > > >  	/* portD */
> > > > > @@ -204,7 +238,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > > >  	v->child3.device_type = DEVICE_TYPE_DP;
> > > > >  	v->child3.dvo_port = DVO_PORT_DPD;
> > > > >  	v->child3.aux_channel = DP_AUX_D;
> > > > > -	v->child3.dp_compat = true;
> > > > > +	v->child3.dp_support = true;
> > > > >  	v->child3.integrated_encoder = true;
> > > > >
> > > > >  	/* driver features */
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > intel-gvt-dev mailing list
> > > > > intel-gvt-dev@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> > > >
> > > > --
> > > > Open Source Technology Center, Intel ltd.
> > > >
> > > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> > > _______________________________________________
> > > intel-gvt-dev mailing list
> > > intel-gvt-dev@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> > 
> > --
> > Open Source Technology Center, Intel ltd.
> > 
> > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> -----Original Message-----
> From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> Sent: Friday, August 31, 2018 1:17 PM
> To: Li, Weinan Z <weinan.z.li@intel.com>
> Cc: Zhang, Xiaolin <xiaolin.zhang@intel.com>;
> intel-gvt-dev@lists.freedesktop.org
> Subject: Re: [PATCH] drm/i915/gvt: use the correct length of
> child_device_config
> 
> On 2018.08.31 05:06:38 +0000, Li, Weinan Z wrote:
> > > On 2018.08.31 03:25:53 +0000, Li, Weinan Z wrote:
> > > > > -----Original Message-----
> > > > > From: Zhenyu Wang [mailto:zhenyuw@linux.intel.com]
> > > > > Sent: Friday, August 31, 2018 10:46 AM
> > > > > To: Li, Weinan Z <weinan.z.li@intel.com>
> > > > > Cc: intel-gvt-dev@lists.freedesktop.org; Zhang, Xiaolin
> > > > > <xiaolin.zhang@intel.com>
> > > > > Subject: Re: [PATCH] drm/i915/gvt: use the correct length of
> > > > > child_device_config
> > > > >
> > > > > On 2018.08.29 12:17:56 +0800, Weinan Li wrote:
> > > > > > GVT-g emualte the opregion for guest with bdb version as '186'
> > > > > > which child_device_config length should be '33'.
> > > > > >
> > > > > > CC: Xiaolin Zhang <xiaolin.zhang@intel.com>
> > > > > > Signed-off-by: Weinan Li <weinan.z.li@intel.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/gvt/opregion.c | 142
> > > > > > ++++++++++++++++++++++--------------
> > > > > >  1 file changed, 88 insertions(+), 54 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/gvt/opregion.c
> > > > > > b/drivers/gpu/drm/i915/gvt/opregion.c
> > > > > > index 82586c8..5895869 100644
> > > > > > --- a/drivers/gpu/drm/i915/gvt/opregion.c
> > > > > > +++ b/drivers/gpu/drm/i915/gvt/opregion.c
> > > > > > @@ -42,8 +42,6 @@
> > > > > >  #define DEVICE_TYPE_EFP3   0x20
> > > > > >  #define DEVICE_TYPE_EFP4   0x10
> > > > > >
> > > > > > -#define DEV_SIZE	38
> > > > > > -
> > > > >
> > > > > Looks what this patch requires is just to fix DEV_SIZE as 33,
> > > > > then other change seems not necessary? Or we have to remove
> > > > > extra fields in struct child_dev_config?
> > > > >
> > > > The size should be the length of child_device_config, otherwise it
> > > > will impact
> > > the guest opregion parser to parse the child_device_config, then may
> > > get unexpected data. So remove the extra fields is necessary I think.
> > >
> > > yeah, we allocate them as packed so need to match struct size vs.
> > > general block definition.
> > > So fix DEV_SIZE and remove extra field, maybe add some build-time
> > > assert for that, then seems to be enough. Other changes required?
> > >
> > Yes, I remove the DEV_SIZE definition and use sizeof(struct) instead.
> > And change the fields which align with the "child_device_config" in
> > i915
> 
> yeah, my point is to see if you can seperate real fix vs. other cleanup e.g align
> with i915 definition, the fix can goto upstream more quickly.
> 

Got it, let me make patch v2.
> > > >
> > > > > As new added config fields seem not to be used, why we have to add
> that?
> > > > >
> > > > > >  struct opregion_header {
> > > > > >  	u8 signature[16];
> > > > > >  	u32 size;
> > > > > > @@ -63,58 +61,92 @@ struct bdb_data_header {
> > > > > >  	u16 size; /* data size */
> > > > > >  } __packed;
> > > > > >
> > > > > > +/* For supporting windows guest with opregion, here hardcode
> > > > > > +the emulated
> > > > > > + * bdb header version as '186', and the corresponding
> > > > > > +child_device_config
> > > > > > + * length should be '33' but not '38'. Define one new struct
> > > > > > +named as
> > > > > > + * efp_child_device_config which is one copy of
> > > > > > +child_device_config but
> > > > > > + * without fields which added from version '195'.
> > > > > > + */
> > > > > >  struct efp_child_device_config {
> > > > > >  	u16 handle;
> > > > > >  	u16 device_type;
> > > > > > -	u16 device_class;
> >
> > device_class remove.
> 
> Is this a difference error? Then should be splitted.

I think it should align with i915 defination.
> 
> > > > > > -	u8 i2c_speed;
> > > > > > -	u8 dp_onboard_redriver; /* 158 */
> > > > > > -	u8 dp_ondock_redriver; /* 158 */
> > > > > > -	u8 hdmi_level_shifter_value:4; /* 169 */
> > > > > > -	u8 hdmi_max_data_rate:4; /* 204 */
> > > > > > -	u16 dtd_buf_ptr; /* 161 */
> > > > > > -	u8 edidless_efp:1; /* 161 */
> > > > > > -	u8 compression_enable:1; /* 198 */
> > > > > > -	u8 compression_method:1; /* 198 */
> > > > > > -	u8 ganged_edp:1; /* 202 */
> > > > > > -	u8 skip0:4;
> > > > > > -	u8 compression_structure_index:4; /* 198 */
> > > > > > -	u8 skip1:4;
> > > > > > -	u8 slave_port; /*  202 */
> > > > > > -	u8 skip2;
> > > > > > +
> > > > > > +	union {
> > > > > > +		u8  device_id[10]; /* ascii string */
> > > > > > +		struct {
> > > > > > +			u8 i2c_speed;
> > > > > > +			u8 dp_onboard_redriver;			/* 158 */
> > > > > > +			u8 dp_ondock_redriver;			/* 158 */
> > > > > > +			u8 hdmi_level_shifter_value:4;		/* 169 */
> > > > > > +			u8 hdmi_max_data_rate:4;		/* 204 */
> > > > > > +			u16 dtd_buf_ptr;			/* 161 */
> > > > > > +			u8 edidless_efp:1;			/* 161 */
> > > > > > +			u8 compression_enable:1;		/* 198 */
> > > > > > +			u8 compression_method:1;		/* 198 */
> > > > > > +			u8 ganged_edp:1;			/* 202 */
> > > > > > +			u8 reserved0:4;
> > > > > > +			u8 compression_structure_index:4;	/* 198 */
> > > > > > +			u8 reserved1:4;
> > > > > > +			u8 slave_port;				/* 202 */
> > > > > > +			u8 reserved2;
> > > > > > +		} __packed;
> > > > > > +	} __packed;
> > > > > > +
> > > > > > +	u16 addin_offset;
> >
> > add addin_offset.
> > > > > >  	u8 dvo_port;
> > > > > > -	u8 i2c_pin; /* for add-in card */
> > > > > > -	u8 slave_addr; /* for add-in card */
> > > > > > +	u8 i2c_pin;
> > > > > > +	u8 slave_addr;
> > > > > >  	u8 ddc_pin;
> > > > > >  	u16 edid_ptr;
> > > > > > -	u8 dvo_config;
> > > > > > -	u8 efp_docked_port:1; /* 158 */
> > > > > > -	u8 lane_reversal:1; /* 184 */
> > > > > > -	u8 onboard_lspcon:1; /* 192 */
> > > > > > -	u8 iboost_enable:1; /* 196 */
> > > > > > -	u8 hpd_invert:1; /* BXT 196 */
> > > > > > -	u8 slip3:3;
> > > > > > -	u8 hdmi_compat:1;
> > > > > > -	u8 dp_compat:1;
> > > > > > -	u8 tmds_compat:1;
> > > > > > -	u8 skip4:5;
> > > > > > -	u8 aux_channel;
> > > > > > -	u8 dongle_detect;
> > > > > > -	u8 pipe_cap:2;
> > > > > > -	u8 sdvo_stall:1; /* 158 */
> > > > > > -	u8 hpd_status:2;
> > > > > > -	u8 integrated_encoder:1;
> > > > > > -	u8 skip5:2;
> > > > > > -	u8 dvo_wiring;
> > > > > > -	u8 mipi_bridge_type; /* 171 */
> > > > > > -	u16 device_class_ext;
> > > > > > +	u8 dvo_cfg; /* See DEVICE_CFG_* above */
> > > > > > +
> > > > > > +	union {
> > > > > > +		struct {
> > > > > > +			u8 dvo2_port;
> > > > > > +			u8 i2c2_pin;
> > > > > > +			u8 slave2_addr;
> > > > > > +			u8 ddc2_pin;
> > > > > > +		} __packed;
> > > > > > +		struct {
> > > > > > +			u8 efp_routed:1;			/* 158 */
> > > > > > +			u8 lane_reversal:1;			/* 184 */
> > > > > > +			u8 lspcon:1;				/* 192 */
> > > > > > +			u8 iboost:1;				/* 196 */
> > > > > > +			u8 hpd_invert:1;			/* 196 */
> > > > > > +			u8 flag_reserved:3;
> > > > > > +			u8 hdmi_support:1;			/* 158 */
> > > > > > +			u8 dp_support:1;			/* 158 */
> > > > > > +			u8 tmds_support:1;			/* 158 */
> > > > > > +			u8 support_reserved:5;
> > > > > > +			u8 aux_channel;
> > > > > > +			u8 dongle_detect;
> > > > > > +		} __packed;
> > > > > > +	} __packed;
> > > > > > +
> > > > > > +	union {
> > > > > > +		u8 capabilities;
> > > > > > +		struct {
> > > > > > +			u8 pipe_cap:2;
> > > > > > +			u8 sdvo_stall:1;
> > > > > > +			u8 hpd_status:2;
> > > > > > +			u8 integrated_encoder:1;		/* 158 */
> > > > > > +			u8 skip5:2;
> > > > > > +		} __packed;
> > > > > > +	} __packed;
> > > > > > +
> > > > > > +	u8 dvo_wiring; /* See DEVICE_WIRE_* above */
> > > > > > +
> > > > > > +	union {
> > > > > > +		u8 dvo2_wiring;
> > > > > > +		u8 mipi_bridge_type;				/* 171 */
> > > > > > +	} __packed;
> > > > > > +
> > > > > > +	u16 extended_type;
> > > > > >  	u8 dvo_function;
> > > > > > -	u8 dp_usb_type_c:1; /* 195 */
> > > > > > -	u8 skip6:7;
> > > > > > -	u8 dp_usb_type_c_2x_gpio_index; /* 195 */
> > > > > > -	u16 dp_usb_type_c_2x_gpio_pin; /* 195 */
> > > > > > -	u8 iboost_dp:4; /* 196 */
> > > > > > -	u8 iboost_hdmi:4; /* 196 */
> > > > > > +	/* u8 flags2;						 195 */
> > > > > > +	/* u8 dp_gpio_index;					 195 */
> > > > > > +	/* u16 dp_gpio_pin_num;					 195 */
> > > > > > +	/* u8 iboost_level;					 196 */
> > > > > >  } __packed;
> > > > > >
> > > > > >  struct vbt {
> > > > > > @@ -155,7 +187,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > > > >  	v->header.bdb_offset = offsetof(struct vbt, bdb_header);
> > > > > >
> > > > > >  	strcpy(&v->bdb_header.signature[0], "BIOS_DATA_BLOCK");
> > > > > > -	v->bdb_header.version = 186; /* child_dev_size = 38 */
> > > > > > +	v->bdb_header.version = 186; /* child_dev_size = 33 */
> > > > > >  	v->bdb_header.header_size = sizeof(v->bdb_header);
> > > > > >
> > > > > >  	v->bdb_header.bdb_size = sizeof(struct vbt) - sizeof(struct
> > > > > > vbt_header) @@ -169,18 +201,20 @@ static void
> > > > > > virt_vbt_generation(struct vbt *v)
> > > > > >
> > > > > >  	/* child device */
> > > > > >  	num_child = 4; /* each port has one child */
> > > > > > +	v->general_definitions.child_dev_size =
> > > > > > +		sizeof(struct efp_child_device_config);
> > > > > >  	v->general_definitions_header.id =
> BDB_GENERAL_DEFINITIONS;
> > > > > >  	/* size will include child devices */
> > > > > >  	v->general_definitions_header.size =
> > > > > > -		sizeof(struct bdb_general_definitions) + num_child * DEV_SIZE;
> > > > > > -	v->general_definitions.child_dev_size = DEV_SIZE;
> > > > > > +		sizeof(struct bdb_general_definitions) +
> > > > > > +			num_child * v->general_definitions.child_dev_size;
> > > > > >
> > > > > >  	/* portA */
> > > > > >  	v->child0.handle = DEVICE_TYPE_EFP1;
> > > > > >  	v->child0.device_type = DEVICE_TYPE_DP;
> > > > > >  	v->child0.dvo_port = DVO_PORT_DPA;
> > > > > >  	v->child0.aux_channel = DP_AUX_A;
> > > > > > -	v->child0.dp_compat = true;
> > > > > > +	v->child0.dp_support = true;
> > > > > >  	v->child0.integrated_encoder = true;
> > > > > >
> > > > > >  	/* portB */
> > > > > > @@ -188,7 +222,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > > > >  	v->child1.device_type = DEVICE_TYPE_DP;
> > > > > >  	v->child1.dvo_port = DVO_PORT_DPB;
> > > > > >  	v->child1.aux_channel = DP_AUX_B;
> > > > > > -	v->child1.dp_compat = true;
> > > > > > +	v->child1.dp_support = true;
> > > > > >  	v->child1.integrated_encoder = true;
> > > > > >
> > > > > >  	/* portC */
> > > > > > @@ -196,7 +230,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > > > >  	v->child2.device_type = DEVICE_TYPE_DP;
> > > > > >  	v->child2.dvo_port = DVO_PORT_DPC;
> > > > > >  	v->child2.aux_channel = DP_AUX_C;
> > > > > > -	v->child2.dp_compat = true;
> > > > > > +	v->child2.dp_support = true;
> > > > > >  	v->child2.integrated_encoder = true;
> > > > > >
> > > > > >  	/* portD */
> > > > > > @@ -204,7 +238,7 @@ static void virt_vbt_generation(struct vbt *v)
> > > > > >  	v->child3.device_type = DEVICE_TYPE_DP;
> > > > > >  	v->child3.dvo_port = DVO_PORT_DPD;
> > > > > >  	v->child3.aux_channel = DP_AUX_D;
> > > > > > -	v->child3.dp_compat = true;
> > > > > > +	v->child3.dp_support = true;
> > > > > >  	v->child3.integrated_encoder = true;
> > > > > >
> > > > > >  	/* driver features */
> > > > > > --
> > > > > > 1.9.1
> > > > > >
> > > > > > _______________________________________________
> > > > > > intel-gvt-dev mailing list
> > > > > > intel-gvt-dev@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> > > > >
> > > > > --
> > > > > Open Source Technology Center, Intel ltd.
> > > > >
> > > > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> > > > _______________________________________________
> > > > intel-gvt-dev mailing list
> > > > intel-gvt-dev@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev
> > >
> > > --
> > > Open Source Technology Center, Intel ltd.
> > >
> > > $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827
> 
> --
> Open Source Technology Center, Intel ltd.
> 
> $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827