diff mbox

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

Message ID 1510075479-17224-2-git-send-email-pmorel@linux.vnet.ibm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Pierre Morel Nov. 7, 2017, 5:24 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 <pmorel@linux.vnet.ibm.com>
Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
---
 hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 26 deletions(-)

Comments

Cornelia Huck Nov. 9, 2017, 4:38 p.m. UTC | #1
On Tue,  7 Nov 2017 18:24:33 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> There are two places where the same endianness conversion
> is done.
> Let's factor this out into a static function.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> ---
>  hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> index 8e088f3..8fcb02d 100644
> --- a/hw/s390x/s390-pci-inst.c
> +++ b/hw/s390x/s390-pci-inst.c
> @@ -314,6 +314,35 @@ out:
>      return 0;
>  }
>  
> +/**
> + * This function swaps the data at ptr according from one
> + * endianness to the other.
> + * valid data in the uint64_t data field.

I'm not sure what that line is supposed to mean?

> + * @ptr: a pointer to a uint64_t data field
> + * @len: the length of the valid data, must be 1,2,4 or 8
> + */
> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
> +{
> +    uint64_t data = *ptr;
> +    switch (len) {
> +    case 1:
> +        break;
> +    case 2:
> +        data = bswap16(data);
> +        break;
> +    case 4:
> +        data = bswap32(data);
> +        break;
> +    case 8:
> +        data = bswap64(data);
> +        break;
> +    default:
> +        return -EINVAL;
> +    }
> +    *ptr = data;
> +    return 0;
> +}
> +

I was expecting more code to use a similar pattern, but it seems
surprisingly uncommon.
Philippe Mathieu-Daudé Nov. 9, 2017, 6:55 p.m. UTC | #2
On 11/09/2017 01:38 PM, Cornelia Huck wrote:
> On Tue,  7 Nov 2017 18:24:33 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> There are two places where the same endianness conversion
>> is done.
>> Let's factor this out into a static function.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>> ---
>>  hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>  1 file changed, 32 insertions(+), 26 deletions(-)
>>
>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>> index 8e088f3..8fcb02d 100644
>> --- a/hw/s390x/s390-pci-inst.c
>> +++ b/hw/s390x/s390-pci-inst.c
>> @@ -314,6 +314,35 @@ out:
>>      return 0;
>>  }
>>  
>> +/**
>> + * This function swaps the data at ptr according from one
>> + * endianness to the other.
>> + * valid data in the uint64_t data field.
> 
> I'm not sure what that line is supposed to mean?
> 
>> + * @ptr: a pointer to a uint64_t data field
>> + * @len: the length of the valid data, must be 1,2,4 or 8
>> + */
>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>> +{
>> +    uint64_t data = *ptr;
>> +    switch (len) {
>> +    case 1:
>> +        break;
>> +    case 2:
>> +        data = bswap16(data);
>> +        break;
>> +    case 4:
>> +        data = bswap32(data);
>> +        break;
>> +    case 8:
>> +        data = bswap64(data);
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +    *ptr = data;
>> +    return 0;
>> +}

This is usually care taken by memory::adjust_endianness() ...

> I was expecting more code to use a similar pattern, but it seems
> surprisingly uncommon.

Which ring a bell for latent bug?

This remind me of a similar issue on ppc:

http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
...
http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
Cornelia Huck Nov. 9, 2017, 7:20 p.m. UTC | #3
On Thu, 9 Nov 2017 15:55:46 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
> > On Tue,  7 Nov 2017 18:24:33 +0100
> > Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >   
> >> There are two places where the same endianness conversion
> >> is done.
> >> Let's factor this out into a static function.
> >>
> >> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >> ---
> >>  hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
> >>  1 file changed, 32 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >> index 8e088f3..8fcb02d 100644
> >> --- a/hw/s390x/s390-pci-inst.c
> >> +++ b/hw/s390x/s390-pci-inst.c
> >> @@ -314,6 +314,35 @@ out:
> >>      return 0;
> >>  }
> >>  
> >> +/**
> >> + * This function swaps the data at ptr according from one
> >> + * endianness to the other.
> >> + * valid data in the uint64_t data field.  
> > 
> > I'm not sure what that line is supposed to mean?
> >   
> >> + * @ptr: a pointer to a uint64_t data field
> >> + * @len: the length of the valid data, must be 1,2,4 or 8
> >> + */
> >> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
> >> +{
> >> +    uint64_t data = *ptr;
> >> +    switch (len) {
> >> +    case 1:
> >> +        break;
> >> +    case 2:
> >> +        data = bswap16(data);
> >> +        break;
> >> +    case 4:
> >> +        data = bswap32(data);
> >> +        break;
> >> +    case 8:
> >> +        data = bswap64(data);
> >> +        break;
> >> +    default:
> >> +        return -EINVAL;
> >> +    }
> >> +    *ptr = data;
> >> +    return 0;
> >> +}  
> 
> This is usually care taken by memory::adjust_endianness() ...

Yes, but that's not a memory region write.

> 
> > I was expecting more code to use a similar pattern, but it seems
> > surprisingly uncommon.  
> 
> Which ring a bell for latent bug?

Looking at this, it seems there *is* a latent bug, which has not popped
up so far as the pci instructions are not wired up in tcg yet. This
code is only called from the kvm path...

> 
> This remind me of a similar issue on ppc:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
> ...
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
Pierre Morel Nov. 13, 2017, 9:34 a.m. UTC | #4
On 09/11/2017 19:55, Philippe Mathieu-Daudé wrote:
> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
>> On Tue,  7 Nov 2017 18:24:33 +0100
>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>
>>> There are two places where the same endianness conversion
>>> is done.
>>> Let's factor this out into a static function.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>> ---
>>>   hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>>   1 file changed, 32 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>> index 8e088f3..8fcb02d 100644
>>> --- a/hw/s390x/s390-pci-inst.c
>>> +++ b/hw/s390x/s390-pci-inst.c
>>> @@ -314,6 +314,35 @@ out:
>>>       return 0;
>>>   }
>>>   
>>> +/**
>>> + * This function swaps the data at ptr according from one
>>> + * endianness to the other.
>>> + * valid data in the uint64_t data field.
>>
>> I'm not sure what that line is supposed to mean?
>>
>>> + * @ptr: a pointer to a uint64_t data field
>>> + * @len: the length of the valid data, must be 1,2,4 or 8
>>> + */
>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>>> +{
>>> +    uint64_t data = *ptr;
>>> +    switch (len) {
>>> +    case 1:
>>> +        break;
>>> +    case 2:
>>> +        data = bswap16(data);
>>> +        break;
>>> +    case 4:
>>> +        data = bswap32(data);
>>> +        break;
>>> +    case 8:
>>> +        data = bswap64(data);
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +    *ptr = data;
>>> +    return 0;
>>> +}
> 
> This is usually care taken by memory::adjust_endianness() ...

We are here intercepting an instruction with the data in a register.
That is what troubles me, but I will take a deeper look.

> 
>> I was expecting more code to use a similar pattern, but it seems
>> surprisingly uncommon.
> 
> Which ring a bell for latent bug?
> 
> This remind me of a similar issue on ppc:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
> ...
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
> 

Thanks for the pointers.
Pierre Morel Nov. 13, 2017, 9:37 a.m. UTC | #5
On 09/11/2017 19:55, Philippe Mathieu-Daudé wrote:
> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
>> On Tue,  7 Nov 2017 18:24:33 +0100
>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>
>>> There are two places where the same endianness conversion
>>> is done.
>>> Let's factor this out into a static function.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>> ---
>>>   hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>>   1 file changed, 32 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>> index 8e088f3..8fcb02d 100644
>>> --- a/hw/s390x/s390-pci-inst.c
>>> +++ b/hw/s390x/s390-pci-inst.c
>>> @@ -314,6 +314,35 @@ out:
>>>       return 0;
>>>   }
>>>   
>>> +/**
>>> + * This function swaps the data at ptr according from one
>>> + * endianness to the other.
>>> + * valid data in the uint64_t data field.
>>
>> I'm not sure what that line is supposed to mean?
>>
>>> + * @ptr: a pointer to a uint64_t data field
>>> + * @len: the length of the valid data, must be 1,2,4 or 8
>>> + */
>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>>> +{
>>> +    uint64_t data = *ptr;
>>> +    switch (len) {
>>> +    case 1:
>>> +        break;
>>> +    case 2:
>>> +        data = bswap16(data);
>>> +        break;
>>> +    case 4:
>>> +        data = bswap32(data);
>>> +        break;
>>> +    case 8:
>>> +        data = bswap64(data);
>>> +        break;
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +    *ptr = data;
>>> +    return 0;
>>> +}
> 
> This is usually care taken by memory::adjust_endianness() ...


We are here intercepting an instruction with the data in a register.
That is what troubles me, but I will take a deeper look.

> 
>> I was expecting more code to use a similar pattern, but it seems
>> surprisingly uncommon.
> 
> Which ring a bell for latent bug?
> 
> This remind me of a similar issue on ppc:
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
> ...
> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
> 

Thanks for the pointers.
Pierre Morel Nov. 13, 2017, 3:36 p.m. UTC | #6
On 09/11/2017 20:20, Cornelia Huck wrote:
> On Thu, 9 Nov 2017 15:55:46 -0300
> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
>> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
>>> On Tue,  7 Nov 2017 18:24:33 +0100
>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>    
>>>> There are two places where the same endianness conversion
>>>> is done.
>>>> Let's factor this out into a static function.
>>>>
>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>> ---
>>>>   hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>>>   1 file changed, 32 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>>> index 8e088f3..8fcb02d 100644
>>>> --- a/hw/s390x/s390-pci-inst.c
>>>> +++ b/hw/s390x/s390-pci-inst.c
>>>> @@ -314,6 +314,35 @@ out:
>>>>       return 0;
>>>>   }
>>>>   
>>>> +/**
>>>> + * This function swaps the data at ptr according from one
>>>> + * endianness to the other.
>>>> + * valid data in the uint64_t data field.
>>>
>>> I'm not sure what that line is supposed to mean?
>>>    
>>>> + * @ptr: a pointer to a uint64_t data field
>>>> + * @len: the length of the valid data, must be 1,2,4 or 8
>>>> + */
>>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>>>> +{
>>>> +    uint64_t data = *ptr;
>>>> +    switch (len) {
>>>> +    case 1:
>>>> +        break;
>>>> +    case 2:
>>>> +        data = bswap16(data);
>>>> +        break;
>>>> +    case 4:
>>>> +        data = bswap32(data);
>>>> +        break;
>>>> +    case 8:
>>>> +        data = bswap64(data);
>>>> +        break;
>>>> +    default:
>>>> +        return -EINVAL;
>>>> +    }
>>>> +    *ptr = data;
>>>> +    return 0;
>>>> +}
>>
>> This is usually care taken by memory::adjust_endianness() ...
> 
> Yes, but that's not a memory region write.
> 
>>
>>> I was expecting more code to use a similar pattern, but it seems
>>> surprisingly uncommon.
>>
>> Which ring a bell for latent bug?
> 
> Looking at this, it seems there *is* a latent bug, which has not popped
> up so far as the pci instructions are not wired up in tcg yet. This
> code is only called from the kvm path...


The value in the register may be read from memory somehow but it may 
also be an immediate value, setup previously by another instruction.

AFAIU the TCG would have already make sure that the value read from 
memory has already been translated to big endian if read from a little 
endian memory region.
So that the value in register is always big endian.

OTOH the PCI memory is always little endian.

So AFAIU we always need to translate from BIG to little, no mater if KVM 
or TCG.

But I am not sure that I did understand right what the TCG does.

@Philippe, It does not seems to be the same problem as you encountered, 
AFAIU your problem was between memory and a LE device and our is between 
a BE register and a LE device.

Did I understood correctly what TCG does when emulating S390 ?


Pierre

> 
>>
>> This remind me of a similar issue on ppc:
>>
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
>> ...
>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
> 
>
Cornelia Huck Nov. 13, 2017, 4:38 p.m. UTC | #7
On Mon, 13 Nov 2017 16:36:34 +0100
Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:

> On 09/11/2017 20:20, Cornelia Huck wrote:
> > On Thu, 9 Nov 2017 15:55:46 -0300
> > Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >   
> >> On 11/09/2017 01:38 PM, Cornelia Huck wrote:  
> >>> On Tue,  7 Nov 2017 18:24:33 +0100
> >>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> >>>      
> >>>> There are two places where the same endianness conversion
> >>>> is done.
> >>>> Let's factor this out into a static function.
> >>>>
> >>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
> >>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
> >>>> ---
> >>>>   hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
> >>>>   1 file changed, 32 insertions(+), 26 deletions(-)
> >>>>
> >>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
> >>>> index 8e088f3..8fcb02d 100644
> >>>> --- a/hw/s390x/s390-pci-inst.c
> >>>> +++ b/hw/s390x/s390-pci-inst.c
> >>>> @@ -314,6 +314,35 @@ out:
> >>>>       return 0;
> >>>>   }
> >>>>   
> >>>> +/**
> >>>> + * This function swaps the data at ptr according from one
> >>>> + * endianness to the other.
> >>>> + * valid data in the uint64_t data field.  
> >>>
> >>> I'm not sure what that line is supposed to mean?
> >>>      
> >>>> + * @ptr: a pointer to a uint64_t data field
> >>>> + * @len: the length of the valid data, must be 1,2,4 or 8
> >>>> + */
> >>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
> >>>> +{
> >>>> +    uint64_t data = *ptr;
> >>>> +    switch (len) {
> >>>> +    case 1:
> >>>> +        break;
> >>>> +    case 2:
> >>>> +        data = bswap16(data);
> >>>> +        break;
> >>>> +    case 4:
> >>>> +        data = bswap32(data);
> >>>> +        break;
> >>>> +    case 8:
> >>>> +        data = bswap64(data);
> >>>> +        break;
> >>>> +    default:
> >>>> +        return -EINVAL;
> >>>> +    }
> >>>> +    *ptr = data;
> >>>> +    return 0;
> >>>> +}  
> >>
> >> This is usually care taken by memory::adjust_endianness() ...  
> > 
> > Yes, but that's not a memory region write.
> >   
> >>  
> >>> I was expecting more code to use a similar pattern, but it seems
> >>> surprisingly uncommon.  
> >>
> >> Which ring a bell for latent bug?  
> > 
> > Looking at this, it seems there *is* a latent bug, which has not popped
> > up so far as the pci instructions are not wired up in tcg yet. This
> > code is only called from the kvm path...  
> 
> 
> The value in the register may be read from memory somehow but it may 
> also be an immediate value, setup previously by another instruction.
> 
> AFAIU the TCG would have already make sure that the value read from 
> memory has already been translated to big endian if read from a little 
> endian memory region.
> So that the value in register is always big endian.
> 
> OTOH the PCI memory is always little endian.
> 
> So AFAIU we always need to translate from BIG to little, no mater if KVM 
> or TCG.
> 
> But I am not sure that I did understand right what the TCG does.
> 
> @Philippe, It does not seems to be the same problem as you encountered, 
> AFAIU your problem was between memory and a LE device and our is between 
> a BE register and a LE device.
> 
> Did I understood correctly what TCG does when emulating S390 ?

So, if this function is supposed to work on a known-BE value, I think
this should be fine. But a comment in the function description would be
good...

> 
> 
> Pierre
> 
> >   
> >>
> >> This remind me of a similar issue on ppc:
> >>
> >> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
> >> ...
> >> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html  
> > 
> >   
> 
>
Pierre Morel Nov. 13, 2017, 4:43 p.m. UTC | #8
On 13/11/2017 17:38, Cornelia Huck wrote:
> On Mon, 13 Nov 2017 16:36:34 +0100
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 09/11/2017 20:20, Cornelia Huck wrote:
>>> On Thu, 9 Nov 2017 15:55:46 -0300
>>> Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>    
>>>> On 11/09/2017 01:38 PM, Cornelia Huck wrote:
>>>>> On Tue,  7 Nov 2017 18:24:33 +0100
>>>>> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
>>>>>       
>>>>>> There are two places where the same endianness conversion
>>>>>> is done.
>>>>>> Let's factor this out into a static function.
>>>>>>
>>>>>> Signed-off-by: Pierre Morel <pmorel@linux.vnet.ibm.com>
>>>>>> Reviewed-by: Yi Min Zhao <zyimin@linux.vnet.ibm.com>
>>>>>> ---
>>>>>>    hw/s390x/s390-pci-inst.c | 58 ++++++++++++++++++++++++++----------------------
>>>>>>    1 file changed, 32 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
>>>>>> index 8e088f3..8fcb02d 100644
>>>>>> --- a/hw/s390x/s390-pci-inst.c
>>>>>> +++ b/hw/s390x/s390-pci-inst.c
>>>>>> @@ -314,6 +314,35 @@ out:
>>>>>>        return 0;
>>>>>>    }
>>>>>>    
>>>>>> +/**
>>>>>> + * This function swaps the data at ptr according from one
>>>>>> + * endianness to the other.
>>>>>> + * valid data in the uint64_t data field.
>>>>>
>>>>> I'm not sure what that line is supposed to mean?
>>>>>       
>>>>>> + * @ptr: a pointer to a uint64_t data field
>>>>>> + * @len: the length of the valid data, must be 1,2,4 or 8
>>>>>> + */
>>>>>> +static int zpci_endian_swap(uint64_t *ptr, uint8_t len)
>>>>>> +{
>>>>>> +    uint64_t data = *ptr;
>>>>>> +    switch (len) {
>>>>>> +    case 1:
>>>>>> +        break;
>>>>>> +    case 2:
>>>>>> +        data = bswap16(data);
>>>>>> +        break;
>>>>>> +    case 4:
>>>>>> +        data = bswap32(data);
>>>>>> +        break;
>>>>>> +    case 8:
>>>>>> +        data = bswap64(data);
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        return -EINVAL;
>>>>>> +    }
>>>>>> +    *ptr = data;
>>>>>> +    return 0;
>>>>>> +}
>>>>
>>>> This is usually care taken by memory::adjust_endianness() ...
>>>
>>> Yes, but that's not a memory region write.
>>>    
>>>>   
>>>>> I was expecting more code to use a similar pattern, but it seems
>>>>> surprisingly uncommon.
>>>>
>>>> Which ring a bell for latent bug?
>>>
>>> Looking at this, it seems there *is* a latent bug, which has not popped
>>> up so far as the pci instructions are not wired up in tcg yet. This
>>> code is only called from the kvm path...
>>
>>
>> The value in the register may be read from memory somehow but it may
>> also be an immediate value, setup previously by another instruction.
>>
>> AFAIU the TCG would have already make sure that the value read from
>> memory has already been translated to big endian if read from a little
>> endian memory region.
>> So that the value in register is always big endian.
>>
>> OTOH the PCI memory is always little endian.
>>
>> So AFAIU we always need to translate from BIG to little, no mater if KVM
>> or TCG.
>>
>> But I am not sure that I did understand right what the TCG does.
>>
>> @Philippe, It does not seems to be the same problem as you encountered,
>> AFAIU your problem was between memory and a LE device and our is between
>> a BE register and a LE device.
>>
>> Did I understood correctly what TCG does when emulating S390 ?
> 
> So, if this function is supposed to work on a known-BE value, I think
> this should be fine. But a comment in the function description would be
> good...

Yes, I will do.
Thanks,

Pierre

> 
>>
>>
>> Pierre
>>
>>>    
>>>>
>>>> This remind me of a similar issue on ppc:
>>>>
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05121.html
>>>> ...
>>>> http://lists.nongnu.org/archive/html/qemu-devel/2017-02/msg05666.html
>>>
>>>    
>>
>>
> 
>
diff mbox

Patch

diff --git a/hw/s390x/s390-pci-inst.c b/hw/s390x/s390-pci-inst.c
index 8e088f3..8fcb02d 100644
--- a/hw/s390x/s390-pci-inst.c
+++ b/hw/s390x/s390-pci-inst.c
@@ -314,6 +314,35 @@  out:
     return 0;
 }
 
+/**
+ * This function swaps the data at ptr according from one
+ * endianness to the other.
+ * valid data in the uint64_t data field.
+ * @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 +414,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 +517,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;
         }