Re: [qemu-s390x] [Qemu-devel] [RFC PATCH v2 1/3] s390x/ccs: add ccw-testdev emulated device

[Halil Pasic]

On 12/07/2017 12:59 PM, Cornelia Huck wrote:
> On Thu, 7 Dec 2017 07:33:19 +0100
> Thomas Huth <address@hidden> wrote:
> 
>> On 08.11.2017 17:54, Halil Pasic wrote:
> 
>>> +static ccw_cb_t get_ccw_cb(CcwTestDevOpMode op_mode)
>>> +{
>>> +    switch (op_mode) {
>>> +    case OP_MODE_FIB:
>>> +        return ccw_testdev_ccw_cb_mode_fib;
>>> +    case OP_MODE_NOP:
>>> +    default:
>>> +        return ccw_testdev_ccw_cb_mode_nop;  
>>
>> Do we really want to use "nop" for unknown modes? Or should there rather
>> be a ccw_testdev_ccw_cb_mode_error instead, too?
> 
> I like the idea of an error mode.

What would be the benefit of the error mode? The idea is that
the tester in the guest has a certain set of tests implemented
each requiring certain behavior from the device. This behavior
is represented by the mode.

If the device does not support the mode, the set of tests can't
be executed meaningfully. The only option is either ignore them
(and preferably report them as ignored), or fail them (not soo
good in my opinion).

The in guest tester should simply iterate over it test sets
and try to select the mode the test set (or suite in other
terminology) requires. If selecting the mode fails, than
means you are working with an old ccw-testdev.

> 
> Related: Should the device have a mechanism to report the supported
> modes?
> 

I don't see the value. See above. I think the set mode operation
reporting failure is sufficient.

But if you have something in mind, please do tell. I'm open.

>>
>>> +    }
>>> +}
> 
> (...)
> 
>>> +/* TODO This is the out-of-band variant. We may want to get rid of it */  
>>
>> I agree, this should rather go away in the final version.
> 
> I'm not sure that the in-band variant with its magic buffer value is
> superior to this version using a dedicated hypercall.
> 

I have a similar todo comment for the in-band variant. It is basically
up to us to decide which one do we want to keep. It's been a while since
I've looked at this, but AFAIR the out-of-band appears cleaner.

>>
>>> +static int set_mode_diag(const uint64_t *args)
>>> +{
>>> +    uint64_t subch_id = args[0];
>>> +    uint64_t op_mode = args[1];
>>> +    SubchDev *sch;
>>> +    CcwTestDevDevice *dev;
>>> +    int cssid, ssid, schid, m;
>>> +
>>> +    if (ioinst_disassemble_sch_ident(subch_id, &m, &cssid, &ssid, &schid)) 
>>> {
>>> +        return -EINVAL;
>>> +    }
>>> +    sch = css_find_subch(m, cssid, ssid, schid);
>>> +    if (!sch || !css_subch_visible(sch)) {
>>> +        return -EINVAL;
>>> +    }
>>> +    dev = CCW_TESTDEV(sch->driver_data);
>>> +    if (dev->op_mode_locked) {
>>> +        return op_mode == dev->op_mode ? 0 : -EINVAL;
>>> +    }
>>> +    dev->op_mode = op_mode;
>>> +    sch->ccw_cb = get_ccw_cb(dev->op_mode);
>>> +    return 0;
>