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.
Thanks, Roger.
