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

[Chuck Zmudzinski <brchuckz@xxxxxxxxxxxx>]

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@xxxxxxx>
> > ---
> >   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,