[Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Cornelia Huck]

We currently pass an integer as the subcode parameter. However,
the upper bits of the register containing the subcode need to
be 0, which is not guaranteed unless we explicitly specify the
subcode to be an unsigned long value.

Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")
Cc: qemu-stable@nongnu.org
Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 pc-bios/s390-ccw/iplb.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
index 5357a36d51..ded20c834e 100644
--- a/pc-bios/s390-ccw/iplb.h
+++ b/pc-bios/s390-ccw/iplb.h
@@ -101,10 +101,11 @@ static inline bool manage_iplb(IplParameterBlock *iplb, bool store)
 {
     register unsigned long addr asm("0") = (unsigned long) iplb;
     register unsigned long rc asm("1") = 0;
+    unsigned long subcode = store ? 6 : 5;
 
     asm volatile ("diag %0,%2,0x308\n"
                   : "+d" (addr), "+d" (rc)
-                  : "d" (store ? 6 : 5)
+                  : "d" (subcode)
                   : "memory", "cc");
     return rc == 0x01;
 }
-- 
2.14.3

Re: [Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Christian Borntraeger]

On 05/02/2018 02:52 PM, Cornelia Huck wrote:
> We currently pass an integer as the subcode parameter. However,
> the upper bits of the register containing the subcode need to
> be 0, which is not guaranteed unless we explicitly specify the
> subcode to be an unsigned long value.
> 
> Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  pc-bios/s390-ccw/iplb.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 5357a36d51..ded20c834e 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -101,10 +101,11 @@ static inline bool manage_iplb(IplParameterBlock *iplb, bool store)
>  {
>      register unsigned long addr asm("0") = (unsigned long) iplb;
>      register unsigned long rc asm("1") = 0;
> +    unsigned long subcode = store ? 6 : 5;
>  
>      asm volatile ("diag %0,%2,0x308\n"
>                    : "+d" (addr), "+d" (rc)
> -                  : "d" (store ? 6 : 5)
> +                  : "d" (subcode)

We could also use 6UL : 5UL instead of a local variable, but I certainly do not care enough.

Acked-by: Christian Borntraeger <borntraeger@de.ibm.com>


>                    : "memory", "cc");
>      return rc == 0x01;
>  }
> 

Re: [Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Thomas Huth]

On 02.05.2018 14:52, Cornelia Huck wrote:
> We currently pass an integer as the subcode parameter. However,
> the upper bits of the register containing the subcode need to
> be 0, which is not guaranteed unless we explicitly specify the
> subcode to be an unsigned long value.
> 
> Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  pc-bios/s390-ccw/iplb.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 5357a36d51..ded20c834e 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -101,10 +101,11 @@ static inline bool manage_iplb(IplParameterBlock *iplb, bool store)
>  {
>      register unsigned long addr asm("0") = (unsigned long) iplb;
>      register unsigned long rc asm("1") = 0;
> +    unsigned long subcode = store ? 6 : 5;
>  
>      asm volatile ("diag %0,%2,0x308\n"
>                    : "+d" (addr), "+d" (rc)
> -                  : "d" (store ? 6 : 5)
> +                  : "d" (subcode)
>                    : "memory", "cc");
>      return rc == 0x01;
>  }

Thanks a lot, this fixes the issue that I'm currently seeing when
building the s390-ccw bios with gcc 4.8.5 and trying to boot from a
virtio-scsi device with bootindex set.

Tested-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[David Hildenbrand]

On 02.05.2018 14:52, Cornelia Huck wrote:
> We currently pass an integer as the subcode parameter. However,
> the upper bits of the register containing the subcode need to
> be 0, which is not guaranteed unless we explicitly specify the
> subcode to be an unsigned long value.
> 
> Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Cornelia Huck <cohuck@redhat.com>
> ---
>  pc-bios/s390-ccw/iplb.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/pc-bios/s390-ccw/iplb.h b/pc-bios/s390-ccw/iplb.h
> index 5357a36d51..ded20c834e 100644
> --- a/pc-bios/s390-ccw/iplb.h
> +++ b/pc-bios/s390-ccw/iplb.h
> @@ -101,10 +101,11 @@ static inline bool manage_iplb(IplParameterBlock *iplb, bool store)
>  {
>      register unsigned long addr asm("0") = (unsigned long) iplb;
>      register unsigned long rc asm("1") = 0;
> +    unsigned long subcode = store ? 6 : 5;
>  
>      asm volatile ("diag %0,%2,0x308\n"
>                    : "+d" (addr), "+d" (rc)
> -                  : "d" (store ? 6 : 5)
> +                  : "d" (subcode)
>                    : "memory", "cc");
>      return rc == 0x01;
>  }
> 

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 

Thanks,

David / dhildenb

Re: [Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Farhan Ali]

On 05/02/2018 08:52 AM, Cornelia Huck wrote:
> We currently pass an integer as the subcode parameter. However,
> the upper bits of the register containing the subcode need to
> be 0, which is not guaranteed unless we explicitly specify the
> subcode to be an unsigned long value.
> 
> Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")
> Cc:qemu-stable@nongnu.org
> Signed-off-by: Cornelia Huck<cohuck@redhat.com>

Sorry for my ignorance, but is there a C standard that says upper bits 
of an int is not guaranteed to be 0?

Thanks
Farhan

Re: [Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Cornelia Huck]

On Thu, 3 May 2018 11:25:08 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 05/02/2018 08:52 AM, Cornelia Huck wrote:
> > We currently pass an integer as the subcode parameter. However,
> > the upper bits of the register containing the subcode need to
> > be 0, which is not guaranteed unless we explicitly specify the
> > subcode to be an unsigned long value.
> > 
> > Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")
> > Cc:qemu-stable@nongnu.org
> > Signed-off-by: Cornelia Huck<cohuck@redhat.com>  
> 
> Sorry for my ignorance, but is there a C standard that says upper bits 
> of an int is not guaranteed to be 0?

The value (5 resp. 6) is small enough to fit into a regular integer,
and the compiler generated a lhi for the load, which did not change any
upper values that might have been in the register previously. Telling
the compiler to treat the value as an unsigned long makes it generate a
lghi.

Re: [Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Eric Blake]

On 05/03/2018 10:25 AM, Farhan Ali wrote:
> On 05/02/2018 08:52 AM, Cornelia Huck wrote:
>> We currently pass an integer as the subcode parameter. However,
>> the upper bits of the register containing the subcode need to
>> be 0, which is not guaranteed unless we explicitly specify the
>> subcode to be an unsigned long value.
>>
>> Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 
>> 308/6")
>> Cc:qemu-stable@nongnu.org
>> Signed-off-by: Cornelia Huck<cohuck@redhat.com>
> 
> Sorry for my ignorance, but is there a C standard that says upper bits 
> of an int is not guaranteed to be 0?

We're outside the bounds of the C standard because of the use of asm(). 
The problem here is that the compiler assigning a 32-bit int into a 
64-bit register uses the shortest sequence possible (leaving the upper 
64 bits garbage), because the compiler assumes you correctly wrote the 
assembly to only use 32-bit operations on that register (which don't 
care about the upper bits).  By using an unsigned long (a 64-bit value), 
the compiler instead emits assembly to write the full 64-bit register 
value, rather than leaving the upper bits as garbage; and this matters 
because we are subsequently using all 64 bits of the register in a later 
operation.  We could also use a signed long, even long long, or written 
it as: (store ? 6ULL : 5ULL) instead of using a temporary variable.  The 
crux of the fix is that you have to tell asm() that you want a 64-bit 
value written (the unpatched (store ? 6 : 5) is only a 32-bit value), 
and not whether that value is signed or unsigned (since the 
representation of both 6 and 5 are the same regardless of whether the 
type being written into the register is signed or not).

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Farhan Ali]

On 05/03/2018 11:48 AM, Eric Blake wrote:
> On 05/03/2018 10:25 AM, Farhan Ali wrote:
>> On 05/02/2018 08:52 AM, Cornelia Huck wrote:
>>> We currently pass an integer as the subcode parameter. However,
>>> the upper bits of the register containing the subcode need to
>>> be 0, which is not guaranteed unless we explicitly specify the
>>> subcode to be an unsigned long value.
>>>
>>> Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 
>>> 308/6")
>>> Cc:qemu-stable@nongnu.org
>>> Signed-off-by: Cornelia Huck<cohuck@redhat.com>
>>
>> Sorry for my ignorance, but is there a C standard that says upper bits 
>> of an int is not guaranteed to be 0?
> 
> We're outside the bounds of the C standard because of the use of asm(). 
> The problem here is that the compiler assigning a 32-bit int into a 
> 64-bit register uses the shortest sequence possible (leaving the upper 
> 64 bits garbage), because the compiler assumes you correctly wrote the 
> assembly to only use 32-bit operations on that register (which don't 
> care about the upper bits).  By using an unsigned long (a 64-bit value), 
> the compiler instead emits assembly to write the full 64-bit register 
> value, rather than leaving the upper bits as garbage; and this matters 
> because we are subsequently using all 64 bits of the register in a later 
> operation.  We could also use a signed long, even long long, or written 
> it as: (store ? 6ULL : 5ULL) instead of using a temporary variable.  The 
> crux of the fix is that you have to tell asm() that you want a 64-bit 
> value written (the unpatched (store ? 6 : 5) is only a 32-bit value), 
> and not whether that value is signed or unsigned (since the 
> representation of both 6 and 5 are the same regardless of whether the 
> type being written into the register is signed or not).
> 

Thank you so much for the detailed explanation :).

I did not think about the instruction that will be used by the compiler 
to handle the values. Definitely learned something new!

Re: [Qemu-devel] [PATCH] s390-ccw: force diag 308 subcode to unsigned long

[Farhan Ali]

On 05/03/2018 11:44 AM, Cornelia Huck wrote:
> On Thu, 3 May 2018 11:25:08 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 05/02/2018 08:52 AM, Cornelia Huck wrote:
>>> We currently pass an integer as the subcode parameter. However,
>>> the upper bits of the register containing the subcode need to
>>> be 0, which is not guaranteed unless we explicitly specify the
>>> subcode to be an unsigned long value.
>>>
>>> Fixes: d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")
>>> Cc:qemu-stable@nongnu.org
>>> Signed-off-by: Cornelia Huck<cohuck@redhat.com>
>>
>> Sorry for my ignorance, but is there a C standard that says upper bits
>> of an int is not guaranteed to be 0?
> 
> The value (5 resp. 6) is small enough to fit into a regular integer,
> and the compiler generated a lhi for the load, which did not change any
> upper values that might have been in the register previously. Telling
> the compiler to treat the value as an unsigned long makes it generate a
> lghi.
> 

This makes sense now. Thanks for the explanation :)