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

[Auger Eric <eric.auger@redhat.com>]

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.

How are the device types checked?
> +    }
> +
> +    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.
> +        }
> +    }
> +}
> +
>  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