qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vdpa: Allow vDPA to work on big-endian machine
@ 2025-02-11 16:19 Konstantin Shkolnyy
  2025-02-12 13:38 ` Eugenio Perez Martin
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Konstantin Shkolnyy @ 2025-02-11 16:19 UTC (permalink / raw)
  To: eperezma; +Cc: mst, sgarzare, mjrosato, qemu-devel, Konstantin Shkolnyy

Add .set_vnet_le() function that always returns success, assuming that
vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and
outputs the message:
"backend does not support LE vnet headers; falling back on userspace virtio"

Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
---
 net/vhost-vdpa.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 231b45246c..7219aa2eee 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
 
 }
 
+static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
+{
+    return 0;
+}
+
 static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc,
                                        Error **errp)
 {
@@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
         .cleanup = vhost_vdpa_cleanup,
         .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
         .has_ufo = vhost_vdpa_has_ufo,
+        .set_vnet_le = vhost_vdpa_set_vnet_le,
         .check_peer_type = vhost_vdpa_check_peer_type,
         .set_steering_ebpf = vhost_vdpa_set_steering_ebpf,
 };
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-11 16:19 [PATCH] vdpa: Allow vDPA to work on big-endian machine Konstantin Shkolnyy
@ 2025-02-12 13:38 ` Eugenio Perez Martin
  2025-02-12 14:47   ` Konstantin Shkolnyy
  2025-02-12 14:52 ` Philippe Mathieu-Daudé
  2025-02-20 15:57 ` Michael S. Tsirkin
  2 siblings, 1 reply; 16+ messages in thread
From: Eugenio Perez Martin @ 2025-02-12 13:38 UTC (permalink / raw)
  To: Konstantin Shkolnyy; +Cc: mst, sgarzare, mjrosato, qemu-devel

On Tue, Feb 11, 2025 at 5:20 PM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote:
>
> Add .set_vnet_le() function that always returns success, assuming that
> vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and
> outputs the message:
> "backend does not support LE vnet headers; falling back on userspace virtio"
>
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>

I guess this patch should be merged after
https://lists.nongnu.org/archive/html/qemu-devel/2025-02/msg02290.html
? Actually, it is better to make it a series of patches, isn't it?

> ---
>  net/vhost-vdpa.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 231b45246c..7219aa2eee 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
>
>  }
>
> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
> +{
> +    return 0;
> +}
> +
>  static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc,
>                                         Error **errp)
>  {
> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
>          .cleanup = vhost_vdpa_cleanup,
>          .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>          .has_ufo = vhost_vdpa_has_ufo,
> +        .set_vnet_le = vhost_vdpa_set_vnet_le,
>          .check_peer_type = vhost_vdpa_check_peer_type,
>          .set_steering_ebpf = vhost_vdpa_set_steering_ebpf,
>  };
> --
> 2.34.1
>



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-12 13:38 ` Eugenio Perez Martin
@ 2025-02-12 14:47   ` Konstantin Shkolnyy
  0 siblings, 0 replies; 16+ messages in thread
From: Konstantin Shkolnyy @ 2025-02-12 14:47 UTC (permalink / raw)
  To: Eugenio Perez Martin; +Cc: mst, sgarzare, mjrosato, qemu-devel

On 2/12/2025 07:38, Eugenio Perez Martin wrote:
> On Tue, Feb 11, 2025 at 5:20 PM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote:
>>
>> Add .set_vnet_le() function that always returns success, assuming that
>> vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and
>> outputs the message:
>> "backend does not support LE vnet headers; falling back on userspace virtio"
>>
>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> 
> I guess this patch should be merged after
> https://lists.nongnu.org/archive/html/qemu-devel/2025-02/msg02290.html
> ? Actually, it is better to make it a series of patches, isn't it?

It doesn't matter if it's before or after. The only (coincidental) 
connection between these 2 patches is that both are needed for QEMU to 
enable vDPA on a big-endian machine.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-11 16:19 [PATCH] vdpa: Allow vDPA to work on big-endian machine Konstantin Shkolnyy
  2025-02-12 13:38 ` Eugenio Perez Martin
@ 2025-02-12 14:52 ` Philippe Mathieu-Daudé
  2025-02-12 17:24   ` Konstantin Shkolnyy
  2025-02-20 15:57 ` Michael S. Tsirkin
  2 siblings, 1 reply; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 14:52 UTC (permalink / raw)
  To: Konstantin Shkolnyy, eperezma; +Cc: mst, sgarzare, mjrosato, qemu-devel

On 11/2/25 17:19, Konstantin Shkolnyy wrote:
> Add .set_vnet_le() function that always returns success, assuming that
> vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and
> outputs the message:
> "backend does not support LE vnet headers; falling back on userspace virtio"
> 
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> ---
>   net/vhost-vdpa.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 231b45246c..7219aa2eee 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
>   
>   }
>   
> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
> +{
> +    return 0;
> +}
> +
>   static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc,
>                                          Error **errp)
>   {
> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
>           .cleanup = vhost_vdpa_cleanup,
>           .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>           .has_ufo = vhost_vdpa_has_ufo,
> +        .set_vnet_le = vhost_vdpa_set_vnet_le,

Dubious mismatch with set_vnet_be handler.

>           .check_peer_type = vhost_vdpa_check_peer_type,
>           .set_steering_ebpf = vhost_vdpa_set_steering_ebpf,
>   };



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-12 14:52 ` Philippe Mathieu-Daudé
@ 2025-02-12 17:24   ` Konstantin Shkolnyy
  2025-02-12 18:07     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 16+ messages in thread
From: Konstantin Shkolnyy @ 2025-02-12 17:24 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, eperezma; +Cc: mst, sgarzare, mjrosato, qemu-devel

On 2/12/2025 08:52, Philippe Mathieu-Daudé wrote:
> On 11/2/25 17:19, Konstantin Shkolnyy wrote:
>> Add .set_vnet_le() function that always returns success, assuming that
>> vDPA h/w always implements LE data format. Otherwise, QEMU disables 
>> vDPA and
>> outputs the message:
>> "backend does not support LE vnet headers; falling back on userspace 
>> virtio"
>>
>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>> ---
>>   net/vhost-vdpa.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 231b45246c..7219aa2eee 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
>>   }
>> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
>> +{
>> +    return 0;
>> +}
>> +
>>   static bool vhost_vdpa_check_peer_type(NetClientState *nc, 
>> ObjectClass *oc,
>>                                          Error **errp)
>>   {
>> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
>>           .cleanup = vhost_vdpa_cleanup,
>>           .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>>           .has_ufo = vhost_vdpa_has_ufo,
>> +        .set_vnet_le = vhost_vdpa_set_vnet_le,
> 
> Dubious mismatch with set_vnet_be handler.

I'm not sure what you are suggesting...



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-12 17:24   ` Konstantin Shkolnyy
@ 2025-02-12 18:07     ` Philippe Mathieu-Daudé
  2025-02-12 20:01       ` Konstantin Shkolnyy
  0 siblings, 1 reply; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-12 18:07 UTC (permalink / raw)
  To: Konstantin Shkolnyy, eperezma; +Cc: mst, sgarzare, mjrosato, qemu-devel

On 12/2/25 18:24, Konstantin Shkolnyy wrote:
> On 2/12/2025 08:52, Philippe Mathieu-Daudé wrote:
>> On 11/2/25 17:19, Konstantin Shkolnyy wrote:
>>> Add .set_vnet_le() function that always returns success, assuming that
>>> vDPA h/w always implements LE data format. Otherwise, QEMU disables 
>>> vDPA and
>>> outputs the message:
>>> "backend does not support LE vnet headers; falling back on userspace 
>>> virtio"
>>>
>>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>>> ---
>>>   net/vhost-vdpa.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>> index 231b45246c..7219aa2eee 100644
>>> --- a/net/vhost-vdpa.c
>>> +++ b/net/vhost-vdpa.c
>>> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
>>>   }
>>> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>>   static bool vhost_vdpa_check_peer_type(NetClientState *nc, 
>>> ObjectClass *oc,
>>>                                          Error **errp)
>>>   {
>>> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
>>>           .cleanup = vhost_vdpa_cleanup,
>>>           .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>>>           .has_ufo = vhost_vdpa_has_ufo,
>>> +        .set_vnet_le = vhost_vdpa_set_vnet_le,
>>
>> Dubious mismatch with set_vnet_be handler.
> 
> I'm not sure what you are suggesting...

Implement set_vnet_le for parity?



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-12 18:07     ` Philippe Mathieu-Daudé
@ 2025-02-12 20:01       ` Konstantin Shkolnyy
  2025-02-18 13:27         ` Konstantin Shkolnyy
  0 siblings, 1 reply; 16+ messages in thread
From: Konstantin Shkolnyy @ 2025-02-12 20:01 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, eperezma; +Cc: mst, sgarzare, mjrosato, qemu-devel

On 2/12/2025 12:07, Philippe Mathieu-Daudé wrote:
> On 12/2/25 18:24, Konstantin Shkolnyy wrote:
>> On 2/12/2025 08:52, Philippe Mathieu-Daudé wrote:
>>> On 11/2/25 17:19, Konstantin Shkolnyy wrote:
>>>> Add .set_vnet_le() function that always returns success, assuming that
>>>> vDPA h/w always implements LE data format. Otherwise, QEMU disables 
>>>> vDPA and
>>>> outputs the message:
>>>> "backend does not support LE vnet headers; falling back on userspace 
>>>> virtio"
>>>>
>>>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>>>> ---
>>>>   net/vhost-vdpa.c | 6 ++++++
>>>>   1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>> index 231b45246c..7219aa2eee 100644
>>>> --- a/net/vhost-vdpa.c
>>>> +++ b/net/vhost-vdpa.c
>>>> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
>>>>   }
>>>> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
>>>> +{
>>>> +    return 0;
>>>> +}
>>>> +
>>>>   static bool vhost_vdpa_check_peer_type(NetClientState *nc, 
>>>> ObjectClass *oc,
>>>>                                          Error **errp)
>>>>   {
>>>> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
>>>>           .cleanup = vhost_vdpa_cleanup,
>>>>           .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>>>>           .has_ufo = vhost_vdpa_has_ufo,
>>>> +        .set_vnet_le = vhost_vdpa_set_vnet_le,
>>>
>>> Dubious mismatch with set_vnet_be handler.
>>
>> I'm not sure what you are suggesting...
> 
> Implement set_vnet_le for parity?

To my (very limited) knowledge, kernel's vhost_vdpa that QEMU talks to 
doesn't have an API to "change h/w endianness". If so, vDPA's 
.set_vnet_le/be(), as well as qemu_set_vnet_le/be() have very limited 
choices. qemu_set_vnet_le/be() behavior with vDPA was to simply assume 
that h/w endianness by default matches host's. This assumption is valid 
for other types of "NetClients" which are implemented in s/w. However, I 
suspect, vDPA h/w might all be going to be LE, to match virtio 1.0. Such 
is the NIC I'm dealing with.

My patch is only fixing a specific use case. Perhaps, for a complete 
fix, qemu_set_vnet_be() also shouldn't unconditionally return success on 
big endian machines, but always call .set_vnet_be() so that vDPA could 
fail it? But then it would start calling .set_vnet_be() on other 
"NetClients" where it didn't before.

That's why I don't want to just add a .set_vnet_be(), before someone 
here even confirms that vDPA h/w is indeed assumed LE, and, hence, what 
the right path is to a complete solution...

int qemu_set_vnet_be(NetClientState *nc, bool is_be)
{
#if HOST_BIG_ENDIAN
     return 0;
#else
     if (!nc || !nc->info->set_vnet_be)
         return -ENOSYS;

     return nc->info->set_vnet_be(nc, is_be);
#endif
}



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-12 20:01       ` Konstantin Shkolnyy
@ 2025-02-18 13:27         ` Konstantin Shkolnyy
  2025-02-18 14:02           ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 16+ messages in thread
From: Konstantin Shkolnyy @ 2025-02-18 13:27 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, Jason Wang
  Cc: mst, sgarzare, mjrosato, qemu-devel

On 2/12/2025 14:01, Konstantin Shkolnyy wrote:
> On 2/12/2025 12:07, Philippe Mathieu-Daudé wrote:
>> On 12/2/25 18:24, Konstantin Shkolnyy wrote:
>>> On 2/12/2025 08:52, Philippe Mathieu-Daudé wrote:
>>>> On 11/2/25 17:19, Konstantin Shkolnyy wrote:
>>>>> Add .set_vnet_le() function that always returns success, assuming that
>>>>> vDPA h/w always implements LE data format. Otherwise, QEMU disables 
>>>>> vDPA and
>>>>> outputs the message:
>>>>> "backend does not support LE vnet headers; falling back on 
>>>>> userspace virtio"
>>>>>
>>>>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>>>>> ---
>>>>>   net/vhost-vdpa.c | 6 ++++++
>>>>>   1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>>> index 231b45246c..7219aa2eee 100644
>>>>> --- a/net/vhost-vdpa.c
>>>>> +++ b/net/vhost-vdpa.c
>>>>> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState 
>>>>> *nc)
>>>>>   }
>>>>> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
>>>>> +{
>>>>> +    return 0;
>>>>> +}
>>>>> +
>>>>>   static bool vhost_vdpa_check_peer_type(NetClientState *nc, 
>>>>> ObjectClass *oc,
>>>>>                                          Error **errp)
>>>>>   {
>>>>> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
>>>>>           .cleanup = vhost_vdpa_cleanup,
>>>>>           .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>>>>>           .has_ufo = vhost_vdpa_has_ufo,
>>>>> +        .set_vnet_le = vhost_vdpa_set_vnet_le,
>>>>
>>>> Dubious mismatch with set_vnet_be handler.
>>>
>>> I'm not sure what you are suggesting...
>>
>> Implement set_vnet_le for parity?
> 
> To my (very limited) knowledge, kernel's vhost_vdpa that QEMU talks to 
> doesn't have an API to "change h/w endianness". If so, 
> vDPA's .set_vnet_le/be(), as well as qemu_set_vnet_le/be() have very 
> limited choices. qemu_set_vnet_le/be() behavior with vDPA was to simply 
> assume that h/w endianness by default matches host's. This assumption is 
> valid for other types of "NetClients" which are implemented in s/w. 
> However, I suspect, vDPA h/w might all be going to be LE, to match 
> virtio 1.0. Such is the NIC I'm dealing with.
> 
> My patch is only fixing a specific use case. Perhaps, for a complete 
> fix, qemu_set_vnet_be() also shouldn't unconditionally return success on 
> big endian machines, but always call .set_vnet_be() so that vDPA could 
> fail it? But then it would start calling .set_vnet_be() on other 
> "NetClients" where it didn't before.
> 
> That's why I don't want to just add a .set_vnet_be(), before someone 
> here even confirms that vDPA h/w is indeed assumed LE, and, hence, what 
> the right path is to a complete solution...
> 
> int qemu_set_vnet_be(NetClientState *nc, bool is_be)
> {
> #if HOST_BIG_ENDIAN
>      return 0;
> #else
>      if (!nc || !nc->info->set_vnet_be)
>          return -ENOSYS;
> 
>      return nc->info->set_vnet_be(nc, is_be);
> #endif
> }
> 

Does anyone have any answers/suggestions?



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-18 13:27         ` Konstantin Shkolnyy
@ 2025-02-18 14:02           ` Philippe Mathieu-Daudé
  2025-02-19  0:29             ` Jason Wang
  0 siblings, 1 reply; 16+ messages in thread
From: Philippe Mathieu-Daudé @ 2025-02-18 14:02 UTC (permalink / raw)
  To: Konstantin Shkolnyy, Jason Wang
  Cc: mst, sgarzare, mjrosato, qemu-devel, Akihiko Odaki, Jason Wang,
	Yuri Benditovich, Laurent Vivier, Hanna Czenczek

Hi Konstantin,

(Cc'ing more developers)

On 18/2/25 14:27, Konstantin Shkolnyy wrote:
> On 2/12/2025 14:01, Konstantin Shkolnyy wrote:
>> On 2/12/2025 12:07, Philippe Mathieu-Daudé wrote:
>>> On 12/2/25 18:24, Konstantin Shkolnyy wrote:
>>>> On 2/12/2025 08:52, Philippe Mathieu-Daudé wrote:
>>>>> On 11/2/25 17:19, Konstantin Shkolnyy wrote:
>>>>>> Add .set_vnet_le() function that always returns success, assuming 
>>>>>> that
>>>>>> vDPA h/w always implements LE data format. Otherwise, QEMU 
>>>>>> disables vDPA and
>>>>>> outputs the message:
>>>>>> "backend does not support LE vnet headers; falling back on 
>>>>>> userspace virtio"
>>>>>>
>>>>>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
>>>>>> ---
>>>>>>   net/vhost-vdpa.c | 6 ++++++
>>>>>>   1 file changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>>>> index 231b45246c..7219aa2eee 100644
>>>>>> --- a/net/vhost-vdpa.c
>>>>>> +++ b/net/vhost-vdpa.c
>>>>>> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState 
>>>>>> *nc)
>>>>>>   }
>>>>>> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
>>>>>> +{
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>   static bool vhost_vdpa_check_peer_type(NetClientState *nc, 
>>>>>> ObjectClass *oc,
>>>>>>                                          Error **errp)
>>>>>>   {
>>>>>> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
>>>>>>           .cleanup = vhost_vdpa_cleanup,
>>>>>>           .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>>>>>>           .has_ufo = vhost_vdpa_has_ufo,
>>>>>> +        .set_vnet_le = vhost_vdpa_set_vnet_le,
>>>>>
>>>>> Dubious mismatch with set_vnet_be handler.
>>>>
>>>> I'm not sure what you are suggesting...
>>>
>>> Implement set_vnet_le for parity?
>>
>> To my (very limited) knowledge, kernel's vhost_vdpa that QEMU talks to 
>> doesn't have an API to "change h/w endianness". If so, 
>> vDPA's .set_vnet_le/be(), as well as qemu_set_vnet_le/be() have very 
>> limited choices. qemu_set_vnet_le/be() behavior with vDPA was to 
>> simply assume that h/w endianness by default matches host's. This 
>> assumption is valid for other types of "NetClients" which are 
>> implemented in s/w. However, I suspect, vDPA h/w might all be going to 
>> be LE, to match virtio 1.0. Such is the NIC I'm dealing with.
>>
>> My patch is only fixing a specific use case. Perhaps, for a complete 
>> fix, qemu_set_vnet_be() also shouldn't unconditionally return success 
>> on big endian machines, but always call .set_vnet_be() so that vDPA 
>> could fail it? But then it would start calling .set_vnet_be() on other 
>> "NetClients" where it didn't before.
>>
>> That's why I don't want to just add a .set_vnet_be(), before someone 
>> here even confirms that vDPA h/w is indeed assumed LE, and, hence, 
>> what the right path is to a complete solution...
>>
>> int qemu_set_vnet_be(NetClientState *nc, bool is_be)
>> {
>> #if HOST_BIG_ENDIAN
>>      return 0;
>> #else
>>      if (!nc || !nc->info->set_vnet_be)
>>          return -ENOSYS;
>>
>>      return nc->info->set_vnet_be(nc, is_be);
>> #endif
>> }
>>
> 
> Does anyone have any answers/suggestions?
> 

Since you mentioned "vDPA h/w always implements LE data format",
I'd expect virtio_is_big_endian(vdev) always return FALSE, and
thus this to be safe:

  static int vhost_vdpa_set_vnet_be(NetClientState *nc, bool is_le)
  {
      g_assert_not_reached();
  }

But I don't know much about vDPA, so I won't object to your patch.

Regards,

Phil.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-18 14:02           ` Philippe Mathieu-Daudé
@ 2025-02-19  0:29             ` Jason Wang
  0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2025-02-19  0:29 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: Konstantin Shkolnyy, mst, sgarzare, mjrosato, qemu-devel,
	Akihiko Odaki, Yuri Benditovich, Laurent Vivier, Hanna Czenczek,
	Dragos Tatulea

On Tue, Feb 18, 2025 at 10:03 PM Philippe Mathieu-Daudé
<philmd@linaro.org> wrote:
>
> Hi Konstantin,
>
> (Cc'ing more developers)
>
> On 18/2/25 14:27, Konstantin Shkolnyy wrote:
> > On 2/12/2025 14:01, Konstantin Shkolnyy wrote:
> >> On 2/12/2025 12:07, Philippe Mathieu-Daudé wrote:
> >>> On 12/2/25 18:24, Konstantin Shkolnyy wrote:
> >>>> On 2/12/2025 08:52, Philippe Mathieu-Daudé wrote:
> >>>>> On 11/2/25 17:19, Konstantin Shkolnyy wrote:
> >>>>>> Add .set_vnet_le() function that always returns success, assuming
> >>>>>> that
> >>>>>> vDPA h/w always implements LE data format. Otherwise, QEMU
> >>>>>> disables vDPA and
> >>>>>> outputs the message:
> >>>>>> "backend does not support LE vnet headers; falling back on
> >>>>>> userspace virtio"
> >>>>>>
> >>>>>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> >>>>>> ---
> >>>>>>   net/vhost-vdpa.c | 6 ++++++
> >>>>>>   1 file changed, 6 insertions(+)
> >>>>>>
> >>>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >>>>>> index 231b45246c..7219aa2eee 100644
> >>>>>> --- a/net/vhost-vdpa.c
> >>>>>> +++ b/net/vhost-vdpa.c
> >>>>>> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState
> >>>>>> *nc)
> >>>>>>   }
> >>>>>> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
> >>>>>> +{
> >>>>>> +    return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>>   static bool vhost_vdpa_check_peer_type(NetClientState *nc,
> >>>>>> ObjectClass *oc,
> >>>>>>                                          Error **errp)
> >>>>>>   {
> >>>>>> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
> >>>>>>           .cleanup = vhost_vdpa_cleanup,
> >>>>>>           .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
> >>>>>>           .has_ufo = vhost_vdpa_has_ufo,
> >>>>>> +        .set_vnet_le = vhost_vdpa_set_vnet_le,
> >>>>>
> >>>>> Dubious mismatch with set_vnet_be handler.
> >>>>
> >>>> I'm not sure what you are suggesting...
> >>>
> >>> Implement set_vnet_le for parity?
> >>
> >> To my (very limited) knowledge, kernel's vhost_vdpa that QEMU talks to
> >> doesn't have an API to "change h/w endianness". If so,
> >> vDPA's .set_vnet_le/be(), as well as qemu_set_vnet_le/be() have very
> >> limited choices. qemu_set_vnet_le/be() behavior with vDPA was to
> >> simply assume that h/w endianness by default matches host's. This
> >> assumption is valid for other types of "NetClients" which are
> >> implemented in s/w. However, I suspect, vDPA h/w might all be going to
> >> be LE, to match virtio 1.0. Such is the NIC I'm dealing with.
> >>
> >> My patch is only fixing a specific use case. Perhaps, for a complete
> >> fix, qemu_set_vnet_be() also shouldn't unconditionally return success
> >> on big endian machines, but always call .set_vnet_be() so that vDPA
> >> could fail it? But then it would start calling .set_vnet_be() on other
> >> "NetClients" where it didn't before.
> >>
> >> That's why I don't want to just add a .set_vnet_be(), before someone
> >> here even confirms that vDPA h/w is indeed assumed LE, and, hence,
> >> what the right path is to a complete solution...

Note that modern (VERION_1) virtio/vDPA device assumes LE.
Transitional devices need to support "native endian" which requires
the interface like what you suggest here when guest and host endian
are different.

But we need to confirm with the vDPA vendors to know if their device
is modern only or transitional.

E.g by looking at mlx5_vdpa drivers, it seems to support big endian:

/* TODO: cross-endian support */
static inline bool mlx5_vdpa_is_little_endian(struct mlx5_vdpa_dev *mvdev)
{
        return virtio_legacy_is_little_endian() ||
                (mvdev->actual_features & BIT_ULL(VIRTIO_F_VERSION_1));
}

But it needs to judge the endian according to what userspace tells us
(as the "TODO"), we probably need to invent new vDPA ioctls for that
besides the Qemu modifications.

Adding Dragos for more thought here.

Thanks

> >>
> >> int qemu_set_vnet_be(NetClientState *nc, bool is_be)
> >> {
> >> #if HOST_BIG_ENDIAN
> >>      return 0;
> >> #else
> >>      if (!nc || !nc->info->set_vnet_be)
> >>          return -ENOSYS;
> >>
> >>      return nc->info->set_vnet_be(nc, is_be);
> >> #endif
> >> }
> >>
> >
> > Does anyone have any answers/suggestions?
> >
>
> Since you mentioned "vDPA h/w always implements LE data format",
> I'd expect virtio_is_big_endian(vdev) always return FALSE, and
> thus this to be safe:
>
>   static int vhost_vdpa_set_vnet_be(NetClientState *nc, bool is_le)
>   {
>       g_assert_not_reached();
>   }
>
> But I don't know much about vDPA, so I won't object to your patch.
>
> Regards,
>
> Phil.
>



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-11 16:19 [PATCH] vdpa: Allow vDPA to work on big-endian machine Konstantin Shkolnyy
  2025-02-12 13:38 ` Eugenio Perez Martin
  2025-02-12 14:52 ` Philippe Mathieu-Daudé
@ 2025-02-20 15:57 ` Michael S. Tsirkin
  2025-02-21  3:40   ` Konstantin Shkolnyy
  2 siblings, 1 reply; 16+ messages in thread
From: Michael S. Tsirkin @ 2025-02-20 15:57 UTC (permalink / raw)
  To: Konstantin Shkolnyy; +Cc: eperezma, sgarzare, mjrosato, qemu-devel

On Tue, Feb 11, 2025 at 10:19:23AM -0600, Konstantin Shkolnyy wrote:
> Add .set_vnet_le() function that always returns success, assuming that
> vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and
> outputs the message:
> "backend does not support LE vnet headers; falling back on userspace virtio"
> 
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>

Thanks for the patch! Yet something to improve:


> ---
>  net/vhost-vdpa.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 231b45246c..7219aa2eee 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
>  
>  }
>  
> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
> +{
> +    return 0;

How about checking is_le is true then?
And add a code comment with an explanation, please.


> +}
> +
>  static bool vhost_vdpa_check_peer_type(NetClientState *nc, ObjectClass *oc,
>                                         Error **errp)
>  {
> @@ -437,6 +442,7 @@ static NetClientInfo net_vhost_vdpa_info = {
>          .cleanup = vhost_vdpa_cleanup,
>          .has_vnet_hdr = vhost_vdpa_has_vnet_hdr,
>          .has_ufo = vhost_vdpa_has_ufo,
> +        .set_vnet_le = vhost_vdpa_set_vnet_le,
>          .check_peer_type = vhost_vdpa_check_peer_type,
>          .set_steering_ebpf = vhost_vdpa_set_steering_ebpf,
>  };
> -- 
> 2.34.1



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine
  2025-02-20 15:57 ` Michael S. Tsirkin
@ 2025-02-21  3:40   ` Konstantin Shkolnyy
  0 siblings, 0 replies; 16+ messages in thread
From: Konstantin Shkolnyy @ 2025-02-21  3:40 UTC (permalink / raw)
  To: Michael S. Tsirkin; +Cc: eperezma, sgarzare, mjrosato, qemu-devel

On 2/20/2025 09:57, Michael S. Tsirkin wrote:
> On Tue, Feb 11, 2025 at 10:19:23AM -0600, Konstantin Shkolnyy wrote:
>> Add .set_vnet_le() function that always returns success, assuming that
>> vDPA h/w always implements LE data format. Otherwise, QEMU disables vDPA and
>> outputs the message:
>> "backend does not support LE vnet headers; falling back on userspace virtio"
>>
>> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> 
> Thanks for the patch! Yet something to improve:
> 
> 
>> ---
>>   net/vhost-vdpa.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 231b45246c..7219aa2eee 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -270,6 +270,11 @@ static bool vhost_vdpa_has_ufo(NetClientState *nc)
>>   
>>   }
>>   
>> +static int vhost_vdpa_set_vnet_le(NetClientState *nc, bool is_le)
>> +{
>> +    return 0;
> 
> How about checking is_le is true then?

As I understand, the control comes here from 
virtio_net_set_vnet_endian(...., bool enable). That "enable" is copied 
into the "is_le" here. Thus, it doesn't look like "is_le = false" means 
BE. Rather, it looks like it means "default endianness of the 
NetClient". If so, the function should probably just return 0 in both cases.

I think, that is also why we have .set_vnet_le() and .set_vnet_be() 
instead of just one function.

If the above is correct, maybe I should rename "is_le" into "enable"? (I 
simply copied the name from another function...)

> And add a code comment with an explanation, please.

Sure.



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH] vdpa: Allow VDPA to work on big-endian machine
@ 2025-06-14 22:44 Konstantin Shkolnyy
  2025-06-16  2:49 ` Jason Wang
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Konstantin Shkolnyy @ 2025-06-14 22:44 UTC (permalink / raw)
  To: jasowang, akihiko.odaki; +Cc: qemu-devel, mjrosato, Konstantin Shkolnyy

After commit 0caed25cd171 vhost_vdpa_net_load_vlan() started seeing
VIRTIO_NET_F_CTRL_VLAN flag and making 4096 calls to the kernel with
VIRTIO_NET_CTRL_VLAN_ADD command. However, it forgot to convert the
16-bit VLAN IDs to LE format. On BE machine, the kernel calls failed
when they saw "VLAN IDs" greater than 4095, and QEMU then said:
"unable to start vhost net: 5: falling back on userspace virtio", and
VDPA became disabled.

Convert the VLAN ID to LE before putting it into virtio queue.

Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
---
 net/vhost-vdpa.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 58d738945d..99c9eb42b9 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -1173,9 +1173,10 @@ static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
                                            struct iovec *in_cursor,
                                            uint16_t vid)
 {
+    __le16 vid_le = cpu_to_le16(vid);
     const struct iovec data = {
-        .iov_base = &vid,
-        .iov_len = sizeof(vid),
+        .iov_base = &vid_le,
+        .iov_len = sizeof(vid_le),
     };
     ssize_t r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
                                         VIRTIO_NET_CTRL_VLAN,
-- 
2.34.1



^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow VDPA to work on big-endian machine
  2025-06-14 22:44 [PATCH] vdpa: Allow VDPA " Konstantin Shkolnyy
@ 2025-06-16  2:49 ` Jason Wang
  2025-06-16  6:19 ` Eugenio Perez Martin
  2025-06-16  8:56 ` Akihiko Odaki
  2 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2025-06-16  2:49 UTC (permalink / raw)
  To: Konstantin Shkolnyy; +Cc: akihiko.odaki, qemu-devel, mjrosato

On Sun, Jun 15, 2025 at 6:44 AM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote:
>
> After commit 0caed25cd171 vhost_vdpa_net_load_vlan() started seeing
> VIRTIO_NET_F_CTRL_VLAN flag and making 4096 calls to the kernel with
> VIRTIO_NET_CTRL_VLAN_ADD command. However, it forgot to convert the
> 16-bit VLAN IDs to LE format. On BE machine, the kernel calls failed
> when they saw "VLAN IDs" greater than 4095, and QEMU then said:
> "unable to start vhost net: 5: falling back on userspace virtio", and
> VDPA became disabled.
>
> Convert the VLAN ID to LE before putting it into virtio queue.
>
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow VDPA to work on big-endian machine
  2025-06-14 22:44 [PATCH] vdpa: Allow VDPA " Konstantin Shkolnyy
  2025-06-16  2:49 ` Jason Wang
@ 2025-06-16  6:19 ` Eugenio Perez Martin
  2025-06-16  8:56 ` Akihiko Odaki
  2 siblings, 0 replies; 16+ messages in thread
From: Eugenio Perez Martin @ 2025-06-16  6:19 UTC (permalink / raw)
  To: Konstantin Shkolnyy; +Cc: jasowang, akihiko.odaki, qemu-devel, mjrosato

On Sun, Jun 15, 2025 at 12:46 AM Konstantin Shkolnyy <kshk@linux.ibm.com> wrote:
>
> After commit 0caed25cd171 vhost_vdpa_net_load_vlan() started seeing
> VIRTIO_NET_F_CTRL_VLAN flag and making 4096 calls to the kernel with
> VIRTIO_NET_CTRL_VLAN_ADD command. However, it forgot to convert the
> 16-bit VLAN IDs to LE format. On BE machine, the kernel calls failed
> when they saw "VLAN IDs" greater than 4095, and QEMU then said:
> "unable to start vhost net: 5: falling back on userspace virtio", and
> VDPA became disabled.
>
> Convert the VLAN ID to LE before putting it into virtio queue.
>
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>

Good catch! You should add the Fixes: tag though :). With that,

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> ---
>  net/vhost-vdpa.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 58d738945d..99c9eb42b9 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -1173,9 +1173,10 @@ static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
>                                             struct iovec *in_cursor,
>                                             uint16_t vid)
>  {
> +    __le16 vid_le = cpu_to_le16(vid);
>      const struct iovec data = {
> -        .iov_base = &vid,
> -        .iov_len = sizeof(vid),
> +        .iov_base = &vid_le,
> +        .iov_len = sizeof(vid_le),
>      };
>      ssize_t r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>                                          VIRTIO_NET_CTRL_VLAN,
> --
> 2.34.1
>
>



^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] vdpa: Allow VDPA to work on big-endian machine
  2025-06-14 22:44 [PATCH] vdpa: Allow VDPA " Konstantin Shkolnyy
  2025-06-16  2:49 ` Jason Wang
  2025-06-16  6:19 ` Eugenio Perez Martin
@ 2025-06-16  8:56 ` Akihiko Odaki
  2 siblings, 0 replies; 16+ messages in thread
From: Akihiko Odaki @ 2025-06-16  8:56 UTC (permalink / raw)
  To: Konstantin Shkolnyy, jasowang; +Cc: qemu-devel, mjrosato

On 2025/06/15 7:44, Konstantin Shkolnyy wrote:
> After commit 0caed25cd171 vhost_vdpa_net_load_vlan() started seeing
> VIRTIO_NET_F_CTRL_VLAN flag and making 4096 calls to the kernel with
> VIRTIO_NET_CTRL_VLAN_ADD command. However, it forgot to convert the
> 16-bit VLAN IDs to LE format. On BE machine, the kernel calls failed
> when they saw "VLAN IDs" greater than 4095, and QEMU then said:
> "unable to start vhost net: 5: falling back on userspace virtio", and
> VDPA became disabled.

Please add the Fixes: tag to refer the commit; see:
docs/devel/submitting-a-patch.rst

> 
> Convert the VLAN ID to LE before putting it into virtio queue.
> 
> Signed-off-by: Konstantin Shkolnyy <kshk@linux.ibm.com>
> ---
>   net/vhost-vdpa.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 58d738945d..99c9eb42b9 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -1173,9 +1173,10 @@ static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
>                                              struct iovec *in_cursor,
>                                              uint16_t vid)
>   {
> +    __le16 vid_le = cpu_to_le16(vid);

docs/devel/style.rst says:
> Don't use Linux kernel internal types like u32, __u32 or __le32.

It's unfortunate that QEMU lacks endian types and a checker for them; 
such a checker could not have caught this particular case, but can catch 
other similar bugs.

Regards,
Akihiko Odaki

>       const struct iovec data = {
> -        .iov_base = &vid,
> -        .iov_len = sizeof(vid),
> +        .iov_base = &vid_le,
> +        .iov_len = sizeof(vid_le),
>       };
>       ssize_t r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>                                           VIRTIO_NET_CTRL_VLAN,



^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2025-06-16  8:57 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-02-11 16:19 [PATCH] vdpa: Allow vDPA to work on big-endian machine Konstantin Shkolnyy
2025-02-12 13:38 ` Eugenio Perez Martin
2025-02-12 14:47   ` Konstantin Shkolnyy
2025-02-12 14:52 ` Philippe Mathieu-Daudé
2025-02-12 17:24   ` Konstantin Shkolnyy
2025-02-12 18:07     ` Philippe Mathieu-Daudé
2025-02-12 20:01       ` Konstantin Shkolnyy
2025-02-18 13:27         ` Konstantin Shkolnyy
2025-02-18 14:02           ` Philippe Mathieu-Daudé
2025-02-19  0:29             ` Jason Wang
2025-02-20 15:57 ` Michael S. Tsirkin
2025-02-21  3:40   ` Konstantin Shkolnyy
  -- strict thread matches above, loose matches on Subject: below --
2025-06-14 22:44 [PATCH] vdpa: Allow VDPA " Konstantin Shkolnyy
2025-06-16  2:49 ` Jason Wang
2025-06-16  6:19 ` Eugenio Perez Martin
2025-06-16  8:56 ` Akihiko Odaki

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).