Re: [Qemu-devel] [PATCH 1/2] s390x/ccs: add ccw-tester emulated device

[Halil Pasic]

On 09/14/2017 04:26 PM, Cornelia Huck wrote:
> On Wed, 13 Sep 2017 15:27:51 +0200
> Halil Pasic <address@hidden> wrote:
> 
>> Add a fake device meant for testing the correctness of our css emulation.
>>
>> What we currently have is writing a Fibonacci sequence of uint32_t to the
>> device via ccw write. The write is going to fail if it ain't a Fibonacci
>> and indicate a device exception in scsw together with the proper residual
>> count.
>>
>> Of course lot's of invalid inputs (besides basic data processing) can be
>> tested with that as well.
>>
>> Usage:
>> 1) fire up a qemu with something like -device ccw-tester,devno=fe.0.0001
>>    on the command line
>> 2) exercise the device from the guest
>>
>> Signed-off-by: Halil Pasic <address@hidden>
>> ---
>>
>> Depends on the series 'add CCW indirect data access support'
>>
>> ---
>>  hw/s390x/Makefile.objs |   1 +
>>  hw/s390x/ccw-tester.c  | 179 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 180 insertions(+)
>>  create mode 100644 hw/s390x/ccw-tester.c
>>
> 
>> +static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
>> +{
>> +    CcwTesterDevice *d = sch->driver_data;
>> +    bool is_fib = true;
>> +    uint32_t sum;
>> +    int ret = 0;
>> +
>> +    ccw_dstream_init(&sch->cds, &ccw, &sch->orb);
>> +    d->fib.next = 0;
>> +    while (ccw_dstream_avail(&sch->cds) > 0) {
>> +        ret = ccw_dstream_read(&sch->cds,
>> +                               d->fib.ring[abs_to_ring(d->fib.next)]);
>> +        if (ret) {
>> +            error(0, -ret, "fib");
>> +            break;
>> +        }
>> +        if (d->fib.next > 2) {
>> +            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
>> +                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
>> +            is_fib = sum ==  d->fib.ring[abs_to_ring(d->fib.next)];
> 
> This is not endian-safe (noticed while testing on my laptop). Trivial
> to fix:
> 
> diff --git a/hw/s390x/ccw-tester.c b/hw/s390x/ccw-tester.c
> index c8017818c4..a425daaa34 100644
> --- a/hw/s390x/ccw-tester.c
> +++ b/hw/s390x/ccw-tester.c
> @@ -58,9 +58,9 @@ static int  ccw_tester_write_fib(SubchDev *sch, CCW1 ccw)
>              break;
>          }
>          if (d->fib.next > 2) {
> -            sum = (d->fib.ring[abs_to_ring(d->fib.next - 1)]
> -                  + d->fib.ring[abs_to_ring(d->fib.next - 2)]);
> -            is_fib = sum ==  d->fib.ring[abs_to_ring(d->fib.next)];
> +            sum = be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next - 1)])
> +                + be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next - 2)]);
> +            is_fib = sum == 
> be32_to_cpu(d->fib.ring[abs_to_ring(d->fib.next)]);
>              if (!is_fib) {
>                  break;
>              }
> 

Nod. 

>> +            if (!is_fib) {
>> +                break;
>> +            }
>> +        }
>> +        ++(d->fib.next);
>> +    }
>> +    if (!is_fib) {
>> +        sch->curr_status.scsw.ctrl &= ~SCSW_ACTL_START_PEND;
>> +        sch->curr_status.scsw.ctrl |= SCSW_STCTL_PRIMARY |
>> +                                      SCSW_STCTL_SECONDARY |
>> +                                      SCSW_STCTL_ALERT |
>> +                                      SCSW_STCTL_STATUS_PEND;
>> +        sch->curr_status.scsw.count = ccw_dstream_residual_count(&sch->cds);
>> +        sch->curr_status.scsw.cpa = sch->channel_prog + 8;
>> +        sch->curr_status.scsw.dstat =  SCSW_DSTAT_UNIT_EXCEP;
>> +        return -EIO;
>> +    }
>> +    return ret;
>> +}
>> +
> (...)
>> +static Property ccw_tester_properties[] = {
>> +    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
>> +                        0x3831),
> 
> 0x4711 would be nice :)

I don't understand the joke/pun/whatever if there is one,
but I'm fine with changing this too.

> 
> If we want to follow up on that testdev idea (and I think we should),
> it might make sense to have a proper type reserve to prevent accidental
> clashes.

I agree. Although I would still keep the cu_type configurable,
because it might make sense to test a particular 'real' driver
(and not a test driver like here). I haven't really thought
this through, but it was an idea I had while agonizing over
not having a proper type reserved.

I suppose you did something like that for virtio, or? I'm in dark
when it comes to the question what process do we/I have to go to
get a type,for example 0x4711, reserved.

> 
> (Or is there already something reserved for "hypervisor use" or
> whatever?)

Not that I know. I can't recall encountering a list of reserved
types. Honestly I've hoped to leverage your experience (again
because of virtio-ccw).

> 
>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
>> +                       0x98),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
> 
> IIUC, pci-testdev provides some unit tests to testers (like kvm-tests)
> itself. This might be an idea to follow up on for ccw as well.
> 

I've just had a first look at pci-testdev, and it does appear to be a similar
concept. 

> There's quite some potential in this. We may want to make this a
> permanent addition.
> 

I'm happy to contribute! I'm not sure how shall we proceed though.
Maybe with making a todo list?