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

[Thibaut Collet]

On Tue, Aug 4, 2015 at 8:22 AM, Jason Wang <address@hidden> wrote:
>
>
> On 08/03/2015 05:22 PM, Thibaut Collet wrote:
>> 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>
>> ---
>>  docs/specs/vhost-user.txt         |   15 +++++++++++++++
>>  hw/net/vhost_net.c                |   16 ++++++++++++++++
>>  hw/virtio/vhost-user.c            |   32 ++++++++++++++++++++++++++++++--
>>  include/hw/virtio/vhost-backend.h |    2 ++
>>  include/net/vhost_net.h           |    1 +
>>  net/vhost-user.c                  |   18 ++++++++++++++++++
>>  6 files changed, 82 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
>> index c2d2e2a..8c05301 100644
>> --- a/docs/specs/vhost-user.txt
>> +++ b/docs/specs/vhost-user.txt
>> @@ -140,6 +140,7 @@ Protocol features
>>  -----------------
>>
>>  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 0
>> +#define VHOST_USER_PROTOCOL_F_RARP      1
>>
>>  Message types
>>  -------------
>> @@ -308,6 +309,20 @@ Message types
>>        invalid FD flag. This flag is set when there is no file descriptor
>>        in the ancillary data.
>>
>> + * VHOST_USER_SEND_RARP
>> +
>> +      Id: 17
>> +      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.
>> +
>>  Migration
>
> If for some reason the announce packet was changed in the future, we may
> then need another kind of message type. Just wonder whether allowing
> qemu to inject packet directly to vhost-user is better.
>

Purpose of the announce packet is to notified switches that MAC
address of the guest has moved to avoid network outage.
So only knowledge of the MAC address is needed to construct the
announce packet (whatever is format)
The new message provides the MAC address.  Vhost user backend can
create an announce packet that is not necessary the same of QEMU

Providing the announce packet directly to vhost user is possible but
needs a lot of additional change:
 - The announce packet can not be copy in the master payload (in the
future if the announce packet changed its size can changed too)
The master payload can be a u64 that gives the size of the message.
The message will be stored in a shared memory between QEMU and backend
and a fd to this shared memory must be provided too.
That can be interesting if QEMU has several type of packets to inject
to vhost user but it is not the case and I prefer to keep my solution
(less complex and risk of bugs or memory leakage is less)

>>  ---------
>>
>> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
>> index 9850520..c3b9664 100644
>> --- a/hw/net/vhost_net.c
>> +++ b/hw/net/vhost_net.c
>> @@ -383,6 +383,17 @@ void vhost_net_cleanup(struct vhost_net *net)
>>      g_free(net);
>>  }
>>
>> +int vhost_net_migrate(struct vhost_net *net, char* mac_addr)
>> +{
>> +    const VhostOps *vhost_ops = net->dev.vhost_ops;
>> +    int r = -1;
>> +
>> +    if (vhost_ops->vhost_backend_migrate)
>> +        r = vhost_ops->vhost_backend_migrate(&net->dev, mac_addr);
>> +
>> +    return r;
>> +}
>> +
>>  bool vhost_net_virtqueue_pending(VHostNetState *net, int idx)
>>  {
>>      return vhost_virtqueue_pending(&net->dev, idx);
>> @@ -456,6 +467,11 @@ void vhost_net_virtqueue_mask(VHostNetState *net, 
>> VirtIODevice *dev,
>>  {
>>  }
>>
>> +int vhost_net_migrate(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 fe75618..693840f 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"
>> @@ -27,8 +28,9 @@
>>
>>  #define VHOST_USER_F_PROTOCOL_FEATURES 30
>>
>> -#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x1ULL
>> +#define VHOST_USER_PROTOCOL_FEATURE_MASK 0x3ULL
>>  #define VHOST_USER_PROTOCOL_F_LOG_SHMFD 0
>> +#define VHOST_USER_PROTOCOL_F_RARP      1
>>
>>  typedef enum VhostUserRequest {
>>      VHOST_USER_NONE = 0,
>> @@ -48,6 +50,7 @@ typedef enum VhostUserRequest {
>>      VHOST_USER_SET_VRING_ERR = 14,
>>      VHOST_USER_GET_PROTOCOL_FEATURES = 15,
>>      VHOST_USER_SET_PROTOCOL_FEATURES = 16,
>> +    VHOST_USER_SEND_RARP = 17,
>>      VHOST_USER_MAX
>>  } VhostUserRequest;
>>
>> @@ -409,9 +412,34 @@ static int vhost_user_cleanup(struct vhost_dev *dev)
>>      return 0;
>>  }
>>
>> +static int vhost_user_migrate(struct vhost_dev *dev, char* mac_addr)
>> +{
>
> Not sure migrate is a good name since migration was not done in this
> function and it was called in vhost_user_receive().
>

OK. I will rename it to vhost_user_notify_migration_done that is the
purpose of this function