The patch set "Enable virtio_net to act as a standby for a passthru
device" [1] deals with live migration of guests that use passthrough
devices. However, that scheme uses the MAC address for pairing
the virtio device and the passthrough device. The thread "netvsc:
refactor notifier/event handling code to use the failover framework"
[2] discusses an alternate mechanism, such as using an UUID, for pairing
the devices. Based on that discussion, proposals "Add "Group Identifier"
to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
store pairing information..." [4] were made.

The current patch set includes all the feedback received for proposals [3]
and [4]. For the sake of completeness, patch for the virtio specification
is also included here. Following is the updated proposal.

1. Extend the virtio specification to include a new virtio PCI capability
   "VIRTIO_PCI_CAP_GROUP_ID_CFG".

2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
   The "uuid" is a string in UUID format.

3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
   The "uuid" is a string in UUID format. Currently, PCIe bridge for
   the Q35 model is supported.

4. The operator creates a unique identifier string using 'uuidgen'.

5. When the virtio device is created, the operator uses the "uuid" option
   (for example, '-device virtio-net-pci,uuid="string"') and specifies
   the UUID created in step 4.

   QEMU stores the UUID in the virtio device's configuration space
   in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".

6. When assigning a PCI device to the guest in passthrough mode, the
   operator first creates a bridge using the "uuid" option (for example,
   '-device pcie-downstream,uuid="string"') to specify the UUID created
   in step 4, and then attaches the passthrough device to the bridge.

   QEMU stores the UUID in the configuration space of the bridge as
   Vendor-Specific capability (0x09). The "Vendor" here is not to be
   confused with a specific organization. Instead, the vendor of the
   bridge is QEMU. To avoid mixing up with other bridges, the bridge
   will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
   device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
   option is specified. Otherwise, current defaults are used.

7. Patch 4 in patch series "Enable virtio_net to act as a standby for
   a passthru device" [1] needs to be modified to use the UUID values
   present in the bridge's configuration space and the virtio device's
   configuration space instead of the MAC address for pairing the devices.

Thanks!

Venu

[1] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00156.html
[2] https://www.spinics.net/lists/netdev/msg499011.html
[3] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00118.html
[4] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00204.html

Changes in v2:
  - As Michael Tsirkin suggested, changed the virtio specification
    to restrict the group identifier to be a 16-byte field, presented
    entirely in the virtio device's configuration space.
  - As Michael Tsirkin suggested, instead of tweaking the ioh3420
    device with Red Hat vendor ID, create a new PCIe bridge device
    named "pcie-downstream" with Red Hat Vendor ID, and include the
    group identifier in this device.
  - Added a new patch to enhance the "pci-bridge" device to support
    the group identifier (for the i440FX model).

Venu Busireddy (4):
  Add a true or false option to the DEFINE_PROP_UUID macro.
  Add "Group Identifier" support to virtio devices.
  Add "Group Identifier" support to Red Hat PCI bridge.
  Add "Group Identifier" support to Red Hat PCI Express bridge.

 default-configs/arm-softmmu.mak             |   1 +
 default-configs/i386-softmmu.mak            |   1 +
 default-configs/x86_64-softmmu.mak          |   1 +
 hw/acpi/vmgenid.c                           |   2 +-
 hw/pci-bridge/Makefile.objs                 |   1 +
 hw/pci-bridge/pci_bridge_dev.c              |   8 +
 hw/pci-bridge/pcie_downstream.c             | 215 ++++++++++++++++++++
 hw/pci-bridge/pcie_downstream.h             |  10 +
 hw/pci/pci_bridge.c                         |  26 +++
 hw/virtio/virtio-pci.c                      |  15 ++
 hw/virtio/virtio-pci.h                      |   3 +-
 include/hw/pci/pci.h                        |   3 +
 include/hw/pci/pcie.h                       |   1 +
 include/hw/qdev-properties.h                |   4 +-
 include/standard-headers/linux/virtio_pci.h |   8 +
 15 files changed, 295 insertions(+), 4 deletions(-)
 create mode 100644 hw/pci-bridge/pcie_downstream.c
 create mode 100644 hw/pci-bridge/pcie_downstream.h

On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> The patch set "Enable virtio_net to act as a standby for a passthru
> device" [1] deals with live migration of guests that use passthrough
> devices. However, that scheme uses the MAC address for pairing
> the virtio device and the passthrough device. The thread "netvsc:
> refactor notifier/event handling code to use the failover framework"
> [2] discusses an alternate mechanism, such as using an UUID, for pairing
> the devices. Based on that discussion, proposals "Add "Group Identifier"
> to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> store pairing information..." [4] were made.

I must have missed something in those threads, but where does this UUID
thing come about?  AFAICS this identifier doesn't need to be
"universally" unique, nor persistent; it only has to be unique across
the VM and stable throughout the VM lifetime.

FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
to be implied in the thread you refer to.

> The current patch set includes all the feedback received for proposals [3]
> and [4]. For the sake of completeness, patch for the virtio specification
> is also included here. Following is the updated proposal.
> 
> 1. Extend the virtio specification to include a new virtio PCI capability
>    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> 
> 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
>    The "uuid" is a string in UUID format.
> 
> 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
>    The "uuid" is a string in UUID format. Currently, PCIe bridge for
>    the Q35 model is supported.
> 
> 4. The operator creates a unique identifier string using 'uuidgen'.
> 
> 5. When the virtio device is created, the operator uses the "uuid" option
>    (for example, '-device virtio-net-pci,uuid="string"') and specifies
>    the UUID created in step 4.
> 
>    QEMU stores the UUID in the virtio device's configuration space
>    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> 
> 6. When assigning a PCI device to the guest in passthrough mode, the
>    operator first creates a bridge using the "uuid" option (for example,
>    '-device pcie-downstream,uuid="string"') to specify the UUID created
>    in step 4, and then attaches the passthrough device to the bridge.
> 
>    QEMU stores the UUID in the configuration space of the bridge as
>    Vendor-Specific capability (0x09). The "Vendor" here is not to be
>    confused with a specific organization. Instead, the vendor of the
>    bridge is QEMU. To avoid mixing up with other bridges, the bridge
>    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
>    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
>    option is specified. Otherwise, current defaults are used.

I wonder if it makes more sense to drop the concept of failover groups,
and just refer to the standby device by device-id, like 

  -device virtio-net-pci,id=foo \
  -device pcie-downstream,failover=foo

The bridge device will then lookup the failover device, figure out the
common identifier to expose to the guest, and defer the visibility of
the PT device behind the bridge until the guest acknowledged the support
for failover on the PV device.

Roman.

On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > The patch set "Enable virtio_net to act as a standby for a passthru
> > device" [1] deals with live migration of guests that use passthrough
> > devices. However, that scheme uses the MAC address for pairing
> > the virtio device and the passthrough device. The thread "netvsc:
> > refactor notifier/event handling code to use the failover framework"
> > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > store pairing information..." [4] were made.
> 
> I must have missed something in those threads, but where does this UUID
> thing come about?  AFAICS this identifier doesn't need to be
> "universally" unique, nor persistent; it only has to be unique across
> the VM and stable throughout the VM lifetime.

The notion of using UUID came up in the thread

   https://www.spinics.net/lists/netdev/msg499011.html

> FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> to be implied in the thread you refer to.

Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
However, what we are doing is similar to that. Instead of 32 bits,
we are using 128 bits.

> > The current patch set includes all the feedback received for proposals [3]
> > and [4]. For the sake of completeness, patch for the virtio specification
> > is also included here. Following is the updated proposal.
> > 
> > 1. Extend the virtio specification to include a new virtio PCI capability
> >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > 
> > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> >    The "uuid" is a string in UUID format.
> > 
> > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> >    the Q35 model is supported.
> > 
> > 4. The operator creates a unique identifier string using 'uuidgen'.
> > 
> > 5. When the virtio device is created, the operator uses the "uuid" option
> >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> >    the UUID created in step 4.
> > 
> >    QEMU stores the UUID in the virtio device's configuration space
> >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > 
> > 6. When assigning a PCI device to the guest in passthrough mode, the
> >    operator first creates a bridge using the "uuid" option (for example,
> >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> >    in step 4, and then attaches the passthrough device to the bridge.
> > 
> >    QEMU stores the UUID in the configuration space of the bridge as
> >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> >    confused with a specific organization. Instead, the vendor of the
> >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> >    option is specified. Otherwise, current defaults are used.
> 
> I wonder if it makes more sense to drop the concept of failover groups,
> and just refer to the standby device by device-id, like 
> 
>   -device virtio-net-pci,id=foo \
>   -device pcie-downstream,failover=foo

Isn't this the same as what this patch series proposes? In your
suggestion, "foo" is the entity that connects the passthrough device
and the failover device. In this patch set, that "foo" is the UUID,
and the options "id" and "failover" are replaced by "uuid". Do you agree?

Regards,

Venu

> The bridge device will then lookup the failover device, figure out the
> common identifier to expose to the guest, and defer the visibility of
> the PT device behind the bridge until the guest acknowledged the support
> for failover on the PV device.
> 
> Roman.

On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > device" [1] deals with live migration of guests that use passthrough
> > > devices. However, that scheme uses the MAC address for pairing
> > > the virtio device and the passthrough device. The thread "netvsc:
> > > refactor notifier/event handling code to use the failover framework"
> > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > store pairing information..." [4] were made.
> > 
> > I must have missed something in those threads, but where does this UUID
> > thing come about?  AFAICS this identifier doesn't need to be
> > "universally" unique, nor persistent; it only has to be unique across
> > the VM and stable throughout the VM lifetime.
> 
> The notion of using UUID came up in the thread
> 
>    https://www.spinics.net/lists/netdev/msg499011.html

That's probably because it was expected one of standard serial number capabilities
(VPD or PCI Express serial #) will be used, which are expected to be unique.

If you are rolling your own vendor specific one, it's just an ID and
does not have to be unique.

> > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > to be implied in the thread you refer to.
> 
> Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
> However, what we are doing is similar to that. Instead of 32 bits,
> we are using 128 bits.

That's OK. The name is confusing though. It's a failover group id,
not a UUID.

> > > The current patch set includes all the feedback received for proposals [3]
> > > and [4]. For the sake of completeness, patch for the virtio specification
> > > is also included here. Following is the updated proposal.
> > > 
> > > 1. Extend the virtio specification to include a new virtio PCI capability
> > >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > 
> > > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> > >    The "uuid" is a string in UUID format.
> > > 
> > > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> > >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> > >    the Q35 model is supported.
> > > 
> > > 4. The operator creates a unique identifier string using 'uuidgen'.
> > > 
> > > 5. When the virtio device is created, the operator uses the "uuid" option
> > >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> > >    the UUID created in step 4.
> > > 
> > >    QEMU stores the UUID in the virtio device's configuration space
> > >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > 
> > > 6. When assigning a PCI device to the guest in passthrough mode, the
> > >    operator first creates a bridge using the "uuid" option (for example,
> > >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> > >    in step 4, and then attaches the passthrough device to the bridge.
> > > 
> > >    QEMU stores the UUID in the configuration space of the bridge as
> > >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> > >    confused with a specific organization. Instead, the vendor of the
> > >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> > >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> > >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> > >    option is specified. Otherwise, current defaults are used.
> > 
> > I wonder if it makes more sense to drop the concept of failover groups,
> > and just refer to the standby device by device-id, like 
> > 
> >   -device virtio-net-pci,id=foo \
> >   -device pcie-downstream,failover=foo
> 
> Isn't this the same as what this patch series proposes? In your
> suggestion, "foo" is the entity that connects the passthrough device
> and the failover device. In this patch set, that "foo" is the UUID,
> and the options "id" and "failover" are replaced by "uuid". Do you agree?
> 
> Regards,
> 
> Venu
> 
> > The bridge device will then lookup the failover device, figure out the
> > common identifier to expose to the guest, and defer the visibility of
> > the PT device behind the bridge until the guest acknowledged the support
> > for failover on the PV device.
> > 
> > Roman.

On 2018-06-27 22:47:05 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> > On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > > On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > > device" [1] deals with live migration of guests that use passthrough
> > > > devices. However, that scheme uses the MAC address for pairing
> > > > the virtio device and the passthrough device. The thread "netvsc:
> > > > refactor notifier/event handling code to use the failover framework"
> > > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > > store pairing information..." [4] were made.
> > > 
> > > I must have missed something in those threads, but where does this UUID
> > > thing come about?  AFAICS this identifier doesn't need to be
> > > "universally" unique, nor persistent; it only has to be unique across
> > > the VM and stable throughout the VM lifetime.
> > 
> > The notion of using UUID came up in the thread
> > 
> >    https://www.spinics.net/lists/netdev/msg499011.html
> 
> That's probably because it was expected one of standard serial number capabilities
> (VPD or PCI Express serial #) will be used, which are expected to be unique.
> 
> If you are rolling your own vendor specific one, it's just an ID and
> does not have to be unique.
> 
> > > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > > to be implied in the thread you refer to.
> > 
> > Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
> > However, what we are doing is similar to that. Instead of 32 bits,
> > we are using 128 bits.
> 
> That's OK. The name is confusing though. It's a failover group id,
> not a UUID.

Sure, we can name it whatever we want. I can change it to
"failover-group-id", if that is what we want to call it.

But what is more important is, what is represented by that name? I thought
we were going to use UUID. The QEMU command line changes in this patch
set expect the user to specify an UUID as the value for this option
(whatever we name it). Are we still in agreement about that, or do you
propose something else to be used? If so, what is it? A 32-bit number, a
64-bit number, or an arbitrary string?

Regards,

Venu

> 
> > > > The current patch set includes all the feedback received for proposals [3]
> > > > and [4]. For the sake of completeness, patch for the virtio specification
> > > > is also included here. Following is the updated proposal.
> > > > 
> > > > 1. Extend the virtio specification to include a new virtio PCI capability
> > > >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > 
> > > > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> > > >    The "uuid" is a string in UUID format.
> > > > 
> > > > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> > > >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> > > >    the Q35 model is supported.
> > > > 
> > > > 4. The operator creates a unique identifier string using 'uuidgen'.
> > > > 
> > > > 5. When the virtio device is created, the operator uses the "uuid" option
> > > >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> > > >    the UUID created in step 4.
> > > > 
> > > >    QEMU stores the UUID in the virtio device's configuration space
> > > >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > 
> > > > 6. When assigning a PCI device to the guest in passthrough mode, the
> > > >    operator first creates a bridge using the "uuid" option (for example,
> > > >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> > > >    in step 4, and then attaches the passthrough device to the bridge.
> > > > 
> > > >    QEMU stores the UUID in the configuration space of the bridge as
> > > >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> > > >    confused with a specific organization. Instead, the vendor of the
> > > >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> > > >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> > > >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> > > >    option is specified. Otherwise, current defaults are used.
> > > 
> > > I wonder if it makes more sense to drop the concept of failover groups,
> > > and just refer to the standby device by device-id, like 
> > > 
> > >   -device virtio-net-pci,id=foo \
> > >   -device pcie-downstream,failover=foo
> > 
> > Isn't this the same as what this patch series proposes? In your
> > suggestion, "foo" is the entity that connects the passthrough device
> > and the failover device. In this patch set, that "foo" is the UUID,
> > and the options "id" and "failover" are replaced by "uuid". Do you agree?
> > 
> > Regards,
> > 
> > Venu
> > 
> > > The bridge device will then lookup the failover device, figure out the
> > > common identifier to expose to the guest, and defer the visibility of
> > > the PT device behind the bridge until the guest acknowledged the support
> > > for failover on the PV device.
> > > 
> > > Roman.

On Wed, Jun 27, 2018 at 02:59:01PM -0500, Venu Busireddy wrote:
> On 2018-06-27 22:47:05 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> > > On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > > > On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > > > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > > > device" [1] deals with live migration of guests that use passthrough
> > > > > devices. However, that scheme uses the MAC address for pairing
> > > > > the virtio device and the passthrough device. The thread "netvsc:
> > > > > refactor notifier/event handling code to use the failover framework"
> > > > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > > > store pairing information..." [4] were made.
> > > > 
> > > > I must have missed something in those threads, but where does this UUID
> > > > thing come about?  AFAICS this identifier doesn't need to be
> > > > "universally" unique, nor persistent; it only has to be unique across
> > > > the VM and stable throughout the VM lifetime.
> > > 
> > > The notion of using UUID came up in the thread
> > > 
> > >    https://www.spinics.net/lists/netdev/msg499011.html
> > 
> > That's probably because it was expected one of standard serial number capabilities
> > (VPD or PCI Express serial #) will be used, which are expected to be unique.
> > 
> > If you are rolling your own vendor specific one, it's just an ID and
> > does not have to be unique.
> > 
> > > > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > > > to be implied in the thread you refer to.
> > > 
> > > Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
> > > However, what we are doing is similar to that. Instead of 32 bits,
> > > we are using 128 bits.
> > 
> > That's OK. The name is confusing though. It's a failover group id,
> > not a UUID.
> 
> Sure, we can name it whatever we want. I can change it to
> "failover-group-id", if that is what we want to call it.
> 
> But what is more important is, what is represented by that name? I thought
> we were going to use UUID. The QEMU command line changes in this patch
> set expect the user to specify an UUID as the value for this option
> (whatever we name it). Are we still in agreement about that, or do you
> propose something else to be used? If so, what is it? A 32-bit number, a
> 64-bit number, or an arbitrary string?
> 
> Regards,
> 
> Venu

If we don't really need a UUID, I'd avoid that requirement.


> > 
> > > > > The current patch set includes all the feedback received for proposals [3]
> > > > > and [4]. For the sake of completeness, patch for the virtio specification
> > > > > is also included here. Following is the updated proposal.
> > > > > 
> > > > > 1. Extend the virtio specification to include a new virtio PCI capability
> > > > >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > 
> > > > > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> > > > >    The "uuid" is a string in UUID format.
> > > > > 
> > > > > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> > > > >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> > > > >    the Q35 model is supported.
> > > > > 
> > > > > 4. The operator creates a unique identifier string using 'uuidgen'.
> > > > > 
> > > > > 5. When the virtio device is created, the operator uses the "uuid" option
> > > > >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> > > > >    the UUID created in step 4.
> > > > > 
> > > > >    QEMU stores the UUID in the virtio device's configuration space
> > > > >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > 
> > > > > 6. When assigning a PCI device to the guest in passthrough mode, the
> > > > >    operator first creates a bridge using the "uuid" option (for example,
> > > > >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> > > > >    in step 4, and then attaches the passthrough device to the bridge.
> > > > > 
> > > > >    QEMU stores the UUID in the configuration space of the bridge as
> > > > >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> > > > >    confused with a specific organization. Instead, the vendor of the
> > > > >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> > > > >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> > > > >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> > > > >    option is specified. Otherwise, current defaults are used.
> > > > 
> > > > I wonder if it makes more sense to drop the concept of failover groups,
> > > > and just refer to the standby device by device-id, like 
> > > > 
> > > >   -device virtio-net-pci,id=foo \
> > > >   -device pcie-downstream,failover=foo
> > > 
> > > Isn't this the same as what this patch series proposes? In your
> > > suggestion, "foo" is the entity that connects the passthrough device
> > > and the failover device. In this patch set, that "foo" is the UUID,
> > > and the options "id" and "failover" are replaced by "uuid". Do you agree?
> > > 
> > > Regards,
> > > 
> > > Venu
> > > 
> > > > The bridge device will then lookup the failover device, figure out the
> > > > common identifier to expose to the guest, and defer the visibility of
> > > > the PT device behind the bridge until the guest acknowledged the support
> > > > for failover on the PV device.
> > > > 
> > > > Roman.

On 2018-06-27 23:12:12 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 27, 2018 at 02:59:01PM -0500, Venu Busireddy wrote:
> > On 2018-06-27 22:47:05 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> > > > On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > > > > On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > > > > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > > > > device" [1] deals with live migration of guests that use passthrough
> > > > > > devices. However, that scheme uses the MAC address for pairing
> > > > > > the virtio device and the passthrough device. The thread "netvsc:
> > > > > > refactor notifier/event handling code to use the failover framework"
> > > > > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > > > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > > > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > > > > store pairing information..." [4] were made.
> > > > > 
> > > > > I must have missed something in those threads, but where does this UUID
> > > > > thing come about?  AFAICS this identifier doesn't need to be
> > > > > "universally" unique, nor persistent; it only has to be unique across
> > > > > the VM and stable throughout the VM lifetime.
> > > > 
> > > > The notion of using UUID came up in the thread
> > > > 
> > > >    https://www.spinics.net/lists/netdev/msg499011.html
> > > 
> > > That's probably because it was expected one of standard serial number capabilities
> > > (VPD or PCI Express serial #) will be used, which are expected to be unique.
> > > 
> > > If you are rolling your own vendor specific one, it's just an ID and
> > > does not have to be unique.
> > > 
> > > > > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > > > > to be implied in the thread you refer to.
> > > > 
> > > > Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
> > > > However, what we are doing is similar to that. Instead of 32 bits,
> > > > we are using 128 bits.
> > > 
> > > That's OK. The name is confusing though. It's a failover group id,
> > > not a UUID.
> > 
> > Sure, we can name it whatever we want. I can change it to
> > "failover-group-id", if that is what we want to call it.
> > 
> > But what is more important is, what is represented by that name? I thought
> > we were going to use UUID. The QEMU command line changes in this patch
> > set expect the user to specify an UUID as the value for this option
> > (whatever we name it). Are we still in agreement about that, or do you
> > propose something else to be used? If so, what is it? A 32-bit number, a
> > 64-bit number, or an arbitrary string?
> > 
> > Regards,
> > 
> > Venu
> 
> If we don't really need a UUID, I'd avoid that requirement.

I don't see the need for a 128-bit UUID. I just took that approach because
UUID was mentioned in "https://www.spinics.net/lists/netdev/msg499011.html".
Since it is unlikely to have more than 4 billion devices in the system,
a 32-bit value would be more than enough to uniquely identify devices!

I am looking for direction from you :-). Roman already opined that UUID
may be an overkill. It appears that you too are leaning that way. Would
it be acceptable if I change the group identifier ("failover-group-id")
to a 32-bit value? If you concur, I will start reworking my patch. Could
you please confirm?

Thanks,

Venu

> 
> 
> > > 
> > > > > > The current patch set includes all the feedback received for proposals [3]
> > > > > > and [4]. For the sake of completeness, patch for the virtio specification
> > > > > > is also included here. Following is the updated proposal.
> > > > > > 
> > > > > > 1. Extend the virtio specification to include a new virtio PCI capability
> > > > > >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > 
> > > > > > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> > > > > >    The "uuid" is a string in UUID format.
> > > > > > 
> > > > > > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> > > > > >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> > > > > >    the Q35 model is supported.
> > > > > > 
> > > > > > 4. The operator creates a unique identifier string using 'uuidgen'.
> > > > > > 
> > > > > > 5. When the virtio device is created, the operator uses the "uuid" option
> > > > > >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> > > > > >    the UUID created in step 4.
> > > > > > 
> > > > > >    QEMU stores the UUID in the virtio device's configuration space
> > > > > >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > 
> > > > > > 6. When assigning a PCI device to the guest in passthrough mode, the
> > > > > >    operator first creates a bridge using the "uuid" option (for example,
> > > > > >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> > > > > >    in step 4, and then attaches the passthrough device to the bridge.
> > > > > > 
> > > > > >    QEMU stores the UUID in the configuration space of the bridge as
> > > > > >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> > > > > >    confused with a specific organization. Instead, the vendor of the
> > > > > >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> > > > > >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> > > > > >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> > > > > >    option is specified. Otherwise, current defaults are used.
> > > > > 
> > > > > I wonder if it makes more sense to drop the concept of failover groups,
> > > > > and just refer to the standby device by device-id, like 
> > > > > 
> > > > >   -device virtio-net-pci,id=foo \
> > > > >   -device pcie-downstream,failover=foo
> > > > 
> > > > Isn't this the same as what this patch series proposes? In your
> > > > suggestion, "foo" is the entity that connects the passthrough device
> > > > and the failover device. In this patch set, that "foo" is the UUID,
> > > > and the options "id" and "failover" are replaced by "uuid". Do you agree?
> > > > 
> > > > Regards,
> > > > 
> > > > Venu
> > > > 
> > > > > The bridge device will then lookup the failover device, figure out the
> > > > > common identifier to expose to the guest, and defer the visibility of
> > > > > the PT device behind the bridge until the guest acknowledged the support
> > > > > for failover on the PV device.
> > > > > 
> > > > > Roman.

On Wed, Jun 27, 2018 at 05:34:17PM -0500, Venu Busireddy wrote:
> On 2018-06-27 23:12:12 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 27, 2018 at 02:59:01PM -0500, Venu Busireddy wrote:
> > > On 2018-06-27 22:47:05 +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> > > > > On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > > > > > On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > > > > > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > > > > > device" [1] deals with live migration of guests that use passthrough
> > > > > > > devices. However, that scheme uses the MAC address for pairing
> > > > > > > the virtio device and the passthrough device. The thread "netvsc:
> > > > > > > refactor notifier/event handling code to use the failover framework"
> > > > > > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > > > > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > > > > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > > > > > store pairing information..." [4] were made.
> > > > > > 
> > > > > > I must have missed something in those threads, but where does this UUID
> > > > > > thing come about?  AFAICS this identifier doesn't need to be
> > > > > > "universally" unique, nor persistent; it only has to be unique across
> > > > > > the VM and stable throughout the VM lifetime.
> > > > > 
> > > > > The notion of using UUID came up in the thread
> > > > > 
> > > > >    https://www.spinics.net/lists/netdev/msg499011.html
> > > > 
> > > > That's probably because it was expected one of standard serial number capabilities
> > > > (VPD or PCI Express serial #) will be used, which are expected to be unique.
> > > > 
> > > > If you are rolling your own vendor specific one, it's just an ID and
> > > > does not have to be unique.
> > > > 
> > > > > > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > > > > > to be implied in the thread you refer to.
> > > > > 
> > > > > Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
> > > > > However, what we are doing is similar to that. Instead of 32 bits,
> > > > > we are using 128 bits.
> > > > 
> > > > That's OK. The name is confusing though. It's a failover group id,
> > > > not a UUID.
> > > 
> > > Sure, we can name it whatever we want. I can change it to
> > > "failover-group-id", if that is what we want to call it.
> > > 
> > > But what is more important is, what is represented by that name? I thought
> > > we were going to use UUID. The QEMU command line changes in this patch
> > > set expect the user to specify an UUID as the value for this option
> > > (whatever we name it). Are we still in agreement about that, or do you
> > > propose something else to be used? If so, what is it? A 32-bit number, a
> > > 64-bit number, or an arbitrary string?
> > > 
> > > Regards,
> > > 
> > > Venu
> > 
> > If we don't really need a UUID, I'd avoid that requirement.
> 
> I don't see the need for a 128-bit UUID. I just took that approach because
> UUID was mentioned in "https://www.spinics.net/lists/netdev/msg499011.html".
> Since it is unlikely to have more than 4 billion devices in the system,
> a 32-bit value would be more than enough to uniquely identify devices!
> 
> I am looking for direction from you :-). Roman already opined that UUID
> may be an overkill. It appears that you too are leaning that way. Would
> it be acceptable if I change the group identifier ("failover-group-id")
> to a 32-bit value? If you concur, I will start reworking my patch. Could
> you please confirm?
> 
> Thanks,
> 
> Venu

I would do a 64 bit one, just in case we want to use PCI Express Device
Serial Number down the road.

> > 
> > 
> > > > 
> > > > > > > The current patch set includes all the feedback received for proposals [3]
> > > > > > > and [4]. For the sake of completeness, patch for the virtio specification
> > > > > > > is also included here. Following is the updated proposal.
> > > > > > > 
> > > > > > > 1. Extend the virtio specification to include a new virtio PCI capability
> > > > > > >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > > 
> > > > > > > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> > > > > > >    The "uuid" is a string in UUID format.
> > > > > > > 
> > > > > > > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> > > > > > >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> > > > > > >    the Q35 model is supported.
> > > > > > > 
> > > > > > > 4. The operator creates a unique identifier string using 'uuidgen'.
> > > > > > > 
> > > > > > > 5. When the virtio device is created, the operator uses the "uuid" option
> > > > > > >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> > > > > > >    the UUID created in step 4.
> > > > > > > 
> > > > > > >    QEMU stores the UUID in the virtio device's configuration space
> > > > > > >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > > 
> > > > > > > 6. When assigning a PCI device to the guest in passthrough mode, the
> > > > > > >    operator first creates a bridge using the "uuid" option (for example,
> > > > > > >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> > > > > > >    in step 4, and then attaches the passthrough device to the bridge.
> > > > > > > 
> > > > > > >    QEMU stores the UUID in the configuration space of the bridge as
> > > > > > >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> > > > > > >    confused with a specific organization. Instead, the vendor of the
> > > > > > >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> > > > > > >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> > > > > > >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> > > > > > >    option is specified. Otherwise, current defaults are used.
> > > > > > 
> > > > > > I wonder if it makes more sense to drop the concept of failover groups,
> > > > > > and just refer to the standby device by device-id, like 
> > > > > > 
> > > > > >   -device virtio-net-pci,id=foo \
> > > > > >   -device pcie-downstream,failover=foo
> > > > > 
> > > > > Isn't this the same as what this patch series proposes? In your
> > > > > suggestion, "foo" is the entity that connects the passthrough device
> > > > > and the failover device. In this patch set, that "foo" is the UUID,
> > > > > and the options "id" and "failover" are replaced by "uuid". Do you agree?
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Venu
> > > > > 
> > > > > > The bridge device will then lookup the failover device, figure out the
> > > > > > common identifier to expose to the guest, and defer the visibility of
> > > > > > the PT device behind the bridge until the guest acknowledged the support
> > > > > > for failover on the PV device.
> > > > > > 
> > > > > > Roman.

On 2018-06-28 04:54:16 +0300, Michael S. Tsirkin wrote:
> On Wed, Jun 27, 2018 at 05:34:17PM -0500, Venu Busireddy wrote:
> > On 2018-06-27 23:12:12 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 27, 2018 at 02:59:01PM -0500, Venu Busireddy wrote:
> > > > On 2018-06-27 22:47:05 +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> > > > > > On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > > > > > > On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > > > > > > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > > > > > > device" [1] deals with live migration of guests that use passthrough
> > > > > > > > devices. However, that scheme uses the MAC address for pairing
> > > > > > > > the virtio device and the passthrough device. The thread "netvsc:
> > > > > > > > refactor notifier/event handling code to use the failover framework"
> > > > > > > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > > > > > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > > > > > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > > > > > > store pairing information..." [4] were made.
> > > > > > > 
> > > > > > > I must have missed something in those threads, but where does this UUID
> > > > > > > thing come about?  AFAICS this identifier doesn't need to be
> > > > > > > "universally" unique, nor persistent; it only has to be unique across
> > > > > > > the VM and stable throughout the VM lifetime.
> > > > > > 
> > > > > > The notion of using UUID came up in the thread
> > > > > > 
> > > > > >    https://www.spinics.net/lists/netdev/msg499011.html
> > > > > 
> > > > > That's probably because it was expected one of standard serial number capabilities
> > > > > (VPD or PCI Express serial #) will be used, which are expected to be unique.
> > > > > 
> > > > > If you are rolling your own vendor specific one, it's just an ID and
> > > > > does not have to be unique.
> > > > > 
> > > > > > > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > > > > > > to be implied in the thread you refer to.
> > > > > > 
> > > > > > Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
> > > > > > However, what we are doing is similar to that. Instead of 32 bits,
> > > > > > we are using 128 bits.
> > > > > 
> > > > > That's OK. The name is confusing though. It's a failover group id,
> > > > > not a UUID.
> > > > 
> > > > Sure, we can name it whatever we want. I can change it to
> > > > "failover-group-id", if that is what we want to call it.
> > > > 
> > > > But what is more important is, what is represented by that name? I thought
> > > > we were going to use UUID. The QEMU command line changes in this patch
> > > > set expect the user to specify an UUID as the value for this option
> > > > (whatever we name it). Are we still in agreement about that, or do you
> > > > propose something else to be used? If so, what is it? A 32-bit number, a
> > > > 64-bit number, or an arbitrary string?
> > > > 
> > > > Regards,
> > > > 
> > > > Venu
> > > 
> > > If we don't really need a UUID, I'd avoid that requirement.
> > 
> > I don't see the need for a 128-bit UUID. I just took that approach because
> > UUID was mentioned in "https://www.spinics.net/lists/netdev/msg499011.html".
> > Since it is unlikely to have more than 4 billion devices in the system,
> > a 32-bit value would be more than enough to uniquely identify devices!
> > 
> > I am looking for direction from you :-). Roman already opined that UUID
> > may be an overkill. It appears that you too are leaning that way. Would
> > it be acceptable if I change the group identifier ("failover-group-id")
> > to a 32-bit value? If you concur, I will start reworking my patch. Could
> > you please confirm?
> > 
> > Thanks,
> > 
> > Venu
> 
> I would do a 64 bit one, just in case we want to use PCI Express Device
> Serial Number down the road.

Will do.

Venu

> > > 
> > > 
> > > > > 
> > > > > > > > The current patch set includes all the feedback received for proposals [3]
> > > > > > > > and [4]. For the sake of completeness, patch for the virtio specification
> > > > > > > > is also included here. Following is the updated proposal.
> > > > > > > > 
> > > > > > > > 1. Extend the virtio specification to include a new virtio PCI capability
> > > > > > > >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > > > 
> > > > > > > > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> > > > > > > >    The "uuid" is a string in UUID format.
> > > > > > > > 
> > > > > > > > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> > > > > > > >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> > > > > > > >    the Q35 model is supported.
> > > > > > > > 
> > > > > > > > 4. The operator creates a unique identifier string using 'uuidgen'.
> > > > > > > > 
> > > > > > > > 5. When the virtio device is created, the operator uses the "uuid" option
> > > > > > > >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> > > > > > > >    the UUID created in step 4.
> > > > > > > > 
> > > > > > > >    QEMU stores the UUID in the virtio device's configuration space
> > > > > > > >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > > > 
> > > > > > > > 6. When assigning a PCI device to the guest in passthrough mode, the
> > > > > > > >    operator first creates a bridge using the "uuid" option (for example,
> > > > > > > >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> > > > > > > >    in step 4, and then attaches the passthrough device to the bridge.
> > > > > > > > 
> > > > > > > >    QEMU stores the UUID in the configuration space of the bridge as
> > > > > > > >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> > > > > > > >    confused with a specific organization. Instead, the vendor of the
> > > > > > > >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> > > > > > > >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> > > > > > > >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> > > > > > > >    option is specified. Otherwise, current defaults are used.
> > > > > > > 
> > > > > > > I wonder if it makes more sense to drop the concept of failover groups,
> > > > > > > and just refer to the standby device by device-id, like 
> > > > > > > 
> > > > > > >   -device virtio-net-pci,id=foo \
> > > > > > >   -device pcie-downstream,failover=foo
> > > > > > 
> > > > > > Isn't this the same as what this patch series proposes? In your
> > > > > > suggestion, "foo" is the entity that connects the passthrough device
> > > > > > and the failover device. In this patch set, that "foo" is the UUID,
> > > > > > and the options "id" and "failover" are replaced by "uuid". Do you agree?
> > > > > > 
> > > > > > Regards,
> > > > > > 
> > > > > > Venu
> > > > > > 
> > > > > > > The bridge device will then lookup the failover device, figure out the
> > > > > > > common identifier to expose to the guest, and defer the visibility of
> > > > > > > the PT device behind the bridge until the guest acknowledged the support
> > > > > > > for failover on the PV device.
> > > > > > > 
> > > > > > > Roman.

On 2018-06-27 22:27:33 -0500, Venu Busireddy wrote:
> On 2018-06-28 04:54:16 +0300, Michael S. Tsirkin wrote:
> > On Wed, Jun 27, 2018 at 05:34:17PM -0500, Venu Busireddy wrote:
> > > On 2018-06-27 23:12:12 +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Jun 27, 2018 at 02:59:01PM -0500, Venu Busireddy wrote:
> > > > > On 2018-06-27 22:47:05 +0300, Michael S. Tsirkin wrote:
> > > > > > On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> > > > > > > On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > > > > > > > On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > > > > > > > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > > > > > > > device" [1] deals with live migration of guests that use passthrough
> > > > > > > > > devices. However, that scheme uses the MAC address for pairing
> > > > > > > > > the virtio device and the passthrough device. The thread "netvsc:
> > > > > > > > > refactor notifier/event handling code to use the failover framework"
> > > > > > > > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > > > > > > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > > > > > > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > > > > > > > store pairing information..." [4] were made.
> > > > > > > > 
> > > > > > > > I must have missed something in those threads, but where does this UUID
> > > > > > > > thing come about?  AFAICS this identifier doesn't need to be
> > > > > > > > "universally" unique, nor persistent; it only has to be unique across
> > > > > > > > the VM and stable throughout the VM lifetime.
> > > > > > > 
> > > > > > > The notion of using UUID came up in the thread
> > > > > > > 
> > > > > > >    https://www.spinics.net/lists/netdev/msg499011.html
> > > > > > 
> > > > > > That's probably because it was expected one of standard serial number capabilities
> > > > > > (VPD or PCI Express serial #) will be used, which are expected to be unique.
> > > > > > 
> > > > > > If you are rolling your own vendor specific one, it's just an ID and
> > > > > > does not have to be unique.
> > > > > > 
> > > > > > > > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > > > > > > > to be implied in the thread you refer to.
> > > > > > > 
> > > > > > > Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
> > > > > > > However, what we are doing is similar to that. Instead of 32 bits,
> > > > > > > we are using 128 bits.
> > > > > > 
> > > > > > That's OK. The name is confusing though. It's a failover group id,
> > > > > > not a UUID.
> > > > > 
> > > > > Sure, we can name it whatever we want. I can change it to
> > > > > "failover-group-id", if that is what we want to call it.
> > > > > 
> > > > > But what is more important is, what is represented by that name? I thought
> > > > > we were going to use UUID. The QEMU command line changes in this patch
> > > > > set expect the user to specify an UUID as the value for this option
> > > > > (whatever we name it). Are we still in agreement about that, or do you
> > > > > propose something else to be used? If so, what is it? A 32-bit number, a
> > > > > 64-bit number, or an arbitrary string?
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Venu
> > > > 
> > > > If we don't really need a UUID, I'd avoid that requirement.
> > > 
> > > I don't see the need for a 128-bit UUID. I just took that approach because
> > > UUID was mentioned in "https://www.spinics.net/lists/netdev/msg499011.html".
> > > Since it is unlikely to have more than 4 billion devices in the system,
> > > a 32-bit value would be more than enough to uniquely identify devices!
> > > 
> > > I am looking for direction from you :-). Roman already opined that UUID
> > > may be an overkill. It appears that you too are leaning that way. Would
> > > it be acceptable if I change the group identifier ("failover-group-id")
> > > to a 32-bit value? If you concur, I will start reworking my patch. Could
> > > you please confirm?
> > > 
> > > Thanks,
> > > 
> > > Venu
> > 
> > I would do a 64 bit one, just in case we want to use PCI Express Device
> > Serial Number down the road.
> 
> Will do.

I have incorporated all the changes suggested by you. They are:

  * Changed the failover group identifier from UUID to a 64-bit value.
  * Changed the command line option from "uuid" to * "failover-group-id".
  * Tweaked the "pci-bridge" device's Device ID to
  * PCI_DEVICE_ID_REDHAT_BRIDGE_FAILOVER.
  * Added to new device "pcie-downstream" with Device ID
    PCI_DEVICE_ID_REDHAT_DOWNPORT_FAILOVER (to support the PCIe case).
  * Changed the patch for virtio specification to reflect the above
    changes.

Shall I post v3 for review?

Thanks,

Venu

> > > > 
> > > > 
> > > > > > 
> > > > > > > > > The current patch set includes all the feedback received for proposals [3]
> > > > > > > > > and [4]. For the sake of completeness, patch for the virtio specification
> > > > > > > > > is also included here. Following is the updated proposal.
> > > > > > > > > 
> > > > > > > > > 1. Extend the virtio specification to include a new virtio PCI capability
> > > > > > > > >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > > > > 
> > > > > > > > > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> > > > > > > > >    The "uuid" is a string in UUID format.
> > > > > > > > > 
> > > > > > > > > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> > > > > > > > >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> > > > > > > > >    the Q35 model is supported.
> > > > > > > > > 
> > > > > > > > > 4. The operator creates a unique identifier string using 'uuidgen'.
> > > > > > > > > 
> > > > > > > > > 5. When the virtio device is created, the operator uses the "uuid" option
> > > > > > > > >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> > > > > > > > >    the UUID created in step 4.
> > > > > > > > > 
> > > > > > > > >    QEMU stores the UUID in the virtio device's configuration space
> > > > > > > > >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > > > > 
> > > > > > > > > 6. When assigning a PCI device to the guest in passthrough mode, the
> > > > > > > > >    operator first creates a bridge using the "uuid" option (for example,
> > > > > > > > >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> > > > > > > > >    in step 4, and then attaches the passthrough device to the bridge.
> > > > > > > > > 
> > > > > > > > >    QEMU stores the UUID in the configuration space of the bridge as
> > > > > > > > >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> > > > > > > > >    confused with a specific organization. Instead, the vendor of the
> > > > > > > > >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> > > > > > > > >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> > > > > > > > >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> > > > > > > > >    option is specified. Otherwise, current defaults are used.
> > > > > > > > 
> > > > > > > > I wonder if it makes more sense to drop the concept of failover groups,
> > > > > > > > and just refer to the standby device by device-id, like 
> > > > > > > > 
> > > > > > > >   -device virtio-net-pci,id=foo \
> > > > > > > >   -device pcie-downstream,failover=foo
> > > > > > > 
> > > > > > > Isn't this the same as what this patch series proposes? In your
> > > > > > > suggestion, "foo" is the entity that connects the passthrough device
> > > > > > > and the failover device. In this patch set, that "foo" is the UUID,
> > > > > > > and the options "id" and "failover" are replaced by "uuid". Do you agree?
> > > > > > > 
> > > > > > > Regards,
> > > > > > > 
> > > > > > > Venu
> > > > > > > 
> > > > > > > > The bridge device will then lookup the failover device, figure out the
> > > > > > > > common identifier to expose to the guest, and defer the visibility of
> > > > > > > > the PT device behind the bridge until the guest acknowledged the support
> > > > > > > > for failover on the PV device.
> > > > > > > > 
> > > > > > > > Roman.
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

On Fri, Jun 29, 2018 at 01:55:07PM -0500, Venu Busireddy wrote:
> On 2018-06-27 22:27:33 -0500, Venu Busireddy wrote:
> > On 2018-06-28 04:54:16 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Jun 27, 2018 at 05:34:17PM -0500, Venu Busireddy wrote:
> > > > On 2018-06-27 23:12:12 +0300, Michael S. Tsirkin wrote:
> > > > > On Wed, Jun 27, 2018 at 02:59:01PM -0500, Venu Busireddy wrote:
> > > > > > On 2018-06-27 22:47:05 +0300, Michael S. Tsirkin wrote:
> > > > > > > On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> > > > > > > > On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > > > > > > > > On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > > > > > > > > > The patch set "Enable virtio_net to act as a standby for a passthru
> > > > > > > > > > device" [1] deals with live migration of guests that use passthrough
> > > > > > > > > > devices. However, that scheme uses the MAC address for pairing
> > > > > > > > > > the virtio device and the passthrough device. The thread "netvsc:
> > > > > > > > > > refactor notifier/event handling code to use the failover framework"
> > > > > > > > > > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > > > > > > > > > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > > > > > > > > > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > > > > > > > > > store pairing information..." [4] were made.
> > > > > > > > > 
> > > > > > > > > I must have missed something in those threads, but where does this UUID
> > > > > > > > > thing come about?  AFAICS this identifier doesn't need to be
> > > > > > > > > "universally" unique, nor persistent; it only has to be unique across
> > > > > > > > > the VM and stable throughout the VM lifetime.
> > > > > > > > 
> > > > > > > > The notion of using UUID came up in the thread
> > > > > > > > 
> > > > > > > >    https://www.spinics.net/lists/netdev/msg499011.html
> > > > > > > 
> > > > > > > That's probably because it was expected one of standard serial number capabilities
> > > > > > > (VPD or PCI Express serial #) will be used, which are expected to be unique.
> > > > > > > 
> > > > > > > If you are rolling your own vendor specific one, it's just an ID and
> > > > > > > does not have to be unique.
> > > > > > > 
> > > > > > > > > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > > > > > > > > to be implied in the thread you refer to.
> > > > > > > > 
> > > > > > > > Yes, Hyper-V uses a serial number (but I think it is 64-bit value).
> > > > > > > > However, what we are doing is similar to that. Instead of 32 bits,
> > > > > > > > we are using 128 bits.
> > > > > > > 
> > > > > > > That's OK. The name is confusing though. It's a failover group id,
> > > > > > > not a UUID.
> > > > > > 
> > > > > > Sure, we can name it whatever we want. I can change it to
> > > > > > "failover-group-id", if that is what we want to call it.
> > > > > > 
> > > > > > But what is more important is, what is represented by that name? I thought
> > > > > > we were going to use UUID. The QEMU command line changes in this patch
> > > > > > set expect the user to specify an UUID as the value for this option
> > > > > > (whatever we name it). Are we still in agreement about that, or do you
> > > > > > propose something else to be used? If so, what is it? A 32-bit number, a
> > > > > > 64-bit number, or an arbitrary string?
> > > > > > 
> > > > > > Regards,
> > > > > > 
> > > > > > Venu
> > > > > 
> > > > > If we don't really need a UUID, I'd avoid that requirement.
> > > > 
> > > > I don't see the need for a 128-bit UUID. I just took that approach because
> > > > UUID was mentioned in "https://www.spinics.net/lists/netdev/msg499011.html".
> > > > Since it is unlikely to have more than 4 billion devices in the system,
> > > > a 32-bit value would be more than enough to uniquely identify devices!
> > > > 
> > > > I am looking for direction from you :-). Roman already opined that UUID
> > > > may be an overkill. It appears that you too are leaning that way. Would
> > > > it be acceptable if I change the group identifier ("failover-group-id")
> > > > to a 32-bit value? If you concur, I will start reworking my patch. Could
> > > > you please confirm?
> > > > 
> > > > Thanks,
> > > > 
> > > > Venu
> > > 
> > > I would do a 64 bit one, just in case we want to use PCI Express Device
> > > Serial Number down the road.
> > 
> > Will do.
> 
> I have incorporated all the changes suggested by you. They are:
> 
>   * Changed the failover group identifier from UUID to a 64-bit value.
>   * Changed the command line option from "uuid" to * "failover-group-id".
>   * Tweaked the "pci-bridge" device's Device ID to
>   * PCI_DEVICE_ID_REDHAT_BRIDGE_FAILOVER.
>   * Added to new device "pcie-downstream" with Device ID
>     PCI_DEVICE_ID_REDHAT_DOWNPORT_FAILOVER (to support the PCIe case).
>   * Changed the patch for virtio specification to reflect the above
>     changes.
> 
> Shall I post v3 for review?
> 
> Thanks,
> 
> Venu

Why not?

> > > > > 
> > > > > 
> > > > > > > 
> > > > > > > > > > The current patch set includes all the feedback received for proposals [3]
> > > > > > > > > > and [4]. For the sake of completeness, patch for the virtio specification
> > > > > > > > > > is also included here. Following is the updated proposal.
> > > > > > > > > > 
> > > > > > > > > > 1. Extend the virtio specification to include a new virtio PCI capability
> > > > > > > > > >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > > > > > 
> > > > > > > > > > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> > > > > > > > > >    The "uuid" is a string in UUID format.
> > > > > > > > > > 
> > > > > > > > > > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> > > > > > > > > >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> > > > > > > > > >    the Q35 model is supported.
> > > > > > > > > > 
> > > > > > > > > > 4. The operator creates a unique identifier string using 'uuidgen'.
> > > > > > > > > > 
> > > > > > > > > > 5. When the virtio device is created, the operator uses the "uuid" option
> > > > > > > > > >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> > > > > > > > > >    the UUID created in step 4.
> > > > > > > > > > 
> > > > > > > > > >    QEMU stores the UUID in the virtio device's configuration space
> > > > > > > > > >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > > > > > > > > > 
> > > > > > > > > > 6. When assigning a PCI device to the guest in passthrough mode, the
> > > > > > > > > >    operator first creates a bridge using the "uuid" option (for example,
> > > > > > > > > >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> > > > > > > > > >    in step 4, and then attaches the passthrough device to the bridge.
> > > > > > > > > > 
> > > > > > > > > >    QEMU stores the UUID in the configuration space of the bridge as
> > > > > > > > > >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> > > > > > > > > >    confused with a specific organization. Instead, the vendor of the
> > > > > > > > > >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> > > > > > > > > >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> > > > > > > > > >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> > > > > > > > > >    option is specified. Otherwise, current defaults are used.
> > > > > > > > > 
> > > > > > > > > I wonder if it makes more sense to drop the concept of failover groups,
> > > > > > > > > and just refer to the standby device by device-id, like 
> > > > > > > > > 
> > > > > > > > >   -device virtio-net-pci,id=foo \
> > > > > > > > >   -device pcie-downstream,failover=foo
> > > > > > > > 
> > > > > > > > Isn't this the same as what this patch series proposes? In your
> > > > > > > > suggestion, "foo" is the entity that connects the passthrough device
> > > > > > > > and the failover device. In this patch set, that "foo" is the UUID,
> > > > > > > > and the options "id" and "failover" are replaced by "uuid". Do you agree?
> > > > > > > > 
> > > > > > > > Regards,
> > > > > > > > 
> > > > > > > > Venu
> > > > > > > > 
> > > > > > > > > The bridge device will then lookup the failover device, figure out the
> > > > > > > > > common identifier to expose to the guest, and defer the visibility of
> > > > > > > > > the PT device behind the bridge until the guest acknowledged the support
> > > > > > > > > for failover on the PV device.
> > > > > > > > > 
> > > > > > > > > Roman.
> > 
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> > 

On Wed, Jun 27, 2018 at 02:29:58PM -0500, Venu Busireddy wrote:
> On 2018-06-27 15:24:58 +0300, Roman Kagan wrote:
> > I must have missed something in those threads, but where does this UUID
> > thing come about?  AFAICS this identifier doesn't need to be
> > "universally" unique, nor persistent; it only has to be unique across
> > the VM and stable throughout the VM lifetime.
> 
> The notion of using UUID came up in the thread
> 
>    https://www.spinics.net/lists/netdev/msg499011.html

Yes I saw it and it looks like UUID came out of miscommunication:

>> >> As Stephen said, Hyper-V supports the serial UUID thing from
>> >> day-one.

but I don't see Stephen's message where he claims that.  In reality

> > FWIW Hyper-V uses a 32-bit integer for this purpose, not a UUID as seems
> > to be implied in the thread you refer to.
> 
> Yes, Hyper-V uses a serial number (but I think it is 64-bit value).

It's u32, see

drivers/pci/controller/pci-hyperv.c:struct pci_function_description::ser

and 

drivers/net/hyperv/hyperv_net.h:struct nvsp_4_send_vf_association::serial

> However, what we are doing is similar to that. Instead of 32 bits,
> we are using 128 bits.

UUID is far less convenient to handle than an integer.  But anyway I
think this should rather be transport-specific, see below.

> > I wonder if it makes more sense to drop the concept of failover groups,
> > and just refer to the standby device by device-id, like 
> > 
> >   -device virtio-net-pci,id=foo \
> >   -device pcie-downstream,failover=foo
> 
> Isn't this the same as what this patch series proposes? In your
> suggestion, "foo" is the entity that connects the passthrough device
> and the failover device. In this patch set, that "foo" is the UUID,
> and the options "id" and "failover" are replaced by "uuid". Do you agree?

No it's not the same.

I think this whole "failover group" concept is a misnomer: there can
only be two participating devices, they need a tighter coupling to each
other than just sharing a common identifier exposed to the guest, and
their relationship is strongly asymmetric.

Rather, the two devices

  - need to be of matching types to be able to do failover (like
    a virtio-net-pci and a pcie-downstream, or a hv-net and a
    hv-pci-bridge, etc.)

  - need to communicate status information, adressing Michael's concern
    re. PT visibility: as I wrote
> > The bridge device will [...] defer the visibility of the PT device
> > behind the bridge until the guest acknowledged the support for
> > failover on the PV device.

  - need to communicate a common identifier and have a
    transport-specific scheme to present it to the guest, but this
    common identifier *doesn't* generally have to be

    * of the same type for all transports[*]
    * globally unique
    * persistent
    * provided by / visible to the management layer

Now linking two devices in QEMU by making one of them know the device-id
of the other seems to be the most natural thing.

[*] e.g. for Hyper-V it's u32 (set by the third party); virtio may
define different schemes depending on the kind of PT device, e.g. u64 if
using a PCIe device (and put it in its device serial number cap),
something else otherwise.

Roman.

On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> The patch set "Enable virtio_net to act as a standby for a passthru
> device" [1] deals with live migration of guests that use passthrough
> devices. However, that scheme uses the MAC address for pairing
> the virtio device and the passthrough device. The thread "netvsc:
> refactor notifier/event handling code to use the failover framework"
> [2] discusses an alternate mechanism, such as using an UUID, for pairing
> the devices. Based on that discussion, proposals "Add "Group Identifier"
> to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> store pairing information..." [4] were made.
> 
> The current patch set includes all the feedback received for proposals [3]
> and [4]. For the sake of completeness, patch for the virtio specification
> is also included here. Following is the updated proposal.
> 
> 1. Extend the virtio specification to include a new virtio PCI capability
>    "VIRTIO_PCI_CAP_GROUP_ID_CFG".

There's still discussion around whether it should be
a virtio pci capability, a virtio net config field or
a new kind of capability.

> 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
>    The "uuid" is a string in UUID format.
> 
> 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
>    The "uuid" is a string in UUID format. Currently, PCIe bridge for
>    the Q35 model is supported.
> 
> 4. The operator creates a unique identifier string using 'uuidgen'.
> 
> 5. When the virtio device is created, the operator uses the "uuid" option
>    (for example, '-device virtio-net-pci,uuid="string"') and specifies
>    the UUID created in step 4.
> 
>    QEMU stores the UUID in the virtio device's configuration space
>    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> 
> 6. When assigning a PCI device to the guest in passthrough mode, the
>    operator first creates a bridge using the "uuid" option (for example,
>    '-device pcie-downstream,uuid="string"') to specify the UUID created
>    in step 4, and then attaches the passthrough device to the bridge.
> 
>    QEMU stores the UUID in the configuration space of the bridge as
>    Vendor-Specific capability (0x09). The "Vendor" here is not to be
>    confused with a specific organization. Instead, the vendor of the
>    bridge is QEMU. To avoid mixing up with other bridges, the bridge
>    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
>    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
>    option is specified. Otherwise, current defaults are used.
> 
> 7. Patch 4 in patch series "Enable virtio_net to act as a standby for
>    a passthru device" [1] needs to be modified to use the UUID values
>    present in the bridge's configuration space and the virtio device's
>    configuration space instead of the MAC address for pairing the devices.
> 
> Thanks!
> 
> Venu

The part where the visibility of a vfio device is controlled by the
virtio driver acknowledging the backup feature is missing here.
 

> [1] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00156.html
> [2] https://www.spinics.net/lists/netdev/msg499011.html
> [3] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00118.html
> [4] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00204.html
> 
> Changes in v2:
>   - As Michael Tsirkin suggested, changed the virtio specification
>     to restrict the group identifier to be a 16-byte field, presented
>     entirely in the virtio device's configuration space.
>   - As Michael Tsirkin suggested, instead of tweaking the ioh3420
>     device with Red Hat vendor ID, create a new PCIe bridge device
>     named "pcie-downstream" with Red Hat Vendor ID, and include the
>     group identifier in this device.
>   - Added a new patch to enhance the "pci-bridge" device to support
>     the group identifier (for the i440FX model).
> 
> Venu Busireddy (4):
>   Add a true or false option to the DEFINE_PROP_UUID macro.
>   Add "Group Identifier" support to virtio devices.
>   Add "Group Identifier" support to Red Hat PCI bridge.
>   Add "Group Identifier" support to Red Hat PCI Express bridge.
> 
>  default-configs/arm-softmmu.mak             |   1 +
>  default-configs/i386-softmmu.mak            |   1 +
>  default-configs/x86_64-softmmu.mak          |   1 +
>  hw/acpi/vmgenid.c                           |   2 +-
>  hw/pci-bridge/Makefile.objs                 |   1 +
>  hw/pci-bridge/pci_bridge_dev.c              |   8 +
>  hw/pci-bridge/pcie_downstream.c             | 215 ++++++++++++++++++++
>  hw/pci-bridge/pcie_downstream.h             |  10 +
>  hw/pci/pci_bridge.c                         |  26 +++
>  hw/virtio/virtio-pci.c                      |  15 ++
>  hw/virtio/virtio-pci.h                      |   3 +-
>  include/hw/pci/pci.h                        |   3 +
>  include/hw/pci/pcie.h                       |   1 +
>  include/hw/qdev-properties.h                |   4 +-
>  include/standard-headers/linux/virtio_pci.h |   8 +
>  15 files changed, 295 insertions(+), 4 deletions(-)
>  create mode 100644 hw/pci-bridge/pcie_downstream.c
>  create mode 100644 hw/pci-bridge/pcie_downstream.h

On 2018-06-27 07:06:42 +0300, Michael S. Tsirkin wrote:
> On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > The patch set "Enable virtio_net to act as a standby for a passthru
> > device" [1] deals with live migration of guests that use passthrough
> > devices. However, that scheme uses the MAC address for pairing
> > the virtio device and the passthrough device. The thread "netvsc:
> > refactor notifier/event handling code to use the failover framework"
> > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > store pairing information..." [4] were made.
> > 
> > The current patch set includes all the feedback received for proposals [3]
> > and [4]. For the sake of completeness, patch for the virtio specification
> > is also included here. Following is the updated proposal.
> > 
> > 1. Extend the virtio specification to include a new virtio PCI capability
> >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> 
> There's still discussion around whether it should be
> a virtio pci capability, a virtio net config field or
> a new kind of capability.
> 
> > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> >    The "uuid" is a string in UUID format.
> > 
> > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> >    the Q35 model is supported.
> > 
> > 4. The operator creates a unique identifier string using 'uuidgen'.
> > 
> > 5. When the virtio device is created, the operator uses the "uuid" option
> >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> >    the UUID created in step 4.
> > 
> >    QEMU stores the UUID in the virtio device's configuration space
> >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > 
> > 6. When assigning a PCI device to the guest in passthrough mode, the
> >    operator first creates a bridge using the "uuid" option (for example,
> >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> >    in step 4, and then attaches the passthrough device to the bridge.
> > 
> >    QEMU stores the UUID in the configuration space of the bridge as
> >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> >    confused with a specific organization. Instead, the vendor of the
> >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> >    option is specified. Otherwise, current defaults are used.
> > 
> > 7. Patch 4 in patch series "Enable virtio_net to act as a standby for
> >    a passthru device" [1] needs to be modified to use the UUID values
> >    present in the bridge's configuration space and the virtio device's
> >    configuration space instead of the MAC address for pairing the devices.
> > 
> > Thanks!
> > 
> > Venu
> 
> The part where the visibility of a vfio device is controlled by the
> virtio driver acknowledging the backup feature is missing here.

Could you please elaborate?

Thanks,

Venu

>  
> 
> > [1] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00156.html
> > [2] https://www.spinics.net/lists/netdev/msg499011.html
> > [3] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00118.html
> > [4] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00204.html
> > 
> > Changes in v2:
> >   - As Michael Tsirkin suggested, changed the virtio specification
> >     to restrict the group identifier to be a 16-byte field, presented
> >     entirely in the virtio device's configuration space.
> >   - As Michael Tsirkin suggested, instead of tweaking the ioh3420
> >     device with Red Hat vendor ID, create a new PCIe bridge device
> >     named "pcie-downstream" with Red Hat Vendor ID, and include the
> >     group identifier in this device.
> >   - Added a new patch to enhance the "pci-bridge" device to support
> >     the group identifier (for the i440FX model).
> > 
> > Venu Busireddy (4):
> >   Add a true or false option to the DEFINE_PROP_UUID macro.
> >   Add "Group Identifier" support to virtio devices.
> >   Add "Group Identifier" support to Red Hat PCI bridge.
> >   Add "Group Identifier" support to Red Hat PCI Express bridge.
> > 
> >  default-configs/arm-softmmu.mak             |   1 +
> >  default-configs/i386-softmmu.mak            |   1 +
> >  default-configs/x86_64-softmmu.mak          |   1 +
> >  hw/acpi/vmgenid.c                           |   2 +-
> >  hw/pci-bridge/Makefile.objs                 |   1 +
> >  hw/pci-bridge/pci_bridge_dev.c              |   8 +
> >  hw/pci-bridge/pcie_downstream.c             | 215 ++++++++++++++++++++
> >  hw/pci-bridge/pcie_downstream.h             |  10 +
> >  hw/pci/pci_bridge.c                         |  26 +++
> >  hw/virtio/virtio-pci.c                      |  15 ++
> >  hw/virtio/virtio-pci.h                      |   3 +-
> >  include/hw/pci/pci.h                        |   3 +
> >  include/hw/pci/pcie.h                       |   1 +
> >  include/hw/qdev-properties.h                |   4 +-
> >  include/standard-headers/linux/virtio_pci.h |   8 +
> >  15 files changed, 295 insertions(+), 4 deletions(-)
> >  create mode 100644 hw/pci-bridge/pcie_downstream.c
> >  create mode 100644 hw/pci-bridge/pcie_downstream.h
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
> 

On Tue, Jun 26, 2018 at 9:06 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
>> The patch set "Enable virtio_net to act as a standby for a passthru
>> device" [1] deals with live migration of guests that use passthrough
>> devices. However, that scheme uses the MAC address for pairing
>> the virtio device and the passthrough device. The thread "netvsc:
>> refactor notifier/event handling code to use the failover framework"
>> [2] discusses an alternate mechanism, such as using an UUID, for pairing
>> the devices. Based on that discussion, proposals "Add "Group Identifier"
>> to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
>> store pairing information..." [4] were made.
>>
>> The current patch set includes all the feedback received for proposals [3]
>> and [4]. For the sake of completeness, patch for the virtio specification
>> is also included here. Following is the updated proposal.
>>
>> 1. Extend the virtio specification to include a new virtio PCI capability
>>    "VIRTIO_PCI_CAP_GROUP_ID_CFG".
>
> There's still discussion around whether it should be
> a virtio pci capability, a virtio net config field or
> a new kind of capability.
>
>> 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
>>    The "uuid" is a string in UUID format.
>>
>> 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
>>    The "uuid" is a string in UUID format. Currently, PCIe bridge for
>>    the Q35 model is supported.
>>
>> 4. The operator creates a unique identifier string using 'uuidgen'.
>>
>> 5. When the virtio device is created, the operator uses the "uuid" option
>>    (for example, '-device virtio-net-pci,uuid="string"') and specifies
>>    the UUID created in step 4.
>>
>>    QEMU stores the UUID in the virtio device's configuration space
>>    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
>>
>> 6. When assigning a PCI device to the guest in passthrough mode, the
>>    operator first creates a bridge using the "uuid" option (for example,
>>    '-device pcie-downstream,uuid="string"') to specify the UUID created
>>    in step 4, and then attaches the passthrough device to the bridge.
>>
>>    QEMU stores the UUID in the configuration space of the bridge as
>>    Vendor-Specific capability (0x09). The "Vendor" here is not to be
>>    confused with a specific organization. Instead, the vendor of the
>>    bridge is QEMU. To avoid mixing up with other bridges, the bridge
>>    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
>>    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
>>    option is specified. Otherwise, current defaults are used.
>>
>> 7. Patch 4 in patch series "Enable virtio_net to act as a standby for
>>    a passthru device" [1] needs to be modified to use the UUID values
>>    present in the bridge's configuration space and the virtio device's
>>    configuration space instead of the MAC address for pairing the devices.
>>
>> Thanks!
>>
>> Venu
>
> The part where the visibility of a vfio device is controlled by the
> virtio driver acknowledging the backup feature is missing here.

While it is still being discussed, I tend to put that effort into a
seperate set of patches, and this patch series comes first just for
enabling guest to use the UUID feature (I've some code pending). Does
this division makes sense? I realize that the vfio's visibility work
would need to take a while.

-Siwei

>
>
>> [1] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00156.html
>> [2] https://www.spinics.net/lists/netdev/msg499011.html
>> [3] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00118.html
>> [4] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00204.html
>>
>> Changes in v2:
>>   - As Michael Tsirkin suggested, changed the virtio specification
>>     to restrict the group identifier to be a 16-byte field, presented
>>     entirely in the virtio device's configuration space.
>>   - As Michael Tsirkin suggested, instead of tweaking the ioh3420
>>     device with Red Hat vendor ID, create a new PCIe bridge device
>>     named "pcie-downstream" with Red Hat Vendor ID, and include the
>>     group identifier in this device.
>>   - Added a new patch to enhance the "pci-bridge" device to support
>>     the group identifier (for the i440FX model).
>>
>> Venu Busireddy (4):
>>   Add a true or false option to the DEFINE_PROP_UUID macro.
>>   Add "Group Identifier" support to virtio devices.
>>   Add "Group Identifier" support to Red Hat PCI bridge.
>>   Add "Group Identifier" support to Red Hat PCI Express bridge.
>>
>>  default-configs/arm-softmmu.mak             |   1 +
>>  default-configs/i386-softmmu.mak            |   1 +
>>  default-configs/x86_64-softmmu.mak          |   1 +
>>  hw/acpi/vmgenid.c                           |   2 +-
>>  hw/pci-bridge/Makefile.objs                 |   1 +
>>  hw/pci-bridge/pci_bridge_dev.c              |   8 +
>>  hw/pci-bridge/pcie_downstream.c             | 215 ++++++++++++++++++++
>>  hw/pci-bridge/pcie_downstream.h             |  10 +
>>  hw/pci/pci_bridge.c                         |  26 +++
>>  hw/virtio/virtio-pci.c                      |  15 ++
>>  hw/virtio/virtio-pci.h                      |   3 +-
>>  include/hw/pci/pci.h                        |   3 +
>>  include/hw/pci/pcie.h                       |   1 +
>>  include/hw/qdev-properties.h                |   4 +-
>>  include/standard-headers/linux/virtio_pci.h |   8 +
>>  15 files changed, 295 insertions(+), 4 deletions(-)
>>  create mode 100644 hw/pci-bridge/pcie_downstream.c
>>  create mode 100644 hw/pci-bridge/pcie_downstream.h
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>

On Wed, 27 Jun 2018 07:06:42 +0300
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jun 26, 2018 at 10:49:30PM -0500, Venu Busireddy wrote:
> > The patch set "Enable virtio_net to act as a standby for a passthru
> > device" [1] deals with live migration of guests that use passthrough
> > devices. However, that scheme uses the MAC address for pairing
> > the virtio device and the passthrough device. The thread "netvsc:
> > refactor notifier/event handling code to use the failover framework"
> > [2] discusses an alternate mechanism, such as using an UUID, for pairing
> > the devices. Based on that discussion, proposals "Add "Group Identifier"
> > to virtio PCI capabilities." [3] and "RFC: Use of bridge devices to
> > store pairing information..." [4] were made.
> > 
> > The current patch set includes all the feedback received for proposals [3]
> > and [4]. For the sake of completeness, patch for the virtio specification
> > is also included here. Following is the updated proposal.
> > 
> > 1. Extend the virtio specification to include a new virtio PCI capability
> >    "VIRTIO_PCI_CAP_GROUP_ID_CFG".  
> 
> There's still discussion around whether it should be
> a virtio pci capability, a virtio net config field or
> a new kind of capability.

Moreover, I hate to be that person again, but...

> 
> > 2. Enhance the QEMU CLI to include a "uuid" option to the virtio device.
> >    The "uuid" is a string in UUID format.
> > 
> > 3. Enhance the QEMU CLI to include a "uuid" option to the bridge device.
> >    The "uuid" is a string in UUID format. Currently, PCIe bridge for
> >    the Q35 model is supported.
> > 
> > 4. The operator creates a unique identifier string using 'uuidgen'.
> > 
> > 5. When the virtio device is created, the operator uses the "uuid" option
> >    (for example, '-device virtio-net-pci,uuid="string"') and specifies
> >    the UUID created in step 4.
> > 
> >    QEMU stores the UUID in the virtio device's configuration space
> >    in the capability "VIRTIO_PCI_CAP_GROUP_ID_CFG".
> > 
> > 6. When assigning a PCI device to the guest in passthrough mode, the
> >    operator first creates a bridge using the "uuid" option (for example,
> >    '-device pcie-downstream,uuid="string"') to specify the UUID created
> >    in step 4, and then attaches the passthrough device to the bridge.
> > 
> >    QEMU stores the UUID in the configuration space of the bridge as
> >    Vendor-Specific capability (0x09). The "Vendor" here is not to be
> >    confused with a specific organization. Instead, the vendor of the
> >    bridge is QEMU. To avoid mixing up with other bridges, the bridge
> >    will be created with vendor ID 0x1b36 (PCI_VENDOR_ID_REDHAT) and
> >    device ID 0x000e (PCI_DEVICE_ID_REDHAT_PCIE_BRIDGE) if the "uuid"
> >    option is specified. Otherwise, current defaults are used.

...if the bridge is the means of getting the uuid to the guest for the
passthrough device, I don't see how this can work on s390. Unless I'm
mistaken, we don't see the pci bridge created in QEMU from the guest
(zPCI uses instructions to list functions, and does not have any
topology view). Adding some folks who have worked on zPCI for their
comments.

> > 
> > 7. Patch 4 in patch series "Enable virtio_net to act as a standby for
> >    a passthru device" [1] needs to be modified to use the UUID values
> >    present in the bridge's configuration space and the virtio device's
> >    configuration space instead of the MAC address for pairing the devices.
> > 
> > Thanks!
> > 
> > Venu  
> 
> The part where the visibility of a vfio device is controlled by the
> virtio driver acknowledging the backup feature is missing here.

Also, shouldn't libvirt be involved at some point in time?

>  
> 
> > [1] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00156.html
> > [2] https://www.spinics.net/lists/netdev/msg499011.html
> > [3] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00118.html
> > [4] https://lists.oasis-open.org/archives/virtio-dev/201805/msg00204.html
> > 
> > Changes in v2:
> >   - As Michael Tsirkin suggested, changed the virtio specification
> >     to restrict the group identifier to be a 16-byte field, presented
> >     entirely in the virtio device's configuration space.
> >   - As Michael Tsirkin suggested, instead of tweaking the ioh3420
> >     device with Red Hat vendor ID, create a new PCIe bridge device
> >     named "pcie-downstream" with Red Hat Vendor ID, and include the
> >     group identifier in this device.
> >   - Added a new patch to enhance the "pci-bridge" device to support
> >     the group identifier (for the i440FX model).
> > 
> > Venu Busireddy (4):
> >   Add a true or false option to the DEFINE_PROP_UUID macro.
> >   Add "Group Identifier" support to virtio devices.
> >   Add "Group Identifier" support to Red Hat PCI bridge.
> >   Add "Group Identifier" support to Red Hat PCI Express bridge.
> > 
> >  default-configs/arm-softmmu.mak             |   1 +
> >  default-configs/i386-softmmu.mak            |   1 +
> >  default-configs/x86_64-softmmu.mak          |   1 +
> >  hw/acpi/vmgenid.c                           |   2 +-
> >  hw/pci-bridge/Makefile.objs                 |   1 +
> >  hw/pci-bridge/pci_bridge_dev.c              |   8 +
> >  hw/pci-bridge/pcie_downstream.c             | 215 ++++++++++++++++++++
> >  hw/pci-bridge/pcie_downstream.h             |  10 +
> >  hw/pci/pci_bridge.c                         |  26 +++
> >  hw/virtio/virtio-pci.c                      |  15 ++
> >  hw/virtio/virtio-pci.h                      |   3 +-
> >  include/hw/pci/pci.h                        |   3 +
> >  include/hw/pci/pcie.h                       |   1 +
> >  include/hw/qdev-properties.h                |   4 +-
> >  include/standard-headers/linux/virtio_pci.h |   8 +
> >  15 files changed, 295 insertions(+), 4 deletions(-)
> >  create mode 100644 hw/pci-bridge/pcie_downstream.c
> >  create mode 100644 hw/pci-bridge/pcie_downstream.h  
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>