[Qemu-devel] [PATCH v5 0/8] pcie: Enhanced link speed and width support

[Qemu-devel] [PATCH v5 0/8] pcie: Enhanced link speed and width support

[Alex Williamson]

v4->v5:
 - Rebase to bb9bf94b3e89 where the v4.0 machine type already exists
   (dropped from this series)
 - Fix casting enum as int as noted by Eric Blake
 - Looking specifically for Acks/Reviews/Sign-offs from PCI
   maintainers, I think the ancillary components all have
   sufficient reviewsi (more always welcome).

v3->v4:
 - v4.0 machine types moved to patch 1/9.  This patch is now for
   reference only with the expectation that it will be merged 
   through Eduardo's tree.  Including here only to have a self
   contained series. (Includes Eduardo's SPAPR loop fix)
 - Add Markus & Philippe's Review-by
 - Add Eric's Review-by and corrections to various patches:
   - qapi: correct release reference to 4.0 in enum definitions
   - link fill: set link bandwidth notification for width > x1
     OR (new) speed > 2.5GT/s with comment update
 - Correct HW_COMPAT_3_1 to the experimental property names

v2->v3:
 - Michael suggested offline that we not commit the pcie-root-port
   driver API to support arbitrary speeds and widths without some
   necessary use case where it's required to set these outside of
   the machine type defaults.  These options therefore become
   experimental, x-speed and x-width, with the expectation that users
   can either update their machine type or use experimental options
   for old machine types.  Patches 6 & 9 affected.  Leaving Geoffrey's
   Tested-by on patch 6 as this is a superficial change.
 - Rolled in David's Ack for spapr 4.0 machine type (patch 8).

v1->v2:
 - Update for QEMU release numbering, next is 4.0 not 3.2.  Only
   patch 8 and the commit log of patch 9 updated.

RFC->v1:
 - Add Cc reported by get_maintainer
 - Fixup some commit logs (no code changes in patches 1-7)
 - Add Geoffrey's Tested-by
 - Add patches 8 & 9 which define a QEMU 3.2 machine type and cranking
   up the link speed and width for that machine type while maintaining
   compatibile speeds for older machine types (testing requested for
   non-x86 machine types)
 - Various other users have also reported success with this series
   (/r/VFIO)

Original cover letter:

QEMU exposes gen1 PCI-express interconnect devices supporting only
2.5GT/s and x1 width.  It might not seem obvious that a virtual
bandwidth limitation can result in a real performance degradation, but
it's been reported that in some configurations assigned GPUs might not
scale their link speed up to the maximum supported value if the
downstream port above it only advertises limited link support.

As proposed[1] this series effectively implements virtual link
negotiation on downstream ports and enhances the generic PCIe root
port to allow user configurable speeds and widths.  The "negotiation"
simply mirrors the link status of the connected downstream device
providing the appearance of dynamic link speed scaling to match the
endpoint device.  Not yet implemented from the proposal is support
for globally updating defaults based on machine type, though the
foundation is provided here by allowing supporting PCIESlots to
implement an instance_init callback which can call into a common
helper for this.

I have not specifically tested migration with this, but we already
consider LNKSTA to be dynamic and the other changes implemented here
are static config space changes with no changes being implemented for
devices using default values, ie. they should be compatible by virtue
of existing config space migration support.

I think I've covered the required link related registers to support
PCIe 4.0, but please let me know if I've missed any.

Testing and feedback appreciated, patch 6/7 [now 6/8] provides example
qemu:arg options and requirements to use with existing libvirt.  Native
libvirt support TBD [unnecessary now with only experimental options
for configuration beyond machine version].  Thanks,

Alex

[1] https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg03086.html
---

Alex Williamson (8):
      pcie: Create enums for link speed and width
      pci: Sync PCIe downstream port LNKSTA on read
      qapi: Define PCIe link speed and width properties
      pcie: Add link speed and width fields to PCIESlot
      pcie: Fill PCIESlot link fields to support higher speeds and widths
      pcie: Allow generic PCIe root port to specify link speed and width
      vfio/pci: Remove PCIe Link Status emulation
      pcie: Fast PCIe root ports for new machines


 hw/core/qdev-properties.c          |  176 ++++++++++++++++++++++++++++++++++++
 hw/pci-bridge/gen_pcie_root_port.c |    4 +
 hw/pci-bridge/pcie_root_port.c     |   14 +++
 hw/pci/pci.c                       |    4 +
 hw/pci/pcie.c                      |  120 ++++++++++++++++++++++++-
 hw/vfio/pci.c                      |    9 --
 include/hw/compat.h                |   10 ++
 include/hw/pci/pci.h               |   13 +++
 include/hw/pci/pcie.h              |    1 
 include/hw/pci/pcie_port.h         |    4 +
 include/hw/pci/pcie_regs.h         |   23 ++++-
 include/hw/qdev-properties.h       |    8 ++
 qapi/common.json                   |   42 +++++++++
 13 files changed, 415 insertions(+), 13 deletions(-)

[Qemu-devel] [PATCH v5 1/8] pcie: Create enums for link speed and width

[Alex Williamson]

In preparation for reporting higher virtual link speeds and widths,
create enums and macros to help us manage them.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Tested-by: Geoffrey McRae <geoff@hostfission.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci/pcie.c              |    7 ++++---
 hw/vfio/pci.c              |    3 ++-
 include/hw/pci/pcie_regs.h |   23 +++++++++++++++++++++--
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 6c91bd44a0a5..914a5261a79b 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -68,11 +68,12 @@ pcie_cap_v1_fill(PCIDevice *dev, uint8_t port, uint8_t type, uint8_t version)
     pci_set_long(exp_cap + PCI_EXP_LNKCAP,
                  (port << PCI_EXP_LNKCAP_PN_SHIFT) |
                  PCI_EXP_LNKCAP_ASPMS_0S |
-                 PCI_EXP_LNK_MLW_1 |
-                 PCI_EXP_LNK_LS_25);
+                 QEMU_PCI_EXP_LNKCAP_MLW(QEMU_PCI_EXP_LNK_X1) |
+                 QEMU_PCI_EXP_LNKCAP_MLS(QEMU_PCI_EXP_LNK_2_5GT));
 
     pci_set_word(exp_cap + PCI_EXP_LNKSTA,
-                 PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25);
+                 QEMU_PCI_EXP_LNKSTA_NLW(QEMU_PCI_EXP_LNK_X1) |
+                 QEMU_PCI_EXP_LNKSTA_CLS(QEMU_PCI_EXP_LNK_2_5GT));
 
     if (dev->cap_present & QEMU_PCIE_LNKSTA_DLLLA) {
         pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA,
diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 5c7bd9698496..74f9a46b4be0 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1897,7 +1897,8 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
                                    PCI_EXP_TYPE_ENDPOINT << 4,
                                    PCI_EXP_FLAGS_TYPE);
             vfio_add_emulated_long(vdev, pos + PCI_EXP_LNKCAP,
-                                   PCI_EXP_LNK_MLW_1 | PCI_EXP_LNK_LS_25, ~0);
+                           QEMU_PCI_EXP_LNKCAP_MLW(QEMU_PCI_EXP_LNK_X1) |
+                           QEMU_PCI_EXP_LNKCAP_MLS(QEMU_PCI_EXP_LNK_2_5GT), ~0);
             vfio_add_emulated_word(vdev, pos + PCI_EXP_LNKCTL, 0, ~0);
         }
 
diff --git a/include/hw/pci/pcie_regs.h b/include/hw/pci/pcie_regs.h
index a95522a13b04..ad4e7808b8ac 100644
--- a/include/hw/pci/pcie_regs.h
+++ b/include/hw/pci/pcie_regs.h
@@ -34,10 +34,29 @@
 
 /* PCI_EXP_LINK{CAP, STA} */
 /* link speed */
-#define PCI_EXP_LNK_LS_25               1
+typedef enum PCIExpLinkSpeed {
+    QEMU_PCI_EXP_LNK_2_5GT = 1,
+    QEMU_PCI_EXP_LNK_5GT,
+    QEMU_PCI_EXP_LNK_8GT,
+    QEMU_PCI_EXP_LNK_16GT,
+} PCIExpLinkSpeed;
+
+#define QEMU_PCI_EXP_LNKCAP_MLS(speed)  (speed)
+#define QEMU_PCI_EXP_LNKSTA_CLS         QEMU_PCI_EXP_LNKCAP_MLS
+
+typedef enum PCIExpLinkWidth {
+    QEMU_PCI_EXP_LNK_X1 = 1,
+    QEMU_PCI_EXP_LNK_X2 = 2,
+    QEMU_PCI_EXP_LNK_X4 = 4,
+    QEMU_PCI_EXP_LNK_X8 = 8,
+    QEMU_PCI_EXP_LNK_X12 = 12,
+    QEMU_PCI_EXP_LNK_X16 = 16,
+    QEMU_PCI_EXP_LNK_X32 = 32,
+} PCIExpLinkWidth;
 
 #define PCI_EXP_LNK_MLW_SHIFT           ctz32(PCI_EXP_LNKCAP_MLW)
-#define PCI_EXP_LNK_MLW_1               (1 << PCI_EXP_LNK_MLW_SHIFT)
+#define QEMU_PCI_EXP_LNKCAP_MLW(width)  (width << PCI_EXP_LNK_MLW_SHIFT)
+#define QEMU_PCI_EXP_LNKSTA_NLW         QEMU_PCI_EXP_LNKCAP_MLW
 
 /* PCI_EXP_LINKCAP */
 #define PCI_EXP_LNKCAP_ASPMS_SHIFT      ctz32(PCI_EXP_LNKCAP_ASPMS)

[Qemu-devel] [PATCH v5 2/8] pci: Sync PCIe downstream port LNKSTA on read

[Alex Williamson]

The PCIe link speed and width between a downstream device and its
upstream port is negotiated on real hardware and susceptible to
dynamic changes due to signal issues and power management.  In the
emulated device case there is no real hardware link, but we still
might wish to have some consistency between endpoint and downstream
port via a virtual negotiation.  There is of course a real link for
assigned devices and this same virtual negotiation allows the
downstream port to match the endpoint, synchronizing on every read
to support underlying physical hardware dynamically adjusting the
link.

This negotiation is intentionally unidirectional for compatibility.
If the endpoint exceeds the capabilities of the downstream port or
there is no endpoint device, the downstream port reports negotiation
to its maximum speed and width, matching the previous case where
negotiation was absent.  De-tuning the endpoint to match a virtual
link doesn't seem to benefit anyone and is a condition we've thus
far reported without functional issues.

Note that PCI_EXP_LNKSTA is already ignored for migration
compatibility via pcie_cap_v1_fill().

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Tested-by: Geoffrey McRae <geoff@hostfission.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci/pci.c          |    4 ++++
 hw/pci/pcie.c         |   39 +++++++++++++++++++++++++++++++++++++++
 include/hw/pci/pci.h  |   13 +++++++++++++
 include/hw/pci/pcie.h |    1 +
 4 files changed, 57 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 56b13b3320ec..495db3b9e18a 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -1353,6 +1353,10 @@ uint32_t pci_default_read_config(PCIDevice *d,
 {
     uint32_t val = 0;
 
+    if (pci_is_express_downstream_port(d) &&
+        ranges_overlap(address, len, d->exp.exp_cap + PCI_EXP_LNKSTA, 2)) {
+        pcie_sync_bridge_lnk(d);
+    }
     memcpy(&val, d->config + address, len);
     return le32_to_cpu(val);
 }
diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 914a5261a79b..61b7b96c52cd 100644
--- a/hw/pci/pcie.c
+++ b/hw/pci/pcie.c
@@ -729,6 +729,45 @@ void pcie_add_capability(PCIDevice *dev,
     memset(dev->cmask + offset, 0xFF, size);
 }
 
+/*
+ * Sync the PCIe Link Status negotiated speed and width of a bridge with the
+ * downstream device.  If downstream device is not present, re-write with the
+ * Link Capability fields.  Limit width and speed to bridge capabilities for
+ * compatibility.  Use config_read to access the downstream device since it
+ * could be an assigned device with volatile link information.
+ */
+void pcie_sync_bridge_lnk(PCIDevice *bridge_dev)
+{
+    PCIBridge *br = PCI_BRIDGE(bridge_dev);
+    PCIBus *bus = pci_bridge_get_sec_bus(br);
+    PCIDevice *target = bus->devices[0];
+    uint8_t *exp_cap = bridge_dev->config + bridge_dev->exp.exp_cap;
+    uint16_t lnksta, lnkcap = pci_get_word(exp_cap + PCI_EXP_LNKCAP);
+
+    if (!target || !target->exp.exp_cap) {
+        lnksta = lnkcap;
+    } else {
+        lnksta = target->config_read(target,
+                                     target->exp.exp_cap + PCI_EXP_LNKSTA,
+                                     sizeof(lnksta));
+
+        if ((lnksta & PCI_EXP_LNKSTA_NLW) > (lnkcap & PCI_EXP_LNKCAP_MLW)) {
+            lnksta &= ~PCI_EXP_LNKSTA_NLW;
+            lnksta |= lnkcap & PCI_EXP_LNKCAP_MLW;
+        }
+
+        if ((lnksta & PCI_EXP_LNKSTA_CLS) > (lnkcap & PCI_EXP_LNKCAP_SLS)) {
+            lnksta &= ~PCI_EXP_LNKSTA_CLS;
+            lnksta |= lnkcap & PCI_EXP_LNKCAP_SLS;
+        }
+    }
+
+    pci_word_test_and_clear_mask(exp_cap + PCI_EXP_LNKSTA,
+                                 PCI_EXP_LNKSTA_CLS | PCI_EXP_LNKSTA_NLW);
+    pci_word_test_and_set_mask(exp_cap + PCI_EXP_LNKSTA, lnksta &
+                               (PCI_EXP_LNKSTA_CLS | PCI_EXP_LNKSTA_NLW));
+}
+
 /**************************************************************************
  * pci express extended capability helper functions
  */
diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
index e6514bba23aa..eb12fa112ed2 100644
--- a/include/hw/pci/pci.h
+++ b/include/hw/pci/pci.h
@@ -737,6 +737,19 @@ static inline int pci_is_express(const PCIDevice *d)
     return d->cap_present & QEMU_PCI_CAP_EXPRESS;
 }
 
+static inline int pci_is_express_downstream_port(const PCIDevice *d)
+{
+    uint8_t type;
+
+    if (!pci_is_express(d) || !d->exp.exp_cap) {
+        return 0;
+    }
+
+    type = pcie_cap_get_type(d);
+
+    return type == PCI_EXP_TYPE_DOWNSTREAM || type == PCI_EXP_TYPE_ROOT_PORT;
+}
+
 static inline uint32_t pci_config_size(const PCIDevice *d)
 {
     return pci_is_express(d) ? PCIE_CONFIG_SPACE_SIZE : PCI_CONFIG_SPACE_SIZE;
diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
index b71e36970345..1976909ab4c8 100644
--- a/include/hw/pci/pcie.h
+++ b/include/hw/pci/pcie.h
@@ -126,6 +126,7 @@ uint16_t pcie_find_capability(PCIDevice *dev, uint16_t cap_id);
 void pcie_add_capability(PCIDevice *dev,
                          uint16_t cap_id, uint8_t cap_ver,
                          uint16_t offset, uint16_t size);
+void pcie_sync_bridge_lnk(PCIDevice *dev);
 
 void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
 void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);

[Qemu-devel] [PATCH v5 3/8] qapi: Define PCIe link speed and width properties

[Alex Williamson]

Create properties to be able to define speeds and widths for PCIe
links.  The only tricky bit here is that our get and set callbacks
translate from the fixed QAPI automagic enums to those we define
in PCI code to represent the actual register segment value.

Cc: Eric Blake <eblake@redhat.com>
Tested-by: Geoffrey McRae <geoff@hostfission.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/core/qdev-properties.c    |  176 ++++++++++++++++++++++++++++++++++++++++++
 include/hw/qdev-properties.h |    8 ++
 qapi/common.json             |   42 ++++++++++
 3 files changed, 226 insertions(+)

diff --git a/hw/core/qdev-properties.c b/hw/core/qdev-properties.c
index bd84c4ea4cd9..943dc2654b2e 100644
--- a/hw/core/qdev-properties.c
+++ b/hw/core/qdev-properties.c
@@ -1297,3 +1297,179 @@ const PropertyInfo qdev_prop_off_auto_pcibar = {
     .set = set_enum,
     .set_default_value = set_default_value_enum,
 };
+
+/* --- PCIELinkSpeed 2_5/5/8/16 -- */
+
+static void get_prop_pcielinkspeed(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    PCIExpLinkSpeed *p = qdev_get_prop_ptr(dev, prop);
+    int speed;
+
+    switch (*p) {
+    case QEMU_PCI_EXP_LNK_2_5GT:
+        speed = PCIE_LINK_SPEED_2_5;
+        break;
+    case QEMU_PCI_EXP_LNK_5GT:
+        speed = PCIE_LINK_SPEED_5;
+        break;
+    case QEMU_PCI_EXP_LNK_8GT:
+        speed = PCIE_LINK_SPEED_8;
+        break;
+    case QEMU_PCI_EXP_LNK_16GT:
+        speed = PCIE_LINK_SPEED_16;
+        break;
+    default:
+        /* Unreachable */
+        abort();
+    }
+
+    visit_type_enum(v, prop->name, &speed, prop->info->enum_table, errp);
+}
+
+static void set_prop_pcielinkspeed(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    PCIExpLinkSpeed *p = qdev_get_prop_ptr(dev, prop);
+    int speed;
+    Error *local_err = NULL;
+
+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }
+
+    visit_type_enum(v, prop->name, &speed, prop->info->enum_table, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    switch (speed) {
+    case PCIE_LINK_SPEED_2_5:
+        *p = QEMU_PCI_EXP_LNK_2_5GT;
+        break;
+    case PCIE_LINK_SPEED_5:
+        *p = QEMU_PCI_EXP_LNK_5GT;
+        break;
+    case PCIE_LINK_SPEED_8:
+        *p = QEMU_PCI_EXP_LNK_8GT;
+        break;
+    case PCIE_LINK_SPEED_16:
+        *p = QEMU_PCI_EXP_LNK_16GT;
+        break;
+    default:
+        /* Unreachable */
+        abort();
+    }
+}
+
+const PropertyInfo qdev_prop_pcie_link_speed = {
+    .name = "PCIELinkSpeed",
+    .description = "2_5/5/8/16",
+    .enum_table = &PCIELinkSpeed_lookup,
+    .get = get_prop_pcielinkspeed,
+    .set = set_prop_pcielinkspeed,
+    .set_default_value = set_default_value_enum,
+};
+
+/* --- PCIELinkWidth 1/2/4/8/12/16/32 -- */
+
+static void get_prop_pcielinkwidth(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    PCIExpLinkWidth *p = qdev_get_prop_ptr(dev, prop);
+    int width;
+
+    switch (*p) {
+    case QEMU_PCI_EXP_LNK_X1:
+        width = PCIE_LINK_WIDTH_1;
+        break;
+    case QEMU_PCI_EXP_LNK_X2:
+        width = PCIE_LINK_WIDTH_2;
+        break;
+    case QEMU_PCI_EXP_LNK_X4:
+        width = PCIE_LINK_WIDTH_4;
+        break;
+    case QEMU_PCI_EXP_LNK_X8:
+        width = PCIE_LINK_WIDTH_8;
+        break;
+    case QEMU_PCI_EXP_LNK_X12:
+        width = PCIE_LINK_WIDTH_12;
+        break;
+    case QEMU_PCI_EXP_LNK_X16:
+        width = PCIE_LINK_WIDTH_16;
+        break;
+    case QEMU_PCI_EXP_LNK_X32:
+        width = PCIE_LINK_WIDTH_32;
+        break;
+    default:
+        /* Unreachable */
+        abort();
+    }
+
+    visit_type_enum(v, prop->name, &width, prop->info->enum_table, errp);
+}
+
+static void set_prop_pcielinkwidth(Object *obj, Visitor *v, const char *name,
+                                   void *opaque, Error **errp)
+{
+    DeviceState *dev = DEVICE(obj);
+    Property *prop = opaque;
+    PCIExpLinkWidth *p = qdev_get_prop_ptr(dev, prop);
+    int width;
+    Error *local_err = NULL;
+
+    if (dev->realized) {
+        qdev_prop_set_after_realize(dev, name, errp);
+        return;
+    }
+
+    visit_type_enum(v, prop->name, &width, prop->info->enum_table, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+
+    switch (width) {
+    case PCIE_LINK_WIDTH_1:
+        *p = QEMU_PCI_EXP_LNK_X1;
+        break;
+    case PCIE_LINK_WIDTH_2:
+        *p = QEMU_PCI_EXP_LNK_X2;
+        break;
+    case PCIE_LINK_WIDTH_4:
+        *p = QEMU_PCI_EXP_LNK_X4;
+        break;
+    case PCIE_LINK_WIDTH_8:
+        *p = QEMU_PCI_EXP_LNK_X8;
+        break;
+    case PCIE_LINK_WIDTH_12:
+        *p = QEMU_PCI_EXP_LNK_X12;
+        break;
+    case PCIE_LINK_WIDTH_16:
+        *p = QEMU_PCI_EXP_LNK_X16;
+        break;
+    case PCIE_LINK_WIDTH_32:
+        *p = QEMU_PCI_EXP_LNK_X32;
+        break;
+    default:
+        /* Unreachable */
+        abort();
+    }
+}
+
+const PropertyInfo qdev_prop_pcie_link_width = {
+    .name = "PCIELinkWidth",
+    .description = "1/2/4/8/12/16/32",
+    .enum_table = &PCIELinkWidth_lookup,
+    .get = get_prop_pcielinkwidth,
+    .set = set_prop_pcielinkwidth,
+    .set_default_value = set_default_value_enum,
+};
diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 3ab9cd2eb69f..b6758c852e11 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -36,6 +36,8 @@ extern const PropertyInfo qdev_prop_uuid;
 extern const PropertyInfo qdev_prop_arraylen;
 extern const PropertyInfo qdev_prop_link;
 extern const PropertyInfo qdev_prop_off_auto_pcibar;
+extern const PropertyInfo qdev_prop_pcie_link_speed;
+extern const PropertyInfo qdev_prop_pcie_link_width;
 
 #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
         .name      = (_name),                                    \
@@ -217,6 +219,12 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
 #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_off_auto_pcibar, \
                         OffAutoPCIBAR)
+#define DEFINE_PROP_PCIE_LINK_SPEED(_n, _s, _f, _d) \
+    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_speed, \
+                        PCIExpLinkSpeed)
+#define DEFINE_PROP_PCIE_LINK_WIDTH(_n, _s, _f, _d) \
+    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_width, \
+                        PCIExpLinkWidth)
 
 #define DEFINE_PROP_UUID(_name, _state, _field) {                  \
         .name      = (_name),                                      \
diff --git a/qapi/common.json b/qapi/common.json
index 021174f04ea4..99d313ef3b59 100644
--- a/qapi/common.json
+++ b/qapi/common.json
@@ -127,6 +127,48 @@
 { 'enum': 'OffAutoPCIBAR',
   'data': [ 'off', 'auto', 'bar0', 'bar1', 'bar2', 'bar3', 'bar4', 'bar5' ] }
 
+##
+# @PCIELinkSpeed:
+#
+# An enumeration of PCIe link speeds in units of GT/s
+#
+# @2_5: 2.5GT/s
+#
+# @5: 5.0GT/s
+#
+# @8: 8.0GT/s
+#
+# @16: 16.0GT/s
+#
+# Since: 4.0
+##
+{ 'enum': 'PCIELinkSpeed',
+  'data': [ '2_5', '5', '8', '16' ] }
+
+##
+# @PCIELinkWidth:
+#
+# An enumeration of PCIe link width
+#
+# @1: x1
+#
+# @2: x2
+#
+# @4: x4
+#
+# @8: x8
+#
+# @12: x12
+#
+# @16: x16
+#
+# @32: x32
+#
+# Since: 4.0
+##
+{ 'enum': 'PCIELinkWidth',
+  'data': [ '1', '2', '4', '8', '12', '16', '32' ] }
+
 ##
 # @SysEmuTarget:
 #

[Qemu-devel] [PATCH v5 4/8] pcie: Add link speed and width fields to PCIESlot

[Alex Williamson]

Add fields allowing the PCIe link speed and width of a PCIESlot to
be configured, with an instance_post_init callback on the root port
parent class to set defaults.  This allows child classes to set these
via properties or via their own instance_init callback, without
requiring all implementions to support arbitrary user selected values.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Tested-by: Geoffrey McRae <geoff@hostfission.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci-bridge/pcie_root_port.c |   14 ++++++++++++++
 include/hw/pci/pcie_port.h     |    4 ++++
 2 files changed, 18 insertions(+)

diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
index 45f9e8cd4a36..34ad76743c44 100644
--- a/hw/pci-bridge/pcie_root_port.c
+++ b/hw/pci-bridge/pcie_root_port.c
@@ -140,6 +140,19 @@ static Property rp_props[] = {
     DEFINE_PROP_END_OF_LIST()
 };
 
+static void rp_instance_post_init(Object *obj)
+{
+    PCIESlot *s = PCIE_SLOT(obj);
+
+    if (!s->speed) {
+        s->speed = QEMU_PCI_EXP_LNK_2_5GT;
+    }
+
+    if (!s->width) {
+        s->width = QEMU_PCI_EXP_LNK_X1;
+    }
+}
+
 static void rp_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -157,6 +170,7 @@ static void rp_class_init(ObjectClass *klass, void *data)
 static const TypeInfo rp_info = {
     .name          = TYPE_PCIE_ROOT_PORT,
     .parent        = TYPE_PCIE_SLOT,
+    .instance_post_init = rp_instance_post_init,
     .class_init    = rp_class_init,
     .abstract      = true,
     .class_size = sizeof(PCIERootPortClass),
diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
index 0736014bfdb4..df242a0cafff 100644
--- a/include/hw/pci/pcie_port.h
+++ b/include/hw/pci/pcie_port.h
@@ -49,6 +49,10 @@ struct PCIESlot {
     /* pci express switch port with slot */
     uint8_t     chassis;
     uint16_t    slot;
+
+    PCIExpLinkSpeed speed;
+    PCIExpLinkWidth width;
+
     QLIST_ENTRY(PCIESlot) next;
 };
 

[Qemu-devel] [PATCH v5 5/8] pcie: Fill PCIESlot link fields to support higher speeds and widths

[Alex Williamson]

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 |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/hw/pci/pcie.c b/hw/pci/pcie.c
index 61b7b96c52cd..8673b163de25 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,76 @@ 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 or multiple link
+     * speeds.
+     */
+    if (s->width > QEMU_PCI_EXP_LNK_X1 ||
+        s->speed > QEMU_PCI_EXP_LNK_2_5GT) {
+        pci_long_test_and_set_mask(exp_cap + PCI_EXP_LNKCAP,
+                                   PCI_EXP_LNKCAP_LBNC);
+    }
+
+    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);
+        }
+    }
+}
+
 int pcie_cap_init(PCIDevice *dev, uint8_t offset,
                   uint8_t type, uint8_t port,
                   Error **errp)
@@ -108,6 +179,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);

[Qemu-devel] [PATCH v5 6/8] pcie: Allow generic PCIe root port to specify link speed and width

[Alex Williamson]

Allow users to experimentally specify speed and width values for the
generic PCIe root port.  Defaults remain at 2.5GT/s & x1 for
compatiblity with the intent to only support changing defaults via
machine types for now.

Note for libvirt testing that pcie-root-port controllers are given
default names like "pci.7" which don't play well with using the
"-set device.$name.$prop=$value" options accessible to us via
<qemu:commandline> options.  The solution is to add an <alias> to the
pcie-root-port <controller>, for example:

    <controller type='pci' index='7' model='pcie-root-port'>
      <model name='pcie-root-port'/>
      <target chassis='7' port='0x15'/>
      <alias name='ua-gfx0'/>
      <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x5'/>
    </controller>

The "ua-" here is a mandatory prefix.  We can then use:

  <qemu:commandline>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.ua-gfx0.x-speed=8'/>
    <qemu:arg value='-set'/>
    <qemu:arg value='device.ua-gfx0.x-width=16'/>
  </qemu:commandline>

or, without an alias, set globals such as:

  <qemu:commandline>
    <qemu:arg value='-global'/>
    <qemu:arg value='pcie-root-port.x-speed=8'/>
    <qemu:arg value='-global'/>
    <qemu:arg value='pcie-root-port.x-width=16'/>
  </qemu:commandline>

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Tested-by: Geoffrey McRae <geoff@hostfission.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci-bridge/gen_pcie_root_port.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
index 299de429ec1e..ca5418a89dd2 100644
--- a/hw/pci-bridge/gen_pcie_root_port.c
+++ b/hw/pci-bridge/gen_pcie_root_port.c
@@ -124,6 +124,10 @@ static Property gen_rp_props[] = {
                      res_reserve.mem_pref_32, -1),
     DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
                      res_reserve.mem_pref_64, -1),
+    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
+                                speed, PCIE_LINK_SPEED_2_5),
+    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
+                                width, PCIE_LINK_WIDTH_1),
     DEFINE_PROP_END_OF_LIST()
 };
 

[Qemu-devel] [PATCH v5 7/8] vfio/pci: Remove PCIe Link Status emulation

[Alex Williamson]

Now that the downstream port will virtually negotiate itself to the
link status of the downstream device, we can remove this emulation.
It's not clear that it was every terribly useful anyway.

Tested-by: Geoffrey McRae <geoff@hostfission.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/vfio/pci.c |    6 ------
 1 file changed, 6 deletions(-)

diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
index 74f9a46b4be0..c0cb1ec28908 100644
--- a/hw/vfio/pci.c
+++ b/hw/vfio/pci.c
@@ -1901,12 +1901,6 @@ static int vfio_setup_pcie_cap(VFIOPCIDevice *vdev, int pos, uint8_t size,
                            QEMU_PCI_EXP_LNKCAP_MLS(QEMU_PCI_EXP_LNK_2_5GT), ~0);
             vfio_add_emulated_word(vdev, pos + PCI_EXP_LNKCTL, 0, ~0);
         }
-
-        /* Mark the Link Status bits as emulated to allow virtual negotiation */
-        vfio_add_emulated_word(vdev, pos + PCI_EXP_LNKSTA,
-                               pci_get_word(vdev->pdev.config + pos +
-                                            PCI_EXP_LNKSTA),
-                               PCI_EXP_LNKCAP_MLW | PCI_EXP_LNKCAP_SLS);
     }
 
     /*

[Qemu-devel] [PATCH v5 8/8] pcie: Fast PCIe root ports for new machines

[Alex Williamson]

Change the default speed and width for new machine types to the
fastest and widest currently supported.  This should be compatible to
the PCIe 4.0 spec.  Pre-QEMU-4.0 machine types remain at 2.5GT/s, x1
width.

Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 hw/pci-bridge/gen_pcie_root_port.c |    4 ++--
 include/hw/compat.h                |   10 +++++++++-
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
index ca5418a89dd2..9766edb44596 100644
--- a/hw/pci-bridge/gen_pcie_root_port.c
+++ b/hw/pci-bridge/gen_pcie_root_port.c
@@ -125,9 +125,9 @@ static Property gen_rp_props[] = {
     DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
                      res_reserve.mem_pref_64, -1),
     DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
-                                speed, PCIE_LINK_SPEED_2_5),
+                                speed, PCIE_LINK_SPEED_16),
     DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
-                                width, PCIE_LINK_WIDTH_1),
+                                width, PCIE_LINK_WIDTH_32),
     DEFINE_PROP_END_OF_LIST()
 };
 
diff --git a/include/hw/compat.h b/include/hw/compat.h
index 70958328fe7a..3ca85b037c04 100644
--- a/include/hw/compat.h
+++ b/include/hw/compat.h
@@ -2,7 +2,15 @@
 #define HW_COMPAT_H
 
 #define HW_COMPAT_3_1 \
-    /* empty */
+    {\
+        .driver   = "pcie-root-port",\
+        .property = "x-speed",\
+        .value    = "2_5",\
+    },{\
+        .driver   = "pcie-root-port",\
+        .property = "x-width",\
+        .value    = "1",\
+    },
 
 #define HW_COMPAT_3_0 \
     /* empty */

Re: [Qemu-devel] [PATCH v5 6/8] pcie: Allow generic PCIe root port to specify link speed and width

[Michael S. Tsirkin]

On Wed, Dec 12, 2018 at 12:39:43PM -0700, Alex Williamson wrote:
> Allow users to experimentally specify speed and width values for the
> generic PCIe root port.  Defaults remain at 2.5GT/s & x1 for
> compatiblity with the intent to only support changing defaults via
> machine types for now.
> 
> Note for libvirt testing that pcie-root-port controllers are given
> default names like "pci.7" which don't play well with using the
> "-set device.$name.$prop=$value" options accessible to us via
> <qemu:commandline> options.  The solution is to add an <alias> to the
> pcie-root-port <controller>, for example:
> 
>     <controller type='pci' index='7' model='pcie-root-port'>
>       <model name='pcie-root-port'/>
>       <target chassis='7' port='0x15'/>
>       <alias name='ua-gfx0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x5'/>
>     </controller>
> 
> The "ua-" here is a mandatory prefix.  We can then use:
> 
>   <qemu:commandline>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.ua-gfx0.x-speed=8'/>
>     <qemu:arg value='-set'/>
>     <qemu:arg value='device.ua-gfx0.x-width=16'/>
>   </qemu:commandline>
> 
> or, without an alias, set globals such as:
> 
>   <qemu:commandline>
>     <qemu:arg value='-global'/>
>     <qemu:arg value='pcie-root-port.x-speed=8'/>
>     <qemu:arg value='-global'/>
>     <qemu:arg value='pcie-root-port.x-width=16'/>
>   </qemu:commandline>
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> Tested-by: Geoffrey McRae <geoff@hostfission.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  hw/pci-bridge/gen_pcie_root_port.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> index 299de429ec1e..ca5418a89dd2 100644
> --- a/hw/pci-bridge/gen_pcie_root_port.c
> +++ b/hw/pci-bridge/gen_pcie_root_port.c
> @@ -124,6 +124,10 @@ static Property gen_rp_props[] = {
>                       res_reserve.mem_pref_32, -1),
>      DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
>                       res_reserve.mem_pref_64, -1),
> +    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
> +                                speed, PCIE_LINK_SPEED_2_5),
> +    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
> +                                width, PCIE_LINK_WIDTH_1),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  

Doesn't seem to build.
Just where is DEFINE_PROP_PCIE_LINK_SPEED defined?

-- 
MST

Re: [Qemu-devel] [PATCH v5 6/8] pcie: Allow generic PCIe root port to specify link speed and width

[Alex Williamson]

On Mon, 17 Dec 2018 20:29:37 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Dec 12, 2018 at 12:39:43PM -0700, Alex Williamson wrote:
> > Allow users to experimentally specify speed and width values for the
> > generic PCIe root port.  Defaults remain at 2.5GT/s & x1 for
> > compatiblity with the intent to only support changing defaults via
> > machine types for now.
> > 
> > Note for libvirt testing that pcie-root-port controllers are given
> > default names like "pci.7" which don't play well with using the
> > "-set device.$name.$prop=$value" options accessible to us via
> > <qemu:commandline> options.  The solution is to add an <alias> to the
> > pcie-root-port <controller>, for example:
> > 
> >     <controller type='pci' index='7' model='pcie-root-port'>
> >       <model name='pcie-root-port'/>
> >       <target chassis='7' port='0x15'/>
> >       <alias name='ua-gfx0'/>
> >       <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x5'/>
> >     </controller>
> > 
> > The "ua-" here is a mandatory prefix.  We can then use:
> > 
> >   <qemu:commandline>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.ua-gfx0.x-speed=8'/>
> >     <qemu:arg value='-set'/>
> >     <qemu:arg value='device.ua-gfx0.x-width=16'/>
> >   </qemu:commandline>
> > 
> > or, without an alias, set globals such as:
> > 
> >   <qemu:commandline>
> >     <qemu:arg value='-global'/>
> >     <qemu:arg value='pcie-root-port.x-speed=8'/>
> >     <qemu:arg value='-global'/>
> >     <qemu:arg value='pcie-root-port.x-width=16'/>
> >   </qemu:commandline>
> > 
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > Tested-by: Geoffrey McRae <geoff@hostfission.com>
> > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  hw/pci-bridge/gen_pcie_root_port.c |    4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> > index 299de429ec1e..ca5418a89dd2 100644
> > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > @@ -124,6 +124,10 @@ static Property gen_rp_props[] = {
> >                       res_reserve.mem_pref_32, -1),
> >      DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
> >                       res_reserve.mem_pref_64, -1),
> > +    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
> > +                                speed, PCIE_LINK_SPEED_2_5),
> > +    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
> > +                                width, PCIE_LINK_WIDTH_1),
> >      DEFINE_PROP_END_OF_LIST()
> >  };
> >    
> 
> Doesn't seem to build.
> Just where is DEFINE_PROP_PCIE_LINK_SPEED defined?

In 3/8:

diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
index 3ab9cd2eb69f..b6758c852e11 100644
--- a/include/hw/qdev-properties.h
+++ b/include/hw/qdev-properties.h
@@ -36,6 +36,8 @@ extern const PropertyInfo qdev_prop_uuid;
 extern const PropertyInfo qdev_prop_arraylen;
 extern const PropertyInfo qdev_prop_link;
 extern const PropertyInfo qdev_prop_off_auto_pcibar;
+extern const PropertyInfo qdev_prop_pcie_link_speed;
+extern const PropertyInfo qdev_prop_pcie_link_width;
 
 #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
         .name      = (_name),                                    \
@@ -217,6 +219,12 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
 #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
     DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_off_auto_pcibar, \
                         OffAutoPCIBAR)
+#define DEFINE_PROP_PCIE_LINK_SPEED(_n, _s, _f, _d) \
+    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_speed, \
+                        PCIExpLinkSpeed)
+#define DEFINE_PROP_PCIE_LINK_WIDTH(_n, _s, _f, _d) \
+    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_width, \
+                        PCIExpLinkWidth)
 
 #define DEFINE_PROP_UUID(_name, _state, _field) {                  \
         .name      = (_name),                                      \

Did something go wrong applying that patch?  I'll double check on my
end.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v5 6/8] pcie: Allow generic PCIe root port to specify link speed and width

[Michael S. Tsirkin]

On Mon, Dec 17, 2018 at 06:44:04PM -0700, Alex Williamson wrote:
> On Mon, 17 Dec 2018 20:29:37 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Wed, Dec 12, 2018 at 12:39:43PM -0700, Alex Williamson wrote:
> > > Allow users to experimentally specify speed and width values for the
> > > generic PCIe root port.  Defaults remain at 2.5GT/s & x1 for
> > > compatiblity with the intent to only support changing defaults via
> > > machine types for now.
> > > 
> > > Note for libvirt testing that pcie-root-port controllers are given
> > > default names like "pci.7" which don't play well with using the
> > > "-set device.$name.$prop=$value" options accessible to us via
> > > <qemu:commandline> options.  The solution is to add an <alias> to the
> > > pcie-root-port <controller>, for example:
> > > 
> > >     <controller type='pci' index='7' model='pcie-root-port'>
> > >       <model name='pcie-root-port'/>
> > >       <target chassis='7' port='0x15'/>
> > >       <alias name='ua-gfx0'/>
> > >       <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x5'/>
> > >     </controller>
> > > 
> > > The "ua-" here is a mandatory prefix.  We can then use:
> > > 
> > >   <qemu:commandline>
> > >     <qemu:arg value='-set'/>
> > >     <qemu:arg value='device.ua-gfx0.x-speed=8'/>
> > >     <qemu:arg value='-set'/>
> > >     <qemu:arg value='device.ua-gfx0.x-width=16'/>
> > >   </qemu:commandline>
> > > 
> > > or, without an alias, set globals such as:
> > > 
> > >   <qemu:commandline>
> > >     <qemu:arg value='-global'/>
> > >     <qemu:arg value='pcie-root-port.x-speed=8'/>
> > >     <qemu:arg value='-global'/>
> > >     <qemu:arg value='pcie-root-port.x-width=16'/>
> > >   </qemu:commandline>
> > > 
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > Tested-by: Geoffrey McRae <geoff@hostfission.com>
> > > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > >  hw/pci-bridge/gen_pcie_root_port.c |    4 ++++
> > >  1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> > > index 299de429ec1e..ca5418a89dd2 100644
> > > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > > @@ -124,6 +124,10 @@ static Property gen_rp_props[] = {
> > >                       res_reserve.mem_pref_32, -1),
> > >      DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
> > >                       res_reserve.mem_pref_64, -1),
> > > +    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
> > > +                                speed, PCIE_LINK_SPEED_2_5),
> > > +    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
> > > +                                width, PCIE_LINK_WIDTH_1),
> > >      DEFINE_PROP_END_OF_LIST()
> > >  };
> > >    
> > 
> > Doesn't seem to build.
> > Just where is DEFINE_PROP_PCIE_LINK_SPEED defined?
> 
> In 3/8:
> 
> diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> index 3ab9cd2eb69f..b6758c852e11 100644
> --- a/include/hw/qdev-properties.h
> +++ b/include/hw/qdev-properties.h
> @@ -36,6 +36,8 @@ extern const PropertyInfo qdev_prop_uuid;
>  extern const PropertyInfo qdev_prop_arraylen;
>  extern const PropertyInfo qdev_prop_link;
>  extern const PropertyInfo qdev_prop_off_auto_pcibar;
> +extern const PropertyInfo qdev_prop_pcie_link_speed;
> +extern const PropertyInfo qdev_prop_pcie_link_width;
>  
>  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
>          .name      = (_name),                                    \
> @@ -217,6 +219,12 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
>  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
>      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_off_auto_pcibar, \
>                          OffAutoPCIBAR)
> +#define DEFINE_PROP_PCIE_LINK_SPEED(_n, _s, _f, _d) \
> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_speed, \
> +                        PCIExpLinkSpeed)
> +#define DEFINE_PROP_PCIE_LINK_WIDTH(_n, _s, _f, _d) \
> +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_width, \
> +                        PCIExpLinkWidth)
>  
>  #define DEFINE_PROP_UUID(_name, _state, _field) {                  \
>          .name      = (_name),                                      \
> 
> Did something go wrong applying that patch?  I'll double check on my
> end.  Thanks,
> 
> Alex

Oh I just wasn't copied. So I missed this patch when applying.

-- 
MST

Re: [Qemu-devel] [PATCH v5 6/8] pcie: Allow generic PCIe root port to specify link speed and width

[Alex Williamson]

On Mon, 17 Dec 2018 20:47:26 -0500
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Mon, Dec 17, 2018 at 06:44:04PM -0700, Alex Williamson wrote:
> > On Mon, 17 Dec 2018 20:29:37 -0500
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >   
> > > On Wed, Dec 12, 2018 at 12:39:43PM -0700, Alex Williamson wrote:  
> > > > Allow users to experimentally specify speed and width values for the
> > > > generic PCIe root port.  Defaults remain at 2.5GT/s & x1 for
> > > > compatiblity with the intent to only support changing defaults via
> > > > machine types for now.
> > > > 
> > > > Note for libvirt testing that pcie-root-port controllers are given
> > > > default names like "pci.7" which don't play well with using the
> > > > "-set device.$name.$prop=$value" options accessible to us via
> > > > <qemu:commandline> options.  The solution is to add an <alias> to the
> > > > pcie-root-port <controller>, for example:
> > > > 
> > > >     <controller type='pci' index='7' model='pcie-root-port'>
> > > >       <model name='pcie-root-port'/>
> > > >       <target chassis='7' port='0x15'/>
> > > >       <alias name='ua-gfx0'/>
> > > >       <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x5'/>
> > > >     </controller>
> > > > 
> > > > The "ua-" here is a mandatory prefix.  We can then use:
> > > > 
> > > >   <qemu:commandline>
> > > >     <qemu:arg value='-set'/>
> > > >     <qemu:arg value='device.ua-gfx0.x-speed=8'/>
> > > >     <qemu:arg value='-set'/>
> > > >     <qemu:arg value='device.ua-gfx0.x-width=16'/>
> > > >   </qemu:commandline>
> > > > 
> > > > or, without an alias, set globals such as:
> > > > 
> > > >   <qemu:commandline>
> > > >     <qemu:arg value='-global'/>
> > > >     <qemu:arg value='pcie-root-port.x-speed=8'/>
> > > >     <qemu:arg value='-global'/>
> > > >     <qemu:arg value='pcie-root-port.x-width=16'/>
> > > >   </qemu:commandline>
> > > > 
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > > Tested-by: Geoffrey McRae <geoff@hostfission.com>
> > > > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > ---
> > > >  hw/pci-bridge/gen_pcie_root_port.c |    4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > > 
> > > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> > > > index 299de429ec1e..ca5418a89dd2 100644
> > > > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > > > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > > > @@ -124,6 +124,10 @@ static Property gen_rp_props[] = {
> > > >                       res_reserve.mem_pref_32, -1),
> > > >      DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
> > > >                       res_reserve.mem_pref_64, -1),
> > > > +    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
> > > > +                                speed, PCIE_LINK_SPEED_2_5),
> > > > +    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
> > > > +                                width, PCIE_LINK_WIDTH_1),
> > > >      DEFINE_PROP_END_OF_LIST()
> > > >  };
> > > >      
> > > 
> > > Doesn't seem to build.
> > > Just where is DEFINE_PROP_PCIE_LINK_SPEED defined?  
> > 
> > In 3/8:
> > 
> > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > index 3ab9cd2eb69f..b6758c852e11 100644
> > --- a/include/hw/qdev-properties.h
> > +++ b/include/hw/qdev-properties.h
> > @@ -36,6 +36,8 @@ extern const PropertyInfo qdev_prop_uuid;
> >  extern const PropertyIn qdev_prop_arraylen;
> >  extern const PropertyInfo qdev_prop_link;
> >  extern const PropertyInfo qdev_prop_off_auto_pcibar;
> > +extern const PropertyInfo qdev_prop_pcie_link_speed;
> > +extern const PropertyInfo qdev_prop_pcie_link_width;
> >  
> >  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
> >          .name      = (_name),                                    \
> > @@ -217,6 +219,12 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
> >  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
> >      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_off_auto_pcibar, \
> >                          OffAutoPCIBAR)
> > +#define DEFINE_PROP_PCIE_LINK_SPEED(_n, _s, _f, _d) \
> > +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_speed, \
> > +                        PCIExpLinkSpeed)
> > +#define DEFINE_PROP_PCIE_LINK_WIDTH(_n, _s, _f, _d) \
> > +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_width, \
> > +                        PCIExpLinkWidth)
> >  
> >  #define DEFINE_PROP_UUID(_name, _state, _field) {                  \
> >          .name      = (_name),                                      \
> > 
> > Did something go wrong applying that patch?  I'll double check on my
> > end.  Thanks,
> > 
> > Alex  
> 
> Oh I just wasn't copied. So I missed this patch when applying.

Ah, sorry, I should have forced your Cc on all the patches.  I'd also
welcome you to include 7/8 in the series to keep things altogether,
you're directly copied on all the others.  Thanks,

Alex

Re: [Qemu-devel] [PATCH v5 6/8] pcie: Allow generic PCIe root port to specify link speed and width

[Michael S. Tsirkin]

On Mon, Dec 17, 2018 at 06:54:32PM -0700, Alex Williamson wrote:
> On Mon, 17 Dec 2018 20:47:26 -0500
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Mon, Dec 17, 2018 at 06:44:04PM -0700, Alex Williamson wrote:
> > > On Mon, 17 Dec 2018 20:29:37 -0500
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >   
> > > > On Wed, Dec 12, 2018 at 12:39:43PM -0700, Alex Williamson wrote:  
> > > > > Allow users to experimentally specify speed and width values for the
> > > > > generic PCIe root port.  Defaults remain at 2.5GT/s & x1 for
> > > > > compatiblity with the intent to only support changing defaults via
> > > > > machine types for now.
> > > > > 
> > > > > Note for libvirt testing that pcie-root-port controllers are given
> > > > > default names like "pci.7" which don't play well with using the
> > > > > "-set device.$name.$prop=$value" options accessible to us via
> > > > > <qemu:commandline> options.  The solution is to add an <alias> to the
> > > > > pcie-root-port <controller>, for example:
> > > > > 
> > > > >     <controller type='pci' index='7' model='pcie-root-port'>
> > > > >       <model name='pcie-root-port'/>
> > > > >       <target chassis='7' port='0x15'/>
> > > > >       <alias name='ua-gfx0'/>
> > > > >       <address type='pci' domain='0x0000' bus='0x00' slot='0x02' function='0x5'/>
> > > > >     </controller>
> > > > > 
> > > > > The "ua-" here is a mandatory prefix.  We can then use:
> > > > > 
> > > > >   <qemu:commandline>
> > > > >     <qemu:arg value='-set'/>
> > > > >     <qemu:arg value='device.ua-gfx0.x-speed=8'/>
> > > > >     <qemu:arg value='-set'/>
> > > > >     <qemu:arg value='device.ua-gfx0.x-width=16'/>
> > > > >   </qemu:commandline>
> > > > > 
> > > > > or, without an alias, set globals such as:
> > > > > 
> > > > >   <qemu:commandline>
> > > > >     <qemu:arg value='-global'/>
> > > > >     <qemu:arg value='pcie-root-port.x-speed=8'/>
> > > > >     <qemu:arg value='-global'/>
> > > > >     <qemu:arg value='pcie-root-port.x-width=16'/>
> > > > >   </qemu:commandline>
> > > > > 
> > > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com>
> > > > > Tested-by: Geoffrey McRae <geoff@hostfission.com>
> > > > > Reviewed-by: Eric Auger <eric.auger@redhat.com>
> > > > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > ---
> > > > >  hw/pci-bridge/gen_pcie_root_port.c |    4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/hw/pci-bridge/gen_pcie_root_port.c b/hw/pci-bridge/gen_pcie_root_port.c
> > > > > index 299de429ec1e..ca5418a89dd2 100644
> > > > > --- a/hw/pci-bridge/gen_pcie_root_port.c
> > > > > +++ b/hw/pci-bridge/gen_pcie_root_port.c
> > > > > @@ -124,6 +124,10 @@ static Property gen_rp_props[] = {
> > > > >                       res_reserve.mem_pref_32, -1),
> > > > >      DEFINE_PROP_SIZE("pref64-reserve", GenPCIERootPort,
> > > > >                       res_reserve.mem_pref_64, -1),
> > > > > +    DEFINE_PROP_PCIE_LINK_SPEED("x-speed", PCIESlot,
> > > > > +                                speed, PCIE_LINK_SPEED_2_5),
> > > > > +    DEFINE_PROP_PCIE_LINK_WIDTH("x-width", PCIESlot,
> > > > > +                                width, PCIE_LINK_WIDTH_1),
> > > > >      DEFINE_PROP_END_OF_LIST()
> > > > >  };
> > > > >      
> > > > 
> > > > Doesn't seem to build.
> > > > Just where is DEFINE_PROP_PCIE_LINK_SPEED defined?  
> > > 
> > > In 3/8:
> > > 
> > > diff --git a/include/hw/qdev-properties.h b/include/hw/qdev-properties.h
> > > index 3ab9cd2eb69f..b6758c852e11 100644
> > > --- a/include/hw/qdev-properties.h
> > > +++ b/include/hw/qdev-properties.h
> > > @@ -36,6 +36,8 @@ extern const PropertyInfo qdev_prop_uuid;
> > >  extern const PropertyIn qdev_prop_arraylen;
> > >  extern const PropertyInfo qdev_prop_link;
> > >  extern const PropertyInfo qdev_prop_off_auto_pcibar;
> > > +extern const PropertyInfo qdev_prop_pcie_link_speed;
> > > +extern const PropertyInfo qdev_prop_pcie_link_width;
> > >  
> > >  #define DEFINE_PROP(_name, _state, _field, _prop, _type) { \
> > >          .name      = (_name),                                    \
> > > @@ -217,6 +219,12 @@ extern const PropertyInfo qdev_prop_off_auto_pcibar;
> > >  #define DEFINE_PROP_OFF_AUTO_PCIBAR(_n, _s, _f, _d) \
> > >      DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_off_auto_pcibar, \
> > >                          OffAutoPCIBAR)
> > > +#define DEFINE_PROP_PCIE_LINK_SPEED(_n, _s, _f, _d) \
> > > +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_speed, \
> > > +                        PCIExpLinkSpeed)
> > > +#define DEFINE_PROP_PCIE_LINK_WIDTH(_n, _s, _f, _d) \
> > > +    DEFINE_PROP_SIGNED(_n, _s, _f, _d, qdev_prop_pcie_link_width, \
> > > +                        PCIExpLinkWidth)
> > >  
> > >  #define DEFINE_PROP_UUID(_name, _state, _field) {                  \
> > >          .name      = (_name),                                      \
> > > 
> > > Did something go wrong applying that patch?  I'll double check on my
> > > end.  Thanks,
> > > 
> > > Alex  
> > 
> > Oh I just wasn't copied. So I missed this patch when applying.
> 
> Ah, sorry, I should have forced your Cc on all the patches.  I'd also
> welcome you to include 7/8 in the series to keep things altogether,
> you're directly copied on all the others.  Thanks,
> 
> Alex

Done.

-- 
MST