From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60301) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fStWh-00006e-5k for qemu-devel@nongnu.org; Tue, 12 Jun 2018 20:20:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fStWd-0002cg-RI for qemu-devel@nongnu.org; Tue, 12 Jun 2018 20:20:39 -0400 Received: from mga02.intel.com ([134.134.136.20]:45200) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fStWd-0002bH-Ct for qemu-devel@nongnu.org; Tue, 12 Jun 2018 20:20:35 -0400 References: <1525734594-11134-1-git-send-email-sridhar.samudrala@intel.com> <20180605152344-mutt-send-email-mst@kernel.org> <20180612144743-mutt-send-email-mst@kernel.org> From: "Samudrala, Sridhar" Message-ID: <7aa6dbe7-bac2-96f6-d96c-f51e5173c556@intel.com> Date: Tue, 12 Jun 2018 17:20:25 -0700 MIME-Version: 1.0 In-Reply-To: <20180612144743-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: Re: [Qemu-devel] [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" , Jason Wang Cc: virtualization@lists.linux-foundation.org, virtio-dev@lists.oasis-open.org, jesse.brandeburg@intel.com, alexander.h.duyck@intel.com, qemu-devel@nongnu.org On 6/12/2018 4:54 AM, Michael S. Tsirkin wrote: > On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote: >> >> On 2018年06月05日 20:33, Michael S. Tsirkin wrote: >>> I don't think this is sufficient. >>> >>> If both primary and standby devices are present, a legacy guest without >>> support for the feature might see two devices with same mac and get >>> confused. >>> >>> I think that we should only make primary visible after guest acked the >>> backup feature bit. >> I think we want exactly the reverse? E.g fail the negotiation when guest >> does not ack backup feature. >> >> Otherwise legacy guest won't even have the chance to see primary device in >> the guest. > That's by design. > >>> And on reset or when backup is cleared in some other way, unplug the >>> primary. >> What if guest does not support hotplug? > It shouldn't ack the backup feature then and it's a good point. > We should both document this and check kernel config has > hotplug support. Sridhar could you take a look pls? Right now we select NET_FAILOVER when virtio-net is enabled,  Should we select PCI_HOTPLUG when NET_FAILOVER is enabled? > >>> Something like the below will do the job: >>> >>> Primary device is added with a special "primary-failover" flag. >>> A virtual machine is then initialized with just a standby virtio >>> device. Primary is not yet added. >> A question is how to do the matching? Qemu knows nothing about e.g mac >> address of a pass-through device I believe? > Supply a flag to the VFIO when it's added, this way QEMU will know. > >>> Later QEMU detects that guest driver device set DRIVER_OK. >>> It then exposes the primary device to the guest, and triggers >>> a device addition event (hot-plug event) for it. >> Do you mean we won't have primary device in the initial qemu cli? > No, that's not what I mean. > > I mean management will supply a flag to VFIO and then > > > - VFIO defers exposing > primary to guest until guest acks the feature bit. > - When we see guest ack, initiate hotplug. > - On reboot, hide it again. > - On reset without reboot, request hot-unplug and on eject hide it again. > > >>> If QEMU detects guest driver removal, it initiates a hot-unplug sequence >>> to remove the primary driver. In particular, if QEMU detects guest >>> re-initialization (e.g. by detecting guest reset) it immediately removes >>> the primary device. >> I believe guest failover module should handle this gracefully? > It can't control enumeration order, if primary is enumerated before > standby then guest will load its driver and it's too late > when failover driver is loaded. Does it mean that if guest unloads virtio-net driver, Qemu should automatically unplug the associated primary device? What about guest unloading/re-loading primary device driver? > >> Thanks >> >>> We can move some of this code to management as well, architecturally it >>> does not make too much sense but it might be easier implementation-wise. >>> >>> HTH >>> >>> On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote: >>>> Ping on this patch now that the kernel patches are accepted into davem's net-next tree. >>>> https://patchwork.ozlabs.org/cover/920005/ >>>> >>>> >>>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote: >>>>> This feature bit can be used by hypervisor to indicate virtio_net device to >>>>> act as a standby for another device with the same MAC address. >>>>> >>>>> I tested this with a small change to the patch to mark the STANDBY feature 'true' >>>>> by default as i am using libvirt to start the VMs. >>>>> Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt >>>>> XML file? >>>>> >>>>> Signed-off-by: Sridhar Samudrala >>>>> --- >>>>> hw/net/virtio-net.c | 2 ++ >>>>> include/standard-headers/linux/virtio_net.h | 3 +++ >>>>> 2 files changed, 5 insertions(+) >>>>> >>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>>> index 90502fca7c..38b3140670 100644 >>>>> --- a/hw/net/virtio-net.c >>>>> +++ b/hw/net/virtio-net.c >>>>> @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = { >>>>> true), >>>>> DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN), >>>>> DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str), >>>>> + DEFINE_PROP_BIT64("standby", VirtIONet, host_features, VIRTIO_NET_F_STANDBY, >>>>> + false), >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h >>>>> index e9f255ea3f..01ec09684c 100644 >>>>> --- a/include/standard-headers/linux/virtio_net.h >>>>> +++ b/include/standard-headers/linux/virtio_net.h >>>>> @@ -57,6 +57,9 @@ >>>>> * Steering */ >>>>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ >>>>> +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device >>>>> + * with the same MAC. >>>>> + */ >>>>> #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ >>>>> #ifndef VIRTIO_NET_NO_LEGACY From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: virtio-dev-return-4356-cohuck=redhat.com@lists.oasis-open.org Sender: List-Post: List-Help: List-Unsubscribe: List-Subscribe: Received: from lists.oasis-open.org (oasis-open.org [66.179.20.138]) by lists.oasis-open.org (Postfix) with ESMTP id 3AEEF58191EA for ; Tue, 12 Jun 2018 17:20:35 -0700 (PDT) References: <1525734594-11134-1-git-send-email-sridhar.samudrala@intel.com> <20180605152344-mutt-send-email-mst@kernel.org> <20180612144743-mutt-send-email-mst@kernel.org> From: "Samudrala, Sridhar" Message-ID: <7aa6dbe7-bac2-96f6-d96c-f51e5173c556@intel.com> Date: Tue, 12 Jun 2018 17:20:25 -0700 MIME-Version: 1.0 In-Reply-To: <20180612144743-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Transfer-Encoding: 8bit Content-Language: en-US Subject: [virtio-dev] Re: [PATCH] qemu: Introduce VIRTIO_NET_F_STANDBY feature bit to virtio_net To: "Michael S. Tsirkin" , Jason Wang Cc: virtualization@lists.linux-foundation.org, virtio-dev@lists.oasis-open.org, jesse.brandeburg@intel.com, alexander.h.duyck@intel.com, qemu-devel@nongnu.org List-ID: On 6/12/2018 4:54 AM, Michael S. Tsirkin wrote: > On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote: >> >> On 2018年06月05日 20:33, Michael S. Tsirkin wrote: >>> I don't think this is sufficient. >>> >>> If both primary and standby devices are present, a legacy guest without >>> support for the feature might see two devices with same mac and get >>> confused. >>> >>> I think that we should only make primary visible after guest acked the >>> backup feature bit. >> I think we want exactly the reverse? E.g fail the negotiation when guest >> does not ack backup feature. >> >> Otherwise legacy guest won't even have the chance to see primary device in >> the guest. > That's by design. > >>> And on reset or when backup is cleared in some other way, unplug the >>> primary. >> What if guest does not support hotplug? > It shouldn't ack the backup feature then and it's a good point. > We should both document this and check kernel config has > hotplug support. Sridhar could you take a look pls? Right now we select NET_FAILOVER when virtio-net is enabled,  Should we select PCI_HOTPLUG when NET_FAILOVER is enabled? > >>> Something like the below will do the job: >>> >>> Primary device is added with a special "primary-failover" flag. >>> A virtual machine is then initialized with just a standby virtio >>> device. Primary is not yet added. >> A question is how to do the matching? Qemu knows nothing about e.g mac >> address of a pass-through device I believe? > Supply a flag to the VFIO when it's added, this way QEMU will know. > >>> Later QEMU detects that guest driver device set DRIVER_OK. >>> It then exposes the primary device to the guest, and triggers >>> a device addition event (hot-plug event) for it. >> Do you mean we won't have primary device in the initial qemu cli? > No, that's not what I mean. > > I mean management will supply a flag to VFIO and then > > > - VFIO defers exposing > primary to guest until guest acks the feature bit. > - When we see guest ack, initiate hotplug. > - On reboot, hide it again. > - On reset without reboot, request hot-unplug and on eject hide it again. > > >>> If QEMU detects guest driver removal, it initiates a hot-unplug sequence >>> to remove the primary driver. In particular, if QEMU detects guest >>> re-initialization (e.g. by detecting guest reset) it immediately removes >>> the primary device. >> I believe guest failover module should handle this gracefully? > It can't control enumeration order, if primary is enumerated before > standby then guest will load its driver and it's too late > when failover driver is loaded. Does it mean that if guest unloads virtio-net driver, Qemu should automatically unplug the associated primary device? What about guest unloading/re-loading primary device driver? > >> Thanks >> >>> We can move some of this code to management as well, architecturally it >>> does not make too much sense but it might be easier implementation-wise. >>> >>> HTH >>> >>> On Mon, Jun 04, 2018 at 06:41:48PM -0700, Samudrala, Sridhar wrote: >>>> Ping on this patch now that the kernel patches are accepted into davem's net-next tree. >>>> https://patchwork.ozlabs.org/cover/920005/ >>>> >>>> >>>> On 5/7/2018 4:09 PM, Sridhar Samudrala wrote: >>>>> This feature bit can be used by hypervisor to indicate virtio_net device to >>>>> act as a standby for another device with the same MAC address. >>>>> >>>>> I tested this with a small change to the patch to mark the STANDBY feature 'true' >>>>> by default as i am using libvirt to start the VMs. >>>>> Is there a way to pass the newly added feature bit 'standby' to qemu via libvirt >>>>> XML file? >>>>> >>>>> Signed-off-by: Sridhar Samudrala >>>>> --- >>>>> hw/net/virtio-net.c | 2 ++ >>>>> include/standard-headers/linux/virtio_net.h | 3 +++ >>>>> 2 files changed, 5 insertions(+) >>>>> >>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>>>> index 90502fca7c..38b3140670 100644 >>>>> --- a/hw/net/virtio-net.c >>>>> +++ b/hw/net/virtio-net.c >>>>> @@ -2198,6 +2198,8 @@ static Property virtio_net_properties[] = { >>>>> true), >>>>> DEFINE_PROP_INT32("speed", VirtIONet, net_conf.speed, SPEED_UNKNOWN), >>>>> DEFINE_PROP_STRING("duplex", VirtIONet, net_conf.duplex_str), >>>>> + DEFINE_PROP_BIT64("standby", VirtIONet, host_features, VIRTIO_NET_F_STANDBY, >>>>> + false), >>>>> DEFINE_PROP_END_OF_LIST(), >>>>> }; >>>>> diff --git a/include/standard-headers/linux/virtio_net.h b/include/standard-headers/linux/virtio_net.h >>>>> index e9f255ea3f..01ec09684c 100644 >>>>> --- a/include/standard-headers/linux/virtio_net.h >>>>> +++ b/include/standard-headers/linux/virtio_net.h >>>>> @@ -57,6 +57,9 @@ >>>>> * Steering */ >>>>> #define VIRTIO_NET_F_CTRL_MAC_ADDR 23 /* Set MAC address */ >>>>> +#define VIRTIO_NET_F_STANDBY 62 /* Act as standby for another device >>>>> + * with the same MAC. >>>>> + */ >>>>> #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed and duplex */ >>>>> #ifndef VIRTIO_NET_NO_LEGACY --------------------------------------------------------------------- To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org