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

[Julien Grall <julien@xxxxxxx>]

On 15/10/2021 18:33, Bertrand Marquis wrote:
> Hi Julien,
Hi Bertrand,

> > 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?
Cheers,

--
Julien Grall