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

[Yi Min Zhao]

在 2017/11/23 下午6:33, Cornelia Huck 写道:

On Thu, 23 Nov 2017 11:25:10 +0100
Thomas Huth  wrote:


On 23.11.2017 11:08, Cornelia Huck wrote:

On Thu, 23 Nov 2017 11:01:23 +0100
Thomas Huth  wrote:
   

On 23.11.2017 10:49, Cornelia Huck wrote:

On Thu, 23 Nov 2017 09:48:41 +0100
Thomas Huth  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?

I have a question, is the data in cpu->regs the guest's endianess?
In our case, the guest is S390. Although the arch is big-endian, the data in
pcilg/stg instructions is little-endian.

Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the 
host endianess?


If the answers to upper two questions are yes, we actually need handle 
two cases.
1) For pcilg, we need to translate the data to little-endian, thus 
cpu_to_le**().
2) For pcistg, we need to translate the data to host endianess, thus 
le**_to_cpu().



[I really need to continue working on wiring up zpci in tcg, but I keep
getting sidetracked.]

Maybe best if you get it running on a big endian host first ... if it is
then not working on a little endian host, you know that you have to look
for things like these "bswapXX()" statements...

That was exactly my reasoning behind getting tcg to run... but getting
it to run at all is the hard part :)

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

[Cornelia Huck]

On Thu, 23 Nov 2017 11:25:10 +0100
Thomas Huth  wrote:

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

> 
> > [I really need to continue working on wiring up zpci in tcg, but I keep
> > getting sidetracked.]  
> 
> Maybe best if you get it running on a big endian host first ... if it is
> then not working on a little endian host, you know that you have to look
> for things like these "bswapXX()" statements...

That was exactly my reasoning behind getting tcg to run... but getting
it to run at all is the hard part :)

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

[Thomas Huth]

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

> [I really need to continue working on wiring up zpci in tcg, but I keep
> getting sidetracked.]

Maybe best if you get it running on a big endian host first ... if it is
then not working on a little endian host, you know that you have to look
for things like these "bswapXX()" statements...

 Thomas

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

[Cornelia Huck]

On Thu, 23 Nov 2017 11:01:23 +0100
Thomas Huth  wrote:

> On 23.11.2017 10:49, Cornelia Huck wrote:
> > On Thu, 23 Nov 2017 09:48:41 +0100
> > Thomas Huth  wrote:
> >   
> >> On 22.11.2017 23:05, Pierre Morel 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 
> >>> Reviewed-by: Yi Min Zhao 
> >>> ---
> >>>  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..3e1f1a0 100644
> >>> --- a/hw/s390x/s390-pci-inst.c
> >>> +++ b/hw/s390x/s390-pci-inst.c
> >>> @@ -314,6 +314,36 @@ out:
> >>>  return 0;
> >>>  }
> >>>  
> >>> +/**
> >>> + * 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.

[I really need to continue working on wiring up zpci in tcg, but I keep
getting sidetracked.]

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

[Thomas Huth]

On 23.11.2017 10:49, Cornelia Huck wrote:
> On Thu, 23 Nov 2017 09:48:41 +0100
> Thomas Huth  wrote:
> 
>> On 22.11.2017 23:05, Pierre Morel 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 
>>> Reviewed-by: Yi Min Zhao 
>>> ---
>>>  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..3e1f1a0 100644
>>> --- a/hw/s390x/s390-pci-inst.c
>>> +++ b/hw/s390x/s390-pci-inst.c
>>> @@ -314,6 +314,36 @@ out:
>>>  return 0;
>>>  }
>>>  
>>> +/**
>>> + * 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?

 Thomas

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

[Cornelia Huck]

On Thu, 23 Nov 2017 09:48:41 +0100
Thomas Huth  wrote:

> On 22.11.2017 23:05, Pierre Morel 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 
> > Reviewed-by: Yi Min Zhao 
> > ---
> >  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..3e1f1a0 100644
> > --- a/hw/s390x/s390-pci-inst.c
> > +++ b/hw/s390x/s390-pci-inst.c
> > @@ -314,6 +314,36 @@ out:
> >  return 0;
> >  }
> >  
> > +/**
> > + * 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.

> though I'm not 100% sure, and we still can also fix
> that later, so:
> 
> Reviewed-by: Thomas Huth 

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

[Thomas Huth]

On 22.11.2017 23:05, Pierre Morel 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 
> Reviewed-by: Yi Min Zhao 
> ---
>  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..3e1f1a0 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -314,6 +314,36 @@ out:
>  return 0;
>  }
>  
> +/**
> + * 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, though I'm not 100% sure, and we still can also fix
that later, so:

Reviewed-by: Thomas Huth 

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

[Pierre Morel]

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 
Reviewed-by: Yi Min Zhao 
---
 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..3e1f1a0 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -314,6 +314,36 @@ out:
 return 0;
 }
 
+/**
+ * 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;
+}
+
 int pcilg_service_call(S390CPU *cpu, uint8_t r1, uint8_t r2)
 {
 CPUS390XState *env = >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(, 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(, len)) {
 program_interrupt(env, PGM_OPERAND, 4);
 return 0;
 }
-- 
2.7.4