[1/2] s390x/ccs: add ccw-tester emulated device

Message ID 20170913132752.8484-2-pasic@linux.vnet.ibm.com
State New
Headers show
Series
  • tests for CCW IDA
Related show

Commit Message

Halil Pasic Sept. 13, 2017, 1:27 p.m.
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 <pasic@linux.vnet.ibm.com>
---

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

Comments

Cornelia Huck Sept. 14, 2017, 2:26 p.m. | #1
On Wed, 13 Sep 2017 15:27:51 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> 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 <pasic@linux.vnet.ibm.com>
> ---
> 
> 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;
             }

> +            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 :)

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.

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

> +    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.

There's quite some potential in this. We may want to make this a
permanent addition.
Halil Pasic Sept. 14, 2017, 4:50 p.m. | #2
On 09/14/2017 04:26 PM, Cornelia Huck wrote:
> On Wed, 13 Sep 2017 15:27:51 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> 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 <pasic@linux.vnet.ibm.com>
>> ---
>>
>> 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?
Cornelia Huck Sept. 15, 2017, 7:27 a.m. | #3
On Thu, 14 Sep 2017 18:50:29 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/14/2017 04:26 PM, Cornelia Huck wrote:
> > On Wed, 13 Sep 2017 15:27:51 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> >> +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.

https://en.wikipedia.org/wiki/4711

That's my default if I need a four-digit number :)

> 
> > 
> > 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.

4711 is more a joke :) It might be worth trying the same channels as
for virtio-ccw.

Christian should know more about that.

> 
> > 
> > (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).

My thought was that the z/VM folks already might have something
(type-wise) that we could use as well. There's a surprising amount of
values that are reserved for one use or the other. But obviously, I
can't find out about that.

> 
> >   
> >> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
> >> +                       0x98),

This might also need re-evaluation - we should not really need a new
chpid type.

> >> +    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?

I think the first step would be to figure out the ids so we don't step
on anyone's toes. Then maybe refactor a bit so that other testers can
be added easily.

For ideas about things to be tested, maybe put a list into the wiki?
Cornelia Huck Sept. 15, 2017, 5:01 p.m. | #4
On Fri, 15 Sep 2017 09:27:58 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Thu, 14 Sep 2017 18:50:29 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
> > On 09/14/2017 04:26 PM, Cornelia Huck wrote:  

> > > 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?  
> 
> I think the first step would be to figure out the ids so we don't step
> on anyone's toes. Then maybe refactor a bit so that other testers can
> be added easily.
> 
> For ideas about things to be tested, maybe put a list into the wiki?

OK, I've created https://wiki.qemu.org/Testing/CCWTestDevice - let me
know if you need an account.
Christian Borntraeger Sept. 18, 2017, 8:30 a.m. | #5
On 09/15/2017 09:27 AM, Cornelia Huck wrote:
> On Thu, 14 Sep 2017 18:50:29 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/14/2017 04:26 PM, Cornelia Huck wrote:
>>> On Wed, 13 Sep 2017 15:27:51 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>>>> +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.
> 
> https://en.wikipedia.org/wiki/4711
> 
> That's my default if I need a four-digit number :)
> 
>>
>>>
>>> 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.
> 
> 4711 is more a joke :) It might be worth trying the same channels as
> for virtio-ccw.
> 
> Christian should know more about that.

Getting a new number was very easy (because it is attached to a machine type
number). I I remember correctly, only numerical values are uses, so maybe
we can use ffff as there will never be such a real value?
Cornelia Huck Sept. 18, 2017, 8:42 a.m. | #6
On Mon, 18 Sep 2017 10:30:32 +0200
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 09/15/2017 09:27 AM, Cornelia Huck wrote:
> > On Thu, 14 Sep 2017 18:50:29 +0200
> > Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> >   
> >> On 09/14/2017 04:26 PM, Cornelia Huck wrote:  
> >>> On Wed, 13 Sep 2017 15:27:51 +0200
> >>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:  
> >   
> >>>> +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.  
> > 
> > https://en.wikipedia.org/wiki/4711
> > 
> > That's my default if I need a four-digit number :)
> >   
> >>  
> >>>
> >>> 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.  
> > 
> > 4711 is more a joke :) It might be worth trying the same channels as
> > for virtio-ccw.
> > 
> > Christian should know more about that.  
> 
> Getting a new number was very easy (because it is attached to a machine type
> number). I I remember correctly, only numerical values are uses, so maybe
> we can use ffff as there will never be such a real value?
> 

Yes, that sounds like the easiest way to do that.
Pierre Morel Sept. 19, 2017, 2:20 p.m. | #7
On 15/09/2017 19:01, Cornelia Huck wrote:
> On Fri, 15 Sep 2017 09:27:58 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
>> On Thu, 14 Sep 2017 18:50:29 +0200
>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>
>>> On 09/14/2017 04:26 PM, Cornelia Huck wrote:
> 
>>>> 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?
>>
>> I think the first step would be to figure out the ids so we don't step
>> on anyone's toes. Then maybe refactor a bit so that other testers can
>> be added easily.
>>
>> For ideas about things to be tested, maybe put a list into the wiki?
> 
> OK, I've created https://wiki.qemu.org/Testing/CCWTestDevice - let me
> know if you need an account.
> 

Great!
I just saw your email, it was waiting in my inbox.
strange things happens some time. A good serie. strange things.
neverless...

Seriously.
Yes, I definitively need an account!


Pierre
Pierre Morel Sept. 19, 2017, 2:26 p.m. | #8
On 14/09/2017 16:26, Cornelia Huck wrote:
> On Wed, 13 Sep 2017 15:27:51 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> 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 <pasic@linux.vnet.ibm.com>
>> ---
>>
>> 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;
>               }
> 
>> +            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 :)

The C0C0 channel would be nice too.
Sorry.

> 
> 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.
> 
> (Or is there already something reserved for "hypervisor use" or
> whatever?)
> 
>> +    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.
> 
> There's quite some potential in this. We may want to make this a
> permanent addition.
>
Halil Pasic Sept. 25, 2017, 3:06 p.m. | #9
On 09/19/2017 04:20 PM, Pierre Morel wrote:
> On 15/09/2017 19:01, Cornelia Huck wrote:
>> On Fri, 15 Sep 2017 09:27:58 +0200
>> Cornelia Huck <cohuck@redhat.com> wrote:
>>
>>> On Thu, 14 Sep 2017 18:50:29 +0200
>>> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
>>>
>>>> On 09/14/2017 04:26 PM, Cornelia Huck wrote:
>>
>>>>> 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?
>>>
>>> I think the first step would be to figure out the ids so we don't step
>>> on anyone's toes. Then maybe refactor a bit so that other testers can
>>> be added easily.
>>>
>>> For ideas about things to be tested, maybe put a list into the wiki?
>>
>> OK, I've created https://wiki.qemu.org/Testing/CCWTestDevice - let me
>> know if you need an account.
>>
> 
> Great!
> I just saw your email, it was waiting in my inbox.
> strange things happens some time. A good serie. strange things.
> neverless...
> 
> Seriously.
> Yes, I definitively need an account!
> 
> 
> Pierre
> 

@Connie
I would also like to have an account. I would also like to do a
v2 of this somewhere in the not too distant future. I intend
to address the issues pointed out by you here (chpid_type,
cu_type). Another question is whether this should go under
hw/misc/ like the pci-testdev? ccw-testdev would also probably
be a better file name (that ccw-tester) regardless of in which
folder does this belong. One more thing I am considering for
v2 is this make it extensible argument. I had something like
mode on my mind from the very beginning (fib would be one mode).
The idea was to provide a control ccw for setting/getting the mode
(something like virtio) as well as a device property for setting
the initial mode (so that guest does not have to know about it).

Regards,
Halil
Cornelia Huck Sept. 25, 2017, 3:39 p.m. | #10
On Mon, 25 Sep 2017 17:06:01 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> @Connie
> I would also like to have an account. 

Done (for both of you).

> I would also like to do a
> v2 of this somewhere in the not too distant future. I intend
> to address the issues pointed out by you here (chpid_type,
> cu_type). Another question is whether this should go under
> hw/misc/ like the pci-testdev? ccw-testdev would also probably
> be a better file name (that ccw-tester) regardless of in which
> folder does this belong. 

Probably not a bad idea to follow pci here.

> One more thing I am considering for
> v2 is this make it extensible argument. I had something like
> mode on my mind from the very beginning (fib would be one mode).
> The idea was to provide a control ccw for setting/getting the mode
> (something like virtio) as well as a device property for setting
> the initial mode (so that guest does not have to know about it).

Yes, a control ccw sounds nice, we should just make sure that it
doesn't collide with future tests (e.g. if we want to do some channel
program fuzzing). Alternatively, this could be controled by a diagnose,
which would be out of band (we can use a high function code for diag
500 that is unlikely to collide with future extensions).
Halil Pasic Oct. 19, 2017, 4:39 p.m. | #11
On 09/15/2017 09:27 AM, Cornelia Huck wrote:
>>>   
>>>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
>>>> +                       0x98),
> This might also need re-evaluation - we should not really need a new
> chpid type.
> 

I'm back at ccw-tester again (for v2). I've realized we did not agree
on what to use here (chpid_type). Shall I use 0x25 (Fibre Channel) or
EMULATED_CCW_3270_CHPID_TYPE, or even 0x32 (virtio-ccw) as a default
value? And should EMULATED_CCW_3270_CHPID_TYPE be called like that
(is it really supposed to be specific to 3270?

Sorry I did not notice sooner.

Regards,
Halil
Cornelia Huck Oct. 20, 2017, 1 p.m. | #12
On Thu, 19 Oct 2017 18:39:37 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> On 09/15/2017 09:27 AM, Cornelia Huck wrote:
> >>>     
> >>>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
> >>>> +                       0x98),  
> > This might also need re-evaluation - we should not really need a new
> > chpid type.
> >   
> 
> I'm back at ccw-tester again (for v2). I've realized we did not agree
> on what to use here (chpid_type). Shall I use 0x25 (Fibre Channel) or
> EMULATED_CCW_3270_CHPID_TYPE, or even 0x32 (virtio-ccw) as a default
> value? And should EMULATED_CCW_3270_CHPID_TYPE be called like that
> (is it really supposed to be specific to 3270?
> 
> Sorry I did not notice sooner.

It might make sense to pick whatever z/VM commonly uses for emulated
devices. (Or ask them if they reserved something explicitly for testing
-- that would be an even better match.)
Halil Pasic Oct. 20, 2017, 2:33 p.m. | #13
On 10/20/2017 03:00 PM, Cornelia Huck wrote:
> On Thu, 19 Oct 2017 18:39:37 +0200
> Halil Pasic <pasic@linux.vnet.ibm.com> wrote:
> 
>> On 09/15/2017 09:27 AM, Cornelia Huck wrote:
>>>>>     
>>>>>> +    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
>>>>>> +                       0x98),  
>>> This might also need re-evaluation - we should not really need a new
>>> chpid type.
>>>   
>>
>> I'm back at ccw-tester again (for v2). I've realized we did not agree
>> on what to use here (chpid_type). Shall I use 0x25 (Fibre Channel) or
>> EMULATED_CCW_3270_CHPID_TYPE, or even 0x32 (virtio-ccw) as a default
>> value? And should EMULATED_CCW_3270_CHPID_TYPE be called like that
>> (is it really supposed to be specific to 3270?
>>
>> Sorry I did not notice sooner.
> 
> It might make sense to pick whatever z/VM commonly uses for emulated
> devices. (Or ask them if they reserved something explicitly for testing
> -- that would be an even better match.)
> 

OK, I will approach the z/VM guys. About EMULATED_CCW_3270_CHPID_TYPE,
does it really make sense to have a separate chpid type for 3270? (You
have missed that question, so I'm asking it again).

Halil
Cornelia Huck Oct. 20, 2017, 3:46 p.m. | #14
On Fri, 20 Oct 2017 16:33:47 +0200
Halil Pasic <pasic@linux.vnet.ibm.com> wrote:

> OK, I will approach the z/VM guys. About EMULATED_CCW_3270_CHPID_TYPE,
> does it really make sense to have a separate chpid type for 3270? (You
> have missed that question, so I'm asking it again).

I had not. But my resources are depleted.

Patch

diff --git a/hw/s390x/Makefile.objs b/hw/s390x/Makefile.objs
index 7ee19d3abc..28eb58a3cb 100644
--- a/hw/s390x/Makefile.objs
+++ b/hw/s390x/Makefile.objs
@@ -18,3 +18,4 @@  obj-y += s390-stattrib.o
 obj-$(CONFIG_KVM) += s390-skeys-kvm.o
 obj-$(CONFIG_KVM) += s390-stattrib-kvm.o
 obj-y += s390-ccw.o
+obj-y += ccw-tester.o
diff --git a/hw/s390x/ccw-tester.c b/hw/s390x/ccw-tester.c
new file mode 100644
index 0000000000..c8017818c4
--- /dev/null
+++ b/hw/s390x/ccw-tester.c
@@ -0,0 +1,179 @@ 
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qemu/module.h"
+#include "cpu.h"
+#include "hw/s390x/css.h"
+#include "hw/s390x/css-bridge.h"
+#include "hw/s390x/3270-ccw.h"
+#include "exec/address-spaces.h"
+#include <error.h>
+
+typedef struct CcwTesterDevice {
+    CcwDevice parent_obj;
+    uint16_t cu_type;
+    uint8_t chpid_type;
+    struct {
+        uint32_t ring[4];
+        unsigned int next;
+    } fib;
+} CcwTesterDevice;
+
+
+typedef struct CcwTesterClass {
+    CCWDeviceClass parent_class;
+    DeviceRealize parent_realize;
+} CcwTesterClass;
+
+#define TYPE_CCW_TESTER "ccw-tester"
+
+#define CCW_TESTER(obj) \
+     OBJECT_CHECK(CcwTesterDevice, (obj), TYPE_CCW_TESTER)
+#define CCW_TESTER_CLASS(klass) \
+     OBJECT_CLASS_CHECK(CcwTesterClass, (klass), TYPE_CCW_TESTER)
+#define CCW_TESTER_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(CcwTesterClass, (obj), TYPE_CCW_TESTER)
+
+#define CCW_CMD_READ 0x01U
+#define CCW_CMD_WRITE 0x02U
+
+static unsigned int abs_to_ring(unsigned int i)
+{
+    return i & 0x3;
+}
+
+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)];
+            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 int ccw_tester_ccw_cb_impl(SubchDev *sch, CCW1 ccw)
+{
+    switch (ccw.cmd_code) {
+    case CCW_CMD_READ:
+        break;
+    case CCW_CMD_WRITE:
+        return ccw_tester_write_fib(sch, ccw);
+    default:
+        return -EINVAL;
+    }
+    return 0;
+}
+
+static void ccw_tester_realize(DeviceState *ds, Error **errp)
+{
+    uint16_t chpid;
+    CcwTesterDevice *dev = CCW_TESTER(ds);
+    CcwTesterClass *ctc = CCW_TESTER_GET_CLASS(dev);
+    CcwDevice *cdev = CCW_DEVICE(ds);
+    BusState *qbus = qdev_get_parent_bus(ds);
+    VirtualCssBus *cbus = VIRTUAL_CSS_BUS(qbus);
+    SubchDev *sch;
+    Error *err = NULL;
+
+    sch = css_create_sch(cdev->devno, true, cbus->squash_mcss, errp);
+    if (!sch) {
+        return;
+    }
+
+    sch->driver_data = dev;
+    cdev->sch = sch;
+    chpid = css_find_free_chpid(sch->cssid);
+
+    if (chpid > MAX_CHPID) {
+        error_setg(&err, "No available chpid to use.");
+        goto out_err;
+    }
+
+    sch->id.reserved = 0xff;
+    sch->id.cu_type = dev->cu_type;
+    css_sch_build_virtual_schib(sch, (uint8_t)chpid,
+                                dev->chpid_type);
+    sch->ccw_cb = ccw_tester_ccw_cb_impl;
+    sch->do_subchannel_work = do_subchannel_work_virtual;
+
+
+    ctc->parent_realize(ds, &err);
+    if (err) {
+        goto out_err;
+    }
+
+    css_generate_sch_crws(sch->cssid, sch->ssid, sch->schid,
+                          ds->hotplugged, 1);
+    return;
+
+out_err:
+    error_propagate(errp, err);
+    css_subch_assign(sch->cssid, sch->ssid, sch->schid, sch->devno, NULL);
+    cdev->sch = NULL;
+    g_free(sch);
+}
+
+static Property ccw_tester_properties[] = {
+    DEFINE_PROP_UINT16("cu_type", CcwTesterDevice, cu_type,
+                        0x3831),
+    DEFINE_PROP_UINT8("chpid_type", CcwTesterDevice, chpid_type,
+                       0x98),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void ccw_tester_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    CcwTesterClass *ctc = CCW_TESTER_CLASS(klass);
+
+    dc->props = ccw_tester_properties;
+    dc->bus_type = TYPE_VIRTUAL_CSS_BUS;
+    ctc->parent_realize = dc->realize;
+    dc->realize = ccw_tester_realize;
+    dc->hotpluggable = false;
+}
+
+static const TypeInfo ccw_tester_info = {
+    .name = TYPE_CCW_TESTER,
+    .parent = TYPE_CCW_DEVICE,
+    .instance_size = sizeof(CcwTesterDevice),
+    .class_init = ccw_tester_class_init,
+    .class_size = sizeof(CcwTesterClass),
+};
+
+static void ccw_tester_register(void)
+{
+    type_register_static(&ccw_tester_info);
+}
+
+type_init(ccw_tester_register)