Re: [PATCH] vdpa: Allow vDPA to work on big-endian machine

[Konstantin Shkolnyy <kshk@linux.ibm.com>]

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.