RE: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV incapable platform

From: Cheng, Collins
Date: Sat May 20 2017 - 06:38:24 EST

Next message: Thomas Gleixner: "[PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes"
Previous message: Joe Perches: "Re: [PATCH] lpfc: nvmet_fc: fix format string"
In reply to: Zytaruk, Kelly: "RE: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV incapable platform"
Next in thread: Zytaruk, Kelly: "RE: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV incapable platform"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Kelly,

This issue also happens in "not SR-IOV capable" SBIOS. It seems some "not SR-IOV capable" SBIOS will directly report error in system BIOS boot stage and doesn't boot to OS. But other "not SR-IOV capable" SBIOS would not report error and boot to Linux.

-Collins Cheng

-----Original Message-----
From: Zytaruk, Kelly
Sent: Saturday, May 20, 2017 6:28 PM
To: Cheng, Collins <Collins.Cheng@xxxxxxx>; Alexander Duyck <alexander.duyck@xxxxxxxxx>; Alex Williamson <alex.williamson@xxxxxxxxxx>
Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Yinghai Lu <yinghai@xxxxxxxxxx>
Subject: RE: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV incapable platform

>-----Original Message-----
>From: Cheng, Collins
>Sent: Saturday, May 20, 2017 12:53 AM
>To: Alexander Duyck; Alex Williamson
>Cc: Bjorn Helgaas; linux-pci@xxxxxxxxxxxxxxx;
>linux-kernel@xxxxxxxxxxxxxxx; Deucher, Alexander; Zytaruk, Kelly;
>Yinghai Lu
>Subject: RE: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV
>incapable platform
>
>Hi Alex,
>
>Yes, I hope kernel can disable SR-IOV and related VF resource
>allocation if the system BIOS is not SR-IOV capable.
>
>Adding the parameter "pci=nosriov" sounds a doable solution, but it
>would need user to add this parameter manually, right? I think an
>automatic detection would be better. My patch is trying to auto detect and bypass VF resource allocation.
>
>
>-Collins Cheng
>

Collins, be careful about this. I don't think that this is what we want. If you add "pci=nosriov" then you are globally disabling SRIOV for all devices. This is not the solution that we are looking for.
Remember that there are 3 types of SBIOS; "not SR-IOV capable", "SR-IOV capable but does not support large resources", "Complete SR-IOV support".

The problem is that we are trying to find a fix for "broken" SBIOS that does support SR-IOV but does not support the full SR-IOV capabilities that devices with large resources require.

Thanks,
Kelly

>
>-----Original Message-----
>From: Alexander Duyck [mailto:alexander.duyck@xxxxxxxxx]
>Sent: Friday, May 19, 2017 11:44 PM
>To: Alex Williamson <alex.williamson@xxxxxxxxxx>
>Cc: Cheng, Collins <Collins.Cheng@xxxxxxx>; Bjorn Helgaas
><bhelgaas@xxxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx; linux-
>kernel@xxxxxxxxxxxxxxx; Deucher, Alexander <Alexander.Deucher@xxxxxxx>;
>Zytaruk, Kelly <Kelly.Zytaruk@xxxxxxx>; Yinghai Lu <yinghai@xxxxxxxxxx>
>Subject: Re: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV
>incapable platform
>
>On Mon, May 15, 2017 at 10:53 AM, Alex Williamson
><alex.williamson@xxxxxxxxxx> wrote:
>> On Mon, 15 May 2017 08:19:28 +0000
>> "Cheng, Collins" <Collins.Cheng@xxxxxxx> wrote:
>>
>>> Hi Williamson,
>>>
>>> We cannot assume BIOS supports SR-IOV, actually only newer server
>motherboard BIOS supports SR-IOV. Normal desktop motherboard BIOS or
>older server motherboard BIOS doesn't support SR-IOV. This issue would
>happen if an user plugs our AMD SR-IOV capable GPU card to a normal desktop motherboard.
>>
>> Servers should be supporting SR-IOV for a long time now. What really
>> is there to a BIOS supporting SR-IOV anyway, it's simply reserving
>> sufficient bus number and MMIO resources such that we can enable the
>> VFs. This process isn't exclusively reserved for the BIOS. Some
>> platforms may choose to only initialize boot devices, leaving the
>> rest for the OS to program. The initial proposal here to disable
>> SR-IOV if not programmed at OS hand-off disables even the possibility
>> of the OS reallocating resources for this device.
>
>There are differences between supporting SR-IOV and supporting SR-IOV
>on devices with massive resources. I know I have seen NICs that will
>keep a system from completing POST if SR-IOV is enabled, and MMIO
>beyond 4G is not. My guess would be that the issues being seen are
>probably that they disable SR-IOV in the BIOS in such a setup and end
>up running into issues when they try to boot into the Linux kernel as
>it goes through and tries to allocate resources for SR-IOV even though it was disabled in the BIOS.
>
>It might make sense to add a kernel parameter something like a "pci=nosriov"
>that would allow for disabling SR-IOV and related resource allocation
>if that is what we are talking about. That way you could plug in these
>types of devices into a system with a legacy bios or that doesn't wan
>to allocate addresses above 32b for MMIO, and this parameter would be
>all that is needed to disable SR-IOV so you could plug in a NIC that has SR-IOV associated with it.
>
>>> I agree that failure to allocate VF resources should leave the
>>> device in no
>worse condition than before it tried. I hope kernel could allocate PF
>device resource before allocating VF device resource, and keep PF
>device resource valid and functional if failed to allocate VF device resource.
>>>
>>> I will send out dmesg log lspci info tomorrow. Thanks.
>>
>> Thanks,
>> Alex
>>
>>> -----Original Message-----
>>> From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
>>> Sent: Friday, May 12, 2017 10:43 PM
>>> To: Cheng, Collins <Collins.Cheng@xxxxxxx>
>>> Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx;
>>> linux-kernel@xxxxxxxxxxxxxxx; Deucher, Alexander
>>> <Alexander.Deucher@xxxxxxx>; Zytaruk, Kelly <Kelly.Zytaruk@xxxxxxx>;
>>> Yinghai Lu <yinghai@xxxxxxxxxx>
>>> Subject: Re: [PATCH] PCI: Make SR-IOV capable GPU working on the
>>> SR-IOV incapable platform
>>>
>>> On Fri, 12 May 2017 04:51:43 +0000
>>> "Cheng, Collins" <Collins.Cheng@xxxxxxx> wrote:
>>>
>>> > Hi Williamson,
>>> >
>>> > I verified the patch is working for both AMD SR-IOV GPU and Intel
>>> > SR-IOV
>NIC. I don't think it is redundant to check the VF BAR valid before
>call sriov_init(), it is safe and saving boot time, also there is no a
>better method to know if system BIOS has correctly initialized the SR-IOV capability or not.
>>>
>>> It also masks an underlying bug and creates a maintenance issue that
>>> we won't
>know when it's safe to remove this workaround. I don't think faster
>boot is valid rationale, in one case SR-IOV is completely disabled, the
>other we attempt to allocate the resources the BIOS failed to provide.
>I expect this is also a corner case, the BIOS should typically support
>SR-IOV, therefore this situation should be an exception.
>>>
>>> > I did not try to fix the issue from the kernel resource allocation
>>> > perspective,
>it is because:
>>> > 1. I am not very familiar with the PCI resource allocation scheme in kernel.
>For example, in sriov_init(), kernel will re-assign the PCI resource
>for both VF and PF. I don't understand why kernel allocates resource
>for VF firstly, then PF. If it is PF firstly, then this issue could be avoided.
>>> > 2. I am not sure if kernel has error handler if PCI resource allocation failed.
>In this case, kernel cannot allocate enough resource to PF. It should
>trigger some error handler to either just keep original BAR values set
>by system BIOS, or disable this device and log errors.
>>>
>>> I think these are the issues we should be trying to solve and I'm
>>> sure folks on the linux-pci list can help us identify the bug.
>>> Minimally, failure to allocate VF resources should leave the device
>>> in no worse condition than before it tried. Perhaps you could post
>>> more details about the issue, boot with pci=earlydump, post dmesg of
>>> a boot where the PF resources are incorrectly re-allocated, and
>>> include lspci -vvv for the SR-IOV device. Also, please test with
>>> the latest upstream kernel, upstream only patches old kernels
>>> through stable backports of commits to the latest kernel. Adding
>>> Yinghai as a resource allocation expert. Thanks,
>>>
>>> Alex
>>>
>>> > -----Original Message-----
>>> > From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
>>> > Sent: Friday, May 12, 2017 12:01 PM
>>> > To: Cheng, Collins <Collins.Cheng@xxxxxxx>
>>> > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>;
>>> > linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Deucher,
>>> > Alexander <Alexander.Deucher@xxxxxxx>; Zytaruk, Kelly
>>> > <Kelly.Zytaruk@xxxxxxx>
>>> > Subject: Re: [PATCH] PCI: Make SR-IOV capable GPU working on the
>>> > SR-IOV incapable platform
>>> >
>>> > On Fri, 12 May 2017 03:42:46 +0000 "Cheng, Collins"
>>> > <Collins.Cheng@xxxxxxx> wrote:
>>> >
>>> > > Hi Williamson,
>>> > >
>>> > > GPU card needs more BAR aperture resource than other PCI
>>> > > devices. For
>example, Intel SR-IOV network card only require 512KB memory resource
>for all VFs. AMD SR-IOV GPU card needs 256MB x16 VF = 4GB memory
>resource for frame buffer BAR aperture.
>>> > >
>>> > > If the system BIOS supports SR-IOV, it will reserve enough
>>> > > resource for all
>VF BARs. If the system BIOS doesn't support SR-IOV or cannot allocate
>the enough resource for VF BARs, only PF BAR will be assigned and VF
>BARs are empty. Then system boots to Linux kernel and kernel doesn't
>check if the VF BARs are empty or valid. Kernel will re-assign the BAR
>resources for PF and all VFs. The problem I saw is that kernel will
>fail to allocate PF BAR resource because some resources are assigned to
>VF, this is not expected. So kernel might need to do some check before
>re-assign the PF/VF resource, so that PF device will be correctly assigned BAR resource and user can use PF device.
>>> >
>>> > So the problem is that something bad happens when the kernel is
>>> > trying to reallocate resources in order to fulfill the
>>> > requirements of the VFs, leaving the PF resources incorrectly
>>> > programmed? Why not just fix that bug rather than creating
>>> > special handling for this vendor/class of device which disables
>>> > any attempt to fixup resources for SR-IOV? IOW, this patch just
>>> > avoids the problem for your devices rather than fixing the bug.
>>> > I'd suggest fixing the bug such that the PF is left in a
>>> > functional state if the kernel is unable to allocate sufficient
>>> > resources for the VFs. Thanks,
>>> >
>>> > Alex
>>> >
>>> > > -----Original Message-----
>>> > > From: Alex Williamson [mailto:alex.williamson@xxxxxxxxxx]
>>> > > Sent: Friday, May 12, 2017 11:21 AM
>>> > > To: Cheng, Collins <Collins.Cheng@xxxxxxx>
>>> > > Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>;
>>> > > linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
>>> > > Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Zytaruk, Kelly
>>> > > <Kelly.Zytaruk@xxxxxxx>
>>> > > Subject: Re: [PATCH] PCI: Make SR-IOV capable GPU working on the
>>> > > SR-IOV incapable platform
>>> > >
>>> > > On Fri, 12 May 2017 02:50:32 +0000 "Cheng, Collins"
>>> > > <Collins.Cheng@xxxxxxx> wrote:
>>> > >
>>> > > > Hi Helgaas,
>>> > > >
>>> > > > Some AMD GPUs have hardware support for graphics SR-IOV.
>>> > > > If the SR-IOV capable GPU is plugged into the SR-IOV incapable
>>> > > > platform. It would cause a problem on PCI resource allocation
>>> > > > in current Linux kernel.
>>> > > >
>>> > > > Therefore in order to allow the PF (Physical Function) device
>>> > > > of SR-IOV capable GPU to work on the SR-IOV incapable
>>> > > > platform, it is required to verify conditions for initializing
>>> > > > BAR resources on AMD SR-IOV capable GPUs.
>>> > > >
>>> > > > If the device is an AMD graphics device and it supports SR-IOV
>>> > > > it will require a large amount of resources.
>>> > > > Before calling sriov_init() must ensure that the system BIOS
>>> > > > also supports SR-IOV and that system BIOS has been able to
>>> > > > allocate enough resources.
>>> > > > If the VF BARs are zero then the system BIOS does not support
>>> > > > SR-IOV or it could not allocate the resources and this
>>> > > > platform will not support AMD graphics SR-IOV.
>>> > > > Therefore do not call sriov_init().
>>> > > > If the system BIOS does support SR-IOV then the VF BARs will
>>> > > > be properly initialized to non-zero values.
>>> > > >
>>> > > > Below is the patch against to Kernel 4.8 & 4.9. Please review.
>>> > > >
>>> > > > I checked the drivers/pci/quirks.c, it looks the
>>> > > > workarounds/fixes in quirks.c are for specific devices and one
>>> > > > or more device ID are defined for the specific devices.
>>> > > > However my patch is for all AMD SR-IOV capable GPUs, that
>>> > > > includes all existing
>and future AMD server GPUs.
>>> > > > So it doesn't seem like a good fit to put the fix in quirks.c.
>>> > >
>>> > >
>>> > > Why is an AMD graphics card unique here? Doesn't sriov_init()
>>> > > always need to be able to deal with devices of any type where
>>> > > the BIOS hasn't initialized the SR-IOV capability? Some SR-IOV
>>> > > devices can fit their VFs within a minimum bridge aperture, most
>>> > > cannot. I don't understand why the VF resource requirements
>>> > > being exceptionally large dictates that they receive special handling.
>>> > > Thanks,
>>> > >
>>> > > Alex
>>> > >
>>> > > > Signed-off-by: Collins Cheng <collins.cheng@xxxxxxx>
>>> > > > ---
>>> > > > drivers/pci/iov.c | 63
>>> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++---
>>> > > > 1 file changed, 60 insertions(+), 3 deletions(-)
>>> > > >
>>> > > > diff --git a/drivers/pci/iov.c b/drivers/pci/iov.c index
>>> > > > e30f05c..e4f1405 100644
>>> > > > --- a/drivers/pci/iov.c
>>> > > > +++ b/drivers/pci/iov.c
>>> > > > @@ -523,6 +523,45 @@ static void sriov_restore_state(struct
>>> > > > pci_dev
>*dev)
>>> > > > msleep(100);
>>> > > > }
>>> > > >
>>> > > > +/*
>>> > > > + * pci_vf_bar_valid - check if VF BARs have resource
>>> > > > +allocated
>>> > > > + * @dev: the PCI device
>>> > > > + * @pos: register offset of SR-IOV capability in PCI config
>>> > > > +space
>>> > > > + * Returns true any VF BAR has resource allocated, false
>>> > > > + * if all VF BARs are empty.
>>> > > > + */
>>> > > > +static bool pci_vf_bar_valid(struct pci_dev *dev, int pos) {
>>> > > > + int i;
>>> > > > + u32 bar_value;
>>> > > > + u32 bar_size_mask = ~(PCI_BASE_ADDRESS_SPACE |
>>> > > > + PCI_BASE_ADDRESS_MEM_TYPE_64 |
>>> > > > + PCI_BASE_ADDRESS_MEM_PREFETCH);
>>> > > > +
>>> > > > + for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>>> > > > + pci_read_config_dword(dev, pos + PCI_SRIOV_BAR
>>> > > > + + i * 4,
>&bar_value);
>>> > > > + if (bar_value & bar_size_mask)
>>> > > > + return true;
>>> > > > + }
>>> > > > +
>>> > > > + return false;
>>> > > > +}
>>> > > > +
>>> > > > +/*
>>> > > > + * is_amd_display_adapter - check if it is an AMD/ATI GPU
>>> > > > +device
>>> > > > + * @dev: the PCI device
>>> > > > + *
>>> > > > + * Returns true if device is an AMD/ATI display adapter,
>>> > > > + * otherwise return false.
>>> > > > + */
>>> > > > +
>>> > > > +static bool is_amd_display_adapter(struct pci_dev *dev) {
>>> > > > + return (((dev->class >> 16) == PCI_BASE_CLASS_DISPLAY) &&
>>> > > > + (dev->vendor == PCI_VENDOR_ID_ATI ||
>>> > > > + dev->vendor == PCI_VENDOR_ID_AMD)); }
>>> > > > +
>>> > > > /**
>>> > > > * pci_iov_init - initialize the IOV capability
>>> > > > * @dev: the PCI device
>>> > > > @@ -537,9 +576,27 @@ int pci_iov_init(struct pci_dev *dev)
>>> > > > return -ENODEV;
>>> > > >
>>> > > > pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_SRIOV);
>>> > > > - if (pos)
>>> > > > - return sriov_init(dev, pos);
>>> > > > -
>>> > > > + if (pos) {
>>> > > > + /*
>>> > > > + * If the device is an AMD graphics device and it supports
>>> > > > + * SR-IOV it will require a large amount of resources.
>>> > > > + * Before calling sriov_init() must ensure that the system
>>> > > > + * BIOS also supports SR-IOV and that system BIOS has been
>>> > > > + * able to allocate enough resources.
>>> > > > + * If the VF BARs are zero then the system BIOS does not
>>> > > > + * support SR-IOV or it could not allocate the resources
>>> > > > + * and this platform will not support AMD graphics SR-IOV.
>>> > > > + * Therefore do not call sriov_init().
>>> > > > + * If the system BIOS does support SR-IOV then the VF BARs
>>> > > > + * will be properly initialized to non-zero values.
>>> > > > + */
>>> > > > + if (is_amd_display_adapter(dev)) {
>>> > > > + if (pci_vf_bar_valid(dev, pos))
>>> > > > + return sriov_init(dev, pos);
>>> > > > + } else {
>>> > > > + return sriov_init(dev, pos);
>>> > > > + }
>>> > > > + }
>>> > > > return -ENODEV;
>>> > > > }
>>> > > >
>>> > >
>>> >
>>>
>>

Next message: Thomas Gleixner: "[PATCH] slub/memcg: Cure the brainless abuse of sysfs attributes"
Previous message: Joe Perches: "Re: [PATCH] lpfc: nvmet_fc: fix format string"
In reply to: Zytaruk, Kelly: "RE: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV incapable platform"
Next in thread: Zytaruk, Kelly: "RE: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV incapable platform"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]