Re: [Qemu-devel] [PATCH] s390-ccw: Fix alignment for CCW1

[Cornelia Huck]

On Mon, 28 Aug 2017 09:52:08 -0400
Farhan Ali <address@hidden> wrote:

> On 08/28/2017 09:24 AM, Christian Borntraeger wrote:
> >
> >
> >
> > On 08/28/2017 03:18 PM, Farhan Ali wrote:  
> >>
> >>
> >> On 08/28/2017 09:06 AM, Cornelia Huck wrote:  
> >>> On Mon, 28 Aug 2017 08:56:42 -0400
> >>> Farhan Ali <address@hidden> wrote:
> >>>  
> >>>> On 08/28/2017 04:22 AM, Cornelia Huck wrote:  
> >>>>> On Fri, 25 Aug 2017 11:05:30 -0400
> >>>>> Farhan Ali <address@hidden> wrote:
> >>>>>  
> >>>>>> On 08/25/2017 10:04 AM, Cornelia Huck wrote:  
> >>>>>>> On Fri, 25 Aug 2017 09:24:46 -0400
> >>>>>>> Farhan Ali <address@hidden> wrote:
> >>>>>>>  
> >>>>>>>> The commit 198c0d1f9df8c4 s390x/css: check ccw address validity
> >>>>>>>> exposes an alignment issue in ccw bios.
> >>>>>>>>
> >>>>>>>> According to PoP the CCW must be doubleword aligned. Let's fix
> >>>>>>>> this in the bios.
> >>>>>>>>
> >>>>>>>> Cc: address@hidden
> >>>>>>>> Signed-off-by: Farhan Ali <address@hidden>
> >>>>>>>> Reviewed-by: Halil Pasic <address@hidden>
> >>>>>>>> Reviewed-by: Eric Farman <address@hidden>
> >>>>>>>> Acked-by: Christian Borntraeger <address@hidden>
> >>>>>>>> ---
> >>>>>>>>  pc-bios/s390-ccw/cio.h | 2 +-
> >>>>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>>
> >>>>>>>> diff --git a/pc-bios/s390-ccw/cio.h b/pc-bios/s390-ccw/cio.h
> >>>>>>>> index f5b4549..55eaeee 100644
> >>>>>>>> --- a/pc-bios/s390-ccw/cio.h
> >>>>>>>> +++ b/pc-bios/s390-ccw/cio.h
> >>>>>>>> @@ -133,7 +133,7 @@ struct ccw1 {
> >>>>>>>>      __u8 flags;
> >>>>>>>>      __u16 count;
> >>>>>>>>      __u32 cda;
> >>>>>>>> -} __attribute__ ((packed));
> >>>>>>>> +} __attribute__ ((packed, aligned(8)));
> >>>>>>>>
> >>>>>>>>  #define CCW_FLAG_DC              0x80
> >>>>>>>>  #define CCW_FLAG_CC              0x40  
> >>>>>>>
> >>>>>>> Currently testing.
> >>>>>>>
> >>>>>>> This looks obviously right, but did you figure out what the (probably
> >>>>>>> unrelated) other failure was?
> >>>>>>>  
> >>>>>>
> >>>>>> That is still under investigation, for some reason it only fails for an
> >>>>>> LDL DASD and it works for SCSIs and CDL DASD.  
> >>>>>
> >>>>> Which are the symptoms of the failure? I'd like to understand this
> >>>>> before I update the (currently working by accident) bios with an
> >>>>> updated version.
> >>>>>
> >>>>> I'll just apply the patch for now.
> >>>>>  
> >>>>
> >>>> Well it's seems like the failure for LDL DASD could be a disk setup
> >>>> failure. We tried the test on a different environment with LDL disks and
> >>>> everything worked fine with the patch applied.  
> >>>
> >>> Odd that it breaks after this change, though. Do you get command
> >>> rejects, or what happens?
> >>>  
> >>
> >> It's the alignment of the CCW which causes the problem.
> >>
> >> The exact error message when starting the guest was:
> >>
> >> ! No virtio device found !
> >>
> >> Since it worked for SCSI and CDL, and failed for LDL disks on that 
> >> particular system, we are not really sure what caused the failure.
> >> Debugging it further showed the CCW for LDL disks were not aligned at 
> >> double word boundary.

This is really, really odd, as the low-level ccw code is the same for
any disk type...

> >>
> >> Trying the test on a different system with LDL disks worked fine, with the 
> >> aligned(8) fix.  
> >
> > Do you happen to have an old s390-ccw.img laying around in the test folder? 
> > QEMU might pick up
> > this one (e.g. when calling it without libvirt from the command line).
> >  
> I explicitly mention the bios to use with '-bios' option and pick up the 
> latest bios. Without the aligned fix I see the error and with the fix it 
> works fine.

Wait, so the fix fixes it? Or am I confused now?