Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 15 Oct 2021, Julien Grall wrote:
> On 15/10/2021 18:33, Bertrand Marquis wrote:
> > > On 15 Oct 2021, at 18:25, Julien Grall <julien@xxxxxxx> wrote:
> > > 
> > > Hi Bertrand,
> > > 
> > > On 15/10/2021 17:51, Bertrand Marquis wrote:
> > > > diff --git a/xen/drivers/passthrough/pci.c
> > > > b/xen/drivers/passthrough/pci.c
> > > > index 3aa8c3175f..35e0190796 100644
> > > > --- a/xen/drivers/passthrough/pci.c
> > > > +++ b/xen/drivers/passthrough/pci.c
> > > > @@ -756,6 +756,19 @@ int pci_add_device(u16 seg, u8 bus, u8 devfn,
> > > >       if ( !pdev->domain )
> > > >       {
> > > >           pdev->domain = hardware_domain;
> > > > +#ifdef CONFIG_ARM
> > > > +        /*
> > > > +         * On ARM PCI devices discovery will be done by Dom0. Add vpci
> > > > handler
> > > > +         * when Dom0 inform XEN to add the PCI devices in XEN.
> > > > +         */
> > > > +        ret = vpci_add_handlers(pdev);
> > > 
> > > I don't seem to find the code to remove __init_hwdom in this series. Are
> > > you intending to fix it separately?
> > 
> > Yes I think it is better to fix that in a new patch as it will require some
> > discussion as it will impact the x86 code if I just remove the flag now.
> For the future patch series, may I ask to keep track of outstanding issues in
> the commit message (if you don't plan to address them before commiting) or
> after --- (if they are meant to be addressed before commiting)?
> 
> In this case, the impact on Arm is this would result to an hypervisor crash if
> called. If we drop __init_hwdom, the impact on x86 is Xen text will slightly
> be bigger after the boot time.
> 
> AFAICT, the code is not reachable on Arm (?). Therefore, one could argue we
> this can wait after the week-end as this is a latent bug. Yet, I am not really
> comfortable to see knowningly buggy code merged.
> 
> Stefano, would you be willing to remove __init_hwdom while committing it? If
> not, can you update the commit message and mention this patch doesn't work as
> intended?

I prefer not to do a change like this on commit as it affects x86.

I added a note in the commit message about it. I also added Roger's ack
that was given to the previous version. FYI this is the only outstanding
TODO as far as I am aware (there are other pending patch series of
course).

After reviewing the whole series again, checking it against all the
reviewers comments, and making it go through gitlab-ci, I committed the
series.

Thank you all for all the efforts that went into this. We made it :-)