Re: [Qemu-devel] [qemu-s390x] [PATCH 10/15] s390-bios: Support for running format-0/1 channel programs

[Jason J. Herne]

On 1/9/19 1:34 PM, Cornelia Huck wrote:
> On Wed, 9 Jan 2019 13:10:26 -0500
> "Jason J. Herne" <address@hidden> wrote:
>
> > On 1/7/19 2:02 PM, Jason J. Herne wrote:
> > > > > +
> > > > >    /*
> > > > >     * sense-id response buffer layout
> > > > >     */
> > > > > @@ -205,6 +265,61 @@ typedef struct senseid {
> > > > >        struct ciw ciw[62];
> > > > >    }  __attribute__ ((packed, aligned(4))) SenseId;
> > > > > +/* architected values for first sense byte */
> > > > > +#define SNS0_CMD_REJECT         0x80
> > > > > +#define SNS0_INTERVENTION_REQ   0x40
> > > > > +#define SNS0_BUS_OUT_CHECK      0x20
> > > > > +#define SNS0_EQUIPMENT_CHECK    0x10
> > > > > +#define SNS0_DATA_CHECK         0x08
> > > > > +#define SNS0_OVERRUN            0x04
> > > > > +#define SNS0_INCOMPL_DOMAIN     0x01
> > > >
> > > > IIRC, only byte 0 is device independent, and the others below are
> > > > (ECKD) dasd specific?
> > > >   
> > > > > +
> > > > > +/* architectured values for second sense byte */
> > > > > +#define SNS1_PERM_ERR           0x80
> > > > > +#define SNS1_INV_TRACK_FORMAT   0x40
> > > > > +#define SNS1_EOC                0x20
> > > > > +#define SNS1_MESSAGE_TO_OPER    0x10
> > > > > +#define SNS1_NO_REC_FOUND       0x08
> > > > > +#define SNS1_FILE_PROTECTED     0x04
> > > > > +#define SNS1_WRITE_INHIBITED    0x02
> > > > > +#define SNS1_INPRECISE_END      0x01
> > > > > +
> > > > > +/* architectured values for third sense byte */
> > > > > +#define SNS2_REQ_INH_WRITE      0x80
> > > > > +#define SNS2_CORRECTABLE        0x40
> > > > > +#define SNS2_FIRST_LOG_ERR      0x20
> > > > > +#define SNS2_ENV_DATA_PRESENT   0x10
> > > > > +#define SNS2_INPRECISE_END      0x04
> > > > > +
> > > > > +/* 24-byte Sense fmt/msg codes */
> > > > > +#define SENSE24_FMT_PROG_SYS    0x0
> > > > > +#define SENSE24_FMT_EQUIPMENT   0x2
> > > > > +#define SENSE24_FMT_CONTROLLER  0x3
> > > > > +#define SENSE24_FMT_MISC        0xF
> > > > > +
> > > > > +#define SENSE24_FMT0_MSG_RESET_NOTIFICATION 0x16
> > > > > +
> > > > > +/* basic sense response buffer layout */
> > > > > +typedef struct senseData {
> > > > > +    uint8_t status[3];
> > > > > +    uint8_t res_count;
> > > > > +    uint8_t phys_drive_id;
> > > > > +    uint8_t low_cyl_addr;
> > > > > +    uint8_t head_high_cyl_addr;
> > > > > +    uint8_t fmt_msg;
> > > > > +    uint64_t fmt_dependent_info[2];
> > > > > +    uint8_t reserved;
> > > > > +    uint8_t program_action_code;
> > > > > +    uint16_t config_info;
> > > > > +    uint8_t mcode_hicyl;
> > > > > +    uint8_t cyl_head_addr[3];
> > > > > +}  __attribute__ ((packed, aligned(4))) SenseData;
> > > >
> > > > And this looks _really_ dasd specific.
> > > >   
> > >
> > > Yep, I glossed over those details while I was furiously tracking down the reset 
> > > bug. I'll
> > > take a look at redesigning this.
> >
> > All of my information for creating these data structures came from an internal 
> > ECKD DASD
> > reference. There are probably some things that could stand a bit of cleanup or 
> > renaming.
> > Aside from that, considering this is in a DASD only (ECKD DASD only at the 
> > moment) code
> > path are you okay with my renaming the struct to senseDataECKD or something 
> > similar?
>
> Renaming this makes sense.
>
> > I'm not sure what value there is in abstracting sense at the moment. I'm not 
> > even sure
> > what other device's sense data looks like. Since my description of the SENSE 
> > CCW comes
> > from an ECKD reference I have not been able to verify any areas of the data 
> > that are
> > common across device types. Thoughts?
>
> There's SA22-7204-01 ("Common I/O Device Commands"), which is from 1992
> (this is what I have on my disk -- is there anything newer?). It
> specifies what bits 0-5 of byte 0 mean and states that bytes 1-31 are
> optional and device-specific.
>
> Maybe some other bits have been specified after 1992, but I have not
> come across documentation for them.

So everything I'm finding (including SA22-7204-01) only speaks of the first 6 bits being 
device agnostic. It is odd that the entire first byte would not have been reserved. 
Perhaps it is and I'm just not looking at the right documentation. My thought process is 
to have the basic_Sense function change to this:

- void basic_sense(SubChannelId schid, SenseData *sd);
+ void basic_sense(SubChannelId schid, void *sense_data);

The caller is free to map it however they see fit depending on the device type they are 
sensing. I'll rename struct senseData to struct senseDataEckdDasd, break out the first 
status byte and define common constants for the first 6 bits of that byte. This should 
clear up the confusion. I'll add a small comment to explain. Does this sound good?


--
-- Jason J. Herne (address@hidden)