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

From: Stefano Stabellini <sstabellini@kernel.org>
To: Julien Grall <julien@xen.org>
Cc: "Bertrand Marquis" <Bertrand.Marquis@arm.com>,
	Xen-devel <xen-devel@lists.xenproject.org>,
	"iwj@xenproject.org" <iwj@xenproject.org>,
	"sstabellini@kernel.org" <sstabellini@kernel.org>,
	"Rahul Singh" <Rahul.Singh@arm.com>,
	"Volodymyr Babchuk" <Volodymyr_Babchuk@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Jan Beulich" <jbeulich@suse.com>, "Wei Liu" <wl@xen.org>,
	"Paul Durrant" <paul@xen.org>,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v8 2/5] xen/arm: Enable the existing x86 virtual PCI support for ARM
Date: Fri, 15 Oct 2021 12:47:17 -0700 (PDT)	[thread overview]
Message-ID: <alpine.DEB.2.21.2110151235100.31303@sstabellini-ThinkPad-T480s> (raw)
In-Reply-To: <a45077dc-e144-4427-9ae2-5815045ecaec@xen.org>

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@xen.org> 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 :-)