On Wed, Jun 27, 2018 at 7:14 PM, Michael S. Tsirkin <m...@redhat.com> wrote: > On Wed, Jun 27, 2018 at 06:07:59PM -0500, Venu Busireddy wrote: >> On 2018-06-26 23:08:12 -0500, Venu Busireddy wrote: >> > On 2018-06-27 07:02:36 +0300, Michael S. Tsirkin wrote: >> > > On Tue, Jun 26, 2018 at 10:49:33PM -0500, Venu Busireddy wrote: >> > > > Add the "Vendor-Specific" capability to the Red Hat PCI bridge device >> > > > "pci-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. >> > > >> > > I think the name should be more explicit. How about "failover-group-id"? >> > >> > I can change it. But don't you think it is bit long? >> > >> > > > >> > > > Signed-off-by: Venu Busireddy <venu.busire...@oracle.com> >> > > >> > > I'd like to also tweak the device id in this case, >> > > to make it easier for guests to know it's a grouping bridge. >> > >> > Could you please recommend a name for the new ID'd definition? Something >> > in lines of PCI_DEVICE_ID_REDHAT_<blah blah>. >> >> How about these names and values for the device IDs? >> >> PCI_DEVICE_ID_REDHAT_PCI_BRIDGE_GRP (0x0010) for pci-bridge, and > > You do not need the second PCI, and group is one of the > functions of bridge anyway. > > PCI_DEVICE_ID_REDHAT_BRIDGE_FAILOVER? > > >> PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE_GRP (0x0011) for pcie-downstream >> >> Thanks, >> >> Venu > > PCI_DEVICE_ID_REDHAT_DOWNPORT_FAILOVER? > > > It's becoming annoying though - we'll need one for each type :( > > Ideas/options: > - use revision ID to distinguish from regular bridge > - use device serial # cap for the bridge when it's an express device > IMO, from guest implementation point of view, it would be the best to use a single consistent mechanism all across. The serial number cap is too PCIE specific.
-Siwei > >> > > > --- >> > > > hw/pci-bridge/pci_bridge_dev.c | 8 ++++++++ >> > > > hw/pci/pci_bridge.c | 26 ++++++++++++++++++++++++++ >> > > > include/hw/pci/pcie.h | 1 + >> > > > 3 files changed, 35 insertions(+) >> > > > >> > > > diff --git a/hw/pci-bridge/pci_bridge_dev.c >> > > > b/hw/pci-bridge/pci_bridge_dev.c >> > > > index b2d861d216..bbbc6fa1c6 100644 >> > > > --- a/hw/pci-bridge/pci_bridge_dev.c >> > > > +++ b/hw/pci-bridge/pci_bridge_dev.c >> > > > @@ -71,6 +71,12 @@ static void pci_bridge_dev_realize(PCIDevice *dev, >> > > > Error **errp) >> > > > bridge_dev->msi = ON_OFF_AUTO_OFF; >> > > > } >> > > > >> > > > + err = pci_bridge_vendor_init(dev, 0, errp); >> > > > + if (err < 0) { >> > > > + error_append_hint(errp, "Can't init group ID, error %d\n", >> > > > err); >> > > > + goto vendor_cap_err; >> > > > + } >> > > > + >> > > > err = slotid_cap_init(dev, 0, bridge_dev->chassis_nr, 0, errp); >> > > > if (err) { >> > > > goto slotid_error; >> > > > @@ -109,6 +115,7 @@ slotid_error: >> > > > if (shpc_present(dev)) { >> > > > shpc_cleanup(dev, &bridge_dev->bar); >> > > > } >> > > > +vendor_cap_err: >> > > > shpc_error: >> > > > pci_bridge_exitfn(dev); >> > > > } >> > > > @@ -162,6 +169,7 @@ static Property pci_bridge_dev_properties[] = { >> > > > ON_OFF_AUTO_AUTO), >> > > > DEFINE_PROP_BIT(PCI_BRIDGE_DEV_PROP_SHPC, PCIBridgeDev, flags, >> > > > PCI_BRIDGE_DEV_F_SHPC_REQ, 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..cb8b3dad2a 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,27 @@ 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; >> > > > + >> > > > + 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; >> > > > + } >> > > > + >> > > > + pci_set_word(d->config + pos + PCI_VENDOR_CAP_LEN_OFFSET, >> > > > + PCI_VENDOR_SIZEOF); >> > > > + memcpy(d->config + pos + 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/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, >> > > >> > > --------------------------------------------------------------------- >> > > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org >> > > For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org >> > > >> > >> > --------------------------------------------------------------------- >> > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org >> > For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org >> > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org > For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org >