| Message ID | 1510854715-7081-2-git-send-email-pmorel@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 16 Nov 2017 18:51:49 +0100 Pierre Morel <pmorel@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 <pmorel@linux.vnet.ibm.com> > Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> > Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> Your two s-o-bs look a bit odd... > --- > hw/s390x/s390-pci-inst.c | 59 +++++++++++++++++++++++++++--------------------- > 1 file changed, 33 insertions(+), 26 deletions(-) > > diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c > index 8e088f3..ded1556 100644 > --- a/hw/s390x/s390-pci-inst.c > +++ b/hw/s390x/s390-pci-inst.c > @@ -314,6 +314,36 @@ out: > return 0; > } > > +/** > + * This function is called when endianess is fixed, whatever the host endianess > + * is, like in our case from s390x BIG endian registers to little endian PCI > + * Bars, to translate the uint64_t pointed from one endianess to the other. I would rephrase this a bit: "Swap data contained in s390x big endian registers to little endian PCI bars." That makes it clear that this function is for a specialized use case. > + * > + * @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; Please add an extra empty line. > + 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; > +} > + > int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) > { > CPUS390XState *env = &cpu->env; Other than the nits above, looks good.
On 21/11/2017 11:35, Cornelia Huck wrote: > On Thu, 16 Nov 2017 18:51:49 +0100 > Pierre Morel <pmorel@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 <pmorel@linux.vnet.ibm.com> >> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com> > > Your two s-o-bs look a bit odd... > >> --- >> hw/s390x/s390-pci-inst.c | 59 +++++++++++++++++++++++++++--------------------- >> 1 file changed, 33 insertions(+), 26 deletions(-) >> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c >> index 8e088f3..ded1556 100644 >> --- a/hw/s390x/s390-pci-inst.c >> +++ b/hw/s390x/s390-pci-inst.c >> @@ -314,6 +314,36 @@ out: >> return 0; >> } >> >> +/** >> + * This function is called when endianess is fixed, whatever the host endianess >> + * is, like in our case from s390x BIG endian registers to little endian PCI >> + * Bars, to translate the uint64_t pointed from one endianess to the other. > > I would rephrase this a bit: > > "Swap data contained in s390x big endian registers to little endian PCI > bars." Simpler and clearer. thanks. > > That makes it clear that this function is for a specialized use case. > >> + * >> + * @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; > > Please add an extra empty line. yes. > >> + 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; >> +} >> + >> int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) >> { >> CPUS390XState *env = &cpu->env; > > Other than the nits above, looks good. > Thanks for the review Pierre
diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c index 8e088f3..ded1556 100644 --- a/hw/s390x/s390-pci-inst.c +++ b/hw/s390x/s390-pci-inst.c @@ -314,6 +314,36 @@ out: return 0; } +/** + * This function is called when endianess is fixed, whatever the host endianess + * is, like in our case from s390x BIG endian registers to little endian PCI + * Bars, to translate the uint64_t pointed from one endianess to the other. + * + * @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; +} + int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) { CPUS390XState *env = &cpu->env; @@ -385,19 +415,7 @@ int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) data = pci_host_config_read_common( pbdev->pdev, offset, pci_config_size(pbdev->pdev), len); - switch (len) { - case 1: - break; - case 2: - data = bswap16(data); - break; - case 4: - data = bswap32(data); - break; - case 8: - data = bswap64(data); - break; - default: + if (zpci_endian_swap(&data, len)) { program_interrupt(env, PGM_OPERAND, 4); return 0; } @@ -500,19 +518,8 @@ int pcistg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2) program_interrupt(env, PGM_OPERAND, 4); return 0; } - switch (len) { - case 1: - break; - case 2: - data = bswap16(data); - break; - case 4: - data = bswap32(data); - break; - case 8: - data = bswap64(data); - break; - default: + + if (zpci_endian_swap(&data, len)) { program_interrupt(env, PGM_OPERAND, 4); return 0; }