Re: [qemu-s390x] [PATCH v2 for-2.11] hw/net/vmxnet3: Fix code to work on big endian hosts, too

[Thomas Huth]

On 15.11.2017 00:33, David Gibson wrote:
> On Tue, 14 Nov 2017 12:20:24 +0100
> Thomas Huth <address@hidden> wrote:
> 
>> Since commit ab06ec43577177a442e8 we test the vmxnet3 device in the
>> pxe-tester, too (when running "make check SPEED=slow"). This now
>> revealed that the code is not working there if the host is a big
>> endian machine (for example ppc64 or s390x) - "make check SPEED=slow"
>> is now failing on such hosts.
>>
>> The vmxnet3 code lacks endianess conversions in a couple of places.
>> Interestingly, the bitfields in the structs in vmxnet3.h already tried to
>> take care of the *bit* endianess of the C compilers - but the code missed
>> to change the *byte* endianess when reading or writing the corresponding
>> structs. So the bitfields are now wrapped into unions which allow to change
>> the byte endianess during runtime with the non-bitfield member of the union.
>> With these changes, "make check SPEED=slow" now properly works on big endian
>> hosts, too.
>>
>> Reported-by: David Gibson <address@hidden>
>> Signed-off-by: Thomas Huth <address@hidden>
>> ---
>>  v2:
>>  - Introduced vmxnet3_ring_read_curr_txdesc() & vmxnet3_pci_dma_write_rxcd()
>>    helper functions to wrap the byte-swapping code that is required in
>>    multiple places (as suggested by Philippe)
[...]
>> +static inline void
>> +vmxnet3_ring_read_curr_txdesc(PCIDevice *pcidev, Vmxnet3Ring *ring,
>> +                              struct Vmxnet3_TxDesc *txd)
>> +{
>> +    vmxnet3_ring_read_curr_cell(pcidev, ring, txd);
>> +    txd->addr = le64_to_cpu(txd->addr);
>> +    txd->val1 = le32_to_cpu(txd->val1);
>> +    txd->val2 = le32_to_cpu(txd->val2);
> 
> Urgh.. and I dislike swabbing struct fields in place even more :(.
> Having to know the endianness of a structure field is bad enough,
> having to know what endianness it's in *right now* is much worse.
> 
> But possibly you're stuck with it, given the existing code?
> 
> Looks like this is a structure determined by the hardware, in which
> case I think it should be left in hardware endian, and only swabbed
> when you go to actually look at the fields within.
[...]
>> diff --git a/hw/net/vmxnet3.h b/hw/net/vmxnet3.h
>> index f9352c4..5b3b76b 100644
>> --- a/hw/net/vmxnet3.h
>> +++ b/hw/net/vmxnet3.h
>> @@ -226,39 +226,49 @@ enum {
>>  struct Vmxnet3_TxDesc {
>>      __le64 addr;
>>  
>> +    union {
>> +        struct {
>>  #ifdef __BIG_ENDIAN_BITFIELD
>> -    u32 msscof:14;  /* MSS, checksum offset, flags */
>> -    u32 ext1:1;
>> -    u32 dtype:1;    /* descriptor type */
>> -    u32 rsvd:1;
>> -    u32 gen:1;      /* generation bit */
>> -    u32 len:14;
>> +            u32 msscof:14;  /* MSS, checksum offset, flags */
>> +            u32 ext1:1;
>> +            u32 dtype:1;    /* descriptor type */
>> +            u32 rsvd:1;
>> +            u32 gen:1;      /* generation bit */
>> +            u32 len:14;
> 
> Blech.  This is one reason I think bitfields are a terrible idea for
> mirroring hardware (or on disk) data.  Still, I guess changing that's
> kind of out of scope for a quick fix.

David, I agree with all of your comments - I had similar thoughts when I
was working with the code. However, as you already indicated, to address
all of this, we would need to rewrite most parts of this device. That's
out of my scope here - I wanted to keep the changes as minimal as
possible, so that there is a chance that we can get the endianness
problem still fixed for 2.11. So do you think that the patch is OK for
this? ... otherwise, I think I'll rather send a patch that removes the
vmxnet3 from the pxe-tester again - then it won't be tested anymore and
we won't get anymore endianness test failures.

 Thomas

signature.asc
Description: OpenPGP digital signature