Re: [Qemu-devel] [PATCH v2 1/4] s390x/css: introduce css data stream

[Pierre Morel <pmorel@linux.vnet.ibm.com>]

On 19/09/2017 11:53, Cornelia Huck wrote:
> On Tue, 19 Sep 2017 11:11:27 +0200
> Pierre Morel <pmorel@linux.vnet.ibm.com> wrote:
> 
>> On 13/09/2017 13:50, Halil Pasic wrote:
>>> This is a preparation for introducing handling for indirect data
>>> addressing and modified indirect data addressing (CCW). Here we introduce
>>> an interface which should make the addressing scheme transparent for the
>>> client code. Here we implement only the basic scheme (no IDA or MIDA).
>>>
>>> Signed-off-by: Halil Pasic <pasic@linux.vnet.ibm.com>
>>> ---
>>>    hw/s390x/css.c         | 55 +++++++++++++++++++++++++++++++++++++++++
>>>    include/hw/s390x/css.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 122 insertions(+)
> 
>>> diff --git a/include/hw/s390x/css.h b/include/hw/s390x/css.h
>>> index 0653d3c9be..79acaf99b7 100644
>>> --- a/include/hw/s390x/css.h
>>> +++ b/include/hw/s390x/css.h
>>> @@ -75,6 +75,29 @@ typedef struct CMBE {
>>>        uint32_t reserved[7];
>>>    } QEMU_PACKED CMBE;
>>>
>>> +typedef enum CcwDataStreamOp {
>>> +    CDS_OP_R = 0,
>>> +    CDS_OP_W = 1,
>>> +    CDS_OP_A = 2
>>> +} CcwDataStreamOp;
>>> +
>>> +/** normal usage is via SuchchDev.cds instead of instantiating */
>>> +typedef struct CcwDataStream {
>>> +#define CDS_F_IDA   0x01
>>> +#define CDS_F_MIDA  0x02
>>> +#define CDS_F_I2K   0x04
>>> +#define CDS_F_C64   0x08
>>> +#define CDS_F_STREAM_BROKEN  0x80
>>> +    uint8_t flags;
>>> +    uint8_t at_idaw;
>>> +    uint16_t at_byte;
>>> +    uint16_t count;
>>> +    uint32_t cda_orig;
>>> +    int (*op_handler)(struct CcwDataStream *cds, void *buff, int len,
>>> +                      CcwDataStreamOp op);
>>
>> I would have prefer one handler per operation instead of a operation
>> parameter.
>>
>> Is it possible to change the name of the "buf" argument to "arg".
>> I just think of the foollowing:
>> If one day we do not want to gather all IDAs into a single buffer but
>> keep them split, we can have something like an array of buffers as argument.
> 
> It should be possible to change the internal format should we decide to
> want to do something different, so I'll leave that as-is for now.
> 
>>
>>
>>
>>> +    hwaddr cda;
>>> +} CcwDataStream;
>>> +
> 
>> otherwise, LGTM
> 
> Is that an ack? :)
> 
> 

Reviewed-by: Pierre Morel<pmorel@linux.vnet.ibm.com>


-- 
Pierre Morel
Linux/KVM/QEMU in Böblingen - Germany