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

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

On Fri, Oct 15, 2021 at 12:47:17PM -0700, Stefano Stabellini wrote:
> 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.

Hello,

Maybe I'm being pedantic, or there was some communication outside the
mailing list, but I think strictly speaking you are missing an Ack
from either Jan or Paul for the xen/drivers/passthrough/pci.c change.

IMHO seeing how that chunk moved from 3 different places in just one
afternoon also doesn't give me a lot of confidence. It's Arm only code
at the end, so it's not going to effect the existing x86 support and
I'm not specially worried, but I would like to avoid having to move it
again.

Regards, Roger.