From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:54213) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fSvgT-0003t3-0f for qemu-devel@nongnu.org; Tue, 12 Jun 2018 22:38:54 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fSvgP-0007Wf-TO for qemu-devel@nongnu.org; Tue, 12 Jun 2018 22:38:53 -0400 Received: from mx3-rdu2.redhat.com ([66.187.233.73]:54846 helo=mx1.redhat.com) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fSvgP-0007WX-N5 for qemu-devel@nongnu.org; Tue, 12 Jun 2018 22:38:49 -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: Jason Wang Message-ID: <5718de66-74c9-1362-a87b-2eba02475b48@redhat.com> Date: Wed, 13 Jun 2018 10:38:41 +0800 MIME-Version: 1.0 In-Reply-To: <20180612144743-mutt-send-email-mst@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: quoted-printable 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" Cc: "Samudrala, Sridhar" , 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 2018=E5=B9=B406=E6=9C=8812=E6=97=A5 19:54, Michael S. Tsirkin wrote: > On Wed, Jun 06, 2018 at 10:29:03AM +0800, Jason Wang wrote: >> >> On 2018=E5=B9=B406=E6=9C=8805=E6=97=A5 20:33, Michael S. Tsirkin wrote= : >>> I don't think this is sufficient. >>> >>> If both primary and standby devices are present, a legacy guest witho= ut >>> 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 th= e >>> backup feature bit. >> I think we want exactly the reverse? E.g fail the negotiation when gue= st >> does not ack backup feature. >> >> Otherwise legacy guest won't even have the chance to see primary devic= e in >> the guest. > That's by design. So management needs to know the capability of guest to set the backup=20 feature. This looks a chicken or egg problem to me. > >>> 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? > >>> 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 agai= n. This sounds much like a kind of bonding in qemu. > >>> If QEMU detects guest driver removal, it initiates a hot-unplug seque= nce >>> to remove the primary driver. In particular, if QEMU detects guest >>> re-initialization (e.g. by detecting guest reset) it immediately remo= ves >>> 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. Well, even if we can delay the visibility of primary until DRIVER_OK,=20 there still be a race I think? And it looks to me it's still a bug of gue= st: E.g primary could be probed before failover_register() in guest. Then we=20 will miss the enslaving of primary forever. Thanks > >> 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-wi= se. >>> >>> 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 dav= em'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 d= evice 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 qem= u 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[] =3D { >>>>> 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 anot= her device >>>>> + * with the same MAC. >>>>> + */ >>>>> #define VIRTIO_NET_F_SPEED_DUPLEX 63 /* Device set linkspeed an= d duplex */ >>>>> #ifndef VIRTIO_NET_NO_LEGACY