Re: [PATCH RFC v2] vPCI: account for hidden devices

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

On Thu, May 25, 2023 at 04:39:51PM +0200, Jan Beulich wrote:
> On 24.05.2023 17:56, Roger Pau Monné wrote:
> > On Wed, May 24, 2023 at 03:45:58PM +0200, Jan Beulich wrote:
> >> --- a/xen/drivers/vpci/header.c
> >> +++ b/xen/drivers/vpci/header.c
> >> @@ -218,6 +218,7 @@ static int modify_bars(const struct pci_
> >>      struct vpci_header *header = &pdev->vpci->header;
> >>      struct rangeset *mem = rangeset_new(NULL, NULL, 0);
> >>      struct pci_dev *tmp, *dev = NULL;
> >> +    const struct domain *d;
> >>      const struct vpci_msix *msix = pdev->vpci->msix;
> >>      unsigned int i;
> >>      int rc;
> >> @@ -285,9 +286,11 @@ static int modify_bars(const struct pci_
> >>  
> >>      /*
> >>       * Check for overlaps with other BARs. Note that only BARs that are
> >> -     * currently mapped (enabled) are checked for overlaps.
> >> +     * currently mapped (enabled) are checked for overlaps. Note also that
> >> +     * for Dom0 we also need to include hidden, i.e. DomXEN's, devices.
> >>       */
> >> -    for_each_pdev ( pdev->domain, tmp )
> >> +for ( d = pdev->domain; ; d = dom_xen ) {//todo
> > 
> > Looking at this again, I think this is slightly more complex, as during
> > runtime dom0 will get here with pdev->domain == hardware_domain OR
> > dom_xen, and hence you also need to account that devices that have
> > pdev->domain == dom_xen need to iterate over devices that belong to
> > the hardware_domain, ie:
> > 
> > for ( d = pdev->domain; ;
> >       d = (pdev->domain == dom_xen) ? hardware_domain : dom_xen )
> 
> Right, something along these lines. To keep loop continuation expression
> and exit condition simple, I'll probably prefer
> 
> for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain;
>       ; d = dom_xen )

LGTM.  I would add parentheses around the pdev->domain != dom_xen
condition, but that's just my personal taste.

We might want to add an

ASSERT(pdev->domain == hardware_domain || pdev->domain == dom_xen);

here, just to remind that this chunk must be revisited when adding
domU support (but you can also argue we haven't done this elsewhere),
I just feel here it's not so obvious we don't want do to this for
domUs.

> > And we likely want to limit this to devices that belong to the
> > hardware_domain or to dom_xen (in preparation for vPCI being used for
> > domUs).
> 
> I'm afraid I don't understand this remark, though.

This was looking forward to domU support, so that you already cater
for pdev->domain not being hardware_domain or dom_xen, but we might
want to leave that for later, when domU support is actually
introduced.

Thanks, Roger.