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

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

On Tue, May 30, 2023 at 10:45:09AM +0200, Jan Beulich wrote:
> On 29.05.2023 10:08, Roger Pau Monné wrote:
> > On Thu, May 25, 2023 at 05:30:54PM +0200, Jan Beulich wrote:
> >> On 25.05.2023 17:02, Roger Pau Monné wrote:
> >>> 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.
> >>
> >> I could add such an assertion, if only ...
> >>
> >>>>> 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.
> >>
> >> ... I understood why this checking doesn't apply to DomU-s as well,
> >> in your opinion.
> > 
> > It's my understanding that domUs can never get hidden or read-only
> > devices assigned, and hence there no need to check for overlap with
> > devices assigned to dom_xen, as those cannot have any BARs mapped in
> > a domU physmap.
> > 
> > So for domUs the overlap check only needs to be performed against
> > devices assigned to pdev->domain.
> 
> I fully agree, but the assertion you suggested doesn't express that. Or
> maybe I'm misunderstanding what you did suggest, and there was an
> implication of some further if() around it.

Maybe I'm getting myself confused, but if you add something like:

for ( d = pdev->domain != dom_xen ? pdev->domain : hardware_domain;
      ; d = dom_xen )

Such loop would need to be avoided for domUs, so my suggestion was to
add the assert in order to remind us that the loop would need
adjusting if we ever add domU support.  But maybe you had already
plans to restrict the loop to dom0 only.

Thanks, Roger.