s390-ccw: force diag 308 subcode to unsigned long
PATCH 1/1 s390-ccw: force diag 308 subcode to unsigned long
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.
--- pc-bios/s390-ccw/iplb.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
pc-bios/s390-ccw/iplb.h +2 -1
@@ -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 ; asmvolatile ("diag %0,%2,0x308\n" :"+d" (addr),"+d" (rc) - :"d" (store ?6 :5 ) + :"d" (subcode) :"memory" ,"cc" );return rc ==0x01 ; }
Signature · 1 line
2.14.3
Quoted 17 lines · 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")Signed-off-by Cornelia Huck <[email protected]>--- pc-bios/s390-ccw/iplb.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)pc-bios/s390-ccw/iplb.h +0
-0
pc-bios/s390-ccw/iplb.h static inline bool manage_iplb(IplParameterBlock *iplb, bool store) · line 108
Quoted 9 lines
pc-bios/s390-ccw/iplb.h +2
-1@@ -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 ; asmvolatile ("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.
Quoted 4 lines
pc-bios/s390-ccw/iplb.h +0
-0: "memory", "cc");return rc ==0x01 ; }
Quoted 19 lines · 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")Signed-off-by Cornelia Huck <[email protected]>--- pc-bios/s390-ccw/iplb.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)pc-bios/s390-ccw/iplb.h +0
-0@@ -101,10 +101,11 @@ static inline bool manage_iplb(IplParameterBlock *iplb, bool store) { register unsigned long addr asm("0") = (unsigned long) iplb;
pc-bios/s390-ccw/iplb.h static inline bool manage_iplb(IplParameterBlock *iplb, bool store) · line 111
Quoted 10 lines
pc-bios/s390-ccw/iplb.h +2
-1register unsigned long rc asm("1") = 0; +unsigned long subcode = store ?6 :5 ; asmvolatile ("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.
Quoted 20 lines · 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")Signed-off-by Cornelia Huck <[email protected]>--- pc-bios/s390-ccw/iplb.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)pc-bios/s390-ccw/iplb.h +0
-0@@ -101,10 +101,11 @@ static inline bool manage_iplb(IplParameterBlock *iplb, bool store) { register unsigned long addr asm("0") = (unsigned long) iplb;
pc-bios/s390-ccw/iplb.h static inline bool manage_iplb(IplParameterBlock *iplb, bool store) · line 111
Quoted 10 lines
pc-bios/s390-ccw/iplb.h +2
-1register unsigned long rc asm("1") = 0; +unsigned long subcode = store ?6 :5 ; asmvolatile ("diag %0,%2,0x308\n" :"+d" (addr),"+d" (rc) - :"d" (store ?6 :5 ) + :"d" (subcode) :"memory" ,"cc" );return rc ==0x01 ; }
Signature · 3 lines
Thanks, David / dhildenb
Quoted 8 lines · 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")Signed-off-by Cornelia Huck<[email protected]>
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
Quoted 6 lines · On Thu, 3 May 2018 11:25:08 -0400 Farhan Ali <[email protected]> wrote:
Quoted 5 lines · 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.
Quoted 6 lines
Quoted 3 lines
Fixes d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")Signed-off-by Cornelia Huck<[email protected]>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.
Quoted 13 lines · Cornelia Huck
Quoted 10 lines · On Thu, 3 May 2018 11:25:08 -0400 Farhan Ali <[email protected]> wrote:
Quoted 8 lines · 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")Signed-off-by Cornelia Huck<[email protected]>
Quoted 9 lines
Quoted 2 lines
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 :)
Quoted 11 lines · Farhan Ali
Quoted 8 lines · 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")Signed-off-by Cornelia Huck<[email protected]>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).
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Quoted 7 lines · Eric Blake
Quoted 6 lines · Farhan Ali
Quoted 5 lines · 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.
Quoted 8 lines
Quoted 5 lines
Quoted 3 lines
Fixes d046c51dad3 ("pc-bios/s390-ccw: Get device address via diag 308/6")Signed-off-by Cornelia Huck<[email protected]>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 tellasm()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!