Message-ID: <BN6PR12MB124962728D0868220428C935ECFA0@BN6PR12MB1249.namprd12.prod.outlook.com>
Date:   Sat, 20 May 2017 10:37:39 +0000
From:   "Cheng, Collins" <Collins.Cheng@....com>
To:     "Zytaruk, Kelly" <Kelly.Zytaruk@....com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Alex Williamson <alex.williamson@...hat.com>
CC:     Bjorn Helgaas <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        Yinghai Lu <yinghai@...nel.org>
Subject: RE: [PATCH] PCI: Make SR-IOV capable GPU working on the SR-IOV
 incapable platform

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@....com>; Alexander Duyck <alexander.duyck@...il.com>; Alex Williamson <alex.williamson@...hat.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>; linux-pci@...r.kernel.org; linux-kernel@...r.kernel.org; Deucher, Alexander <Alexander.Deucher@....com>; Yinghai Lu <yinghai@...nel.org>
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@...r.kernel.org; 
>linux-kernel@...r.kernel.org; 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@...il.com]
>Sent: Friday, May 19, 2017 11:44 PM
>To: Alex Williamson <alex.williamson@...hat.com>
>Cc: Cheng, Collins <Collins.Cheng@....com>; Bjorn Helgaas 
><bhelgaas@...gle.com>; linux-pci@...r.kernel.org; linux- 
>kernel@...r.kernel.org; Deucher, Alexander <Alexander.Deucher@....com>; 
>Zytaruk, Kelly <Kelly.Zytaruk@....com>; Yinghai Lu <yinghai@...nel.org>
>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@...hat.com> wrote:
>> On Mon, 15 May 2017 08:19:28 +0000
>> "Cheng, Collins" <Collins.Cheng@....com> 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@...hat.com]
>>> Sent: Friday, May 12, 2017 10:43 PM
>>> To: Cheng, Collins <Collins.Cheng@....com>
>>> Cc: Bjorn Helgaas <bhelgaas@...gle.com>; linux-pci@...r.kernel.org; 
>>> linux-kernel@...r.kernel.org; Deucher, Alexander 
>>> <Alexander.Deucher@....com>; Zytaruk, Kelly <Kelly.Zytaruk@....com>; 
>>> Yinghai Lu <yinghai@...nel.org>
>>> 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@....com> 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@...hat.com]
>>> > Sent: Friday, May 12, 2017 12:01 PM
>>> > To: Cheng, Collins <Collins.Cheng@....com>
>>> > Cc: Bjorn Helgaas <bhelgaas@...gle.com>; 
>>> > linux-pci@...r.kernel.org; linux-kernel@...r.kernel.org; Deucher, 
>>> > Alexander <Alexander.Deucher@....com>; Zytaruk, Kelly 
>>> > <Kelly.Zytaruk@....com>
>>> > 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@....com> 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@...hat.com]
>>> > > Sent: Friday, May 12, 2017 11:21 AM
>>> > > To: Cheng, Collins <Collins.Cheng@....com>
>>> > > Cc: Bjorn Helgaas <bhelgaas@...gle.com>; 
>>> > > linux-pci@...r.kernel.org; linux-kernel@...r.kernel.org; 
>>> > > Deucher, Alexander <Alexander.Deucher@....com>; Zytaruk, Kelly 
>>> > > <Kelly.Zytaruk@....com>
>>> > > 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@....com> 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@....com>
>>> > > > ---
>>> > > >  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;
>>> > > >  }
>>> > > >
>>> > >
>>> >
>>>
>>

Powered by blists - more mailing lists