[v3,1/7] s390x/pci: factor out endianess conversion

Commit Message

Pierre Morel Nov. 22, 2017, 10:05 p.m. UTC

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 <[e-mail]>
Reviewed-by: Yi Min Zhao <[e-mail]>
---
 hw/s390x/s390-pci-inst.c | 59 +++++++++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 26 deletions(-)

Comments

Thomas Huth Nov. 23, 2017, 8:48 a.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Yi Min Zhao <[e-mail]>
> ---
>  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 <[e-mail]>

Cornelia Huck Nov. 23, 2017, 9:49 a.m. UTC | #2

On Thu, 23 Nov 2017 09:48:41 +0100
Thomas Huth <[e-mail]> 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 <[e-mail]>
> > Reviewed-by: Yi Min Zhao <[e-mail]>
> > ---
> >  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 <[e-mail]>

Thomas Huth Nov. 23, 2017, 10:01 a.m. UTC | #3

On 23.11.2017 10:49, Cornelia Huck wrote:
> On Thu, 23 Nov 2017 09:48:41 +0100
> Thomas Huth <[e-mail]> 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 <[e-mail]>
>>> Reviewed-by: Yi Min Zhao <[e-mail]>
>>> ---
>>>  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

Cornelia Huck Nov. 23, 2017, 10:08 a.m. UTC | #4

On Thu, 23 Nov 2017 11:01:23 +0100
Thomas Huth <[e-mail]> wrote:

> On 23.11.2017 10:49, Cornelia Huck wrote:
> > On Thu, 23 Nov 2017 09:48:41 +0100
> > Thomas Huth <[e-mail]> 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 <[e-mail]>
> >>> Reviewed-by: Yi Min Zhao <[e-mail]>
> >>> ---
> >>>  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.]

Thomas Huth Nov. 23, 2017, 10:25 a.m. UTC | #5

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

Cornelia Huck Nov. 23, 2017, 10:33 a.m. UTC | #6

On Thu, 23 Nov 2017 11:25:10 +0100
Thomas Huth <[e-mail]> wrote:

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

Thomas Huth Nov. 23, 2017, 11:35 a.m. UTC | #7

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

Yi Min Zhao Nov. 23, 2017, 12:07 p.m. UTC | #8

在 2017/11/23 下午6:33, Cornelia Huck 写道:
> On Thu, 23 Nov 2017 11:25:10 +0100
> Thomas Huth <[e-mail]> wrote:
>
>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>> Thomas Huth <[e-mail]> wrote:
>>>    
>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>> Thomas Huth <[e-mail]> 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 :)
>
>

Thomas Huth Nov. 23, 2017, 12:18 p.m. UTC | #9

On 23.11.2017 13:07, Yi Min Zhao wrote:
> 
> 
> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>> On Thu, 23 Nov 2017 11:25:10 +0100
>> Thomas Huth <[e-mail]> wrote:
>>
>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>> Thomas Huth <[e-mail]> wrote:
>>>>   
>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>> Thomas Huth <[e-mail]> 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?

As far as I know, it's host endianness, so on x86 with TCG emulation,
it's little endian.

> In our case, the guest is S390. Although the arch is big-endian, the
> data in
> pcilg/stg instructions is little-endian.

PCI memory is always little endian, right.

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

Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
confusing :-/

> 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 think we've got to byte-swap if the host is big endian (s390x), but
not if the host is little endian (x86 with TCG).

 Thomas

Yi Min Zhao Nov. 24, 2017, 6:19 a.m. UTC | #10

在 2017/11/23 下午8:18, Thomas Huth 写道:
> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>
>> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>>> On Thu, 23 Nov 2017 11:25:10 +0100
>>> Thomas Huth <[e-mail]> wrote:
>>>
>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>    
>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>> Thomas Huth <[e-mail]> 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?
> As far as I know, it's host endianness, so on x86 with TCG emulation,
> it's little endian.
>
>> In our case, the guest is S390. Although the arch is big-endian, the
>> data in
>> pcilg/stg instructions is little-endian.
> PCI memory is always little endian, right.
>
>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>> host endianess?
> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
> confusing :-/
>
>> 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 think we've got to byte-swap if the host is big endian (s390x), but
> not if the host is little endian (x86 with TCG).


Thanks for your replies! We will send the new version ASAP to udpate 
this patch.
>
>   Thomas
>
>

Pierre Morel Nov. 25, 2017, 1:49 p.m. UTC | #11

On 24/11/2017 07:19, Yi Min Zhao wrote:
> 
> 
> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>
>>> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>>>> On Thu, 23 Nov 2017 11:25:10 +0100
>>>> Thomas Huth <[e-mail]> wrote:
>>>>
>>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>>> Thomas Huth <[e-mail]> 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?
>> As far as I know, it's host endianness, so on x86 with TCG emulation,
>> it's little endian.
>>
>>> In our case, the guest is S390. Although the arch is big-endian, the
>>> data in
>>> pcilg/stg instructions is little-endian.
>> PCI memory is always little endian, right.
>>
>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>>> host endianess?
>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>> confusing :-/
>>
>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>> not if the host is little endian (x86 with TCG).



Here is my comprehension of this funny swapping:

- TCG for a BE guest and a le host swap bytes because if we do (register 
& 0x01) in the zPCI interception code it must work what ever the 
endianess is.

- Guest always write data Little Endian because it think it writes to PCI.

- Kernel standard PCI code needs to swap endianness for a BE host but 
not for a le host.


So it follows:

Z Guest writes data BE in its register and swap its data to le before 
issuing zPCI

QEMU intercepts, receives the data from the register and store it
	-> Native: it stores as is: -> le
	-> TCG: it stores swapping data -> BE

QEMU-PCI swaps the bytes always
	-> Native : data is now BE
	-> TCG: data is now le

QEMU send the data to the PCI card
	-> Native, it goes through kernel which swap BE ->le
	-> TCG: data is directly written to PCI memory: -> le

So for my view, we must always swap data. if we want it le at the end

It comes because
1) guest and host kernels both make and translation BE->le
    the QEMU PCI needs to swap back the data before sending to the host

2) TCG swap the bytes, i.e. le->BE on saving registers
    the QEMU PCI writes directly to the memory then it needs to swap to
    back to le

I may have miss something or misunderstood something else so : Is it right?

Regards,

Pierre


> 
> 
> Thanks for your replies! We will send the new version ASAP to udpate 
> this patch.
>>
>>   Thomas
>>
>>
>

Yi Min Zhao Nov. 27, 2017, 6:31 a.m. UTC | #12

在 2017/11/25 下午9:49, Pierre Morel 写道:
> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>
>>
>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>
>>>> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>>>>> On Thu, 23 Nov 2017 11:25:10 +0100
>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>
>>>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>>>> Thomas Huth <[e-mail]> 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?
>>> As far as I know, it's host endianness, so on x86 with TCG emulation,
>>> it's little endian.
>>>
>>>> In our case, the guest is S390. Although the arch is big-endian, the
>>>> data in
>>>> pcilg/stg instructions is little-endian.
>>> PCI memory is always little endian, right.
>>>
>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean 
>>>> the
>>>> host endianess?
>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>> confusing :-/
>>>
>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>> not if the host is little endian (x86 with TCG).
>
>
>
> Here is my comprehension of this funny swapping:
>
> - TCG for a BE guest and a le host swap bytes because if we do 
> (register & 0x01) in the zPCI interception code it must work what ever 
> the endianess is.
>
> - Guest always write data Little Endian because it think it writes to 
> PCI.
>
> - Kernel standard PCI code needs to swap endianness for a BE host but 
> not for a le host.
>
>
> So it follows:
>
> Z Guest writes data BE in its register and swap its data to le before 
> issuing zPCI
The data in register has been already le. For any zPCI instruction accessing
PCI data, the endianess is little-endian. Although s390 is be, its PCI 
instructions
follow PCI Spec (byte ordering is le).

In kernel, s390 pci code swaps the data to le before it really issues 
pcistg.
>
> QEMU intercepts, receives the data from the register and store it
>     -> Native: it stores as is: -> le
I think you talked about PCI stg (storing data to PCI device).
The data from the register is le. But we swapped it back to be
because qemu in s390 is be. Then any pci_config write would
transfer data from be to le finally. The process is:
1) data from register : le (because the data in pcistg is in le)
2) pcistg intercept handler in qemu : le->be
3) pci->config_write : be->le
>     -> TCG: it stores swapping data -> BE
For this case, we only talk about the case that the host is le.
As my understanding, the data in the register should be in
the byte ordering which the guest is.

So, for s390 guest, the data in pcistg is le. Then pcistg intercept
handler swaps the data from le to be, thus the final callback
would write the data with the wrong byte ordering to PCI device
because the host is le and cpu_to_le32() would not swaps the data.
>
> QEMU-PCI swaps the bytes always
>     -> Native : data is now BE
>     -> TCG: data is now le
Why is the data le under TCG? Isn't the data stored in register
the same as the guest's endianess?
>
> QEMU send the data to the PCI card
>     -> Native, it goes through kernel which swap BE ->le
>     -> TCG: data is directly written to PCI memory: -> le
>
> So for my view, we must always swap data. if we want it le at the end
>
> It comes because
> 1) guest and host kernels both make and translation BE->le
>    the QEMU PCI needs to swap back the data before sending to the host
>
> 2) TCG swap the bytes, i.e. le->BE on saving registers
>    the QEMU PCI writes directly to the memory then it needs to swap to
>    back to le
>
> I may have miss something or misunderstood something else so : Is it 
> right?
>
> Regards,
>
> Pierre
>
>
>>
>>
>> Thanks for your replies! We will send the new version ASAP to udpate 
>> this patch.
>>>
>>>   Thomas
>>>
>>>
>>
>
>

Thomas Huth Nov. 27, 2017, 6:59 a.m. UTC | #13

On 25.11.2017 14:49, Pierre Morel wrote:
> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>
>>
>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>
>>>> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>>>>> On Thu, 23 Nov 2017 11:25:10 +0100
>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>
>>>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>>>> Thomas Huth <[e-mail]> 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?
>>> As far as I know, it's host endianness, so on x86 with TCG emulation,
>>> it's little endian.
>>>
>>>> In our case, the guest is S390. Although the arch is big-endian, the
>>>> data in
>>>> pcilg/stg instructions is little-endian.
>>> PCI memory is always little endian, right.
>>>
>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>>>> host endianess?
>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>> confusing :-/
>>>
>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>> not if the host is little endian (x86 with TCG).
> 
> Here is my comprehension of this funny swapping:
> 
> - TCG for a BE guest and a le host swap bytes because if we do (register
> & 0x01) in the zPCI interception code it must work what ever the
> endianess is.

Uhhh, I might have missed that the value has already been byte-swapped
once by TCG for env->regs[r1] ...
Now I'm pretty much completely confused ... sorry for the noise if I was
wrong... I think it's best you ignore my comment for now (i.e. go with
bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
TCG, we still can fix this if necessary.

 Thomas

Pierre Morel Nov. 27, 2017, 8:22 a.m. UTC | #14

On 27/11/2017 07:59, Thomas Huth wrote:
> On 25.11.2017 14:49, Pierre Morel wrote:
>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>
>>>
>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>>
>>>>> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>>>>>> On Thu, 23 Nov 2017 11:25:10 +0100
>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>
>>>>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>>>>> Thomas Huth <[e-mail]> 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?
>>>> As far as I know, it's host endianness, so on x86 with TCG emulation,
>>>> it's little endian.
>>>>
>>>>> In our case, the guest is S390. Although the arch is big-endian, the
>>>>> data in
>>>>> pcilg/stg instructions is little-endian.
>>>> PCI memory is always little endian, right.
>>>>
>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>>>>> host endianess?
>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>>> confusing :-/
>>>>
>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>>> not if the host is little endian (x86 with TCG).
>>
>> Here is my comprehension of this funny swapping:
>>
>> - TCG for a BE guest and a le host swap bytes because if we do (register
>> & 0x01) in the zPCI interception code it must work what ever the
>> endianess is.
> 
> Uhhh, I might have missed that the value has already been byte-swapped
> once by TCG for env->regs[r1] ...
> Now I'm pretty much completely confused ... sorry for the noise if I was
> wrong... I think it's best you ignore my comment for now (i.e. go with
> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
> TCG, we still can fix this if necessary.
> 
>   Thomas
> 

OK, thanks.

Pierre

Yi Min Zhao Nov. 27, 2017, 10:09 a.m. UTC | #15

在 2017/11/27 下午2:59, Thomas Huth 写道:
> On 25.11.2017 14:49, Pierre Morel wrote:
>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>
>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>>>>>> On Thu, 23 Nov 2017 11:25:10 +0100
>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>
>>>>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>>>>> Thomas Huth <[e-mail]> 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?
>>>> As far as I know, it's host endianness, so on x86 with TCG emulation,
>>>> it's little endian.
>>>>
>>>>> In our case, the guest is S390. Although the arch is big-endian, the
>>>>> data in
>>>>> pcilg/stg instructions is little-endian.
>>>> PCI memory is always little endian, right.
>>>>
>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>>>>> host endianess?
>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>>> confusing :-/
>>>>
>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>>> not if the host is little endian (x86 with TCG).
>> Here is my comprehension of this funny swapping:
>>
>> - TCG for a BE guest and a le host swap bytes because if we do (register
>> & 0x01) in the zPCI interception code it must work what ever the
>> endianess is.
> Uhhh, I might have missed that the value has already been byte-swapped
> once by TCG for env->regs[r1] ...
I want to ask a question. For this case, BE guest and LE host, is 
env->regs[r1] in LE byte ordering?
> Now I'm pretty much completely confused ... sorry for the noise if I was
> wrong... I think it's best you ignore my comment for now (i.e. go with
> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
> TCG, we still can fix this if necessary.
>
>   Thomas
>
>

Cornelia Huck Nov. 27, 2017, 11:02 a.m. UTC | #16

On Mon, 27 Nov 2017 07:59:36 +0100
Thomas Huth <[e-mail]> wrote:

> On 25.11.2017 14:49, Pierre Morel wrote:
> > On 24/11/2017 07:19, Yi Min Zhao wrote:  
> >>
> >>
> >> 在 2017/11/23 下午8:18, Thomas Huth 写道:  
> >>> On 23.11.2017 13:07, Yi Min Zhao wrote:  

> >>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
> >>>> host endianess?  
> >>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
> >>> confusing :-/
> >>>  
> >>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
> >>> not if the host is little endian (x86 with TCG).  
> > 
> > Here is my comprehension of this funny swapping:
> > 
> > - TCG for a BE guest and a le host swap bytes because if we do (register
> > & 0x01) in the zPCI interception code it must work what ever the
> > endianess is.  
> 
> Uhhh, I might have missed that the value has already been byte-swapped
> once by TCG for env->regs[r1] ...
> Now I'm pretty much completely confused ... sorry for the noise if I was
> wrong... I think it's best you ignore my comment for now (i.e. go with
> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
> TCG, we still can fix this if necessary.

I'll try my current pci/tcg patches on LPAR with this (or a v4) on top.
If it works there (it doesn't yet on my laptop), we do have a
endianness issue... (unfortunately, the reverse isn't true.)

I'll post my pci/tcg patches once I get it mostly working (and it does
not look like a horror from the crypt anymore).

Thomas Huth Nov. 27, 2017, 11:13 a.m. UTC | #17

On 27.11.2017 11:09, Yi Min Zhao wrote:
> 
> 
> 在 2017/11/27 下午2:59, Thomas Huth 写道:
>> On 25.11.2017 14:49, Pierre Morel wrote:
>>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>>
>>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>>> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>>>>>>> On Thu, 23 Nov 2017 11:25:10 +0100
>>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>>
>>>>>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>>>>>> Thomas Huth <[e-mail]> 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?
>>>>> As far as I know, it's host endianness, so on x86 with TCG emulation,
>>>>> it's little endian.
>>>>>
>>>>>> In our case, the guest is S390. Although the arch is big-endian, the
>>>>>> data in
>>>>>> pcilg/stg instructions is little-endian.
>>>>> PCI memory is always little endian, right.
>>>>>
>>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu()
>>>>>> mean the
>>>>>> host endianess?
>>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>>>> confusing :-/
>>>>>
>>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>>>> not if the host is little endian (x86 with TCG).
>>> Here is my comprehension of this funny swapping:
>>>
>>> - TCG for a BE guest and a le host swap bytes because if we do (register
>>> & 0x01) in the zPCI interception code it must work what ever the
>>> endianess is.
>> Uhhh, I might have missed that the value has already been byte-swapped
>> once by TCG for env->regs[r1] ...
>
> I want to ask a question. For this case, BE guest and LE host, is
> env->regs[r1] in LE byte ordering?

Generally env->regs[] are in host byte order, so LE if the host is LE.
Not sure which byte-order is stored in the register by the guest,
though, since I don't have the zPCI spec ... so if the (BE) guest wrote
a LE value in the register, and TCG byte-swapped it again, the value is
suddenly BE again and thus we have to always byte-swap it again...?
Sorry, it's hard to say without having the spec available.

 Thomas

Cornelia Huck Nov. 27, 2017, 2:34 p.m. UTC | #18

On Mon, 27 Nov 2017 12:02:55 +0100
Cornelia Huck <[e-mail]> wrote:

> On Mon, 27 Nov 2017 07:59:36 +0100
> Thomas Huth <[e-mail]> wrote:
> 
> > On 25.11.2017 14:49, Pierre Morel wrote:  
> > > On 24/11/2017 07:19, Yi Min Zhao wrote:    
> > >>
> > >>
> > >> 在 2017/11/23 下午8:18, Thomas Huth 写道:    
> > >>> On 23.11.2017 13:07, Yi Min Zhao wrote:    
> 
> > >>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
> > >>>> host endianess?    
> > >>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
> > >>> confusing :-/
> > >>>    
> > >>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
> > >>> not if the host is little endian (x86 with TCG).    
> > > 
> > > Here is my comprehension of this funny swapping:
> > > 
> > > - TCG for a BE guest and a le host swap bytes because if we do (register
> > > & 0x01) in the zPCI interception code it must work what ever the
> > > endianess is.    
> > 
> > Uhhh, I might have missed that the value has already been byte-swapped
> > once by TCG for env->regs[r1] ...
> > Now I'm pretty much completely confused ... sorry for the noise if I was
> > wrong... I think it's best you ignore my comment for now (i.e. go with
> > bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
> > TCG, we still can fix this if necessary.  
> 
> I'll try my current pci/tcg patches on LPAR with this (or a v4) on top.
> If it works there (it doesn't yet on my laptop), we do have a
> endianness issue... (unfortunately, the reverse isn't true.)

It does not look too bad: I can get a nice enP1p0s0 device from a
virtio-net-pci with my tcg patches on my laptop (with these patches as
well, of course). So, endianness is likely mostly fine.

> 
> I'll post my pci/tcg patches once I get it mostly working (and it does
> not look like a horror from the crypt anymore).

Time to get out the make up kit for these.

Pierre Morel Nov. 27, 2017, 3:24 p.m. UTC | #19

On 27/11/2017 15:34, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 12:02:55 +0100
> Cornelia Huck <[e-mail]> wrote:
> 
>> On Mon, 27 Nov 2017 07:59:36 +0100
>> Thomas Huth <[e-mail]> wrote:
>>
>>> On 25.11.2017 14:49, Pierre Morel wrote:
>>>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>>>
>>>>>
>>>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>
>>>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>>>>>>> host endianess?
>>>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>>>>> confusing :-/
>>>>>>     
>>>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>>>>> not if the host is little endian (x86 with TCG).
>>>>
>>>> Here is my comprehension of this funny swapping:
>>>>
>>>> - TCG for a BE guest and a le host swap bytes because if we do (register
>>>> & 0x01) in the zPCI interception code it must work what ever the
>>>> endianess is.
>>>
>>> Uhhh, I might have missed that the value has already been byte-swapped
>>> once by TCG for env->regs[r1] ...
>>> Now I'm pretty much completely confused ... sorry for the noise if I was
>>> wrong... I think it's best you ignore my comment for now (i.e. go with
>>> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
>>> TCG, we still can fix this if necessary.
>>
>> I'll try my current pci/tcg patches on LPAR with this (or a v4) on top.
>> If it works there (it doesn't yet on my laptop), we do have a
>> endianness issue... (unfortunately, the reverse isn't true.)
> 
> It does not look too bad: I can get a nice enP1p0s0 device from a
> virtio-net-pci with my tcg patches on my laptop (with these patches as
> well, of course). So, endianness is likely mostly fine.

On the Lpar and on the Laptop or only on the Lpar ?

> 
>>
>> I'll post my pci/tcg patches once I get it mostly working (and it does
>> not look like a horror from the crypt anymore).
> 
> Time to get out the make up kit for these.
>

Cornelia Huck Nov. 27, 2017, 3:30 p.m. UTC | #20

On Mon, 27 Nov 2017 16:24:08 +0100
Pierre Morel <[e-mail]> wrote:

> On 27/11/2017 15:34, Cornelia Huck wrote:
> > On Mon, 27 Nov 2017 12:02:55 +0100
> > Cornelia Huck <[e-mail]> wrote:
> >   
> >> On Mon, 27 Nov 2017 07:59:36 +0100
> >> Thomas Huth <[e-mail]> wrote:
> >>  
> >>> On 25.11.2017 14:49, Pierre Morel wrote:  
> >>>> On 24/11/2017 07:19, Yi Min Zhao wrote:  
> >>>>>
> >>>>>
> >>>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:  
> >>>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:  
> >>  
> >>>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
> >>>>>>> host endianess?  
> >>>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
> >>>>>> confusing :-/
> >>>>>>       
> >>>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
> >>>>>> not if the host is little endian (x86 with TCG).  
> >>>>
> >>>> Here is my comprehension of this funny swapping:
> >>>>
> >>>> - TCG for a BE guest and a le host swap bytes because if we do (register
> >>>> & 0x01) in the zPCI interception code it must work what ever the
> >>>> endianess is.  
> >>>
> >>> Uhhh, I might have missed that the value has already been byte-swapped
> >>> once by TCG for env->regs[r1] ...
> >>> Now I'm pretty much completely confused ... sorry for the noise if I was
> >>> wrong... I think it's best you ignore my comment for now (i.e. go with
> >>> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
> >>> TCG, we still can fix this if necessary.  
> >>
> >> I'll try my current pci/tcg patches on LPAR with this (or a v4) on top.
> >> If it works there (it doesn't yet on my laptop), we do have a
> >> endianness issue... (unfortunately, the reverse isn't true.)  
> > 
> > It does not look too bad: I can get a nice enP1p0s0 device from a
> > virtio-net-pci with my tcg patches on my laptop (with these patches as
> > well, of course). So, endianness is likely mostly fine.  
> 
> On the Lpar and on the Laptop or only on the Lpar ?

Both :)

> 
> >   
> >>
> >> I'll post my pci/tcg patches once I get it mostly working (and it does
> >> not look like a horror from the crypt anymore).  
> > 
> > Time to get out the make up kit for these.
> >   
> 
> 
>

Pierre Morel Nov. 27, 2017, 3:53 p.m. UTC | #21

On 27/11/2017 16:30, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 16:24:08 +0100
> Pierre Morel <[e-mail]> wrote:
> 
>> On 27/11/2017 15:34, Cornelia Huck wrote:
>>> On Mon, 27 Nov 2017 12:02:55 +0100
>>> Cornelia Huck <[e-mail]> wrote:
>>>    
>>>> On Mon, 27 Nov 2017 07:59:36 +0100
>>>> Thomas Huth <[e-mail]> wrote:
>>>>   
>>>>> On 25.11.2017 14:49, Pierre Morel wrote:
>>>>>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>>>>>
>>>>>>>
>>>>>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>>>>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>   
>>>>>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>>>>>>>>> host endianess?
>>>>>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>>>>>>> confusing :-/
>>>>>>>>        
>>>>>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>>>>>>> not if the host is little endian (x86 with TCG).
>>>>>>
>>>>>> Here is my comprehension of this funny swapping:
>>>>>>
>>>>>> - TCG for a BE guest and a le host swap bytes because if we do (register
>>>>>> & 0x01) in the zPCI interception code it must work what ever the
>>>>>> endianess is.
>>>>>
>>>>> Uhhh, I might have missed that the value has already been byte-swapped
>>>>> once by TCG for env->regs[r1] ...
>>>>> Now I'm pretty much completely confused ... sorry for the noise if I was
>>>>> wrong... I think it's best you ignore my comment for now (i.e. go with
>>>>> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
>>>>> TCG, we still can fix this if necessary.
>>>>
>>>> I'll try my current pci/tcg patches on LPAR with this (or a v4) on top.
>>>> If it works there (it doesn't yet on my laptop), we do have a
>>>> endianness issue... (unfortunately, the reverse isn't true.)
>>>
>>> It does not look too bad: I can get a nice enP1p0s0 device from a
>>> virtio-net-pci with my tcg patches on my laptop (with these patches as
>>> well, of course). So, endianness is likely mostly fine.
>>
>> On the Lpar and on the Laptop or only on the Lpar ?
> 
> Both :)

That's great! :)

> 
>>
>>>    
>>>>
>>>> I'll post my pci/tcg patches once I get it mostly working (and it does
>>>> not look like a horror from the crypt anymore).
>>>
>>> Time to get out the make up kit for these.
>>>    
>>
>>
>>
>

Cornelia Huck Nov. 27, 2017, 4:02 p.m. UTC | #22

On Mon, 27 Nov 2017 16:53:04 +0100
Pierre Morel <[e-mail]> wrote:

> On 27/11/2017 16:30, Cornelia Huck wrote:
> > On Mon, 27 Nov 2017 16:24:08 +0100
> > Pierre Morel <[e-mail]> wrote:
> >   
> >> On 27/11/2017 15:34, Cornelia Huck wrote:  
> >>> On Mon, 27 Nov 2017 12:02:55 +0100
> >>> Cornelia Huck <[e-mail]> wrote:
> >>>      
> >>>> On Mon, 27 Nov 2017 07:59:36 +0100
> >>>> Thomas Huth <[e-mail]> wrote:
> >>>>     
> >>>>> On 25.11.2017 14:49, Pierre Morel wrote:  
> >>>>>> On 24/11/2017 07:19, Yi Min Zhao wrote:  
> >>>>>>>
> >>>>>>>
> >>>>>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:  
> >>>>>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:  
> >>>>     
> >>>>>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
> >>>>>>>>> host endianess?  
> >>>>>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
> >>>>>>>> confusing :-/
> >>>>>>>>          
> >>>>>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
> >>>>>>>> not if the host is little endian (x86 with TCG).  
> >>>>>>
> >>>>>> Here is my comprehension of this funny swapping:
> >>>>>>
> >>>>>> - TCG for a BE guest and a le host swap bytes because if we do (register
> >>>>>> & 0x01) in the zPCI interception code it must work what ever the
> >>>>>> endianess is.  
> >>>>>
> >>>>> Uhhh, I might have missed that the value has already been byte-swapped
> >>>>> once by TCG for env->regs[r1] ...
> >>>>> Now I'm pretty much completely confused ... sorry for the noise if I was
> >>>>> wrong... I think it's best you ignore my comment for now (i.e. go with
> >>>>> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
> >>>>> TCG, we still can fix this if necessary.  
> >>>>
> >>>> I'll try my current pci/tcg patches on LPAR with this (or a v4) on top.
> >>>> If it works there (it doesn't yet on my laptop), we do have a
> >>>> endianness issue... (unfortunately, the reverse isn't true.)  
> >>>
> >>> It does not look too bad: I can get a nice enP1p0s0 device from a
> >>> virtio-net-pci with my tcg patches on my laptop (with these patches as
> >>> well, of course). So, endianness is likely mostly fine.  
> >>
> >> On the Lpar and on the Laptop or only on the Lpar ?  
> > 
> > Both :)  
> 
> That's great! :)

Btw, lspci says

0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
        Subsystem: Red Hat, Inc. Device 0001
        Physical Slot: 00000000
        Flags: bus master, fast devsel, latency 0
        I/O ports at <unassigned> [disabled]
        [virtual] Memory at 8001000000000000 (32-bit, non-prefetchable) [size=4K]
        Memory at 8002000000000000 (64-bit, prefetchable) [size=16K]
        Expansion ROM at <unassigned> [disabled] [size=256K]
        Capabilities: [98] MSI-X: Enable+ Count=3 Masked-
        Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
        Capabilities: [70] Vendor Specific Information: VirtIO: Notify
        Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
        Capabilities: [50] Vendor Specific Information: VirtIO: ISR
        Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
        Kernel driver in use: virtio-pci

Does that look reasonable to you?

Pierre Morel Nov. 27, 2017, 4:40 p.m. UTC | #23

On 27/11/2017 17:02, Cornelia Huck wrote:
> On Mon, 27 Nov 2017 16:53:04 +0100
> Pierre Morel <[e-mail]> wrote:
> 
>> On 27/11/2017 16:30, Cornelia Huck wrote:
>>> On Mon, 27 Nov 2017 16:24:08 +0100
>>> Pierre Morel <[e-mail]> wrote:
>>>    
>>>> On 27/11/2017 15:34, Cornelia Huck wrote:
>>>>> On Mon, 27 Nov 2017 12:02:55 +0100
>>>>> Cornelia Huck <[e-mail]> wrote:
>>>>>       
>>>>>> On Mon, 27 Nov 2017 07:59:36 +0100
>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>      
>>>>>>> On 25.11.2017 14:49, Pierre Morel wrote:
>>>>>>>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>>>>>>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>>>      
>>>>>>>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>>>>>>>>>>> host endianess?
>>>>>>>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>>>>>>>>> confusing :-/
>>>>>>>>>>           
>>>>>>>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>>>>>>>>> not if the host is little endian (x86 with TCG).
>>>>>>>>
>>>>>>>> Here is my comprehension of this funny swapping:
>>>>>>>>
>>>>>>>> - TCG for a BE guest and a le host swap bytes because if we do (register
>>>>>>>> & 0x01) in the zPCI interception code it must work what ever the
>>>>>>>> endianess is.
>>>>>>>
>>>>>>> Uhhh, I might have missed that the value has already been byte-swapped
>>>>>>> once by TCG for env->regs[r1] ...
>>>>>>> Now I'm pretty much completely confused ... sorry for the noise if I was
>>>>>>> wrong... I think it's best you ignore my comment for now (i.e. go with
>>>>>>> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
>>>>>>> TCG, we still can fix this if necessary.
>>>>>>
>>>>>> I'll try my current pci/tcg patches on LPAR with this (or a v4) on top.
>>>>>> If it works there (it doesn't yet on my laptop), we do have a
>>>>>> endianness issue... (unfortunately, the reverse isn't true.)
>>>>>
>>>>> It does not look too bad: I can get a nice enP1p0s0 device from a
>>>>> virtio-net-pci with my tcg patches on my laptop (with these patches as
>>>>> well, of course). So, endianness is likely mostly fine.
>>>>
>>>> On the Lpar and on the Laptop or only on the Lpar ?
>>>
>>> Both :)
>>
>> That's great! :)
> 
> Btw, lspci says
> 
> 0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
>          Subsystem: Red Hat, Inc. Device 0001
>          Physical Slot: 00000000
>          Flags: bus master, fast devsel, latency 0
>          I/O ports at <unassigned> [disabled]
>          [virtual] Memory at 8001000000000000 (32-bit, non-prefetchable) [size=4K]
>          Memory at 8002000000000000 (64-bit, prefetchable) [size=16K]
>          Expansion ROM at <unassigned> [disabled] [size=256K]
>          Capabilities: [98] MSI-X: Enable+ Count=3 Masked-
>          Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
>          Capabilities: [70] Vendor Specific Information: VirtIO: Notify
>          Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
>          Capabilities: [50] Vendor Specific Information: VirtIO: ISR
>          Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
>          Kernel driver in use: virtio-pci
> 
> Does that look reasonable to you?
> 

Yes, I see no problem in what is listed.
Slot, memory, ROM, I/O port look reasonable to me.
The 4 Vendor Specific informations look like the VIRTIO subregions from BAR

Yi Min Zhao Nov. 28, 2017, 6:41 a.m. UTC | #24

在 2017/11/27 下午7:13, Thomas Huth 写道:
> On 27.11.2017 11:09, Yi Min Zhao wrote:
>>
>> 在 2017/11/27 下午2:59, Thomas Huth 写道:
>>> On 25.11.2017 14:49, Pierre Morel wrote:
>>>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>>>> 在 2017/11/23 下午6:33, Cornelia Huck 写道:
>>>>>>>> On Thu, 23 Nov 2017 11:25:10 +0100
>>>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>>>
>>>>>>>>> On 23.11.2017 11:08, Cornelia Huck wrote:
>>>>>>>>>> On Thu, 23 Nov 2017 11:01:23 +0100
>>>>>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>>>>>> On 23.11.2017 10:49, Cornelia Huck wrote:
>>>>>>>>>>>> On Thu, 23 Nov 2017 09:48:41 +0100
>>>>>>>>>>>> Thomas Huth <[e-mail]> 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?
>>>>>> As far as I know, it's host endianness, so on x86 with TCG emulation,
>>>>>> it's little endian.
>>>>>>
>>>>>>> In our case, the guest is S390. Although the arch is big-endian, the
>>>>>>> data in
>>>>>>> pcilg/stg instructions is little-endian.
>>>>>> PCI memory is always little endian, right.
>>>>>>
>>>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu()
>>>>>>> mean the
>>>>>>> host endianess?
>>>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>>>>> confusing :-/
>>>>>>
>>>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>>>>> not if the host is little endian (x86 with TCG).
>>>> Here is my comprehension of this funny swapping:
>>>>
>>>> - TCG for a BE guest and a le host swap bytes because if we do (register
>>>> & 0x01) in the zPCI interception code it must work what ever the
>>>> endianess is.
>>> Uhhh, I might have missed that the value has already been byte-swapped
>>> once by TCG for env->regs[r1] ...
>> I want to ask a question. For this case, BE guest and LE host, is
>> env->regs[r1] in LE byte ordering?
> Generally env->regs[] are in host byte order, so LE if the host is LE.
> Not sure which byte-order is stored in the register by the guest,
> though, since I don't have the zPCI spec ... so if the (BE) guest wrote
> a LE value in the register, and TCG byte-swapped it again, the value is
> suddenly BE again and thus we have to always byte-swap it again...?
> Sorry, it's hard to say without having the spec available.
Yes, your understanding is right. The guest is BE but the data in 
register for zPCI instruction is LE.
>
>   Thomas
>
>

Yi Min Zhao Nov. 28, 2017, 6:48 a.m. UTC | #25

在 2017/11/28 上午12:02, Cornelia Huck 写道:
> On Mon, 27 Nov 2017 16:53:04 +0100
> Pierre Morel <[e-mail]> wrote:
>
>> On 27/11/2017 16:30, Cornelia Huck wrote:
>>> On Mon, 27 Nov 2017 16:24:08 +0100
>>> Pierre Morel <[e-mail]> wrote:
>>>    
>>>> On 27/11/2017 15:34, Cornelia Huck wrote:
>>>>> On Mon, 27 Nov 2017 12:02:55 +0100
>>>>> Cornelia Huck <[e-mail]> wrote:
>>>>>       
>>>>>> On Mon, 27 Nov 2017 07:59:36 +0100
>>>>>> Thomas Huth <[e-mail]> wrote:
>>>>>>      
>>>>>>> On 25.11.2017 14:49, Pierre Morel wrote:
>>>>>>>> On 24/11/2017 07:19, Yi Min Zhao wrote:
>>>>>>>>>
>>>>>>>>> 在 2017/11/23 下午8:18, Thomas Huth 写道:
>>>>>>>>>> On 23.11.2017 13:07, Yi Min Zhao wrote:
>>>>>>      
>>>>>>>>>>> Another question, does 'cpu' in cpu_to_le**() or le**_to_cpu() mean the
>>>>>>>>>>> host endianess?
>>>>>>>>>> Yes, the "cpu" in cpu_to_le or le_to_cpu means the host, indeed. It's
>>>>>>>>>> confusing :-/
>>>>>>>>>>           
>>>>>>>>>>> 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 think we've got to byte-swap if the host is big endian (s390x), but
>>>>>>>>>> not if the host is little endian (x86 with TCG).
>>>>>>>> Here is my comprehension of this funny swapping:
>>>>>>>>
>>>>>>>> - TCG for a BE guest and a le host swap bytes because if we do (register
>>>>>>>> & 0x01) in the zPCI interception code it must work what ever the
>>>>>>>> endianess is.
>>>>>>> Uhhh, I might have missed that the value has already been byte-swapped
>>>>>>> once by TCG for env->regs[r1] ...
>>>>>>> Now I'm pretty much completely confused ... sorry for the noise if I was
>>>>>>> wrong... I think it's best you ignore my comment for now (i.e. go with
>>>>>>> bswapXX() instead of le_to_cpuXX()), and if we later wire up zPCI with
>>>>>>> TCG, we still can fix this if necessary.
>>>>>> I'll try my current pci/tcg patches on LPAR with this (or a v4) on top.
>>>>>> If it works there (it doesn't yet on my laptop), we do have a
>>>>>> endianness issue... (unfortunately, the reverse isn't true.)
>>>>> It does not look too bad: I can get a nice enP1p0s0 device from a
>>>>> virtio-net-pci with my tcg patches on my laptop (with these patches as
>>>>> well, of course). So, endianness is likely mostly fine.
>>>> On the Lpar and on the Laptop or only on the Lpar ?
>>> Both :)
>> That's great! :)
> Btw, lspci says
>
> 0001:00:00.0 Ethernet controller: Red Hat, Inc. Virtio network device
>          Subsystem: Red Hat, Inc. Device 0001
>          Physical Slot: 00000000
>          Flags: bus master, fast devsel, latency 0
>          I/O ports at <unassigned> [disabled]
>          [virtual] Memory at 8001000000000000 (32-bit, non-prefetchable) [size=4K]
>          Memory at 8002000000000000 (64-bit, prefetchable) [size=16K]
>          Expansion ROM at <unassigned> [disabled] [size=256K]
>          Capabilities: [98] MSI-X: Enable+ Count=3 Masked-
>          Capabilities: [84] Vendor Specific Information: VirtIO: <unknown>
>          Capabilities: [70] Vendor Specific Information: VirtIO: Notify
>          Capabilities: [60] Vendor Specific Information: VirtIO: DeviceCfg
>          Capabilities: [50] Vendor Specific Information: VirtIO: ISR
>          Capabilities: [40] Vendor Specific Information: VirtIO: CommonCfg
>          Kernel driver in use: virtio-pci
>
> Does that look reasonable to you?
>
>
Great! That means the data in env->register has been swapped.
Thanks!

Michael S. Tsirkin Nov. 28, 2017, 5:28 p.m. UTC | #26

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

Patch

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 = &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;
         }