Re: [Qemu-devel] [qemu-s390x] [PATCH v3 1/7] s390x/pci: factor out endianess conversion

[Michael S. Tsirkin]

On Thu, Nov 23, 2017 at 12:35:19PM +0100, Thomas Huth wrote:
> On 23.11.2017 11:33, Cornelia Huck wrote:
> > On Thu, 23 Nov 2017 11:25:10 +0100
> > Thomas Huth <address@hidden> wrote:
> > 
> >> On 23.11.2017 11:08, Cornelia Huck wrote:
> >>> On Thu, 23 Nov 2017 11:01:23 +0100
> >>> Thomas Huth <address@hidden> wrote:
> >>>   
> >>>> On 23.11.2017 10:49, Cornelia Huck wrote:  
> >>>>> On Thu, 23 Nov 2017 09:48:41 +0100
> >>>>> Thomas Huth <address@hidden> wrote:  
> >>>>>> On 22.11.2017 23:05, Pierre Morel wrote:    
> >> [...]
> >>>>>>> +/**
> >>>>>>> + * Swap data contained in s390x big endian registers to little endian
> >>>>>>> + * PCI bars.
> >>>>>>> + *
> >>>>>>> + * @ptr: a pointer to a uint64_t data field
> >>>>>>> + * @len: the length of the valid data, must be 1,2,4 or 8
> >>>>>>> + */
> >>>>>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
> >>>>>>> +{
> >>>>>>> +    uint64_t data = *ptr;
> >>>>>>> +
> >>>>>>> +    switch (len) {
> >>>>>>> +    case 1:
> >>>>>>> +        break;
> >>>>>>> +    case 2:
> >>>>>>> +        data = bswap16(data);
> >>>>>>> +        break;
> >>>>>>> +    case 4:
> >>>>>>> +        data = bswap32(data);
> >>>>>>> +        break;
> >>>>>>> +    case 8:
> >>>>>>> +        data = bswap64(data);
> >>>>>>> +        break;
> >>>>>>> +    default:
> >>>>>>> +        return -EINVAL;
> >>>>>>> +    }
> >>>>>>> +    *ptr = data;
> >>>>>>> +    return 0;
> >>>>>>> +}      
> >>>>>>
> >>>>>> While you're at it, I think that should rather be leXX_to_cpu() instead
> >>>>>> of bswapXX() here,    
> >>>>>
> >>>>> I don't think that's correct, as this is supposed to swap BE registers
> >>>>> to LE PCI bars.    
> >>>>
> >>>> Yes, but for the CPU emulation, the registers are stored in the host's
> >>>> endianness in the CPUS390XState structure. Or why do we byte-swap them
> >>>> again with cpu_to_be64() during s390_store_status(), for example?  
> >>>
> >>> Gah, endian conversion is eating my brain...
> >>>
> >>> So, is the content we get BE or not? I thought in our last discussion
> >>> we came to the conclusion that it is.  
> >>
> >> data is read from / written to env->regs[r1], so this is host endian, as
> >> far as I know. PCI is little endian, so using le32_to_cpu() /
> >> cpu_to_le32() should IMHO be the right way to go here.
> >>
> >> By the way, if we want to use both, cpu_to_le and le_to_cpu, depending
> >> on whether we read from or write to PCI, we should maybe *not* put this
> >> code into a separate function?
> > 
> > Yes, if your assessment is correct, we need two functions (I think this
> > conversion is used in other places in later patches as well). Or are
> > there mechanisms for that already available?
> 
> Well, leXX_to_cpu() is the very same as cpu_to_leXX(), so we could cheat
> here and still use one wrapper function. Depends on whether we want to
> be more explicit about the data flow or not (and maybe whether we want
> to use sparse one day to statically check the endianness automatically).
> 
>  Thomas

I personally think it's not just sparse. Using leXX_to and to_leXX
allows reasoning about data endian-ness. So do annotations even
when not used by sparse.

-- 
MST