Re: [PATCH] vfio/pci: Add support for PCIe extended capabilities

From: Will Deacon <will@kernel.org>
To: Andre Przywara <andre.przywara@arm.com>
Cc: Vivek Gautam <vivek.gautam@arm.com>,
	julien.thierry.kdev@gmail.com, kvm@vger.kernel.org,
	alexandru.elisei@arm.com, lorenzo.pieralisi@arm.com,
	jean-philippe@linaro.org, eric.auger@redhat.com
Subject: Re: [PATCH] vfio/pci: Add support for PCIe extended capabilities
Date: Wed, 1 Sep 2021 11:44:52 +0100	[thread overview]
Message-ID: <20210901104451.GA1023@willie-the-truck> (raw)
In-Reply-To: <4f5307cf-0cea-461a-838f-85e82805c499@arm.com>

On Wed, Sep 01, 2021 at 11:27:21AM +0100, Andre Przywara wrote:
> On 8/31/21 3:54 PM, Will Deacon wrote:
> 
> Hi Will,
> 
> > On Tue, Aug 10, 2021 at 11:55:14AM +0530, Vivek Gautam wrote:
> > > Add support to parse extended configuration space for vfio based
> > > assigned PCIe devices and add extended capabilities for the device
> > > in the guest. This allows the guest to see and work on extended
> > > capabilities, for example to toggle PRI extended cap to enable and
> > > disable Shared virtual addressing (SVA) support.
> > > PCIe extended capability header that is the first DWORD of all
> > > extended caps is shown below -
> > > 
> > >     31               20  19   16  15                 0
> > >     ____________________|_______|_____________________
> > >    |    Next cap off    |  1h   |     Cap ID          |
> > >    |____________________|_______|_____________________|
> > > 
> > > Out of the two upper bytes of extended cap header, the
> > > lower nibble is actually cap version - 0x1.
> > > 'next cap offset' if present at bits [31:20], should be
> > > right shifted by 4 bits to calculate the position of next
> > > capability.
> > > This change supports parsing and adding ATS, PRI and PASID caps.
> > > 
> > > Signed-off-by: Vivek Gautam <vivek.gautam@arm.com>
> > > ---
> > >   include/kvm/pci.h |   6 +++
> > >   vfio/pci.c        | 104 ++++++++++++++++++++++++++++++++++++++++++----
> > >   2 files changed, 103 insertions(+), 7 deletions(-)
> > 
> > Does this work correctly for architectures which don't define ARCH_HAS_PCI_EXP?
> 
> I think it does: the code compiles fine, and the whole functionality is
> guarded by:
> +	/* Extended cap only for PCIe devices */
> +	if (!arch_has_pci_exp())
> +		return 0;
> 
> A clever compiler might even decide to not include this code at all.
> 
> Did you see any particular problem?

The part I was worried about is that PCI_DEV_CFG_MASK (which is used by
the cfg space dispatch code) is derived from PCI_DEV_CFG_SIZE, but actually
I think this patch might _fix_ that problem because it removes the explicit
usage of PCI_DEV_CFG_SIZE_LEGACY!

Will