Hi Alex,
On 12/6/18 5:00 PM, Alex Williamson wrote:
> On Thu, 6 Dec 2018 12:08:09 +0100
> Auger Eric <eric.au...@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 <m...@redhat.com>
>>> Cc: Marcel Apfelbaum <marcel.apfelb...@gmail.com>
>>> Tested-by: Geoffrey McRae <ge...@hostfission.com>
>>> Signed-off-by: Alex Williamson <alex.william...@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
>
