Re: [PATCH] vhost-user: Implement an empty VHOST_USER_SEND_RARP command

On 24/01/2025 19:40, Stefano Brivio wrote:
...
  On Fri, 24 Jan 2025 19:35:33 +0100
 Laurent Vivier <lvivier(a)redhat.com> wrote:
 
  On 24/01/2025 17:10, Stefano Brivio wrote:
  On Fri, 24 Jan 2025 15:21:37 +0100
 Laurent Vivier <lvivier(a)redhat.com> wrote:
    
  Passt cannot manage and doesn't need to
manage the broadcast of a fake RARP,
 but QEMU will report an error message if Passt doesn't implement it.

 Implement an empty SEND_RARP command to silence QEMU error message.

 Signed-off-by: Laurent Vivier <lvivier(a)redhat.com>
 ---
    vhost_user.c | 28 +++++++++++++++++++++++++++-
    1 file changed, 27 insertions(+), 1 deletion(-)

 diff --git a/vhost_user.c b/vhost_user.c
 index f12dec5ddc58..e6633ae75ce8 100644
 --- a/vhost_user.c
 +++ b/vhost_user.c
 @@ -914,7 +914,8 @@ static bool vu_get_protocol_features_exec(struct vu_dev *vdev,
    {
    	uint64_t features = 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK |
    			    1ULL << VHOST_USER_PROTOCOL_F_LOG_SHMFD |
 -			    1ULL << VHOST_USER_PROTOCOL_F_DEVICE_STATE;
 +			    1ULL << VHOST_USER_PROTOCOL_F_DEVICE_STATE |
 +			    1ULL << VHOST_USER_PROTOCOL_F_RARP;
    
    	(void)vdev;
    	vmsg_set_reply_u64(msg, features);
 @@ -981,6 +982,30 @@ static bool vu_set_vring_enable_exec(struct vu_dev *vdev,
    	return false;
    }
    
 +/**
 + * vu_set_send_rarp_exec() - Broadcast a fake RARP to notify the migration
 + *			     is terminated 
 Fine, so we need to add this.

 But can we at least make it clear for our future benefit? That is,
 there's no such thing as "fake RARP". The only thing that's actually
 fake here is this callback. For others, see thread at:

     https://lore.kernel.org/qemu-devel/20250121100029.1106973-1-lvivier@redhat.…

 What about "Do nothing to silence QEMU bogus error message"? Claiming
 we are broadcasting a RARP message and not doing it is... confusing. 
 I think it's interesting to have this comment as it comes from the vhost-user
 specification as it describes the aim of the command, and we can add something like
"but
 as passt don't need to update any ARP table we do nothing only to silence QEMU bogus
error
 message". 
 
 Eh, if we're already making it verbose, maybe we can go with something
 like:
 
 	vhost-user specification says: "Broadcast ...", but passt ... 
As you prefer...

If you want you can update the patch when you commit it.

...
  
 
 
  + * @vdev:	vhost-user device
 + * @vmsg:	vhost-user message
 + *
 + * Return: False as no reply is requested
 + */
 +static bool vu_send_rarp_exec(struct vu_dev *vdev,
 +			      struct vhost_user_msg *msg)
 +{
 +	char macstr[ETH_ADDRSTRLEN];
 +
 +	(void)vdev;
 +
 +	/* ignore the command */
 +
 +	debug("Ignore command VHOST_USER_SEND_RARP from %s", 
 This is also a bit confusing, but I'm not sure I got it right.

 We don't actually get any message *from* the guest, correct? We get a
 message from QEMU telling us we should send a RARP broadcast *for* a
 given MAC address...? If I got it right, s/from/for/? 
 Yes, you're right.
 The purpose of this debug message was to identify for :) which interface QEMU wants this
 broadcast.

>    
>> +	      eth_ntop((unsigned char *)&msg->payload.u64, macstr,
>> +		       sizeof(macstr)));
>> +
>> +	return false;
>> +}
>> +
>>    /**
>>     * vu_set_migration_watch() - Add the migration file descriptor to epoll
>>     * @vdev:	vhost-user device
>> @@ -1177,6 +1202,7 @@ static bool (*vu_handle[VHOST_USER_MAX])(struct vu_dev
*vdev,
>>    	[VHOST_USER_SET_VRING_CALL]	   = vu_set_vring_call_exec,
>>    	[VHOST_USER_SET_VRING_ERR]	   = vu_set_vring_err_exec,
>>    	[VHOST_USER_SET_VRING_ENABLE]	   = vu_set_vring_enable_exec,
>> +	[VHOST_USER_SEND_RARP]		   = vu_send_rarp_exec,
>>    	[VHOST_USER_SET_DEVICE_STATE_FD]   = vu_set_device_state_fd_exec,
>>    	[VHOST_USER_CHECK_DEVICE_STATE]    = vu_check_device_state_exec,
>>    }; 
  
Thanks,
Laurent