[PATCH v2 0/2] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model

[PATCH v2 0/2] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model

[Nicholas Piggin]

Hi,

This adds a new USB XHCI PCI host controller model, the TI TUSB73X0.

The motivation for this is that IBM's AIX and PowerVM do not support
the NEC driver.

hcd-xhci-pci code is changed in patch 1 to make PCI settings
configurable where the new model differs from existing. E.g., the
option to add the PM cap, and option to use exclusive MSIX BAR.

Changes since v1:
- Remove an unused variable noticed by Philippe.

Thanks,
Nick

Nicholas Piggin (2):
  hw/usb/hcd-xhci-pci: Make PCI device more configurable
  hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model

 hw/usb/hcd-xhci-pci.h           |  9 ++++
 include/hw/pci/pci_ids.h        |  1 +
 include/hw/usb/xhci.h           |  1 +
 hw/usb/hcd-xhci-nec.c           | 10 ++++
 hw/usb/hcd-xhci-pci.c           | 69 +++++++++++++++++++++----
 hw/usb/hcd-xhci-ti.c            | 92 +++++++++++++++++++++++++++++++++
 tests/qtest/usb-hcd-xhci-test.c | 21 +++++---
 hw/usb/Kconfig                  |  5 ++
 hw/usb/meson.build              |  1 +
 9 files changed, 193 insertions(+), 16 deletions(-)
 create mode 100644 hw/usb/hcd-xhci-ti.c

-- 
2.45.2

[PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[Nicholas Piggin]

To prepare to support another USB PCI Host Controller, make some PCI
configuration dynamic.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 hw/usb/hcd-xhci-pci.h |  9 ++++++
 hw/usb/hcd-xhci-nec.c | 10 +++++++
 hw/usb/hcd-xhci-pci.c | 69 ++++++++++++++++++++++++++++++++++++-------
 3 files changed, 78 insertions(+), 10 deletions(-)

diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
index 08f70ce97cc..213076aabf6 100644
--- a/hw/usb/hcd-xhci-pci.h
+++ b/hw/usb/hcd-xhci-pci.h
@@ -40,6 +40,15 @@ typedef struct XHCIPciState {
     XHCIState xhci;
     OnOffAuto msi;
     OnOffAuto msix;
+    uint8_t cache_line_size;
+    uint8_t pm_cap_off;
+    uint8_t pcie_cap_off;
+    uint8_t msi_cap_off;
+    uint8_t msix_cap_off;
+    int msix_bar_nr;
+    uint64_t msix_bar_size;
+    uint32_t msix_table_off;
+    uint32_t msix_pba_off;
 } XHCIPciState;
 
 #endif
diff --git a/hw/usb/hcd-xhci-nec.c b/hw/usb/hcd-xhci-nec.c
index 0e61c6c4f06..6ac1dc7764c 100644
--- a/hw/usb/hcd-xhci-nec.c
+++ b/hw/usb/hcd-xhci-nec.c
@@ -52,6 +52,16 @@ static void nec_xhci_instance_init(Object *obj)
 
     pci->xhci.numintrs = nec->intrs;
     pci->xhci.numslots = nec->slots;
+
+    pci->cache_line_size = 0x10;
+    pci->pm_cap_off = 0;
+    pci->pcie_cap_off = 0xa0;
+    pci->msi_cap_off = 0x70;
+    pci->msix_cap_off = 0x90;
+    pci->msix_bar_nr = 0;
+    pci->msix_bar_size = 0;
+    pci->msix_table_off = 0x3000;
+    pci->msix_pba_off = 0x3800;
 }
 
 static void nec_xhci_class_init(ObjectClass *klass, void *data)
diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
index a039f5778a6..948d75b7379 100644
--- a/hw/usb/hcd-xhci-pci.c
+++ b/hw/usb/hcd-xhci-pci.c
@@ -32,8 +32,9 @@
 #include "trace.h"
 #include "qapi/error.h"
 
-#define OFF_MSIX_TABLE  0x3000
-#define OFF_MSIX_PBA    0x3800
+#define MSIX_BAR_SIZE   0x800000
+#define OFF_MSIX_TABLE  0x0000
+#define OFF_MSIX_PBA    0x1000
 
 static void xhci_pci_intr_update(XHCIState *xhci, int n, bool enable)
 {
@@ -104,6 +105,31 @@ static int xhci_pci_vmstate_post_load(void *opaque, int version_id)
    return 0;
 }
 
+static int xhci_pci_add_pm_capability(PCIDevice *pci_dev, uint8_t offset,
+                                      Error **errp)
+{
+    int err;
+
+    err = pci_add_capability(pci_dev, PCI_CAP_ID_PM, offset,
+                             PCI_PM_SIZEOF, errp);
+    if (err < 0) {
+        return err;
+    }
+
+    pci_set_word(pci_dev->config + offset + PCI_PM_PMC,
+                 PCI_PM_CAP_VER_1_2 |
+                 PCI_PM_CAP_D1 | PCI_PM_CAP_D2 |
+                 PCI_PM_CAP_PME_D0 | PCI_PM_CAP_PME_D1 |
+                 PCI_PM_CAP_PME_D2 | PCI_PM_CAP_PME_D3hot);
+    pci_set_word(pci_dev->wmask + offset + PCI_PM_PMC, 0);
+    pci_set_word(pci_dev->config + offset + PCI_PM_CTRL,
+                 PCI_PM_CTRL_NO_SOFT_RESET);
+    pci_set_word(pci_dev->wmask + offset + PCI_PM_CTRL,
+                 PCI_PM_CTRL_STATE_MASK);
+
+    return 0;
+}
+
 static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
 {
     int ret;
@@ -112,7 +138,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
 
     dev->config[PCI_CLASS_PROG] = 0x30;    /* xHCI */
     dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
-    dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
+    dev->config[PCI_CACHE_LINE_SIZE] = s->cache_line_size;
     dev->config[0x60] = 0x30; /* release number */
 
     object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
@@ -125,8 +151,16 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
         s->xhci.nec_quirks = true;
     }
 
+    if (s->pm_cap_off) {
+        if (xhci_pci_add_pm_capability(dev, s->pm_cap_off, &err)) {
+            error_propagate(errp, err);
+            return;
+        }
+    }
+
     if (s->msi != ON_OFF_AUTO_OFF) {
-        ret = msi_init(dev, 0x70, s->xhci.numintrs, true, false, &err);
+        ret = msi_init(dev, s->msi_cap_off, s->xhci.numintrs,
+                       true, false, &err);
         /*
          * Any error other than -ENOTSUP(board's MSI support is broken)
          * is a programming error
@@ -143,22 +177,37 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
         /* With msi=auto, we fall back to MSI off silently */
         error_free(err);
     }
+
     pci_register_bar(dev, 0,
                      PCI_BASE_ADDRESS_SPACE_MEMORY |
                      PCI_BASE_ADDRESS_MEM_TYPE_64,
                      &s->xhci.mem);
 
     if (pci_bus_is_express(pci_get_bus(dev))) {
-        ret = pcie_endpoint_cap_init(dev, 0xa0);
+        ret = pcie_endpoint_cap_init(dev, s->pcie_cap_off);
         assert(ret > 0);
     }
 
     if (s->msix != ON_OFF_AUTO_OFF) {
-        /* TODO check for errors, and should fail when msix=on */
-        msix_init(dev, s->xhci.numintrs,
-                  &s->xhci.mem, 0, OFF_MSIX_TABLE,
-                  &s->xhci.mem, 0, OFF_MSIX_PBA,
-                  0x90, NULL);
+        MemoryRegion *msix_bar = &s->xhci.mem;
+        if (s->msix_bar_nr != 0) {
+            memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev),
+                               "xhci-msix", s->msix_bar_size);
+            msix_bar = &dev->msix_exclusive_bar;
+        }
+
+        ret = msix_init(dev, s->xhci.numintrs,
+                        msix_bar, s->msix_bar_nr, s->msix_table_off,
+                        msix_bar, s->msix_bar_nr, s->msix_pba_off,
+                        s->msix_cap_off, errp);
+        if (ret) {
+            return;
+        }
+
+        pci_register_bar(dev, s->msix_bar_nr,
+                         PCI_BASE_ADDRESS_SPACE_MEMORY |
+                         PCI_BASE_ADDRESS_MEM_TYPE_64,
+                         msix_bar);
     }
     s->xhci.as = pci_get_address_space(dev);
 }
-- 
2.45.2

[PATCH v2 2/2] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model

[Nicholas Piggin]

The TI TUSB73X0 controller has some interesting differences from NEC,
notably a separate BAR for MSIX, and PM capabilities. The spec is freely
available without sign-up.

This controller is accepted by IBM Power proprietary firmware and
software (when the subsystem IDs are set to Power servers, which is not
done here). IBM code is picky about device support, so the NEC device
can not be used.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/hw/pci/pci_ids.h        |  1 +
 include/hw/usb/xhci.h           |  1 +
 hw/usb/hcd-xhci-ti.c            | 92 +++++++++++++++++++++++++++++++++
 tests/qtest/usb-hcd-xhci-test.c | 21 +++++---
 hw/usb/Kconfig                  |  5 ++
 hw/usb/meson.build              |  1 +
 6 files changed, 115 insertions(+), 6 deletions(-)
 create mode 100644 hw/usb/hcd-xhci-ti.c

diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
index f1a53fea8d6..fdb692db513 100644
--- a/include/hw/pci/pci_ids.h
+++ b/include/hw/pci/pci_ids.h
@@ -182,6 +182,7 @@
 #define PCI_VENDOR_ID_HP                 0x103c
 
 #define PCI_VENDOR_ID_TI                 0x104c
+#define PCI_DEVICE_ID_TI_TUSB73X0        0x8241
 
 #define PCI_VENDOR_ID_MOTOROLA           0x1057
 #define PCI_DEVICE_ID_MOTOROLA_MPC106    0x0002
diff --git a/include/hw/usb/xhci.h b/include/hw/usb/xhci.h
index 5c90e1373e5..203ec1fca32 100644
--- a/include/hw/usb/xhci.h
+++ b/include/hw/usb/xhci.h
@@ -3,6 +3,7 @@
 
 #define TYPE_XHCI "base-xhci"
 #define TYPE_NEC_XHCI "nec-usb-xhci"
+#define TYPE_TI_XHCI "ti-usb-xhci"
 #define TYPE_QEMU_XHCI "qemu-xhci"
 #define TYPE_XHCI_SYSBUS "sysbus-xhci"
 
diff --git a/hw/usb/hcd-xhci-ti.c b/hw/usb/hcd-xhci-ti.c
new file mode 100644
index 00000000000..6d4b44f6aaf
--- /dev/null
+++ b/hw/usb/hcd-xhci-ti.c
@@ -0,0 +1,92 @@
+/*
+ * USB xHCI controller emulation
+ * Datasheet https://www.ti.com/product/TUSB7340
+ *
+ * Copyright (c) 2011 Securiforest
+ * Date: 2011-05-11 ;  Author: Hector Martin <hector@marcansoft.com>
+ * Based on usb-xhci-nec.c, emulates TI TUSB73X0
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/usb.h"
+#include "qemu/module.h"
+#include "hw/pci/pci.h"
+#include "hw/qdev-properties.h"
+
+#include "hcd-xhci-pci.h"
+
+OBJECT_DECLARE_SIMPLE_TYPE(XHCITiState, TI_XHCI)
+
+struct XHCITiState {
+    /*< private >*/
+    XHCIPciState parent_obj;
+    /*< public >*/
+    uint32_t intrs;
+    uint32_t slots;
+};
+
+static Property ti_xhci_properties[] = {
+    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
+    DEFINE_PROP_UINT32("intrs", XHCITiState, intrs, 8),
+    DEFINE_PROP_UINT32("slots", XHCITiState, slots, XHCI_MAXSLOTS),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ti_xhci_instance_init(Object *obj)
+{
+    XHCIPciState *pci = XHCI_PCI(obj);
+    XHCITiState *ti = TI_XHCI(obj);
+
+    pci->xhci.numintrs = ti->intrs;
+    pci->xhci.numslots = ti->slots;
+
+    pci->cache_line_size = 0x0;
+    pci->pm_cap_off = 0x40;
+    pci->pcie_cap_off = 0x70;
+    pci->msi_cap_off = 0x48;
+    pci->msix_cap_off = 0xc0;
+    pci->msix_bar_nr = 0x2;
+    pci->msix_bar_size = 0x800000;
+    pci->msix_table_off = 0x0;
+    pci->msix_pba_off = 0x1000;
+}
+
+static void ti_xhci_class_init(ObjectClass *klass, void *data)
+{
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    device_class_set_props(dc, ti_xhci_properties);
+    k->vendor_id    = PCI_VENDOR_ID_TI;
+    k->device_id    = PCI_DEVICE_ID_TI_TUSB73X0;
+    k->revision     = 0x02;
+}
+
+static const TypeInfo ti_xhci_info = {
+    .name          = TYPE_TI_XHCI,
+    .parent        = TYPE_XHCI_PCI,
+    .instance_size = sizeof(XHCITiState),
+    .instance_init = ti_xhci_instance_init,
+    .class_init    = ti_xhci_class_init,
+};
+
+static void ti_xhci_register_types(void)
+{
+    type_register_static(&ti_xhci_info);
+}
+
+type_init(ti_xhci_register_types)
diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
index 93614e55461..d4a0d8cc217 100644
--- a/tests/qtest/usb-hcd-xhci-test.c
+++ b/tests/qtest/usb-hcd-xhci-test.c
@@ -59,6 +59,9 @@ typedef struct XHCIQState {
 #define XHCI_NEC_ID (PCI_DEVICE_ID_NEC_UPD720200 << 16 | \
                      PCI_VENDOR_ID_NEC)
 
+#define XHCI_TI_ID  (PCI_DEVICE_ID_TI_TUSB73X0 << 16 | \
+                     PCI_VENDOR_ID_TI)
+
 /**
  * Locate, verify, and return a handle to the XHCI device.
  */
@@ -78,6 +81,8 @@ static QPCIDevice *get_xhci_device(QTestState *qts, uint32_t *fingerprint)
     switch (xhci_fingerprint) {
     case XHCI_NEC_ID:
         break;
+    case XHCI_TI_ID:
+        break;
     default:
         /* Unknown device. */
         g_assert_not_reached();
@@ -134,11 +139,12 @@ static XHCIQState *xhci_boot(const char *cli, ...)
         va_end(ap);
     } else {
         s = xhci_boot("-M q35 "
-                      "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
+                      "-device %s,id=xhci,bus=pcie.0,addr=1d.0 "
                       "-drive id=drive0,if=none,file=null-co://,"
-                          "file.read-zeroes=on,format=raw");
+                          "file.read-zeroes=on,format=raw",
+                      qtest_has_device("ti-usb-xhci") ?
+                          "ti-usb-xhci" : "nec-usb-xhci");
     }
-
     return s;
 }
 
@@ -392,10 +398,12 @@ static void pci_xhci_stress_rings(void)
     int i;
 
     s = xhci_boot("-M q35 "
-            "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
+            "-device %s,id=xhci,bus=pcie.0,addr=1d.0 "
             "-device usb-storage,bus=xhci.0,drive=drive0 "
             "-drive id=drive0,if=none,file=null-co://,"
-                "file.read-zeroes=on,format=raw "
+                "file.read-zeroes=on,format=raw ",
+            qtest_has_device("ti-usb-xhci") ?
+                "ti-usb-xhci" : "nec-usb-xhci"
             );
 
     hcsparams1 = xhci_cap_readl(s, 0x4); /* HCSPARAMS1 */
@@ -567,7 +575,8 @@ int main(int argc, char **argv)
         return 0;
     }
 
-    if (!qtest_has_device("nec-usb-xhci")) {
+    if (!qtest_has_device("nec-usb-xhci") &&
+        !qtest_has_device("ti-usb-xhci")) {
         return 0;
     }
 
diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
index 5fbecd2f43b..8e5c4747af9 100644
--- a/hw/usb/Kconfig
+++ b/hw/usb/Kconfig
@@ -49,6 +49,11 @@ config USB_XHCI_NEC
     default y if PCI_DEVICES
     select USB_XHCI_PCI
 
+config USB_XHCI_TI
+    bool
+    default y if PCI_DEVICES
+    select USB_XHCI_PCI
+
 config USB_XHCI_SYSBUS
     bool
     select USB_XHCI
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 1b4d1507e41..b874a93f16e 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -23,6 +23,7 @@ system_ss.add(when: 'CONFIG_USB_XHCI', if_true: files('hcd-xhci.c'))
 system_ss.add(when: 'CONFIG_USB_XHCI_PCI', if_true: files('hcd-xhci-pci.c'))
 system_ss.add(when: 'CONFIG_USB_XHCI_SYSBUS', if_true: files('hcd-xhci-sysbus.c'))
 system_ss.add(when: 'CONFIG_USB_XHCI_NEC', if_true: files('hcd-xhci-nec.c'))
+system_ss.add(when: 'CONFIG_USB_XHCI_TI', if_true: files('hcd-xhci-ti.c'))
 system_ss.add(when: 'CONFIG_USB_DWC2', if_true: files('hcd-dwc2.c'))
 system_ss.add(when: 'CONFIG_USB_DWC3', if_true: files('hcd-dwc3.c'))
 
-- 
2.45.2

Re: [PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 8208 bytes --]

Hey Nicholas,

I'm not an XHCI & PCI expert (yet?) so apologies if I've got some of this
wrong, but I've asked some questions and made some comments inline:

On Thu, 12 Dec 2024 at 09:52, Nicholas Piggin <npiggin@gmail.com> wrote:

> To prepare to support another USB PCI Host Controller, make some PCI
> configuration dynamic.
>
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>  hw/usb/hcd-xhci-pci.h |  9 ++++++
>  hw/usb/hcd-xhci-nec.c | 10 +++++++
>  hw/usb/hcd-xhci-pci.c | 69 ++++++++++++++++++++++++++++++++++++-------
>  3 files changed, 78 insertions(+), 10 deletions(-)
>
> diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
> index 08f70ce97cc..213076aabf6 100644
> --- a/hw/usb/hcd-xhci-pci.h
> +++ b/hw/usb/hcd-xhci-pci.h
> @@ -40,6 +40,15 @@ typedef struct XHCIPciState {
>      XHCIState xhci;
>      OnOffAuto msi;
>      OnOffAuto msix;
> +    uint8_t cache_line_size;
> +    uint8_t pm_cap_off;
> +    uint8_t pcie_cap_off;
> +    uint8_t msi_cap_off;
> +    uint8_t msix_cap_off;
> +    int msix_bar_nr;
> +    uint64_t msix_bar_size;
> +    uint32_t msix_table_off;
> +    uint32_t msix_pba_off;
>  } XHCIPciState;
>
>  #endif
> diff --git a/hw/usb/hcd-xhci-nec.c b/hw/usb/hcd-xhci-nec.c
> index 0e61c6c4f06..6ac1dc7764c 100644
> --- a/hw/usb/hcd-xhci-nec.c
> +++ b/hw/usb/hcd-xhci-nec.c
> @@ -52,6 +52,16 @@ static void nec_xhci_instance_init(Object *obj)
>
>      pci->xhci.numintrs = nec->intrs;
>      pci->xhci.numslots = nec->slots;
> +
> +    pci->cache_line_size = 0x10;
> +    pci->pm_cap_off = 0;
> +    pci->pcie_cap_off = 0xa0;
> +    pci->msi_cap_off = 0x70;
> +    pci->msix_cap_off = 0x90;
> +    pci->msix_bar_nr = 0;
> +    pci->msix_bar_size = 0;
> +    pci->msix_table_off = 0x3000;
> +    pci->msix_pba_off = 0x3800;
>  }


What about the "qemu-xhci" device, does that need similar treatment? I
suspect it does at least for a bunch of these settings. Perhaps
xhci_instance_init() in the abstract "pci-xhci" base might be a better
place for these "sensible defaults" and then override them only in the
specific implementations that need to do so, such as the new TI model?
And/or have suitably named helper init function for configuring single-BAR
PCI XHCI controllers so we can get some meaning behind all these magic
numbers?


>  static void nec_xhci_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> index a039f5778a6..948d75b7379 100644
> --- a/hw/usb/hcd-xhci-pci.c
> +++ b/hw/usb/hcd-xhci-pci.c
> @@ -32,8 +32,9 @@
>  #include "trace.h"
>  #include "qapi/error.h"
>
> -#define OFF_MSIX_TABLE  0x3000
> -#define OFF_MSIX_PBA    0x3800
> +#define MSIX_BAR_SIZE   0x800000
>

MSIX_BAR_SIZE doesn't seem to be used anywhere, and patch 2/2 uses 0x800000
explicitly. (8 MiB also seems… huge? But I'm guessing you're matching this
with the physical TI controller hardware - either way I don't think it
belongs in this file.)


> +#define OFF_MSIX_TABLE  0x0000
> +#define OFF_MSIX_PBA    0x1000
>

Maybe instead of redefining these constants to only apply to the split BAR
device variants, there should be 2 variants of them, one for single-BAR
controllers, and one for controllers with separate BARs. That would also
help make sense of the "magic numbers" in nec_xhci_instance_init().



>  static void xhci_pci_intr_update(XHCIState *xhci, int n, bool enable)
>  {
> @@ -104,6 +105,31 @@ static int xhci_pci_vmstate_post_load(void *opaque,
> int version_id)
>     return 0;
>  }
>
> +static int xhci_pci_add_pm_capability(PCIDevice *pci_dev, uint8_t offset,
> +                                      Error **errp)
> +{
> +    int err;
> +
> +    err = pci_add_capability(pci_dev, PCI_CAP_ID_PM, offset,
> +                             PCI_PM_SIZEOF, errp);
> +    if (err < 0) {
> +        return err;
> +    }
> +
> +    pci_set_word(pci_dev->config + offset + PCI_PM_PMC,
> +                 PCI_PM_CAP_VER_1_2 |
> +                 PCI_PM_CAP_D1 | PCI_PM_CAP_D2 |
> +                 PCI_PM_CAP_PME_D0 | PCI_PM_CAP_PME_D1 |
> +                 PCI_PM_CAP_PME_D2 | PCI_PM_CAP_PME_D3hot);
> +    pci_set_word(pci_dev->wmask + offset + PCI_PM_PMC, 0);
> +    pci_set_word(pci_dev->config + offset + PCI_PM_CTRL,
> +                 PCI_PM_CTRL_NO_SOFT_RESET);
> +    pci_set_word(pci_dev->wmask + offset + PCI_PM_CTRL,
> +                 PCI_PM_CTRL_STATE_MASK);
> +
> +    return 0;
> +}
> +
>  static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>  {
>      int ret;
> @@ -112,7 +138,7 @@ static void usb_xhci_pci_realize(struct PCIDevice
> *dev, Error **errp)
>
>      dev->config[PCI_CLASS_PROG] = 0x30;    /* xHCI */
>      dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
> -    dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
> +    dev->config[PCI_CACHE_LINE_SIZE] = s->cache_line_size;
>      dev->config[0x60] = 0x30; /* release number */
>
>      object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
> @@ -125,8 +151,16 @@ static void usb_xhci_pci_realize(struct PCIDevice
> *dev, Error **errp)
>          s->xhci.nec_quirks = true;
>      }
>
> +    if (s->pm_cap_off) {
> +        if (xhci_pci_add_pm_capability(dev, s->pm_cap_off, &err)) {
> +            error_propagate(errp, err);
> +            return;
>

Can't we just pass errp straight to xhci_pci_add_pm_capability and skip the
error_propagate() here?


> +        }
> +    }
> +
>      if (s->msi != ON_OFF_AUTO_OFF) {
> -        ret = msi_init(dev, 0x70, s->xhci.numintrs, true, false, &err);
> +        ret = msi_init(dev, s->msi_cap_off, s->xhci.numintrs,
> +                       true, false, &err);
>          /*
>           * Any error other than -ENOTSUP(board's MSI support is broken)
>           * is a programming error
> @@ -143,22 +177,37 @@ static void usb_xhci_pci_realize(struct PCIDevice
> *dev, Error **errp)
>          /* With msi=auto, we fall back to MSI off silently */
>          error_free(err);
>      }
> +
>      pci_register_bar(dev, 0,
>                       PCI_BASE_ADDRESS_SPACE_MEMORY |
>                       PCI_BASE_ADDRESS_MEM_TYPE_64,
>                       &s->xhci.mem);
>
>      if (pci_bus_is_express(pci_get_bus(dev))) {
> -        ret = pcie_endpoint_cap_init(dev, 0xa0);
> +        ret = pcie_endpoint_cap_init(dev, s->pcie_cap_off);
>          assert(ret > 0);
>      }
>
>      if (s->msix != ON_OFF_AUTO_OFF) {
> -        /* TODO check for errors, and should fail when msix=on */
> -        msix_init(dev, s->xhci.numintrs,
> -                  &s->xhci.mem, 0, OFF_MSIX_TABLE,
> -                  &s->xhci.mem, 0, OFF_MSIX_PBA,
> -                  0x90, NULL);
> +        MemoryRegion *msix_bar = &s->xhci.mem;
> +        if (s->msix_bar_nr != 0) {
> +            memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev),
> +                               "xhci-msix", s->msix_bar_size);
> +            msix_bar = &dev->msix_exclusive_bar;
> +        }
> +
> +        ret = msix_init(dev, s->xhci.numintrs,
> +                        msix_bar, s->msix_bar_nr, s->msix_table_off,
> +                        msix_bar, s->msix_bar_nr, s->msix_pba_off,
> +                        s->msix_cap_off, errp);
> +        if (ret) {
> +            return;
> +        }
>

Surely we should only propagate the error and fail realize() iff s->msix is
ON_OFF_AUTO_ON?

For ON_OFF_AUTO_AUTO, msix_init returning failure isn't a critical error.


> +
> +        pci_register_bar(dev, s->msix_bar_nr,
> +                         PCI_BASE_ADDRESS_SPACE_MEMORY |
> +                         PCI_BASE_ADDRESS_MEM_TYPE_64,
> +                         msix_bar);
>

Is it safe to call pci_register_bar() again for the msix_bar_nr = 0 case?
Even if it is safe, is it sensible? If we're calling it twice for the same
BAR, and the arguments of either of the calls changes in future, the other
needs to change too. Doesn't seem ideal.


>      }
>      s->xhci.as = pci_get_address_space(dev);
>  }
> --
> 2.45.2
>
>

[-- Attachment #2: Type: text/html, Size: 10938 bytes --]

Re: [PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[Nicholas Piggin]

On Thu Dec 12, 2024 at 8:41 PM AEST, Phil Dennis-Jordan wrote:
> Hey Nicholas,
>
> I'm not an XHCI & PCI expert (yet?) so apologies if I've got some of this
> wrong, but I've asked some questions and made some comments inline:

Hey Phil,

Thanks for the review, looks like you are the expert now :)

>
> On Thu, 12 Dec 2024 at 09:52, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> > To prepare to support another USB PCI Host Controller, make some PCI
> > configuration dynamic.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  hw/usb/hcd-xhci-pci.h |  9 ++++++
> >  hw/usb/hcd-xhci-nec.c | 10 +++++++
> >  hw/usb/hcd-xhci-pci.c | 69 ++++++++++++++++++++++++++++++++++++-------
> >  3 files changed, 78 insertions(+), 10 deletions(-)
> >
> > diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
> > index 08f70ce97cc..213076aabf6 100644
> > --- a/hw/usb/hcd-xhci-pci.h
> > +++ b/hw/usb/hcd-xhci-pci.h
> > @@ -40,6 +40,15 @@ typedef struct XHCIPciState {
> >      XHCIState xhci;
> >      OnOffAuto msi;
> >      OnOffAuto msix;
> > +    uint8_t cache_line_size;
> > +    uint8_t pm_cap_off;
> > +    uint8_t pcie_cap_off;
> > +    uint8_t msi_cap_off;
> > +    uint8_t msix_cap_off;
> > +    int msix_bar_nr;
> > +    uint64_t msix_bar_size;
> > +    uint32_t msix_table_off;
> > +    uint32_t msix_pba_off;
> >  } XHCIPciState;
> >
> >  #endif
> > diff --git a/hw/usb/hcd-xhci-nec.c b/hw/usb/hcd-xhci-nec.c
> > index 0e61c6c4f06..6ac1dc7764c 100644
> > --- a/hw/usb/hcd-xhci-nec.c
> > +++ b/hw/usb/hcd-xhci-nec.c
> > @@ -52,6 +52,16 @@ static void nec_xhci_instance_init(Object *obj)
> >
> >      pci->xhci.numintrs = nec->intrs;
> >      pci->xhci.numslots = nec->slots;
> > +
> > +    pci->cache_line_size = 0x10;
> > +    pci->pm_cap_off = 0;
> > +    pci->pcie_cap_off = 0xa0;
> > +    pci->msi_cap_off = 0x70;
> > +    pci->msix_cap_off = 0x90;
> > +    pci->msix_bar_nr = 0;
> > +    pci->msix_bar_size = 0;
> > +    pci->msix_table_off = 0x3000;
> > +    pci->msix_pba_off = 0x3800;
> >  }
>
>
> What about the "qemu-xhci" device, does that need similar treatment? I
> suspect it does at least for a bunch of these settings. Perhaps
> xhci_instance_init() in the abstract "pci-xhci" base might be a better
> place for these "sensible defaults" and then override them only in the
> specific implementations that need to do so, such as the new TI model?
> And/or have suitably named helper init function for configuring single-BAR
> PCI XHCI controllers so we can get some meaning behind all these magic
> numbers?

No you're right, I missed this entirely and the qemu-xhci dev is
indeed broken after this patch. Just moving it into the parent
instance init gets it to work.

> >  static void nec_xhci_class_init(ObjectClass *klass, void *data)
> > diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> > index a039f5778a6..948d75b7379 100644
> > --- a/hw/usb/hcd-xhci-pci.c
> > +++ b/hw/usb/hcd-xhci-pci.c
> > @@ -32,8 +32,9 @@
> >  #include "trace.h"
> >  #include "qapi/error.h"
> >
> > -#define OFF_MSIX_TABLE  0x3000
> > -#define OFF_MSIX_PBA    0x3800
> > +#define MSIX_BAR_SIZE   0x800000
> >
>
> MSIX_BAR_SIZE doesn't seem to be used anywhere, and patch 2/2 uses 0x800000
> explicitly. (8 MiB also seems… huge? But I'm guessing you're matching this
> with the physical TI controller hardware - either way I don't think it
> belongs in this file.)
>
>
> > +#define OFF_MSIX_TABLE  0x0000
> > +#define OFF_MSIX_PBA    0x1000
> >
>
> Maybe instead of redefining these constants to only apply to the split BAR
> device variants, there should be 2 variants of them, one for single-BAR
> controllers, and one for controllers with separate BARs. That would also
> help make sense of the "magic numbers" in nec_xhci_instance_init().

You're right on both counts, I tidied these up.

> >  static void xhci_pci_intr_update(XHCIState *xhci, int n, bool enable)
> >  {
> > @@ -104,6 +105,31 @@ static int xhci_pci_vmstate_post_load(void *opaque,
> > int version_id)
> >     return 0;
> >  }
> >
> > +static int xhci_pci_add_pm_capability(PCIDevice *pci_dev, uint8_t offset,
> > +                                      Error **errp)
> > +{
> > +    int err;
> > +
> > +    err = pci_add_capability(pci_dev, PCI_CAP_ID_PM, offset,
> > +                             PCI_PM_SIZEOF, errp);
> > +    if (err < 0) {
> > +        return err;
> > +    }
> > +
> > +    pci_set_word(pci_dev->config + offset + PCI_PM_PMC,
> > +                 PCI_PM_CAP_VER_1_2 |
> > +                 PCI_PM_CAP_D1 | PCI_PM_CAP_D2 |
> > +                 PCI_PM_CAP_PME_D0 | PCI_PM_CAP_PME_D1 |
> > +                 PCI_PM_CAP_PME_D2 | PCI_PM_CAP_PME_D3hot);
> > +    pci_set_word(pci_dev->wmask + offset + PCI_PM_PMC, 0);
> > +    pci_set_word(pci_dev->config + offset + PCI_PM_CTRL,
> > +                 PCI_PM_CTRL_NO_SOFT_RESET);
> > +    pci_set_word(pci_dev->wmask + offset + PCI_PM_CTRL,
> > +                 PCI_PM_CTRL_STATE_MASK);
> > +
> > +    return 0;
> > +}
> > +
> >  static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
> >  {
> >      int ret;
> > @@ -112,7 +138,7 @@ static void usb_xhci_pci_realize(struct PCIDevice
> > *dev, Error **errp)
> >
> >      dev->config[PCI_CLASS_PROG] = 0x30;    /* xHCI */
> >      dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
> > -    dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
> > +    dev->config[PCI_CACHE_LINE_SIZE] = s->cache_line_size;
> >      dev->config[0x60] = 0x30; /* release number */
> >
> >      object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
> > @@ -125,8 +151,16 @@ static void usb_xhci_pci_realize(struct PCIDevice
> > *dev, Error **errp)
> >          s->xhci.nec_quirks = true;
> >      }
> >
> > +    if (s->pm_cap_off) {
> > +        if (xhci_pci_add_pm_capability(dev, s->pm_cap_off, &err)) {
> > +            error_propagate(errp, err);
> > +            return;
> >
>
> Can't we just pass errp straight to xhci_pci_add_pm_capability and skip the
> error_propagate() here?

Yes I think so.

> > +        }
> > +    }
> > +
> >      if (s->msi != ON_OFF_AUTO_OFF) {
> > -        ret = msi_init(dev, 0x70, s->xhci.numintrs, true, false, &err);
> > +        ret = msi_init(dev, s->msi_cap_off, s->xhci.numintrs,
> > +                       true, false, &err);
> >          /*
> >           * Any error other than -ENOTSUP(board's MSI support is broken)
> >           * is a programming error
> > @@ -143,22 +177,37 @@ static void usb_xhci_pci_realize(struct PCIDevice
> > *dev, Error **errp)
> >          /* With msi=auto, we fall back to MSI off silently */
> >          error_free(err);
> >      }
> > +
> >      pci_register_bar(dev, 0,
> >                       PCI_BASE_ADDRESS_SPACE_MEMORY |
> >                       PCI_BASE_ADDRESS_MEM_TYPE_64,
> >                       &s->xhci.mem);
> >
> >      if (pci_bus_is_express(pci_get_bus(dev))) {
> > -        ret = pcie_endpoint_cap_init(dev, 0xa0);
> > +        ret = pcie_endpoint_cap_init(dev, s->pcie_cap_off);
> >          assert(ret > 0);
> >      }
> >
> >      if (s->msix != ON_OFF_AUTO_OFF) {
> > -        /* TODO check for errors, and should fail when msix=on */
> > -        msix_init(dev, s->xhci.numintrs,
> > -                  &s->xhci.mem, 0, OFF_MSIX_TABLE,
> > -                  &s->xhci.mem, 0, OFF_MSIX_PBA,
> > -                  0x90, NULL);
> > +        MemoryRegion *msix_bar = &s->xhci.mem;
> > +        if (s->msix_bar_nr != 0) {
> > +            memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev),
> > +                               "xhci-msix", s->msix_bar_size);
> > +            msix_bar = &dev->msix_exclusive_bar;
> > +        }
> > +
> > +        ret = msix_init(dev, s->xhci.numintrs,
> > +                        msix_bar, s->msix_bar_nr, s->msix_table_off,
> > +                        msix_bar, s->msix_bar_nr, s->msix_pba_off,
> > +                        s->msix_cap_off, errp);
> > +        if (ret) {
> > +            return;
> > +        }
> >
>
> Surely we should only propagate the error and fail realize() iff s->msix is
> ON_OFF_AUTO_ON?
>
> For ON_OFF_AUTO_AUTO, msix_init returning failure isn't a critical error.

Yep you're right... you had been testing with msix disabled. I wonder if
there is a good way to force fail this in qtests?

> > +
> > +        pci_register_bar(dev, s->msix_bar_nr,
> > +                         PCI_BASE_ADDRESS_SPACE_MEMORY |
> > +                         PCI_BASE_ADDRESS_MEM_TYPE_64,
> > +                         msix_bar);
> >
>
> Is it safe to call pci_register_bar() again for the msix_bar_nr = 0 case?
> Even if it is safe, is it sensible? If we're calling it twice for the same
> BAR, and the arguments of either of the calls changes in future, the other
> needs to change too. Doesn't seem ideal.

Good catch. It looks like it "works" so long as the bar wasn't mapped,
but I'm sure bad practice... Interesting there is no assertion in
there though. I'll fix it though.

Thanks,
Nick

Re: [PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[Phil Dennis-Jordan]

[-- Attachment #1: Type: text/plain, Size: 11613 bytes --]

On Wed, 18 Dec 2024 at 02:19, Nicholas Piggin <npiggin@gmail.com> wrote:

> On Thu Dec 12, 2024 at 8:41 PM AEST, Phil Dennis-Jordan wrote:
> > Hey Nicholas,
> >
> > I'm not an XHCI & PCI expert (yet?) so apologies if I've got some of this
> > wrong, but I've asked some questions and made some comments inline:
>
> Hey Phil,
>
> Thanks for the review, looks like you are the expert now :)
>

The "hot potato" method for determining maintainership. :-)


> >
> > On Thu, 12 Dec 2024 at 09:52, Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > > To prepare to support another USB PCI Host Controller, make some PCI
> > > configuration dynamic.
> > >
> > > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > > ---
> > >  hw/usb/hcd-xhci-pci.h |  9 ++++++
> > >  hw/usb/hcd-xhci-nec.c | 10 +++++++
> > >  hw/usb/hcd-xhci-pci.c | 69 ++++++++++++++++++++++++++++++++++++-------
> > >  3 files changed, 78 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
> > > index 08f70ce97cc..213076aabf6 100644
> > > --- a/hw/usb/hcd-xhci-pci.h
> > > +++ b/hw/usb/hcd-xhci-pci.h
> > > @@ -40,6 +40,15 @@ typedef struct XHCIPciState {
> > >      XHCIState xhci;
> > >      OnOffAuto msi;
> > >      OnOffAuto msix;
> > > +    uint8_t cache_line_size;
> > > +    uint8_t pm_cap_off;
> > > +    uint8_t pcie_cap_off;
> > > +    uint8_t msi_cap_off;
> > > +    uint8_t msix_cap_off;
> > > +    int msix_bar_nr;
> > > +    uint64_t msix_bar_size;
> > > +    uint32_t msix_table_off;
> > > +    uint32_t msix_pba_off;
> > >  } XHCIPciState;
> > >
> > >  #endif
> > > diff --git a/hw/usb/hcd-xhci-nec.c b/hw/usb/hcd-xhci-nec.c
> > > index 0e61c6c4f06..6ac1dc7764c 100644
> > > --- a/hw/usb/hcd-xhci-nec.c
> > > +++ b/hw/usb/hcd-xhci-nec.c
> > > @@ -52,6 +52,16 @@ static void nec_xhci_instance_init(Object *obj)
> > >
> > >      pci->xhci.numintrs = nec->intrs;
> > >      pci->xhci.numslots = nec->slots;
> > > +
> > > +    pci->cache_line_size = 0x10;
> > > +    pci->pm_cap_off = 0;
> > > +    pci->pcie_cap_off = 0xa0;
> > > +    pci->msi_cap_off = 0x70;
> > > +    pci->msix_cap_off = 0x90;
> > > +    pci->msix_bar_nr = 0;
> > > +    pci->msix_bar_size = 0;
> > > +    pci->msix_table_off = 0x3000;
> > > +    pci->msix_pba_off = 0x3800;
> > >  }
> >
> >
> > What about the "qemu-xhci" device, does that need similar treatment? I
> > suspect it does at least for a bunch of these settings. Perhaps
> > xhci_instance_init() in the abstract "pci-xhci" base might be a better
> > place for these "sensible defaults" and then override them only in the
> > specific implementations that need to do so, such as the new TI model?
> > And/or have suitably named helper init function for configuring
> single-BAR
> > PCI XHCI controllers so we can get some meaning behind all these magic
> > numbers?
>
> No you're right, I missed this entirely and the qemu-xhci dev is
> indeed broken after this patch. Just moving it into the parent
> instance init gets it to work.
>
> > >  static void nec_xhci_class_init(ObjectClass *klass, void *data)
> > > diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> > > index a039f5778a6..948d75b7379 100644
> > > --- a/hw/usb/hcd-xhci-pci.c
> > > +++ b/hw/usb/hcd-xhci-pci.c
> > > @@ -32,8 +32,9 @@
> > >  #include "trace.h"
> > >  #include "qapi/error.h"
> > >
> > > -#define OFF_MSIX_TABLE  0x3000
> > > -#define OFF_MSIX_PBA    0x3800
> > > +#define MSIX_BAR_SIZE   0x800000
> > >
> >
> > MSIX_BAR_SIZE doesn't seem to be used anywhere, and patch 2/2 uses
> 0x800000
> > explicitly. (8 MiB also seems… huge? But I'm guessing you're matching
> this
> > with the physical TI controller hardware - either way I don't think it
> > belongs in this file.)
> >
> >
> > > +#define OFF_MSIX_TABLE  0x0000
> > > +#define OFF_MSIX_PBA    0x1000
> > >
> >
> > Maybe instead of redefining these constants to only apply to the split
> BAR
> > device variants, there should be 2 variants of them, one for single-BAR
> > controllers, and one for controllers with separate BARs. That would also
> > help make sense of the "magic numbers" in nec_xhci_instance_init().
>
> You're right on both counts, I tidied these up.
>
> > >  static void xhci_pci_intr_update(XHCIState *xhci, int n, bool enable)
> > >  {
> > > @@ -104,6 +105,31 @@ static int xhci_pci_vmstate_post_load(void
> *opaque,
> > > int version_id)
> > >     return 0;
> > >  }
> > >
> > > +static int xhci_pci_add_pm_capability(PCIDevice *pci_dev, uint8_t
> offset,
> > > +                                      Error **errp)
> > > +{
> > > +    int err;
> > > +
> > > +    err = pci_add_capability(pci_dev, PCI_CAP_ID_PM, offset,
> > > +                             PCI_PM_SIZEOF, errp);
> > > +    if (err < 0) {
> > > +        return err;
> > > +    }
> > > +
> > > +    pci_set_word(pci_dev->config + offset + PCI_PM_PMC,
> > > +                 PCI_PM_CAP_VER_1_2 |
> > > +                 PCI_PM_CAP_D1 | PCI_PM_CAP_D2 |
> > > +                 PCI_PM_CAP_PME_D0 | PCI_PM_CAP_PME_D1 |
> > > +                 PCI_PM_CAP_PME_D2 | PCI_PM_CAP_PME_D3hot);
> > > +    pci_set_word(pci_dev->wmask + offset + PCI_PM_PMC, 0);
> > > +    pci_set_word(pci_dev->config + offset + PCI_PM_CTRL,
> > > +                 PCI_PM_CTRL_NO_SOFT_RESET);
> > > +    pci_set_word(pci_dev->wmask + offset + PCI_PM_CTRL,
> > > +                 PCI_PM_CTRL_STATE_MASK);
> > > +
> > > +    return 0;
> > > +}
> > > +
> > >  static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
> > >  {
> > >      int ret;
> > > @@ -112,7 +138,7 @@ static void usb_xhci_pci_realize(struct PCIDevice
> > > *dev, Error **errp)
> > >
> > >      dev->config[PCI_CLASS_PROG] = 0x30;    /* xHCI */
> > >      dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
> > > -    dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
> > > +    dev->config[PCI_CACHE_LINE_SIZE] = s->cache_line_size;
> > >      dev->config[0x60] = 0x30; /* release number */
> > >
> > >      object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s),
> NULL);
> > > @@ -125,8 +151,16 @@ static void usb_xhci_pci_realize(struct PCIDevice
> > > *dev, Error **errp)
> > >          s->xhci.nec_quirks = true;
> > >      }
> > >
> > > +    if (s->pm_cap_off) {
> > > +        if (xhci_pci_add_pm_capability(dev, s->pm_cap_off, &err)) {
> > > +            error_propagate(errp, err);
> > > +            return;
> > >
> >
> > Can't we just pass errp straight to xhci_pci_add_pm_capability and skip
> the
> > error_propagate() here?
>
> Yes I think so.
>
> > > +        }
> > > +    }
> > > +
> > >      if (s->msi != ON_OFF_AUTO_OFF) {
> > > -        ret = msi_init(dev, 0x70, s->xhci.numintrs, true, false,
> &err);
> > > +        ret = msi_init(dev, s->msi_cap_off, s->xhci.numintrs,
> > > +                       true, false, &err);
> > >          /*
> > >           * Any error other than -ENOTSUP(board's MSI support is
> broken)
> > >           * is a programming error
> > > @@ -143,22 +177,37 @@ static void usb_xhci_pci_realize(struct PCIDevice
> > > *dev, Error **errp)
> > >          /* With msi=auto, we fall back to MSI off silently */
> > >          error_free(err);
> > >      }
> > > +
> > >      pci_register_bar(dev, 0,
> > >                       PCI_BASE_ADDRESS_SPACE_MEMORY |
> > >                       PCI_BASE_ADDRESS_MEM_TYPE_64,
> > >                       &s->xhci.mem);
> > >
> > >      if (pci_bus_is_express(pci_get_bus(dev))) {
> > > -        ret = pcie_endpoint_cap_init(dev, 0xa0);
> > > +        ret = pcie_endpoint_cap_init(dev, s->pcie_cap_off);
> > >          assert(ret > 0);
> > >      }
> > >
> > >      if (s->msix != ON_OFF_AUTO_OFF) {
> > > -        /* TODO check for errors, and should fail when msix=on */
> > > -        msix_init(dev, s->xhci.numintrs,
> > > -                  &s->xhci.mem, 0, OFF_MSIX_TABLE,
> > > -                  &s->xhci.mem, 0, OFF_MSIX_PBA,
> > > -                  0x90, NULL);
> > > +        MemoryRegion *msix_bar = &s->xhci.mem;
> > > +        if (s->msix_bar_nr != 0) {
> > > +            memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev),
> > > +                               "xhci-msix", s->msix_bar_size);
> > > +            msix_bar = &dev->msix_exclusive_bar;
> > > +        }
> > > +
> > > +        ret = msix_init(dev, s->xhci.numintrs,
> > > +                        msix_bar, s->msix_bar_nr, s->msix_table_off,
> > > +                        msix_bar, s->msix_bar_nr, s->msix_pba_off,
> > > +                        s->msix_cap_off, errp);
> > > +        if (ret) {
> > > +            return;
> > > +        }
> > >
> >
> > Surely we should only propagate the error and fail realize() iff s->msix
> is
> > ON_OFF_AUTO_ON?
> >
> > For ON_OFF_AUTO_AUTO, msix_init returning failure isn't a critical error.
>
> Yep you're right... you had been testing with msix disabled. I wonder if
> there is a good way to force fail this in qtests?
>

I'm really the wrong person to ask about qtest, I'm only just beginning to
get to grips with it. It seems the only real reason msix_init fails other
than misconfiguration of the device/BAR is when msi_nonbroken = false.

At least on x86(-64), msi_nonbroken=true is unconditionally set in
apic_realize(). (I think real hardware would not support MSI(-X) on the
i440FX chipset - I was fairly certain it was the PCI root/southbridge
catching the writes to the reserved memory region, and I didn't think the
PIIX did this; but at least in QEMU it doesn't seem to be implemented in a
chipset-dependent way.) I'm not sure it's possible to run QEMU without an
APIC?

On aarch64, the GICv3 needs to explicitly enable support (via the ITS), so
perhaps it's possible to set up an aarch64 qtest with ITS disabled? It
looks like the 'virt' machine type only supports the ITS from version 6.2,
so older versions will disable it.

Sorry, clutching at straws here.


> > > +
> > > +        pci_register_bar(dev, s->msix_bar_nr,
> > > +                         PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > +                         PCI_BASE_ADDRESS_MEM_TYPE_64,
> > > +                         msix_bar);
> > >
> >
> > Is it safe to call pci_register_bar() again for the msix_bar_nr = 0 case?
> > Even if it is safe, is it sensible? If we're calling it twice for the
> same
> > BAR, and the arguments of either of the calls changes in future, the
> other
> > needs to change too. Doesn't seem ideal.
>
> Good catch. It looks like it "works" so long as the bar wasn't mapped,
> but I'm sure bad practice... Interesting there is no assertion in
> there though. I'll fix it though.
>

I notice there's a msix_init_exclusive_bar()… I wonder if it'd be simpler
to use that and modify it so it allows you to choose a size and layout for
the BAR, rather than adding all that extra code to deal with the extra BAR
in the XHCI?
(It already calls pci_register_bar() and msix_init() internally, but seems
to set the BAR's size to 4096 and places the PBA at halfway through the
BAR. Perhaps rename it to something like
msix_init_exclusive_bar_with_layout and pass the bar_size and
bar_pba_offset in as parameters; then make msix_init_exclusive_bar() a
wrapper for that function with the existing defaults for those variables?)

Just kicking around some ideas here, I have no idea if that actually ends
up making things simpler…


> Thanks,
> Nick
>

[-- Attachment #2: Type: text/html, Size: 15205 bytes --]

Re: [PATCH v2 2/2] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model

[Bernhard Beschow]

Am 12. Dezember 2024 08:52:07 UTC schrieb Nicholas Piggin <npiggin@gmail.com>:
>The TI TUSB73X0 controller has some interesting differences from NEC,
>notably a separate BAR for MSIX, and PM capabilities. The spec is freely
>available without sign-up.
>
>This controller is accepted by IBM Power proprietary firmware and
>software (when the subsystem IDs are set to Power servers, which is not
>done here). IBM code is picky about device support, so the NEC device
>can not be used.
>
>Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>---
> include/hw/pci/pci_ids.h        |  1 +
> include/hw/usb/xhci.h           |  1 +
> hw/usb/hcd-xhci-ti.c            | 92 +++++++++++++++++++++++++++++++++
> tests/qtest/usb-hcd-xhci-test.c | 21 +++++---
> hw/usb/Kconfig                  |  5 ++
> hw/usb/meson.build              |  1 +
> 6 files changed, 115 insertions(+), 6 deletions(-)
> create mode 100644 hw/usb/hcd-xhci-ti.c
>
>diff --git a/include/hw/pci/pci_ids.h b/include/hw/pci/pci_ids.h
>index f1a53fea8d6..fdb692db513 100644
>--- a/include/hw/pci/pci_ids.h
>+++ b/include/hw/pci/pci_ids.h
>@@ -182,6 +182,7 @@
> #define PCI_VENDOR_ID_HP                 0x103c
> 
> #define PCI_VENDOR_ID_TI                 0x104c
>+#define PCI_DEVICE_ID_TI_TUSB73X0        0x8241
> 
> #define PCI_VENDOR_ID_MOTOROLA           0x1057
> #define PCI_DEVICE_ID_MOTOROLA_MPC106    0x0002
>diff --git a/include/hw/usb/xhci.h b/include/hw/usb/xhci.h
>index 5c90e1373e5..203ec1fca32 100644
>--- a/include/hw/usb/xhci.h
>+++ b/include/hw/usb/xhci.h
>@@ -3,6 +3,7 @@
> 
> #define TYPE_XHCI "base-xhci"
> #define TYPE_NEC_XHCI "nec-usb-xhci"
>+#define TYPE_TI_XHCI "ti-usb-xhci"
> #define TYPE_QEMU_XHCI "qemu-xhci"
> #define TYPE_XHCI_SYSBUS "sysbus-xhci"
> 
>diff --git a/hw/usb/hcd-xhci-ti.c b/hw/usb/hcd-xhci-ti.c
>new file mode 100644
>index 00000000000..6d4b44f6aaf
>--- /dev/null
>+++ b/hw/usb/hcd-xhci-ti.c
>@@ -0,0 +1,92 @@
>+/*
>+ * USB xHCI controller emulation
>+ * Datasheet https://www.ti.com/product/TUSB7340
>+ *
>+ * Copyright (c) 2011 Securiforest
>+ * Date: 2011-05-11 ;  Author: Hector Martin <hector@marcansoft.com>
>+ * Based on usb-xhci-nec.c, emulates TI TUSB73X0
>+ *
>+ * This library is free software; you can redistribute it and/or
>+ * modify it under the terms of the GNU Lesser General Public
>+ * License as published by the Free Software Foundation; either
>+ * version 2.1 of the License, or (at your option) any later version.
>+ *
>+ * This library is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ * Lesser General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU Lesser General Public
>+ * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>+ */
>+
>+#include "qemu/osdep.h"
>+#include "hw/usb.h"
>+#include "qemu/module.h"
>+#include "hw/pci/pci.h"
>+#include "hw/qdev-properties.h"
>+
>+#include "hcd-xhci-pci.h"
>+
>+OBJECT_DECLARE_SIMPLE_TYPE(XHCITiState, TI_XHCI)
>+
>+struct XHCITiState {
>+    /*< private >*/
>+    XHCIPciState parent_obj;
>+    /*< public >*/

These markers are obsolete. Instead, a blank line after parent_obj should be inserted.

>+    uint32_t intrs;
>+    uint32_t slots;
>+};
>+
>+static Property ti_xhci_properties[] = {

s/static Property/static const Property/ as of recent tree-wide changes.

Best regards,
Bernhard

>+    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
>+    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
>+    DEFINE_PROP_UINT32("intrs", XHCITiState, intrs, 8),
>+    DEFINE_PROP_UINT32("slots", XHCITiState, slots, XHCI_MAXSLOTS),
>+    DEFINE_PROP_END_OF_LIST(),
>+};
>+
>+static void ti_xhci_instance_init(Object *obj)
>+{
>+    XHCIPciState *pci = XHCI_PCI(obj);
>+    XHCITiState *ti = TI_XHCI(obj);
>+
>+    pci->xhci.numintrs = ti->intrs;
>+    pci->xhci.numslots = ti->slots;
>+
>+    pci->cache_line_size = 0x0;
>+    pci->pm_cap_off = 0x40;
>+    pci->pcie_cap_off = 0x70;
>+    pci->msi_cap_off = 0x48;
>+    pci->msix_cap_off = 0xc0;
>+    pci->msix_bar_nr = 0x2;
>+    pci->msix_bar_size = 0x800000;
>+    pci->msix_table_off = 0x0;
>+    pci->msix_pba_off = 0x1000;
>+}
>+
>+static void ti_xhci_class_init(ObjectClass *klass, void *data)
>+{
>+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>+    DeviceClass *dc = DEVICE_CLASS(klass);
>+
>+    device_class_set_props(dc, ti_xhci_properties);
>+    k->vendor_id    = PCI_VENDOR_ID_TI;
>+    k->device_id    = PCI_DEVICE_ID_TI_TUSB73X0;
>+    k->revision     = 0x02;
>+}
>+
>+static const TypeInfo ti_xhci_info = {
>+    .name          = TYPE_TI_XHCI,
>+    .parent        = TYPE_XHCI_PCI,
>+    .instance_size = sizeof(XHCITiState),
>+    .instance_init = ti_xhci_instance_init,
>+    .class_init    = ti_xhci_class_init,
>+};
>+
>+static void ti_xhci_register_types(void)
>+{
>+    type_register_static(&ti_xhci_info);
>+}
>+
>+type_init(ti_xhci_register_types)
>diff --git a/tests/qtest/usb-hcd-xhci-test.c b/tests/qtest/usb-hcd-xhci-test.c
>index 93614e55461..d4a0d8cc217 100644
>--- a/tests/qtest/usb-hcd-xhci-test.c
>+++ b/tests/qtest/usb-hcd-xhci-test.c
>@@ -59,6 +59,9 @@ typedef struct XHCIQState {
> #define XHCI_NEC_ID (PCI_DEVICE_ID_NEC_UPD720200 << 16 | \
>                      PCI_VENDOR_ID_NEC)
> 
>+#define XHCI_TI_ID  (PCI_DEVICE_ID_TI_TUSB73X0 << 16 | \
>+                     PCI_VENDOR_ID_TI)
>+
> /**
>  * Locate, verify, and return a handle to the XHCI device.
>  */
>@@ -78,6 +81,8 @@ static QPCIDevice *get_xhci_device(QTestState *qts, uint32_t *fingerprint)
>     switch (xhci_fingerprint) {
>     case XHCI_NEC_ID:
>         break;
>+    case XHCI_TI_ID:
>+        break;
>     default:
>         /* Unknown device. */
>         g_assert_not_reached();
>@@ -134,11 +139,12 @@ static XHCIQState *xhci_boot(const char *cli, ...)
>         va_end(ap);
>     } else {
>         s = xhci_boot("-M q35 "
>-                      "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
>+                      "-device %s,id=xhci,bus=pcie.0,addr=1d.0 "
>                       "-drive id=drive0,if=none,file=null-co://,"
>-                          "file.read-zeroes=on,format=raw");
>+                          "file.read-zeroes=on,format=raw",
>+                      qtest_has_device("ti-usb-xhci") ?
>+                          "ti-usb-xhci" : "nec-usb-xhci");
>     }
>-
>     return s;
> }
> 
>@@ -392,10 +398,12 @@ static void pci_xhci_stress_rings(void)
>     int i;
> 
>     s = xhci_boot("-M q35 "
>-            "-device nec-usb-xhci,id=xhci,bus=pcie.0,addr=1d.0 "
>+            "-device %s,id=xhci,bus=pcie.0,addr=1d.0 "
>             "-device usb-storage,bus=xhci.0,drive=drive0 "
>             "-drive id=drive0,if=none,file=null-co://,"
>-                "file.read-zeroes=on,format=raw "
>+                "file.read-zeroes=on,format=raw ",
>+            qtest_has_device("ti-usb-xhci") ?
>+                "ti-usb-xhci" : "nec-usb-xhci"
>             );
> 
>     hcsparams1 = xhci_cap_readl(s, 0x4); /* HCSPARAMS1 */
>@@ -567,7 +575,8 @@ int main(int argc, char **argv)
>         return 0;
>     }
> 
>-    if (!qtest_has_device("nec-usb-xhci")) {
>+    if (!qtest_has_device("nec-usb-xhci") &&
>+        !qtest_has_device("ti-usb-xhci")) {
>         return 0;
>     }
> 
>diff --git a/hw/usb/Kconfig b/hw/usb/Kconfig
>index 5fbecd2f43b..8e5c4747af9 100644
>--- a/hw/usb/Kconfig
>+++ b/hw/usb/Kconfig
>@@ -49,6 +49,11 @@ config USB_XHCI_NEC
>     default y if PCI_DEVICES
>     select USB_XHCI_PCI
> 
>+config USB_XHCI_TI
>+    bool
>+    default y if PCI_DEVICES
>+    select USB_XHCI_PCI
>+
> config USB_XHCI_SYSBUS
>     bool
>     select USB_XHCI
>diff --git a/hw/usb/meson.build b/hw/usb/meson.build
>index 1b4d1507e41..b874a93f16e 100644
>--- a/hw/usb/meson.build
>+++ b/hw/usb/meson.build
>@@ -23,6 +23,7 @@ system_ss.add(when: 'CONFIG_USB_XHCI', if_true: files('hcd-xhci.c'))
> system_ss.add(when: 'CONFIG_USB_XHCI_PCI', if_true: files('hcd-xhci-pci.c'))
> system_ss.add(when: 'CONFIG_USB_XHCI_SYSBUS', if_true: files('hcd-xhci-sysbus.c'))
> system_ss.add(when: 'CONFIG_USB_XHCI_NEC', if_true: files('hcd-xhci-nec.c'))
>+system_ss.add(when: 'CONFIG_USB_XHCI_TI', if_true: files('hcd-xhci-ti.c'))
> system_ss.add(when: 'CONFIG_USB_DWC2', if_true: files('hcd-dwc2.c'))
> system_ss.add(when: 'CONFIG_USB_DWC3', if_true: files('hcd-dwc3.c'))
> 

Re: [PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[Nicholas Piggin]

On Thu Dec 19, 2024 at 7:06 AM AEST, Phil Dennis-Jordan wrote:
> On Wed, 18 Dec 2024 at 02:19, Nicholas Piggin <npiggin@gmail.com> wrote:
>
> > On Thu Dec 12, 2024 at 8:41 PM AEST, Phil Dennis-Jordan wrote:

[...]

> > > > @@ -143,22 +177,37 @@ static void usb_xhci_pci_realize(struct PCIDevice
> > > > *dev, Error **errp)
> > > >          /* With msi=auto, we fall back to MSI off silently */
> > > >          error_free(err);
> > > >      }
> > > > +
> > > >      pci_register_bar(dev, 0,
> > > >                       PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > >                       PCI_BASE_ADDRESS_MEM_TYPE_64,
> > > >                       &s->xhci.mem);
> > > >
> > > >      if (pci_bus_is_express(pci_get_bus(dev))) {
> > > > -        ret = pcie_endpoint_cap_init(dev, 0xa0);
> > > > +        ret = pcie_endpoint_cap_init(dev, s->pcie_cap_off);
> > > >          assert(ret > 0);
> > > >      }
> > > >
> > > >      if (s->msix != ON_OFF_AUTO_OFF) {
> > > > -        /* TODO check for errors, and should fail when msix=on */
> > > > -        msix_init(dev, s->xhci.numintrs,
> > > > -                  &s->xhci.mem, 0, OFF_MSIX_TABLE,
> > > > -                  &s->xhci.mem, 0, OFF_MSIX_PBA,
> > > > -                  0x90, NULL);
> > > > +        MemoryRegion *msix_bar = &s->xhci.mem;
> > > > +        if (s->msix_bar_nr != 0) {
> > > > +            memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev),
> > > > +                               "xhci-msix", s->msix_bar_size);
> > > > +            msix_bar = &dev->msix_exclusive_bar;
> > > > +        }
> > > > +
> > > > +        ret = msix_init(dev, s->xhci.numintrs,
> > > > +                        msix_bar, s->msix_bar_nr, s->msix_table_off,
> > > > +                        msix_bar, s->msix_bar_nr, s->msix_pba_off,
> > > > +                        s->msix_cap_off, errp);
> > > > +        if (ret) {
> > > > +            return;
> > > > +        }
> > > >
> > >
> > > Surely we should only propagate the error and fail realize() iff s->msix
> > is
> > > ON_OFF_AUTO_ON?
> > >
> > > For ON_OFF_AUTO_AUTO, msix_init returning failure isn't a critical error.
> >
> > Yep you're right... you had been testing with msix disabled. I wonder if
> > there is a good way to force fail this in qtests?
> >
>
> I'm really the wrong person to ask about qtest, I'm only just beginning to
> get to grips with it.

I'm not an expert in it, for the most part it can set up a machine as
usual, but the test case itself pokes at the machine directly by
talking to an interface on the host that can run memory access, qmp
commands, etc.

Can just make things easier and faster to set up and orchestrate than
doing it from within the target machine code.

> It seems the only real reason msix_init fails other
> than misconfiguration of the device/BAR is when msi_nonbroken = false.
>
> At least on x86(-64), msi_nonbroken=true is unconditionally set in
> apic_realize(). (I think real hardware would not support MSI(-X) on the
> i440FX chipset - I was fairly certain it was the PCI root/southbridge
> catching the writes to the reserved memory region, and I didn't think the
> PIIX did this; but at least in QEMU it doesn't seem to be implemented in a
> chipset-dependent way.) I'm not sure it's possible to run QEMU without an
> APIC?
>
> On aarch64, the GICv3 needs to explicitly enable support (via the ITS), so
> perhaps it's possible to set up an aarch64 qtest with ITS disabled? It
> looks like the 'virt' machine type only supports the ITS from version 6.2,
> so older versions will disable it.
>
> Sorry, clutching at straws here.

No that's okay, thanks for the input. Finding a platform with
broken msi could be an interesting test. I'll check it out.

> > > > +
> > > > +        pci_register_bar(dev, s->msix_bar_nr,
> > > > +                         PCI_BASE_ADDRESS_SPACE_MEMORY |
> > > > +                         PCI_BASE_ADDRESS_MEM_TYPE_64,
> > > > +                         msix_bar);
> > > >
> > >
> > > Is it safe to call pci_register_bar() again for the msix_bar_nr = 0 case?
> > > Even if it is safe, is it sensible? If we're calling it twice for the
> > same
> > > BAR, and the arguments of either of the calls changes in future, the
> > other
> > > needs to change too. Doesn't seem ideal.
> >
> > Good catch. It looks like it "works" so long as the bar wasn't mapped,
> > but I'm sure bad practice... Interesting there is no assertion in
> > there though. I'll fix it though.
> >
>
> I notice there's a msix_init_exclusive_bar()… I wonder if it'd be simpler
> to use that and modify it so it allows you to choose a size and layout for
> the BAR, rather than adding all that extra code to deal with the extra BAR
> in the XHCI?
> (It already calls pci_register_bar() and msix_init() internally, but seems
> to set the BAR's size to 4096 and places the PBA at halfway through the
> BAR. Perhaps rename it to something like
> msix_init_exclusive_bar_with_layout and pass the bar_size and
> bar_pba_offset in as parameters; then make msix_init_exclusive_bar() a
> wrapper for that function with the existing defaults for those variables?)
>
> Just kicking around some ideas here, I have no idea if that actually ends
> up making things simpler…

Yeah, I ended up beginning with that, but ended up running into some of
these issues and ended up being more code due to duplicating the non
exclusive case.

I'll stick with open-coding it for now, but it almost seems like there
could be an API call that could encompass exclusive and non-exclusive
cases in one. Would probably be good to have more than one caller before
trying to refactor it though.

Thanks,
Nick

Re: [PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3749 bytes --]

On Wed, 18 Dec 2024, Phil Dennis-Jordan wrote:
> On Wed, 18 Dec 2024 at 02:19, Nicholas Piggin <npiggin@gmail.com> wrote:
>> On Thu Dec 12, 2024 at 8:41 PM AEST, Phil Dennis-Jordan wrote:
>>> Hey Nicholas,
>>>
>>> I'm not an XHCI & PCI expert (yet?) so apologies if I've got some of this
>>> wrong, but I've asked some questions and made some comments inline:
>>
>> Hey Phil,
>>
>> Thanks for the review, looks like you are the expert now :)
>>
>
> The "hot potato" method for determining maintainership. :-)

That's how I got some parts I'm maintainer of now. :-)

[...]
>>> On Thu, 12 Dec 2024 at 09:52, Nicholas Piggin <npiggin@gmail.com> wrote:
>>> Surely we should only propagate the error and fail realize() iff s->msix
>> is
>>> ON_OFF_AUTO_ON?
>>>
>>> For ON_OFF_AUTO_AUTO, msix_init returning failure isn't a critical error.
>>
>> Yep you're right... you had been testing with msix disabled. I wonder if
>> there is a good way to force fail this in qtests?
>>
>
> I'm really the wrong person to ask about qtest, I'm only just beginning to
> get to grips with it. It seems the only real reason msix_init fails other
> than misconfiguration of the device/BAR is when msi_nonbroken = false.
>
> At least on x86(-64), msi_nonbroken=true is unconditionally set in
> apic_realize(). (I think real hardware would not support MSI(-X) on the
> i440FX chipset - I was fairly certain it was the PCI root/southbridge
> catching the writes to the reserved memory region, and I didn't think the
> PIIX did this; but at least in QEMU it doesn't seem to be implemented in a
> chipset-dependent way.) I'm not sure it's possible to run QEMU without an
> APIC?

There's isapc but you can't attach PCI card to that. It seems according to 
-machine pc,help that there's a PIC=<OnOffAuto> option but no similar for 
APIC. Maybe that could be added but not sure it would work. (Adding 
Bernhard to cc to quickly pass on the potato.)

Regards,
BALATON Zoltan

> On aarch64, the GICv3 needs to explicitly enable support (via the ITS), so
> perhaps it's possible to set up an aarch64 qtest with ITS disabled? It
> looks like the 'virt' machine type only supports the ITS from version 6.2,
> so older versions will disable it.
>
> Sorry, clutching at straws here.
>
>
>>>> +
>>>> +        pci_register_bar(dev, s->msix_bar_nr,
>>>> +                         PCI_BASE_ADDRESS_SPACE_MEMORY |
>>>> +                         PCI_BASE_ADDRESS_MEM_TYPE_64,
>>>> +                         msix_bar);
>>>>
>>>
>>> Is it safe to call pci_register_bar() again for the msix_bar_nr = 0 case?
>>> Even if it is safe, is it sensible? If we're calling it twice for the
>> same
>>> BAR, and the arguments of either of the calls changes in future, the
>> other
>>> needs to change too. Doesn't seem ideal.
>>
>> Good catch. It looks like it "works" so long as the bar wasn't mapped,
>> but I'm sure bad practice... Interesting there is no assertion in
>> there though. I'll fix it though.
>>
>
> I notice there's a msix_init_exclusive_bar()… I wonder if it'd be simpler
> to use that and modify it so it allows you to choose a size and layout for
> the BAR, rather than adding all that extra code to deal with the extra BAR
> in the XHCI?
> (It already calls pci_register_bar() and msix_init() internally, but seems
> to set the BAR's size to 4096 and places the PBA at halfway through the
> BAR. Perhaps rename it to something like
> msix_init_exclusive_bar_with_layout and pass the bar_size and
> bar_pba_offset in as parameters; then make msix_init_exclusive_bar() a
> wrapper for that function with the existing defaults for those variables?)
>
> Just kicking around some ideas here, I have no idea if that actually ends
> up making things simpler…
>
>
>> Thanks,
>> Nick
>>
>

Re: [PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[Bernhard Beschow]

Am 19. Dezember 2024 09:23:13 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 18 Dec 2024, Phil Dennis-Jordan wrote:
>> On Wed, 18 Dec 2024 at 02:19, Nicholas Piggin <npiggin@gmail.com> wrote:
>>> On Thu Dec 12, 2024 at 8:41 PM AEST, Phil Dennis-Jordan wrote:
>>>> Hey Nicholas,
>>>> 
>>>> I'm not an XHCI & PCI expert (yet?) so apologies if I've got some of this
>>>> wrong, but I've asked some questions and made some comments inline:
>>> 
>>> Hey Phil,
>>> 
>>> Thanks for the review, looks like you are the expert now :)
>>> 
>> 
>> The "hot potato" method for determining maintainership. :-)
>
>That's how I got some parts I'm maintainer of now. :-)
>
>[...]
>>>> On Thu, 12 Dec 2024 at 09:52, Nicholas Piggin <npiggin@gmail.com> wrote:
>>>> Surely we should only propagate the error and fail realize() iff s->msix
>>> is
>>>> ON_OFF_AUTO_ON?
>>>> 
>>>> For ON_OFF_AUTO_AUTO, msix_init returning failure isn't a critical error.
>>> 
>>> Yep you're right... you had been testing with msix disabled. I wonder if
>>> there is a good way to force fail this in qtests?
>>> 
>> 
>> I'm really the wrong person to ask about qtest, I'm only just beginning to
>> get to grips with it. It seems the only real reason msix_init fails other
>> than misconfiguration of the device/BAR is when msi_nonbroken = false.
>> 
>> At least on x86(-64), msi_nonbroken=true is unconditionally set in
>> apic_realize(). (I think real hardware would not support MSI(-X) on the
>> i440FX chipset - I was fairly certain it was the PCI root/southbridge
>> catching the writes to the reserved memory region, and I didn't think the
>> PIIX did this; but at least in QEMU it doesn't seem to be implemented in a
>> chipset-dependent way.) I'm not sure it's possible to run QEMU without an
>> APIC?
>
>There's isapc but you can't attach PCI card to that. It seems according to -machine pc,help that there's a PIC=<OnOffAuto> option but no similar for APIC. Maybe that could be added but not sure it would work. (Adding Bernhard to cc to quickly pass on the potato.)

I agree, the only x86 machine with no APIC is the isapc machine. All others (i440fx, q35, microvm) have it always enabled. I guess there is just no point in disabling it since the APIC is part of the architecture for ages and SMP requires it. Although PIIX3+ didn't have an APIC built-in it had interfaces for handling a dedicated one which is basically what QEMU emulates.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> On aarch64, the GICv3 needs to explicitly enable support (via the ITS), so
>> perhaps it's possible to set up an aarch64 qtest with ITS disabled? It
>> looks like the 'virt' machine type only supports the ITS from version 6.2,
>> so older versions will disable it.
>> 
>> Sorry, clutching at straws here.
>> 
>> 
>>>>> +
>>>>> +        pci_register_bar(dev, s->msix_bar_nr,
>>>>> +                         PCI_BASE_ADDRESS_SPACE_MEMORY |
>>>>> +                         PCI_BASE_ADDRESS_MEM_TYPE_64,
>>>>> +                         msix_bar);
>>>>> 
>>>> 
>>>> Is it safe to call pci_register_bar() again for the msix_bar_nr = 0 case?
>>>> Even if it is safe, is it sensible? If we're calling it twice for the
>>> same
>>>> BAR, and the arguments of either of the calls changes in future, the
>>> other
>>>> needs to change too. Doesn't seem ideal.
>>> 
>>> Good catch. It looks like it "works" so long as the bar wasn't mapped,
>>> but I'm sure bad practice... Interesting there is no assertion in
>>> there though. I'll fix it though.
>>> 
>> 
>> I notice there's a msix_init_exclusive_bar()… I wonder if it'd be simpler
>> to use that and modify it so it allows you to choose a size and layout for
>> the BAR, rather than adding all that extra code to deal with the extra BAR
>> in the XHCI?
>> (It already calls pci_register_bar() and msix_init() internally, but seems
>> to set the BAR's size to 4096 and places the PBA at halfway through the
>> BAR. Perhaps rename it to something like
>> msix_init_exclusive_bar_with_layout and pass the bar_size and
>> bar_pba_offset in as parameters; then make msix_init_exclusive_bar() a
>> wrapper for that function with the existing defaults for those variables?)
>> 
>> Just kicking around some ideas here, I have no idea if that actually ends
>> up making things simpler…
>> 
>> 
>>> Thanks,
>>> Nick
>>> 
>>

Re: [PATCH v2 1/2] hw/usb/hcd-xhci-pci: Make PCI device more configurable

[Akihiko Odaki]

On 2024/12/12 17:52, Nicholas Piggin wrote:
> To prepare to support another USB PCI Host Controller, make some PCI
> configuration dynamic.
> 
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   hw/usb/hcd-xhci-pci.h |  9 ++++++
>   hw/usb/hcd-xhci-nec.c | 10 +++++++
>   hw/usb/hcd-xhci-pci.c | 69 ++++++++++++++++++++++++++++++++++++-------
>   3 files changed, 78 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/usb/hcd-xhci-pci.h b/hw/usb/hcd-xhci-pci.h
> index 08f70ce97cc..213076aabf6 100644
> --- a/hw/usb/hcd-xhci-pci.h
> +++ b/hw/usb/hcd-xhci-pci.h
> @@ -40,6 +40,15 @@ typedef struct XHCIPciState {
>       XHCIState xhci;
>       OnOffAuto msi;
>       OnOffAuto msix;
> +    uint8_t cache_line_size;
> +    uint8_t pm_cap_off;
> +    uint8_t pcie_cap_off;
> +    uint8_t msi_cap_off;
> +    uint8_t msix_cap_off;
> +    int msix_bar_nr;
> +    uint64_t msix_bar_size;
> +    uint32_t msix_table_off;
> +    uint32_t msix_pba_off;

Let's make these class variables so that they won't be duplicated for 
each instance.

>   } XHCIPciState;
>   
>   #endif
> diff --git a/hw/usb/hcd-xhci-nec.c b/hw/usb/hcd-xhci-nec.c
> index 0e61c6c4f06..6ac1dc7764c 100644
> --- a/hw/usb/hcd-xhci-nec.c
> +++ b/hw/usb/hcd-xhci-nec.c
> @@ -52,6 +52,16 @@ static void nec_xhci_instance_init(Object *obj)
>   
>       pci->xhci.numintrs = nec->intrs;
>       pci->xhci.numslots = nec->slots;
> +
> +    pci->cache_line_size = 0x10;
> +    pci->pm_cap_off = 0;
> +    pci->pcie_cap_off = 0xa0;
> +    pci->msi_cap_off = 0x70;
> +    pci->msix_cap_off = 0x90;
> +    pci->msix_bar_nr = 0;
> +    pci->msix_bar_size = 0;
> +    pci->msix_table_off = 0x3000;
> +    pci->msix_pba_off = 0x3800;
>   }
>   
>   static void nec_xhci_class_init(ObjectClass *klass, void *data)
> diff --git a/hw/usb/hcd-xhci-pci.c b/hw/usb/hcd-xhci-pci.c
> index a039f5778a6..948d75b7379 100644
> --- a/hw/usb/hcd-xhci-pci.c
> +++ b/hw/usb/hcd-xhci-pci.c
> @@ -32,8 +32,9 @@
>   #include "trace.h"
>   #include "qapi/error.h"
>   
> -#define OFF_MSIX_TABLE  0x3000
> -#define OFF_MSIX_PBA    0x3800
> +#define MSIX_BAR_SIZE   0x800000
> +#define OFF_MSIX_TABLE  0x0000
> +#define OFF_MSIX_PBA    0x1000
>   
>   static void xhci_pci_intr_update(XHCIState *xhci, int n, bool enable)
>   {
> @@ -104,6 +105,31 @@ static int xhci_pci_vmstate_post_load(void *opaque, int version_id)
>      return 0;
>   }
>   
> +static int xhci_pci_add_pm_capability(PCIDevice *pci_dev, uint8_t offset,
> +                                      Error **errp)
> +{
> +    int err;
> +
> +    err = pci_add_capability(pci_dev, PCI_CAP_ID_PM, offset,
> +                             PCI_PM_SIZEOF, errp);
> +    if (err < 0) {
> +        return err;
> +    }
> +
> +    pci_set_word(pci_dev->config + offset + PCI_PM_PMC,
> +                 PCI_PM_CAP_VER_1_2 |
> +                 PCI_PM_CAP_D1 | PCI_PM_CAP_D2 |
> +                 PCI_PM_CAP_PME_D0 | PCI_PM_CAP_PME_D1 |
> +                 PCI_PM_CAP_PME_D2 | PCI_PM_CAP_PME_D3hot);
> +    pci_set_word(pci_dev->wmask + offset + PCI_PM_PMC, 0);
> +    pci_set_word(pci_dev->config + offset + PCI_PM_CTRL,
> +                 PCI_PM_CTRL_NO_SOFT_RESET);
> +    pci_set_word(pci_dev->wmask + offset + PCI_PM_CTRL,
> +                 PCI_PM_CTRL_STATE_MASK);
> +
> +    return 0;
> +}
> +
>   static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>   {
>       int ret;
> @@ -112,7 +138,7 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>   
>       dev->config[PCI_CLASS_PROG] = 0x30;    /* xHCI */
>       dev->config[PCI_INTERRUPT_PIN] = 0x01; /* interrupt pin 1 */
> -    dev->config[PCI_CACHE_LINE_SIZE] = 0x10;
> +    dev->config[PCI_CACHE_LINE_SIZE] = s->cache_line_size;
>       dev->config[0x60] = 0x30; /* release number */
>   
>       object_property_set_link(OBJECT(&s->xhci), "host", OBJECT(s), NULL);
> @@ -125,8 +151,16 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>           s->xhci.nec_quirks = true;
>       }
>   
> +    if (s->pm_cap_off) {
> +        if (xhci_pci_add_pm_capability(dev, s->pm_cap_off, &err)) {
> +            error_propagate(errp, err);

Pass errp to xhci_pci_add_pm_capability() and avoid error_propagate().
include/qapi/error.h has more explanation.

> +            return;
> +        }
> +    }
> +
>       if (s->msi != ON_OFF_AUTO_OFF) {
> -        ret = msi_init(dev, 0x70, s->xhci.numintrs, true, false, &err);
> +        ret = msi_init(dev, s->msi_cap_off, s->xhci.numintrs,
> +                       true, false, &err);
>           /*
>            * Any error other than -ENOTSUP(board's MSI support is broken)
>            * is a programming error
> @@ -143,22 +177,37 @@ static void usb_xhci_pci_realize(struct PCIDevice *dev, Error **errp)
>           /* With msi=auto, we fall back to MSI off silently */
>           error_free(err);
>       }
> +
>       pci_register_bar(dev, 0,
>                        PCI_BASE_ADDRESS_SPACE_MEMORY |
>                        PCI_BASE_ADDRESS_MEM_TYPE_64,
>                        &s->xhci.mem);
>   
>       if (pci_bus_is_express(pci_get_bus(dev))) {
> -        ret = pcie_endpoint_cap_init(dev, 0xa0);
> +        ret = pcie_endpoint_cap_init(dev, s->pcie_cap_off);
>           assert(ret > 0);
>       }
>   
>       if (s->msix != ON_OFF_AUTO_OFF) {
> -        /* TODO check for errors, and should fail when msix=on */
> -        msix_init(dev, s->xhci.numintrs,
> -                  &s->xhci.mem, 0, OFF_MSIX_TABLE,
> -                  &s->xhci.mem, 0, OFF_MSIX_PBA,
> -                  0x90, NULL);
> +        MemoryRegion *msix_bar = &s->xhci.mem;
> +        if (s->msix_bar_nr != 0) {
> +            memory_region_init(&dev->msix_exclusive_bar, OBJECT(dev),
> +                               "xhci-msix", s->msix_bar_size);
> +            msix_bar = &dev->msix_exclusive_bar;
> +        }
> +
> +        ret = msix_init(dev, s->xhci.numintrs,
> +                        msix_bar, s->msix_bar_nr, s->msix_table_off,
> +                        msix_bar, s->msix_bar_nr, s->msix_pba_off,
> +                        s->msix_cap_off, errp);
> +        if (ret) {
> +            return;
> +        }
> +
> +        pci_register_bar(dev, s->msix_bar_nr,
> +                         PCI_BASE_ADDRESS_SPACE_MEMORY |
> +                         PCI_BASE_ADDRESS_MEM_TYPE_64,
> +                         msix_bar);
>       }
>       s->xhci.as = pci_get_address_space(dev);
>   }

Re: [PATCH v2 2/2] hw/usb/hcd-xhci-pci: Add TI TUSB73X0 XHCI controller model

[Philippe Mathieu-Daudé]

On 19/12/24 01:48, Bernhard Beschow wrote:
> 
> 
> Am 12. Dezember 2024 08:52:07 UTC schrieb Nicholas Piggin <npiggin@gmail.com>:
>> The TI TUSB73X0 controller has some interesting differences from NEC,
>> notably a separate BAR for MSIX, and PM capabilities. The spec is freely
>> available without sign-up.
>>
>> This controller is accepted by IBM Power proprietary firmware and
>> software (when the subsystem IDs are set to Power servers, which is not
>> done here). IBM code is picky about device support, so the NEC device
>> can not be used.
>>
>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>> ---
>> include/hw/pci/pci_ids.h        |  1 +
>> include/hw/usb/xhci.h           |  1 +
>> hw/usb/hcd-xhci-ti.c            | 92 +++++++++++++++++++++++++++++++++
>> tests/qtest/usb-hcd-xhci-test.c | 21 +++++---
>> hw/usb/Kconfig                  |  5 ++
>> hw/usb/meson.build              |  1 +
>> 6 files changed, 115 insertions(+), 6 deletions(-)
>> create mode 100644 hw/usb/hcd-xhci-ti.c


>> diff --git a/hw/usb/hcd-xhci-ti.c b/hw/usb/hcd-xhci-ti.c
>> new file mode 100644
>> index 00000000000..6d4b44f6aaf
>> --- /dev/null
>> +++ b/hw/usb/hcd-xhci-ti.c
>> @@ -0,0 +1,92 @@
>> +/*
>> + * USB xHCI controller emulation
>> + * Datasheet https://www.ti.com/product/TUSB7340
>> + *
>> + * Copyright (c) 2011 Securiforest
>> + * Date: 2011-05-11 ;  Author: Hector Martin <hector@marcansoft.com>
>> + * Based on usb-xhci-nec.c, emulates TI TUSB73X0
>> + *
>> + * This library is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU Lesser General Public
>> + * License as published by the Free Software Foundation; either
>> + * version 2.1 of the License, or (at your option) any later version.
>> + *
>> + * This library is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + * Lesser General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU Lesser General Public
>> + * License along with this library; if not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/usb.h"
>> +#include "qemu/module.h"
>> +#include "hw/pci/pci.h"
>> +#include "hw/qdev-properties.h"
>> +
>> +#include "hcd-xhci-pci.h"
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(XHCITiState, TI_XHCI)
>> +
>> +struct XHCITiState {
>> +    /*< private >*/
>> +    XHCIPciState parent_obj;
>> +    /*< public >*/
> 
> These markers are obsolete. Instead, a blank line after parent_obj should be inserted.
> 
>> +    uint32_t intrs;
>> +    uint32_t slots;
>> +};
>> +
>> +static Property ti_xhci_properties[] = {
> 
> s/static Property/static const Property/ as of recent tree-wide changes.
> 
> Best regards,
> Bernhard
> 
>> +    DEFINE_PROP_ON_OFF_AUTO("msi", XHCIPciState, msi, ON_OFF_AUTO_AUTO),
>> +    DEFINE_PROP_ON_OFF_AUTO("msix", XHCIPciState, msix, ON_OFF_AUTO_AUTO),
>> +    DEFINE_PROP_UINT32("intrs", XHCITiState, intrs, 8),
>> +    DEFINE_PROP_UINT32("slots", XHCITiState, slots, XHCI_MAXSLOTS),
>> +    DEFINE_PROP_END_OF_LIST(),

Also remove this "DEFINE_PROP_END_OF_LIST()" line.

>> +};