Re: [PATCH] vpci/header: cope with devices not having vpci allocated

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Thu, May 25, 2023 at 11:05:52AM +0200, Jan Beulich wrote:
> On 25.05.2023 10:30, Roger Pau Monne wrote:
> > When traversing the list of pci devices assigned to a domain cope with
> > some of them not having the vpci struct allocated. It's possible for
> > the hardware domain to have read-only devices assigned that are not
> > handled by vPCI, or for unprivileged domains to have some devices
> > handled by an emulator different than vPCI.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> >  xen/drivers/vpci/header.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/xen/drivers/vpci/header.c b/xen/drivers/vpci/header.c
> > index ec2e978a4e6b..3c1fcfb208cf 100644
> > --- a/xen/drivers/vpci/header.c
> > +++ b/xen/drivers/vpci/header.c
> > @@ -289,6 +289,20 @@ static int modify_bars(const struct pci_dev *pdev, 
> > uint16_t cmd, bool rom_only)
> >       */
> >      for_each_pdev ( pdev->domain, tmp )
> >      {
> > +        if ( !tmp->vpci )
> > +            /*
> > +             * For the hardware domain it's possible to have devices 
> > assigned
> > +             * to it that are not handled by vPCI, either because those are
> > +             * read-only devices, or because vPCI setup has failed.
> 
> So this really is a forward looking comment, becoming true only (aiui)
> when my patch also makes it in.

The r/o part yes, device setup failing is already possible.

I think it's fine to have the r/o part added already.

> > +             * For unprivileged domains we should aim for passthrough 
> > devices
> > +             * to be capable of being handled by different emulators, and 
> > hence
> > +             * a domain could have some devices handled by vPCI and others 
> > by
> > +             * QEMU for example, and the later won't have pdev->vpci
> > +             * allocated.
> 
> This, otoh, I don't understand: Do we really intend to have pass-through
> devices handled by qemu or alike, for PVH? Or are you thinking of hybrid
> HVM (some vPCI, some qemu)?

I was thinking about hybrid.

> Plus, when considering hybrid guests, won't
> we need to take into account BARs of externally handled devices as well,
> to avoid overlaps?

On that scenario we would request non-overlapping BARs for things to
work as expected, I think that's already the case for HVM if you mix
QEMU and demu for the same domain.

> In any event, until the DomU side picture is more clear, I'd suggest we
> avoid making statements pinning down expectations. That said, you're the
> maintainer, so if you really want it like this, so be it.

OK, I don't have a strong opinion, so I'm fine with dropping the "For
unprivileged domains ..." paragraph.

Would you like me to resend with that dropped?

I assume you also want the commit message adjusted to not mention
unprivileged domains?

Thanks, Roger.