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

[Akihiko Odaki <akihiko.odaki@daynix.com>]

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);
>   }