Re: [Qemu-devel] [PATCH v5 15/21] vhost user: add rarp sending after live migration for legacy guest

[Marc-André Lureau]

Hi

On Sun, Sep 27, 2015 at 3:12 PM, Michael S. Tsirkin <address@hidden> wrote:
> On Thu, Sep 24, 2015 at 05:53:05PM -0400, Marc-André Lureau wrote:
>> Hi
>>
>> ----- Original Message -----
>> > On Thu, Sep 24, 2015 at 6:22 PM,  <address@hidden> wrote:
>> > > From: Thibaut Collet <address@hidden>
>> > >
>> > > A new vhost user message is added to allow QEMU to ask to vhost user
>> > > backend to
>> > > broadcast a fake RARP after live migration for guest without 
>> > > GUEST_ANNOUNCE
>> > > capability.
>> > >
>> > > This new message is sent only if the backend supports the new
>> > > VHOST_USER_PROTOCOL_F_RARP protocol feature.
>> > > The payload of this new message is the MAC address of the guest (not 
>> > > known
>> > > by
>> > > the backend). The MAC address is copied in the first 6 bytes of a u64 to
>> > > avoid
>> > > to create a new payload message type.
>> > >
>> > > This new message has no equivalent ioctl so a new callback is added in 
>> > > the
>> > > userOps structure to send the request.
>> > >
>> > > Upon reception of this new message the vhost user backend must generate 
>> > > and
>> > > broadcast a fake RARP request to notify the migration is terminated.
>> > >
>> > > Signed-off-by: Thibaut Collet <address@hidden>
>> > > [Rebased and fixed checkpatch errors - Marc-André]
>> > > Signed-off-by: Marc-André Lureau <address@hidden>
>> > > ---
>> > >  docs/specs/vhost-user.txt         | 15 +++++++++++++++
>> > >  hw/net/vhost_net.c                | 17 +++++++++++++++++
>> > >  hw/virtio/vhost-user.c            | 30 ++++++++++++++++++++++++++++++
>> > >  include/hw/virtio/vhost-backend.h |  3 +++
>> > >  include/net/vhost_net.h           |  1 +
>> > >  net/vhost-user.c                  | 24 ++++++++++++++++++++++--
>> > >  6 files changed, 88 insertions(+), 2 deletions(-)
>> > >
>> > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>> > > index e0292a0..e0d71e2 100644
>> > > --- a/docs/specs/vhost-user.txt
>> > > +++ b/docs/specs/vhost-user.txt
>> > > @@ -194,6 +194,7 @@ Protocol features
>> > >
>> > >  #define VHOST_USER_PROTOCOL_F_MQ             0
>> > >  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD      1
>> > > +#define VHOST_USER_PROTOCOL_F_RARP           2
>> > >
>> > >  Message types
>> > >  -------------
>> > > @@ -381,3 +382,17 @@ Message types
>> > >        Master payload: vring state description
>> > >
>> > >        Signal slave to enable or disable corresponding vring.
>> > > +
>> > > + * VHOST_USER_SEND_RARP
>> > > +
>> > > +      Id: 19
>> > > +      Equivalent ioctl: N/A
>> > > +      Master payload: u64
>> > > +
>> > > +      Ask vhost user backend to broadcast a fake RARP to notify the
>> > > migration
>> > > +      is terminated for guest that does not support GUEST_ANNOUNCE.
>> > > +      Only legal if feature bit VHOST_USER_F_PROTOCOL_FEATURES is 
>> > > present
>> > > in
>> > > +      VHOST_USER_GET_FEATURES and protocol feature bit
>> > > VHOST_USER_PROTOCOL_F_RARP
>> > > +      is present in VHOST_USER_GET_PROTOCOL_FEATURES.
>> > > +      The first 6 bytes of the payload contain the mac address of the
>> > > guest to
>> > > +      allow the vhost user backend to construct and broadcast the fake
>> > > RARP.
>> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> > > index 840f443..da66b64 100644
>> > > --- a/hw/net/vhost_net.c
>> > > +++ b/hw/net/vhost_net.c
>> > > @@ -388,6 +388,18 @@ void vhost_net_cleanup(struct vhost_net *net)
>> > >      g_free(net);
>> > >  }
>> > >
>> > > +int vhost_net_notify_migration_done(struct vhost_net *net, char* 
>> > > mac_addr)
>> > > +{
>> > > +    const VhostOps *vhost_ops = net->dev.vhost_ops;
>> > > +    int r = -1;
>> > > +
>> > > +    if (vhost_ops->vhost_migration_done) {
>> > > +        r = vhost_ops->vhost_migration_done(&net->dev, mac_addr);
>> > > +    }
>> > > +
>> > > +    return r;
>> > > +}
>> > > +
>> > >  bool vhost_net_virtqueue_pending(VHostNetState *net, int idx)
>> > >  {
>> > >      return vhost_virtqueue_pending(&net->dev, idx);
>> > > @@ -479,6 +491,11 @@ void vhost_net_virtqueue_mask(VHostNetState *net,
>> > > VirtIODevice *dev,
>> > >  {
>> > >  }
>> > >
>> > > +int vhost_net_notify_migration_done(struct vhost_net *net)
>> > > +{
>> > > +    return -1;
>> > > +}
>> > > +
>> > >  VHostNetState *get_vhost_net(NetClientState *nc)
>> > >  {
>> > >      return 0;
>> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
>> > > index 455caba..b7f3699 100644
>> > > --- a/hw/virtio/vhost-user.c
>> > > +++ b/hw/virtio/vhost-user.c
>> > > @@ -10,6 +10,7 @@
>> > >
>> > >  #include "hw/virtio/vhost.h"
>> > >  #include "hw/virtio/vhost-backend.h"
>> > > +#include "hw/virtio/virtio-net.h"
>> > >  #include "sysemu/char.h"
>> > >  #include "sysemu/kvm.h"
>> > >  #include "qemu/error-report.h"
>> > > @@ -30,6 +31,7 @@
>> > >  #define VHOST_USER_PROTOCOL_FEATURE_MASK 0x3ULL
>> > >  #define VHOST_USER_PROTOCOL_F_MQ         0
>> > >  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD  1
>> > > +#define VHOST_USER_PROTOCOL_F_RARP       2
>> >
>> > The VHOST_USER_PROTOCOL_FEATURE_MASK  must be changed and set to 0x7ULL
>> >
> Better, change VHOST_USER_PROTOCOL_FEATURE_MASK to be
> calculated based on other macros.

yeah, I actually thought about something like this a few days ago:
https://github.com/elmarco/qemu/commit/b4e4ec2e4c78f6b12e34822d8a3f3a101d065d80

Is that what you have in mind or rather make a bit mask of all the
features? Maybe the bitmask of all the features is simpler.

>
>>
>> Good catch (too many rebases, having a test would help to prevent this kind 
>> of mistake)
>>
>> thanks
>
> So there will be v6 with a fix?

If there is no further changes in the series, I guess you could fix it
when cherry-picking. But if I have to send a v6, I'll include the fix.

thanks



-- 
Marc-André Lureau