Re: [Qemu-devel] [for-4.0 PATCH v3 5/9] pcie: Fill PCIESlot link fields to support higher speeds and widths

From: Auger Eric <eric.auger@redhat.com>
To: Alex Williamson <alex.williamson@redhat.com>
Cc: qemu-devel@nongnu.org, Geoffrey McRae <geoff@hostfission.com>,
	"Michael S. Tsirkin" <mst@redhat.com>
Subject: Re: [Qemu-devel] [for-4.0 PATCH v3 5/9] pcie: Fill PCIESlot link fields to support higher speeds and widths
Date: Thu, 6 Dec 2018 17:35:30 +0100	[thread overview]
Message-ID: <0ecea9bd-1f2c-ed23-5288-ea1595ebc99f@redhat.com> (raw)
In-Reply-To: <20181206090044.7f22a365@x1.home>

Hi Alex,

On 12/6/18 5:00 PM, Alex Williamson wrote:
> On Thu, 6 Dec 2018 12:08:09 +0100
> Auger Eric <eric.auger@redhat.com> wrote:
> 
>> Hi Alex,
>>
>> On 12/4/18 5:26 PM, Alex Williamson wrote:
>>> Make use of the PCIESlot speed and width fields to update link
>>> information beyond those configured in pcie_cap_v1_fill().  This is
>>> only called for devices supporting a version 2 capability and
>>> automatically skips any non-PCIESlot devices.  Only devices with
>>> increased link values generate any visible config space differences.
>>>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
>>> Tested-by: Geoffrey McRae <geoff@hostfission.com>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>  hw/pci/pcie.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 72 insertions(+)
>>>
>>> diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
>>> index 61b7b96c52cd..556ec19925b9 100644
>>> --- a/hw/pci/pcie.c
>>> +++ b/hw/pci/pcie.c
>>> @@ -27,6 +27,7 @@
>>>  #include "hw/pci/msi.h"
>>>  #include "hw/pci/pci_bus.h"
>>>  #include "hw/pci/pcie_regs.h"
>>> +#include "hw/pci/pcie_port.h"
>>>  #include "qemu/range.h"
>>>  
>>>  //#define DEBUG_PCIE
>>> @@ -87,6 +88,74 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
>>>      pci_set_word(cmask + PCI_EXP_LNKSTA, 0);
>>>  }
>>>  
>>> +static void pcie_cap_fill_slot_lnk(PCIDevice *dev)
>>> +{
>>> +    PCIESlot *s = (PCIESlot *)object_dynamic_cast(OBJECT(dev), TYPE_PCIE_SLOT);
>>> +    uint8_t *exp_cap = dev->config + dev->exp.exp_cap;
>>> +
>>> +    /* Skip anything that isn't a PCIESlot */
>>> +    if (!s) {
>>> +        return;
>>> +    }
>>> +
>>> +    /* Clear and fill LNKCAP from what was configured above */
>>> +    pci_long_test_and_clear_mask(exp_cap + PCI_EXP_LNKCAP,
>>> +                                 PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
>>> +    pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
>>> +                               QEMU_PCI_EXP_LNKCAP_MLW(s->width) |
>>> +                               QEMU_PCI_EXP_LNKCAP_MLS(s->speed));
>>> +
>>> +    /*
>>> +     * Link bandwidth notification is required for all root ports and
>>> +     * downstream ports supporting links wider than x1.  
>>
>>
>>> +     */
>>> +    if (s->width > QEMU_PCI_EXP_LNK_X1) {
>>> +        pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
>>> +                                   PCI_EXP_LNKCAP_LBNC);  
>> spec also says "This capability is required for all Root
>> Ports and Switch Downstream Ports supporting Links wider than
>> x1 and/or multiple Link speeds."
>>
>> I see below you are likely to report several speed vectors if speed >
>> 5GT/s so you may also add a test on the speed.
> 
> Good catch, so I'll change the above test to:
> 
>     if (s->width > QEMU_PCI_EXP_LNK_X1 ||
>         s->speed > QEMU_PCI_EXP_LNK_2_5GT) {
> 
>> How are the device types checked?
> 
> Via the object_dynamic_cast() at the top of the function, we'll drop
> anything that isn't a descendant of TYPE_PCIE_SLOT.

Ah OK
> 
>>> +    }
>>> +
>>> +    if (s->speed > QEMU_PCI_EXP_LNK_2_5GT) {
>>> +        /*
>>> +         * Hot-plug capable downstream ports and downstream ports supporting
>>> +         * link speeds greater than 5GT/s must hardwire PCI_EXP_LNKCAP_DLLLARC
>>> +         * to 1b.  PCI_EXP_LNKCAP_DLLLARC implies PCI_EXP_LNKSTA_DLLLA, which
>>> +         * we also hardwire to 1b here.  2.5GT/s hot-plug slots should also
>>> +         * technically implement this, but it's not done here for compatibility.
>>> +         */
>>> +        pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
>>> +                                   PCI_EXP_LNKCAP_DLLLARC);
>>> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
>>> +                                   PCI_EXP_LNKSTA_DLLLA);
>>> +
>>> +        /*
>>> +         * Target Link Speed defaults to the highest link speed supported by
>>> +         * the component.  2.5GT/s devices are permitted to hardwire to zero.
>>> +         */
>>> +        pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKCTL2,
>>> +                                     PCI_EXP_LNKCTL2_TLS);
>>> +        pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKCTL2,
>>> +                                   QEMU_PCI_EXP_LNKCAP_MLS(s->speed) &
>>> +                                   PCI_EXP_LNKCTL2_TLS);
>>> +    }
>>> +
>>> +    /*
>>> +     * 2.5 & 5.0GT/s can be fully described by LNKCAP, but 8.0GT/s is
>>> +     * actually a reference to the highest bit supported in this register.
>>> +     * We assume the device supports all link speeds.
>>> +     */
>>> +    if (s->speed > QEMU_PCI_EXP_LNK_5GT) {
>>> +        pci_long_test_and_clear_mask(exp_cap + PCI_EXP_LNKCAP2, ~0U);
>>> +        pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
>>> +                                   PCI_EXP_LNKCAP2_SLS_2_5GB |
>>> +                                   PCI_EXP_LNKCAP2_SLS_5_0GB |
>>> +                                   PCI_EXP_LNKCAP2_SLS_8_0GB);
>>> +        if (s->speed > QEMU_PCI_EXP_LNK_8GT) {
>>> +            pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP2,
>>> +                                       PCI_EXP_LNKCAP2_SLS_16_0GB);  
>> I fail to understand why for 5GB you don't see both 2.5 and 5 as well.
> 
> Because there was a bit of a glitch in the PCIe 2.0 spec where lnkcap2
> is listed as a placeholder, so a strictly gen2 device doesn't need to
> implement lnkcap2.  In gen1, lnkcap supported link speeds (SLS) vector
> defined 0001b for 2.5GT/s, gen2 came along and defined 0010b for
> 2.5GT/s AND 5GT/s, then in the 3.0 spec they decided to slightly
> re-purpose this and SLS became MLS (max link speed), and introduced
> lnkcap2 to indicate which speeds were supported.  So the original spec
> definition of SLS didn't really give hardware the flexibility if they
> should decide they don't want to validate intermediate link speeds.

OK Thanks

Eric
> Thanks,
> 
> Alex
> 
>>> +        }
>>> +    }
>>> +}
>>> +
>>>  int pcie_cap_init(PCIDevice *dev, uint8_t offset,
>>>                    uint8_t type, uint8_t port,
>>>                    Error **errp)
>>> @@ -108,6 +177,9 @@ int pcie_cap_init(PCIDevice *dev, uint8_t offset,
>>>      /* Filling values common with v1 */
>>>      pcie_cap_v1_fill(dev, port, type, PCI_EXP_FLAGS_VER2);
>>>  
>>> +    /* Fill link speed and width options */
>>> +    pcie_cap_fill_slot_lnk(dev);
>>> +
>>>      /* Filling v2 specific values */
>>>      pci_set_long(exp_cap + PCI_EXP_DEVCAP2,
>>>                   PCI_EXP_DEVCAP2_EFF | PCI_EXP_DEVCAP2_EETLPP);
>>>
>>>   
>>
>> Thanks
>>
>> Eric
> 