Re: [Qemu-devel] [qemu-s390x] [PATCH v5 01/15] s390 vfio-ccw: Add bootindex property and IPLB data

Re: [Qemu-devel] [qemu-s390x] [PATCH v5 01/15] s390 vfio-ccw: Add bootindex property and IPLB data

[Cornelia Huck]

On Tue, 26 Mar 2019 13:49:41 +0100
Thomas Huth <thuth@redhat.com> wrote:

> On 13/03/2019 17.31, Jason J. Herne wrote:
> > Add bootindex property and iplb data for vfio-ccw devices. This allows us to
> > forward boot information into the bios for vfio-ccw devices.
> > 
> > Refactor s390_get_ccw_device() to return device type. This prevents us from
> > having to use messy casting logic in several places.
> > 
> > Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
> > Acked-by: Halil Pasic <pasic@linux.vnet.ibm.com>
> > Reviewed-by: Cornelia Huck <cohuck@redhat.com>
> > ---  

> [...]
> > diff --git a/include/hw/s390x/vfio-ccw.h b/include/hw/s390x/vfio-ccw.h
> > new file mode 100644
> > index 0000000..2fceaa2
> > --- /dev/null
> > +++ b/include/hw/s390x/vfio-ccw.h
> > @@ -0,0 +1,28 @@
> > +/*
> > + * vfio based subchannel assignment support
> > + *
> > + * Copyright 2017 IBM Corp.  
> 
> Maybe bump the year to 2019 ?

Not sure, the code is simply extracted and not changed, isn't it?

(If bumped, it probably should be 2017,2019.)

> 
> > + * Author(s): Dong Jia Shi <bjsdjshi@linux.vnet.ibm.com>
> > + *            Xiao Feng Ren <renxiaof@linux.vnet.ibm.com>
> > + *            Pierre Morel <pmorel@linux.vnet.ibm.com>
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or (at
> > + * your option) any later version. See the COPYING file in the top-level
> > + * directory.
> > + */
> > +
> > +#ifndef HW_VFIO_CCW_H
> > +#define HW_VFIO_CCW_H
> > +
> > +#include "hw/vfio/vfio-common.h"
> > +#include "hw/s390x/s390-ccw.h"
> > +#include "hw/s390x/ccw-device.h"
> > +
> > +#define TYPE_VFIO_CCW "vfio-ccw"
> > +#define VFIO_CCW(obj) \
> > +        OBJECT_CHECK(VFIOCCWDevice, (obj), TYPE_VFIO_CCW)
> > +
> > +#define TYPE_VFIO_CCW "vfio-ccw"
> > +typedef struct VFIOCCWDevice VFIOCCWDevice;
> > +
> > +#endif  
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [Qemu-devel] [qemu-s390x] [PATCH v5 11/15] s390-bios: cio error handling

[Jason J. Herne]

On 3/27/19 6:47 AM, Thomas Huth wrote:
> On 13/03/2019 17.31, Jason J. Herne wrote:
>> Add verbose error output for when unexpected i/o errors happen. This eases the
>> burden of debugging and reporting i/o errors. No error information is printed
>> in the success case, here is an example of what is output on error:
>>
>> cio device error
>>    ssid  : 0x0000000000000000
>>    cssid : 0x0000000000000000
>>    sch_no: 0x0000000000000000
>>
>> Interrupt Response Block Data:
>>      Function Ctrl : [Start]
>>      Activity Ctrl : [Start-Pending]
>>      Status Ctrl : [Alert] [Primary] [Secondary] [Status-Pending]
>>      Device Status : [Unit-Check]
>>      Channel Status :
>>      cpa=: 0x000000007f8d6038
>>      prev_ccw=: 0x0000000000000000
>>      this_ccw=: 0x0000000000000000
>> Eckd Dasd Sense Data (fmt 32-bytes):
>>      Sense Condition Flags :
>>      Residual Count     =: 0x0000000000000000
>>      Phys Drive ID      =: 0x000000000000009e
>>      low cyl address    =: 0x0000000000000000
>>      head addr & hi cyl =: 0x0000000000000000
>>      format/message     =: 0x0000000000000008
>>      fmt-dependent[0-7] =: 0x0000000000000004
>>      fmt-dependent[8-15]=: 0xe561282305082fff
>>      prog action code   =: 0x0000000000000016
>>      Configuration info =: 0x00000000000040e0
>>      mcode / hi-cyl     =: 0x0000000000000000
>>      cyl & head addr [0]=: 0x0000000000000000
>>      cyl & head addr [1]=: 0x0000000000000000
>>      cyl & head addr [2]=: 0x0000000000000000
>>
>> The Sense Data section is currently only printed for ECKD DASD.
>>
>> Signed-off-by: Jason J. Herne <jjherne@linux.ibm.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>> ---
>>   pc-bios/s390-ccw/cio.c  | 237 +++++++++++++++++++++++++++++++++++++++++++++++-
>>   pc-bios/s390-ccw/libc.h |  11 +++
>>   2 files changed, 247 insertions(+), 1 deletion(-)
>>
>> diff --git a/pc-bios/s390-ccw/cio.c b/pc-bios/s390-ccw/cio.c
>> index 58f63e4..54fd6ad 100644
>> --- a/pc-bios/s390-ccw/cio.c
>> +++ b/pc-bios/s390-ccw/cio.c
>> @@ -85,6 +85,228 @@ static bool irb_error(Irb *irb)
>>       return irb->scsw.dstat != (SCSW_DSTAT_DEVEND | SCSW_DSTAT_CHEND);
>>   }
>>   
>> +static void print_eckd_dasd_sense_data(SenseDataEckdDasd *sd)
>> +{
>> +    char msgline[512];
>> +
>> +    if (sd->config_info & 0x8000) {
>> +        sclp_print("Eckd Dasd Sense Data (fmt 24-bytes):\n");
>> +    } else {
>> +        sclp_print("Eckd Dasd Sense Data (fmt 32-bytes):\n");
>> +    }
>> +
>> +    strcat(msgline, "    Sense Condition Flags :");
>> +    if (sd->status[0] & SNS_STAT0_CMD_REJECT) {
>> +        strcat(msgline, " [Cmd-Reject]");
>> +    }
>> +    if (sd->status[0] & SNS_STAT0_INTERVENTION_REQ) {
>> +        strcat(msgline, " [Intervention-Required]");
>> +    }
>> +    if (sd->status[0] & SNS_STAT0_BUS_OUT_CHECK) {
>> +        strcat(msgline, " [Bus-Out-Parity-Check]");
>> +    }
>> +    if (sd->status[0] & SNS_STAT0_EQUIPMENT_CHECK) {
>> +        strcat(msgline, " [Equipment-Check]");
>> +    }
>> +    if (sd->status[0] & SNS_STAT0_DATA_CHECK) {
>> +        strcat(msgline, " [Data-Check]");
>> +    }
>> +    if (sd->status[0] & SNS_STAT0_OVERRUN) {
>> +        strcat(msgline, " [Overrun]");
>> +    }
>> +    if (sd->status[0] & SNS_STAT0_INCOMPL_DOMAIN) {
>> +        strcat(msgline, " [Incomplete-Domain]");
>> +    }
>> +
>> +    if (sd->status[1] & SNS_STAT1_PERM_ERR) {
>> +        strcat(msgline, " [Permanent-Error]");
>> +    }
>> +    if (sd->status[1] & SNS_STAT1_INV_TRACK_FORMAT) {
>> +        strcat(msgline, " [Invalid-Track-Fmt]");
>> +    }
>> +    if (sd->status[1] & SNS_STAT1_EOC) {
>> +        strcat(msgline, " [End-of-Cyl]");
>> +    }
>> +    if (sd->status[1] & SNS_STAT1_MESSAGE_TO_OPER) {
>> +        strcat(msgline, " [Operator-Msg]");
>> +    }
>> +    if (sd->status[1] & SNS_STAT1_NO_REC_FOUND) {
>> +        strcat(msgline, " [No-Record-Found]");
>> +    }
>> +    if (sd->status[1] & SNS_STAT1_FILE_PROTECTED) {
>> +        strcat(msgline, " [File-Protected]");
>> +    }
>> +    if (sd->status[1] & SNS_STAT1_WRITE_INHIBITED) {
>> +        strcat(msgline, " [Write-Inhibited]");
>> +    }
>> +    if (sd->status[1] & SNS_STAT1_IMPRECISE_END) {
>> +        strcat(msgline, " [Imprecise-Ending]");
>> +    }
>> +
>> +    if (sd->status[2] & SNS_STAT2_REQ_INH_WRITE) {
>> +        strcat(msgline, " [Req-Inhibit-Write]");
>> +    }
>> +    if (sd->status[2] & SNS_STAT2_CORRECTABLE) {
>> +        strcat(msgline, " [Correctable-Data-Check]");
>> +    }
>> +    if (sd->status[2] & SNS_STAT2_FIRST_LOG_ERR) {
>> +        strcat(msgline, " [First-Error-Log]");
>> +    }
>> +    if (sd->status[2] & SNS_STAT2_ENV_DATA_PRESENT) {
>> +        strcat(msgline, " [Env-Data-Present]");
>> +    }
>> +    if (sd->status[2] & SNS_STAT2_IMPRECISE_END) {
>> +        strcat(msgline, " [Imprecise-End]");
>> +    }
> 
> GCC 8 aborts here:
> 
> pc-bios/s390-ccw/cio.c: In function ‘do_cio’:
> pc-bios/s390-ccw/cio.c:146:19: error: array subscript 2 is above array
> bounds of ‘uint8_t[2]’ {aka ‘unsigned char[2]’} [-Werror=array-bounds]
>       if (sd->status[2] & SNS_STAT2_REQ_INH_WRITE) {
>           ~~~~~~~~~~^~~
> 
>   Thomas
> 

This is a result of incomplete refactoring. We renamed status byte 0 to common_status and 
I never updated the references. Thanks for catching this. Myself and my compiler missed it.

-- 
-- Jason J. Herne (jjherne@linux.ibm.com)

Re: [Qemu-devel] [qemu-s390x] [PATCH v5 15/15] s390-bios: Support booting from real dasd device

[Jason J. Herne]

On 3/29/19 4:33 AM, Thomas Huth wrote:
> On 13/03/2019 17.31, Jason J. Herne wrote:
> 
> s/tansfers/transfers/
> 

Will fix this, and the other typos you've pointed out.

...
>> +static char prefix_page[PAGE_SIZE * 2]
>> +            __attribute__((__aligned__(PAGE_SIZE * 2)));
>> +
>> +static void enable_prefixing(void)
>> +{
>> +    memcpy(&prefix_page, (void *)0, 4096);
> 
> You could use the "lowcore" variable from s390-arch.h here instead of
> "(void *)0", I guess.
>

Agreed.

>> +    set_prefix(ptr2u32(&prefix_page));
>> +}
>> +
>> +static void disable_prefixing(void)
>> +{
>> +    set_prefix(0);
>> +    /* Copy io interrupt info back to low core */
>> +    memcpy((void *)0xB8, prefix_page + 0xB8, 12);
> 
> Maybe use &lowcore->subchannel_id instead of 0xB8 ? ... not sure whether
> that's nicer here, though...
> 

I think it is nicer using the named field. I Will make that change.

Note: We need to keep the (void*) cast to prevent a compiler warning about discarding 
const qualifier.

>> +static void ipl1_fixup(void)
>> +{
>> +    Ccw0 *ccwSeek = (Ccw0 *) 0x08;
>> +    Ccw0 *ccwSearchID = (Ccw0 *) 0x10;
>> +    Ccw0 *ccwSearchTic = (Ccw0 *) 0x18;
>> +    Ccw0 *ccwRead = (Ccw0 *) 0x20;
>> +    CcwSeekData *seekData = (CcwSeekData *) 0x30;
>> +    CcwSearchIdData *searchData = (CcwSearchIdData *) 0x38;
>> +
>> +    /* move IPL1 CCWs to make room for CCWs needed to locate record 2 */
>> +    memcpy(ccwRead, (void *)0x08, 16);
> 
> lowcore->ccw1 ?
> 

All the other CCWs in this section still need to use the numeric memory references. I 
think it makes it slightly more confusing to switch just the one to using the struct. For 
this reason I prefer it the way it is, if you're okay with that.

...
>> +static void lpsw(void *psw_addr)
>> +{
>> +    PSWLegacy *pswl = (PSWLegacy *) psw_addr;
>> +
>> +    pswl->mask |= PSW_MASK_EAMODE;   /* Force z-mode */
>> +    pswl->addr |= PSW_MASK_BAMODE;
>> +    asm volatile("  llgtr 0,0\n llgtr 1,1\n"     /* Some OS's expect to be */
>> +                 "  llgtr 2,2\n llgtr 3,3\n"     /* in 32-bit mode. Clear  */
>> +                 "  llgtr 4,4\n llgtr 5,5\n"     /* high part of regs to   */
>> +                 "  llgtr 6,6\n llgtr 7,7\n"     /* avoid messing up       */
>> +                 "  llgtr 8,8\n llgtr 9,9\n"     /* instructions that work */
>> +                 "  llgtr 10,10\n llgtr 11,11\n" /* in both addressing     */
>> +                 "  llgtr 12,12\n llgtr 13,13\n" /* modes, like servc.     */
>> +                 "  llgtr 14,14\n llgtr 15,15\n"
>> +                 "  lpsw %0\n"
>> +                 : : "Q" (*pswl) : "cc");
>> +}
> 
> Have you tried to use jump_to_low_kernel() already? ... it might be
> cleaner to do the diag 0x308 reset here, too, to avoid that some part of
> the machine is in an unexpected state...
> 
>   Thomas

I had not tried jump_to_low_kernel() until just now. It *does* seem to work... and 
eliminates the need for the manual register clearing. I assume the diag 308 reset baked 
into the jump_to_low_kernel() is responsible for that? I guess switching to 
jump_to_low_kernel() would be a good thing ... I'll admit I'm slightly uneasy about it 
since I've been testing my way for so long :) But jump_to_low_kernel() works for all of my 
manual test cases.

-- 
-- Jason J. Herne (jjherne@linux.ibm.com)

Re: [Qemu-devel] [qemu-s390x] [PATCH v5 15/15] s390-bios: Support booting from real dasd device

[Thomas Huth]

On 01/04/2019 17.35, Jason J. Herne wrote:
> On 3/29/19 4:33 AM, Thomas Huth wrote:
>> On 13/03/2019 17.31, Jason J. Herne wrote:
[...]
>>> +static void ipl1_fixup(void)
>>> +{
>>> +    Ccw0 *ccwSeek = (Ccw0 *) 0x08;
>>> +    Ccw0 *ccwSearchID = (Ccw0 *) 0x10;
>>> +    Ccw0 *ccwSearchTic = (Ccw0 *) 0x18;
>>> +    Ccw0 *ccwRead = (Ccw0 *) 0x20;
>>> +    CcwSeekData *seekData = (CcwSeekData *) 0x30;
>>> +    CcwSearchIdData *searchData = (CcwSearchIdData *) 0x38;
>>> +
>>> +    /* move IPL1 CCWs to make room for CCWs needed to locate record
>>> 2 */
>>> +    memcpy(ccwRead, (void *)0x08, 16);
>>
>> lowcore->ccw1 ?
>>
> 
> All the other CCWs in this section still need to use the numeric memory
> references. I think it makes it slightly more confusing to switch just
> the one to using the struct. For this reason I prefer it the way it is,
> if you're okay with that.

Fine for me.

> ...
>>> +static void lpsw(void *psw_addr)
>>> +{
>>> +    PSWLegacy *pswl = (PSWLegacy *) psw_addr;
>>> +
>>> +    pswl->mask |= PSW_MASK_EAMODE;   /* Force z-mode */
>>> +    pswl->addr |= PSW_MASK_BAMODE;
>>> +    asm volatile("  llgtr 0,0\n llgtr 1,1\n"     /* Some OS's expect
>>> to be */
>>> +                 "  llgtr 2,2\n llgtr 3,3\n"     /* in 32-bit mode.
>>> Clear  */
>>> +                 "  llgtr 4,4\n llgtr 5,5\n"     /* high part of
>>> regs to   */
>>> +                 "  llgtr 6,6\n llgtr 7,7\n"     /* avoid messing
>>> up       */
>>> +                 "  llgtr 8,8\n llgtr 9,9\n"     /* instructions
>>> that work */
>>> +                 "  llgtr 10,10\n llgtr 11,11\n" /* in both
>>> addressing     */
>>> +                 "  llgtr 12,12\n llgtr 13,13\n" /* modes, like
>>> servc.     */
>>> +                 "  llgtr 14,14\n llgtr 15,15\n"
>>> +                 "  lpsw %0\n"
>>> +                 : : "Q" (*pswl) : "cc");
>>> +}
>>
>> Have you tried to use jump_to_low_kernel() already? ... it might be
>> cleaner to do the diag 0x308 reset here, too, to avoid that some part of
>> the machine is in an unexpected state...
> 
> I had not tried jump_to_low_kernel() until just now. It *does* seem to
> work... and eliminates the need for the manual register clearing. I
> assume the diag 308 reset baked into the jump_to_low_kernel() is
> responsible for that? I guess switching to jump_to_low_kernel() would be
> a good thing ... I'll admit I'm slightly uneasy about it since I've been
> testing my way for so long :) But jump_to_low_kernel() works for all of
> my manual test cases.

I originally used lpsw in the netboot code, too, but there was the risk
that some device activity in the background could confuse the OS kernel
later ... so Christian pointed me to the diag diag 308 reset code, which
is certainly the better way to hand over the control to the OS. Thus if
it passes your test cases, I'd say let's try to do it the better way
here right from the start and switch to jump_to_low_kernel() instead.

 Thomas