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

[Laurent Vivier <lvivier@redhat.com>]

On 24/01/2025 19:40, Stefano Brivio wrote:
> On Fri, 24 Jan 2025 19:35:33 +0100
> Laurent Vivier <lvivier@redhat.com> wrote:
> 
>> On 24/01/2025 17:10, Stefano Brivio wrote:
>>> On Fri, 24 Jan 2025 15:21:37 +0100
>>> Laurent Vivier <lvivier@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@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.com/
>>>
>>> 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