Re: [PATCH RFC] vPCI: account for hidden devices in modify_bars()

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

On 31.08.21 09:51, Jan Beulich wrote:
> On 31.08.2021 07:35, Oleksandr Andrushchenko wrote:
> > Hello, Jan!
> >
> > On 30.08.21 16:04, Jan Beulich wrote:
> > > Hidden devices (e.g. an add-in PCI serial card used for Xen's serial
> > > console) are associated with DomXEN, not Dom0. This means that while
> > > looking for overlapping BARs such devices cannot be found on Dom0's
> > > list of devices; DomXEN's list also needs to be scanned.
> > >
> > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > ---
> > > RFC: Patch intentionally mis-formatted, as the necessary re-indentation
> > >        would make the diff difficult to read. At this point I'd merely
> > >        like to gather input towards possible better approaches to solve
> > >        the issue (not the least because quite possibly there are further
> > >        places needing changing).
> > >
> > > --- a/xen/drivers/vpci/header.c
> > > +++ b/xen/drivers/vpci/header.c
> > > @@ -206,6 +206,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;
> > > @@ -265,7 +266,8 @@ 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.
> > >         */
> > > -    for_each_pdev ( pdev->domain, tmp )
> > > +for ( d = pdev->domain; ; d = dom_xen ) {//todo
> > I am not quite sure this will be correct for the cases where pdev->domain != 
> > dom0,
> > e.g. in the series for PCI passthrough for Arm this can be any guest. For such 
> > cases
> > we'll force running the loop for dom_xen which I am not sure is desirable.
> It is surely not desirable, but it also doesn't happen - see the
> is_hardware_domain() check further down (keeping context below).
Right
> > Another question is why such a hidden device has its pdev->domain not set 
> > correctly,
> > so we need to work this around?
> Please see _setup_hwdom_pci_devices() and commit e46ea4d44dc0
> ("PCI: don't allow guest assignment of devices used by Xen")
> introducing that temporary override. To permit limited
> visibility to Dom0, these devices still need setting up in the
> IOMMU for Dom0. Consequently BAR overlap detection also needs
> to take these into account (i.e. the goal here is not just to
> prevent triggering the ASSERT() in question).
So, why don't we set pdev->domain = dom_xen for such devices and call

modify_bars or something from pci_hide_device for instance (I didn't get too

much into implementation details though)? If pci_hide_device already handles

such exceptions, so it should also take care of the correct BAR overlaps etc.

Otherwise it looks like we put some unrelated logic into vpci which is for 
hiding

the devices (on x86).

Thank you,

Oleksandr

> Jan
>
> > > @@ -308,6 +311,7 @@ static int modify_bars(const struct pci_
> > >                }
> > >            }
> > >        }
> > > +if ( !is_hardware_domain(d) ) break; }//todo
> > >    
> > >        ASSERT(dev);
> > >