| Message ID | 20180502125221.4877-1-cohuck@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390-ccw: force diag 308 subcode to unsigned long | expand |
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; > } >
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>
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>
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
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.
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).
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!
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 :)
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; }
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(-)