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

[Stefano Brivio <sbrivio@redhat.com>]

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 ...

> >> + * @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,
> >>   };  

-- 
Stefano