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

newer
[PATCH v2] vhost-user: Implement...

Laurent Vivier

24 Jan 2025 24 Jan '25

3:21 p.m.

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

Show replies by date

Stefano Brivio

24 Jan 24 Jan

5:10 p.m.

On Fri, 24 Jan 2025 15:21:37 +0100 Laurent Vivier 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 --- 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.c... What about "Do nothing to silence QEMU bogus error message"? Claiming we are broadcasting a RARP message and not doing it is... confusing.

...

+ * @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/?

...

+ 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

Laurent Vivier

7:35 p.m.

On 24/01/2025 17:10, Stefano Brivio wrote:

...

On Fri, 24 Jan 2025 15:21:37 +0100 Laurent Vivier 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 --- 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.c...

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

...

...
+ * @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

Stefano Brivio

7:40 p.m.

On Fri, 24 Jan 2025 19:35:33 +0100 Laurent Vivier wrote:

...

On 24/01/2025 17:10, Stefano Brivio wrote:

...
On Fri, 24 Jan 2025 15:21:37 +0100 Laurent Vivier 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 --- 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.c...

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

Laurent Vivier

7:50 p.m.

On 24/01/2025 19:40, Stefano Brivio wrote:

...

On Fri, 24 Jan 2025 19:35:33 +0100 Laurent Vivier wrote:

...
On 24/01/2025 17:10, Stefano Brivio wrote:

...
On Fri, 24 Jan 2025 15:21:37 +0100 Laurent Vivier 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 --- 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.c...

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

Stefano Brivio

7:57 p.m.

On Fri, 24 Jan 2025 19:50:57 +0100 Laurent Vivier wrote:

...

On 24/01/2025 19:40, Stefano Brivio wrote:

...
On Fri, 24 Jan 2025 19:35:33 +0100 Laurent Vivier wrote:

...
On 24/01/2025 17:10, Stefano Brivio wrote:

...
On Fri, 24 Jan 2025 15:21:37 +0100 Laurent Vivier 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 --- 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.c...

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.

Given it's already two edits it would save me some time if you could write/format it properly... -- Stefano

110

Age (days ago)

110

Last active (days ago)

List overview

Download

5 comments

2 participants

participants (2)

Laurent Vivier
Stefano Brivio