Re: [PATCH 2/3] Add "Group Identifier" support to PCIe bridges.

["Michael S. Tsirkin" <mst@redhat.com>]

On Tue, Jun 19, 2018 at 11:32:26AM -0500, Venu Busireddy wrote:
> Add a "Vendor-Specific" capability to the PCIe bridge, to contain the
> "Group Identifier" (UUID) that will be used to pair a virtio device with
> the passthrough device attached to that bridge.
> 
> This capability is added to the bridge iff the "uuid" option is specified
> for the bridge device, via the qemu command line. Also, the bridge's
> Device ID is changed to PCI_VENDOR_ID_REDHAT, and Vendor ID is changed
> to PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE (from the default values), when the
> "uuid" option is present.
> 
> Signed-off-by: Venu Busireddy <venu.busireddy@oracle.com>

I don't see why we should add it to all bridges.
Let's just add it to ones that already have the RH vendor ID?


> ---
>  hw/pci-bridge/ioh3420.c        |  2 ++
>  hw/pci-bridge/pcie_root_port.c |  7 +++++++
>  hw/pci/pci_bridge.c            | 32 ++++++++++++++++++++++++++++++++
>  include/hw/pci/pci.h           |  2 ++
>  include/hw/pci/pcie.h          |  1 +
>  include/hw/pci/pcie_port.h     |  1 +
>  6 files changed, 45 insertions(+)
> 
> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> index a451d74ee6..b6b9ebc726 100644
> --- a/hw/pci-bridge/ioh3420.c
> +++ b/hw/pci-bridge/ioh3420.c
> @@ -35,6 +35,7 @@
>  #define IOH_EP_MSI_SUPPORTED_FLAGS      PCI_MSI_FLAGS_MASKBIT
>  #define IOH_EP_MSI_NR_VECTOR            2
>  #define IOH_EP_EXP_OFFSET               0x90
> +#define IOH_EP_VENDOR_OFFSET            0xCC
>  #define IOH_EP_AER_OFFSET               0x100
>  
>  /*
> @@ -111,6 +112,7 @@ static void ioh3420_class_init(ObjectClass *klass, void *data)
>      rpc->exp_offset = IOH_EP_EXP_OFFSET;
>      rpc->aer_offset = IOH_EP_AER_OFFSET;
>      rpc->ssvid_offset = IOH_EP_SSVID_OFFSET;
> +    rpc->vendor_offset = IOH_EP_VENDOR_OFFSET;
>      rpc->ssid = IOH_EP_SSVID_SSID;
>  }
>  
> diff --git a/hw/pci-bridge/pcie_root_port.c b/hw/pci-bridge/pcie_root_port.c
> index 45f9e8cd4a..ba470c7fda 100644
> --- a/hw/pci-bridge/pcie_root_port.c
> +++ b/hw/pci-bridge/pcie_root_port.c
> @@ -71,6 +71,12 @@ static void rp_realize(PCIDevice *d, Error **errp)
>          goto err_bridge;
>      }
>  
> +    rc = pci_bridge_vendor_init(d, rpc->vendor_offset, errp);
> +    if (rc < 0) {
> +        error_append_hint(errp, "Can't init group ID, error %d\n", rc);
> +        goto err_bridge;
> +    }
> +
>      if (rpc->interrupts_init) {
>          rc = rpc->interrupts_init(d, errp);
>          if (rc < 0) {
> @@ -137,6 +143,7 @@ static void rp_exit(PCIDevice *d)
>  static Property rp_props[] = {
>      DEFINE_PROP_BIT(COMPAT_PROP_PCP, PCIDevice, cap_present,
>                      QEMU_PCIE_SLTCAP_PCP_BITNR, true),
> +    DEFINE_PROP_UUID(COMPAT_PROP_UUID, PCIDevice, uuid, false),
>      DEFINE_PROP_END_OF_LIST()
>  };
>  
> diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
> index 40a39f57cb..c63bc439f7 100644
> --- a/hw/pci/pci_bridge.c
> +++ b/hw/pci/pci_bridge.c
> @@ -34,12 +34,17 @@
>  #include "hw/pci/pci_bus.h"
>  #include "qemu/range.h"
>  #include "qapi/error.h"
> +#include "qemu/uuid.h"
>  
>  /* PCI bridge subsystem vendor ID helper functions */
>  #define PCI_SSVID_SIZEOF        8
>  #define PCI_SSVID_SVID          4
>  #define PCI_SSVID_SSID          6
>  
> +#define PCI_VENDOR_SIZEOF             20
> +#define PCI_VENDOR_CAP_LEN_OFFSET      2
> +#define PCI_VENDOR_GROUP_ID_OFFSET     4
> +
>  int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
>                            uint16_t svid, uint16_t ssid,
>                            Error **errp)
> @@ -57,6 +62,33 @@ int pci_bridge_ssvid_init(PCIDevice *dev, uint8_t offset,
>      return pos;
>  }
>  
> +int pci_bridge_vendor_init(PCIDevice *d, uint8_t offset, Error **errp)
> +{
> +    int pos;
> +    PCIDeviceClass *dc = PCI_DEVICE_GET_CLASS(d);
> +
> +    if (qemu_uuid_is_null(&d->uuid)) {
> +        return 0;
> +    }
> +
> +    pos = pci_add_capability(d, PCI_CAP_ID_VNDR, offset, PCI_VENDOR_SIZEOF,
> +            errp);
> +    if (pos < 0) {
> +        return pos;
> +    }
> +
> +    dc->vendor_id = PCI_VENDOR_ID_REDHAT;
> +    dc->device_id = PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE;
> +    pci_set_word(d->config + PCI_VENDOR_ID, PCI_VENDOR_ID_REDHAT);
> +    pci_set_word(d->config + PCI_DEVICE_ID, PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE);
> +
> +    pci_set_word(d->config + pos + PCI_VENDOR_CAP_LEN_OFFSET,
> +            PCI_VENDOR_SIZEOF);
> +    memcpy(d->config + offset + PCI_VENDOR_GROUP_ID_OFFSET, &d->uuid,
> +            sizeof(QemuUUID));
> +    return pos;
> +}
> +
>  /* Accessor function to get parent bridge device from pci bus. */
>  PCIDevice *pci_bridge_get_device(PCIBus *bus)
>  {
> diff --git a/include/hw/pci/pci.h b/include/hw/pci/pci.h
> index 990d6fcbde..ee234c5a6f 100644
> --- a/include/hw/pci/pci.h
> +++ b/include/hw/pci/pci.h
> @@ -4,6 +4,7 @@
>  #include "hw/qdev.h"
>  #include "exec/memory.h"
>  #include "sysemu/dma.h"
> +#include "qemu/uuid.h"
>  
>  /* PCI includes legacy ISA access.  */
>  #include "hw/isa/isa.h"
> @@ -343,6 +344,7 @@ struct PCIDevice {
>      bool has_rom;
>      MemoryRegion rom;
>      uint32_t rom_bar;
> +    QemuUUID uuid;
>  
>      /* INTx routing notifier */
>      PCIINTxRoutingNotifier intx_routing_notifier;
> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h
> index b71e369703..b4189d0ce3 100644
> --- a/include/hw/pci/pcie.h
> +++ b/include/hw/pci/pcie.h
> @@ -82,6 +82,7 @@ struct PCIExpressDevice {
>  };
>  
>  #define COMPAT_PROP_PCP "power_controller_present"
> +#define COMPAT_PROP_UUID "uuid"
>  
>  /* PCI express capability helper functions */
>  int pcie_cap_init(PCIDevice *dev, uint8_t offset, uint8_t type,
> diff --git a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h
> index 0736014bfd..40db6dbbe7 100644
> --- a/include/hw/pci/pcie_port.h
> +++ b/include/hw/pci/pcie_port.h
> @@ -74,6 +74,7 @@ typedef struct PCIERootPortClass {
>      int exp_offset;
>      int aer_offset;
>      int ssvid_offset;
> +    int vendor_offset;
>      int ssid;
>  } PCIERootPortClass;
>