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

From: Will Deacon <will@kernel.org>
To: Vivek Kumar Gautam <vivek.gautam@arm.com>
Cc: Alexandru Elisei <alexandru.elisei@arm.com>,
	julien.thierry.kdev@gmail.com, kvm@vger.kernel.org,
	andre.przywara@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: Mon, 4 Oct 2021 10:47:13 +0100	[thread overview]
Message-ID: <20211004094713.GB27173@willie-the-truck> (raw)
In-Reply-To: <867e8db7-c173-5ad2-dca4-69085c89d956@arm.com>

On Wed, Sep 08, 2021 at 03:02:20PM +0530, Vivek Kumar Gautam wrote:
> On 9/3/21 8:45 PM, Alexandru Elisei wrote:
> > On 9/2/21 11:48 AM, Vivek Kumar Gautam wrote:
> > > On 9/2/21 3:29 PM, Alexandru Elisei wrote:
> > I think I found the card that doesn't work when overwriting the extended device
> > configuration space. I tried device assignment with a Realtek 8168 Gigabit
> > Ethernet card on a Seattle machine, and the host freezes when I try to start a VM.
> > Even after reset, the machine doesn't boot anymore and it gets stuck during the
> > boot process at this message:
> > 
> > NewPackageList status: EFI_SUCCESS
> > BDS.SignalConnectDriversEvent(feeb6d60)
> > BDS.ConnectRootBridgeHandles(feeb6db0)
> > 
> > It doesn't go away no matter how many times I reset the machine, to get it booting
> > again I have to pull the plug and plug it again. I tried assigning the device to a
> > VM several times, and this happened every time. The card doesn't have the caps
> > that you added, this is caused entirely by the config space write (tried it with
> > only the config space change).
> > 
> > It could be a problem kvmtool, with Linux or with the machine, but this is the
> > only machine where device assignment works and I would like to keep it working
> > with this NIC.

There is at least a problem with the machine/firmware if you can get it
into this state.

> Sorry for the delay in responding. I took sometime off work.
> Sure, we will try to keep your machine working :)
> 
> > 
> > One solution I see is to add a field to vfio_pci_device (something like has_pcie),
> > and based on that, vfio_pci_fixup_cfg_space() could overwrite only the first 256
> > bytes or the entire device configuration space.
> 
> Does the card support PCI extended caps (as seen from the PCI spec v5.0
> section-7.5)?
> If no, then I guess the check that I am planning to add - to check if
> the device supports extended Caps - can help here. Since we would add
> extended caps based on the mentioned check, it seems only valid to have
> that check before overwriting the configuration space.
> 
> > 
> > It's also not clear to me what you are trying to achieve with this patch. Is there
> > a particular device that you want to get working? Or an entire class of devices
> > which have those features? If it's the former, you could have the size of the
> > config space write depend on the vendor + device id. If it's the latter, we could
> > key the size of the config space write based on the presence of those particular
> > PCIE caps and try and fix other devices if they break.
> 
> Absolutely, we can check for the presence of PCI extended capabilities
> and based on that write the configuration space. If the device has issue
> with only a specific extended capability we can try to fix that by
> keying the DevID-VendorID pair? What do you think?
> 
> > 
> > Will, Andre, do you see other solutions? Do you have a preference?
> 
> Will, Andre, please let me know as well if you have any preferences.

If it's straightforward to keep this working on Seattle, then let's do it,
but I don't think we should bend over backwards to support device assignment
on a dead platform (and I say that as a regular user of one of these
things!)

Will