| Message ID | 20170913132752.8484-2-pasic@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | tests for CCW IDA | expand |
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.
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?
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?
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.
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?
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.
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
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. >
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
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).
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
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.)
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
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.
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)
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