Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM.

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Jan Beulich <jbeulich@suse.com>
Cc: Ian Jackson <iwj@xenproject.org>,
	Bertrand Marquis <Bertrand.Marquis@arm.com>,
	Rahul Singh <Rahul.Singh@arm.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Julien Grall <julien@xen.org>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>, Wei Liu <wl@xen.org>,
	Paul Durrant <paul@xen.org>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM.
Date: Fri, 15 Oct 2021 13:35:15 +0200	[thread overview]
Message-ID: <YWlnc3b0sj4akUWa@MacBook-Air-de-Roger.local> (raw)
In-Reply-To: <b8fc68e6-2bba-ef11-4c4a-50c2046ecb04@suse.com>

On Fri, Oct 15, 2021 at 12:18:59PM +0200, Jan Beulich wrote:
> On 15.10.2021 12:14, Ian Jackson wrote:
> > Bertrand Marquis writes ("Re: [PATCH v6 2/3] xen/arm: Enable the existing x86 virtual PCI support for ARM."):
> >>> On 15 Oct 2021, at 09:00, Jan Beulich <jbeulich@suse.com> wrote:
> >>> The latter is fine to be put here (i.e. FTAOD I'm fine with it
> >>> staying here). For the former I even question its original placement
> >>> in asm-x86/pci.h: It's not generally correct as per the PCI spec, as
> >>> the bus portion of the address can be anywhere from 1 to 8 bits. And
> >>> in fact there is a reason why this macro was/is used in only a
> >>> single place, but not e.g. in x86'es handling of physical MCFG. It
> >>> is merely an implementation choice in vPCI that the entire segment 0
> >>> has a linear address range covering all 256 buses. Hence I think
> >>> this wants to move to xen/vpci.h and then perhaps also be named
> >>> VPCI_ECAM_BDF().
> >>
> >> On previous version it was request to renamed this to ECAM and agreed
> >> to put is here. Now you want me to rename it to VPCI and move it again.
> >> I would like to have a confirmation that this is ok and the final move if possible.
> >>
> >> @Roger can you confirm this is what is wanted ?
> > 
> > I think Roger is not available today I'm afraid.
> > 
> > Bertrand, can you give me a link to the comment from Roger ?
> > Assuming that it says what I think it will say:
> > 
> > I think the best thing to do will be to leave the name as it was in
> > the most recent version of your series.  I don't think it makes sense
> > to block this patch over a naming disagreement.  And it would be best
> > to minimise unnecessary churn.
> > 
> > I would be happy to release-ack a name change (perhaps proposed bo Jan
> > or Roger) supposing that that is the ultimate maintainer consensus.
> > 
> > Jan, would that approach be OK with you ?
> 
> Well, yes, if a subsequent name change is okay, then I could live with
> that. I'd still find it odd to rename a function immediately after it
> already got renamed. As expressed elsewhere, I suspect in his request
> Roger did not pay attention to a use of the function in non-ECAM code.

Using MMCFG_BDF was original requested by Julien, not myself I think:

https://lore.kernel.org/xen-devel/a868e1e7-8400-45df-6eaa-69f1e2c99383@xen.org/

I'm slightly loss in so many messages. On x86 we subtract the MCFG
start address from the passed one before getting the BDF, and then we
add the startting bus address passed in the ACPI table. This is so far
not need on Arm AFAICT because of the fixed nature of the selected
virtual ECAM region.

Thanks, Roger.