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 <th...@redhat.com> wrote: > > > >> On 23.11.2017 11:08, Cornelia Huck wrote: > >>> On Thu, 23 Nov 2017 11:01:23 +0100 > >>> Thomas Huth <th...@redhat.com> wrote: > >>> > >>>> On 23.11.2017 10:49, Cornelia Huck wrote: > >>>>> On Thu, 23 Nov 2017 09:48:41 +0100 > >>>>> Thomas Huth <th...@redhat.com> 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