On Thu, 9 Nov 2017 15:55:46 -0300 Philippe Mathieu-Daudé <f4...@amsat.org> wrote:
> On 11/09/2017 01:38 PM, Cornelia Huck wrote: > > On Tue, 7 Nov 2017 18:24:33 +0100 > > Pierre Morel <pmo...@linux.vnet.ibm.com> wrote: > > > >> There are two places where the same endianness conversion > >> is done. > >> Let's factor this out into a static function. > >> > >> Signed-off-by: Pierre Morel <pmo...@linux.vnet.ibm.com> > >> Reviewed-by: Yi Min Zhao <zyi...@linux.vnet.ibm.com> > >> --- > >> hw/s390x/s390-pci-inst.c | 58 > >> ++++++++++++++++++++++++++---------------------- > >> 1 file changed, 32 insertions(+), 26 deletions(-) > >> > >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > >> index 8e088f3..8fcb02d 100644 > >> --- a/hw/s390x/s390-pci-inst.c > >> +++ b/hw/s390x/s390-pci-inst.c > >> @@ -314,6 +314,35 @@ out: > >> return 0; > >> } > >> > >> +/** > >> + * This function swaps the data at ptr according from one > >> + * endianness to the other. > >> + * valid data in the uint64_t data field. > > > > I'm not sure what that line is supposed to mean? > > > >> + * @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; > >> +} > > This is usually care taken by memory::adjust_endianness() ... Yes, but that's not a memory region write. > > > I was expecting more code to use a similar pattern, but it seems > > surprisingly uncommon. > > Which ring a bell for latent bug? Looking at this, it seems there *is* a latent bug, which has not popped up so far as the pci instructions are not wired up in tcg yet. This code is only called from the kvm path... > > This remind me of a similar issue on ppc: > > http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html > ... > http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html