[PATCH] xen/pass-through: don't create needless register group

[PATCH] xen/pass-through: don't create needless register group

[Chuck Zmudzinski]

Currently we are creating a register group for the Intel IGD OpRegion
for every device we pass through, but the XEN_PCI_INTEL_OPREGION
register group is only valid for an Intel IGD. Add a check to make
sure the device is an Intel IGD and a check that the administrator has
enabled gfx_passthru in the xl domain configuration. Require both checks
to be true before creating the register group. Use the existing
is_igd_vga_passthrough() function to check for a graphics device from
any vendor and that the administrator enabled gfx_passthru in the xl
domain configuration, but further require that the vendor be Intel,
because only Intel IGD devices have an Intel OpRegion. These are the
same checks hvmloader and libxl do to determine if the Intel OpRegion
needs to be mapped into the guest's memory.

Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
---
 hw/xen/xen_pt_config_init.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index c5c4e943a8..ffd915654c 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -2037,6 +2037,10 @@ void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
          * therefore the size should be 0xff.
          */
         if (xen_pt_emu_reg_grps[i].grp_id == XEN_PCI_INTEL_OPREGION) {
+            if (!is_igd_vga_passthrough(&s->real_device) ||
+                s->real_device.vendor_id != PCI_VENDOR_ID_INTEL) {
+                continue;
+            }
             reg_grp_offset = XEN_PCI_INTEL_OPREGION;
         }
 
-- 
2.36.1

Re: [PATCH] xen/pass-through: don't create needless register group

[Anthony PERARD]

On Fri, Jun 10, 2022 at 12:23:35PM -0400, Chuck Zmudzinski wrote:
> Currently we are creating a register group for the Intel IGD OpRegion
> for every device we pass through, but the XEN_PCI_INTEL_OPREGION
> register group is only valid for an Intel IGD. Add a check to make
> sure the device is an Intel IGD and a check that the administrator has
> enabled gfx_passthru in the xl domain configuration. Require both checks
> to be true before creating the register group. Use the existing
> is_igd_vga_passthrough() function to check for a graphics device from
> any vendor and that the administrator enabled gfx_passthru in the xl
> domain configuration, but further require that the vendor be Intel,
> because only Intel IGD devices have an Intel OpRegion. These are the
> same checks hvmloader and libxl do to determine if the Intel OpRegion
> needs to be mapped into the guest's memory.
> 
> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> ---
>  hw/xen/xen_pt_config_init.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index c5c4e943a8..ffd915654c 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -2037,6 +2037,10 @@ void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
>           * therefore the size should be 0xff.
>           */

Could you move that comment? I think it would make more sense to comment
the "reg_grp_offset=XEN_PCI_INTEL_OPREGION" line now that the `if` block
also skip setting up the group on non-intel devices.

>          if (xen_pt_emu_reg_grps[i].grp_id == XEN_PCI_INTEL_OPREGION) {
> +            if (!is_igd_vga_passthrough(&s->real_device) ||
> +                s->real_device.vendor_id != PCI_VENDOR_ID_INTEL) {
> +                continue;
> +            }
>              reg_grp_offset = XEN_PCI_INTEL_OPREGION;
>          }

Thanks,

-- 
Anthony PERARD

Re: [PATCH] xen/pass-through: don't create needless register group

[Anthony PERARD via]

On Fri, Jun 10, 2022 at 12:23:35PM -0400, Chuck Zmudzinski wrote:
> Currently we are creating a register group for the Intel IGD OpRegion
> for every device we pass through, but the XEN_PCI_INTEL_OPREGION
> register group is only valid for an Intel IGD. Add a check to make
> sure the device is an Intel IGD and a check that the administrator has
> enabled gfx_passthru in the xl domain configuration. Require both checks
> to be true before creating the register group. Use the existing
> is_igd_vga_passthrough() function to check for a graphics device from
> any vendor and that the administrator enabled gfx_passthru in the xl
> domain configuration, but further require that the vendor be Intel,
> because only Intel IGD devices have an Intel OpRegion. These are the
> same checks hvmloader and libxl do to determine if the Intel OpRegion
> needs to be mapped into the guest's memory.
> 
> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
> ---
>  hw/xen/xen_pt_config_init.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index c5c4e943a8..ffd915654c 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -2037,6 +2037,10 @@ void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
>           * therefore the size should be 0xff.
>           */

Could you move that comment? I think it would make more sense to comment
the "reg_grp_offset=XEN_PCI_INTEL_OPREGION" line now that the `if` block
also skip setting up the group on non-intel devices.

>          if (xen_pt_emu_reg_grps[i].grp_id == XEN_PCI_INTEL_OPREGION) {
> +            if (!is_igd_vga_passthrough(&s->real_device) ||
> +                s->real_device.vendor_id != PCI_VENDOR_ID_INTEL) {
> +                continue;
> +            }
>              reg_grp_offset = XEN_PCI_INTEL_OPREGION;
>          }

Thanks,

-- 
Anthony PERARD

Re: [PATCH] xen/pass-through: don't create needless register group

[Chuck Zmudzinski]

On 6/17/22 9:07 AM, Anthony PERARD wrote:
> On Fri, Jun 10, 2022 at 12:23:35PM -0400, Chuck Zmudzinski wrote:
>> Currently we are creating a register group for the Intel IGD OpRegion
>> for every device we pass through, but the XEN_PCI_INTEL_OPREGION
>> register group is only valid for an Intel IGD. Add a check to make
>> sure the device is an Intel IGD and a check that the administrator has
>> enabled gfx_passthru in the xl domain configuration. Require both checks
>> to be true before creating the register group. Use the existing
>> is_igd_vga_passthrough() function to check for a graphics device from
>> any vendor and that the administrator enabled gfx_passthru in the xl
>> domain configuration, but further require that the vendor be Intel,
>> because only Intel IGD devices have an Intel OpRegion. These are the
>> same checks hvmloader and libxl do to determine if the Intel OpRegion
>> needs to be mapped into the guest's memory.
>>
>> Signed-off-by: Chuck Zmudzinski <brchuckz@aol.com>
>> ---
>>   hw/xen/xen_pt_config_init.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
>> index c5c4e943a8..ffd915654c 100644
>> --- a/hw/xen/xen_pt_config_init.c
>> +++ b/hw/xen/xen_pt_config_init.c
>> @@ -2037,6 +2037,10 @@ void xen_pt_config_init(XenPCIPassthroughState *s, Error **errp)
>>            * therefore the size should be 0xff.
>>            */
> Could you move that comment? I think it would make more sense to comment
> the "reg_grp_offset=XEN_PCI_INTEL_OPREGION" line now that the `if` block
> also skip setting up the group on non-intel devices.

OK. I just e-mailed interested parties v2 that moves the comment
and mentions that the comment is moved in the commit message.

Best Regards,

Chuck

>
>>           if (xen_pt_emu_reg_grps[i].grp_id == XEN_PCI_INTEL_OPREGION) {
>> +            if (!is_igd_vga_passthrough(&s->real_device) ||
>> +                s->real_device.vendor_id != PCI_VENDOR_ID_INTEL) {
>> +                continue;
>> +            }
>>               reg_grp_offset = XEN_PCI_INTEL_OPREGION;
>>           }
> Thanks,
>