[kvm-unit-tests PATCH 0/7] Extending VIRTIO with a data transfer test

[kvm-unit-tests PATCH 0/7] Extending VIRTIO with a data transfer test

[Pierre Morel]

Hello All,

This series implement VIRTIO-CCW transport and a VIRTIO-CCW data
transfer test.

The first patch is for allowing different architectures to use the
existing VIRTIO framework used by ARM.

We then need a callback in the CSS enumeration to select a device,
and bind it with the VIRTIO transport, patches 2 and 3.

We need to be able to disable IRQ on the processor to protect
part of the code until we are ready to receive an IRQ, and we need
to have the registration and handling of IRQ at the VIRTIO level,
this is patch 4.

To be able to receive data we extend the comon VIRTIO code with an
input routine, patch 5.

Patch 6 and 7 are the two tests patches to implement simple VIRTIO
initialization, very few tests of integrity now which can be expend
later, and a data transfer test to check various alignement and sizes
of buffers and to check the data integrity with a simple checksum.

To test the last part you will need the associated QEMU device published
in another series on the QEMU mailing list.

Regards,
Pierre

Pierre Morel (7):
  arm: virtio: move VIRTIO transport initialization inside virtio-mmio
  s390x: css: add callback for emnumeration
  s390x: virtio: CCW transport implementation
  s390x: css: registering IRQ
  virtio: implement the virtio_add_inbuf routine
  s390x: virtio tests setup
  s390x: virtio data transfer

 lib/s390x/css.h        |  24 ++-
 lib/s390x/css_lib.c    |  31 +++-
 lib/s390x/virtio-ccw.c | 374 +++++++++++++++++++++++++++++++++++++++++
 lib/s390x/virtio-ccw.h | 111 ++++++++++++
 lib/virtio-config.h    |  30 ++++
 lib/virtio-mmio.c      |   2 +-
 lib/virtio-mmio.h      |   2 -
 lib/virtio.c           |  37 +++-
 lib/virtio.h           |   2 +
 s390x/Makefile         |   3 +
 s390x/css.c            |   2 +-
 s390x/unittests.cfg    |   4 +
 s390x/virtio_pong.c    | 315 ++++++++++++++++++++++++++++++++++
 13 files changed, 924 insertions(+), 13 deletions(-)
 create mode 100644 lib/s390x/virtio-ccw.c
 create mode 100644 lib/s390x/virtio-ccw.h
 create mode 100644 lib/virtio-config.h
 create mode 100644 s390x/virtio_pong.c

-- 
2.25.1

[kvm-unit-tests PATCH 1/7] arm: virtio: move VIRTIO transport initialization inside virtio-mmio

[Pierre Morel]

To be able to use different VIRTIO transport in the future we need
the initialisation entry call of the transport to be inside the
transport file and keep the VIRTIO level transport agnostic.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/virtio-mmio.c | 2 +-
 lib/virtio-mmio.h | 2 --
 lib/virtio.c      | 5 -----
 3 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/lib/virtio-mmio.c b/lib/virtio-mmio.c
index e5e8f660..fb8a86a3 100644
--- a/lib/virtio-mmio.c
+++ b/lib/virtio-mmio.c
@@ -173,7 +173,7 @@ static struct virtio_device *virtio_mmio_dt_bind(u32 devid)
 	return &vm_dev->vdev;
 }
 
-struct virtio_device *virtio_mmio_bind(u32 devid)
+struct virtio_device *virtio_bind(u32 devid)
 {
 	return virtio_mmio_dt_bind(devid);
 }
diff --git a/lib/virtio-mmio.h b/lib/virtio-mmio.h
index 250f28a0..73ddbd23 100644
--- a/lib/virtio-mmio.h
+++ b/lib/virtio-mmio.h
@@ -60,6 +60,4 @@ struct virtio_mmio_device {
 	void *base;
 };
 
-extern struct virtio_device *virtio_mmio_bind(u32 devid);
-
 #endif /* _VIRTIO_MMIO_H_ */
diff --git a/lib/virtio.c b/lib/virtio.c
index 69054757..e10153b9 100644
--- a/lib/virtio.c
+++ b/lib/virtio.c
@@ -123,8 +123,3 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
 
 	return ret;
 }
-
-struct virtio_device *virtio_bind(u32 devid)
-{
-	return virtio_mmio_bind(devid);
-}
-- 
2.25.1

Re: [kvm-unit-tests PATCH 1/7] arm: virtio: move VIRTIO transport initialization inside virtio-mmio

[Thomas Huth]

Sorry for the late reply - still trying to get my Inbox under control again ...

On 27/08/2021 12.17, Pierre Morel wrote:
> To be able to use different VIRTIO transport in the future we need
> the initialisation entry call of the transport to be inside the
> transport file and keep the VIRTIO level transport agnostic.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/virtio-mmio.c | 2 +-
>   lib/virtio-mmio.h | 2 --
>   lib/virtio.c      | 5 -----
>   3 files changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/lib/virtio-mmio.c b/lib/virtio-mmio.c
> index e5e8f660..fb8a86a3 100644
> --- a/lib/virtio-mmio.c
> +++ b/lib/virtio-mmio.c
> @@ -173,7 +173,7 @@ static struct virtio_device *virtio_mmio_dt_bind(u32 devid)
>   	return &vm_dev->vdev;
>   }
>   
> -struct virtio_device *virtio_mmio_bind(u32 devid)
> +struct virtio_device *virtio_bind(u32 devid)
>   {
>   	return virtio_mmio_dt_bind(devid);
>   }
> diff --git a/lib/virtio-mmio.h b/lib/virtio-mmio.h
> index 250f28a0..73ddbd23 100644
> --- a/lib/virtio-mmio.h
> +++ b/lib/virtio-mmio.h
> @@ -60,6 +60,4 @@ struct virtio_mmio_device {
>   	void *base;
>   };
>   
> -extern struct virtio_device *virtio_mmio_bind(u32 devid);
> -
>   #endif /* _VIRTIO_MMIO_H_ */
> diff --git a/lib/virtio.c b/lib/virtio.c
> index 69054757..e10153b9 100644
> --- a/lib/virtio.c
> +++ b/lib/virtio.c
> @@ -123,8 +123,3 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>   
>   	return ret;
>   }
> -
> -struct virtio_device *virtio_bind(u32 devid)
> -{
> -	return virtio_mmio_bind(devid);
> -}
> 

I agree that this needs to be improved somehow, but I'm not sure whether 
moving the function to virtio-mmio.c is the right solution. I guess the 
original idea was that virtio_bind() could cope with multiple transports, 
i.e. when there is support for virtio-pci, it could choose between mmio and 
pci on ARM, or between CCW and PCI on s390x.

So maybe this should rather get an "#if defined(__arm__) || 
defined(__aarch64__)" instead? Drew, what's your opinion here?

  Thomas

Re: [kvm-unit-tests PATCH 1/7] arm: virtio: move VIRTIO transport initialization inside virtio-mmio

[Andrew Jones]

On Wed, Nov 03, 2021 at 08:00:09AM +0100, Thomas Huth wrote:
> Sorry for the late reply - still trying to get my Inbox under control again ...
> 
> On 27/08/2021 12.17, Pierre Morel wrote:
> > To be able to use different VIRTIO transport in the future we need
> > the initialisation entry call of the transport to be inside the
> > transport file and keep the VIRTIO level transport agnostic.
> > 
> > Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> > ---
> >   lib/virtio-mmio.c | 2 +-
> >   lib/virtio-mmio.h | 2 --
> >   lib/virtio.c      | 5 -----
> >   3 files changed, 1 insertion(+), 8 deletions(-)
> > 
> > diff --git a/lib/virtio-mmio.c b/lib/virtio-mmio.c
> > index e5e8f660..fb8a86a3 100644
> > --- a/lib/virtio-mmio.c
> > +++ b/lib/virtio-mmio.c
> > @@ -173,7 +173,7 @@ static struct virtio_device *virtio_mmio_dt_bind(u32 devid)
> >   	return &vm_dev->vdev;
> >   }
> > -struct virtio_device *virtio_mmio_bind(u32 devid)
> > +struct virtio_device *virtio_bind(u32 devid)
> >   {
> >   	return virtio_mmio_dt_bind(devid);
> >   }
> > diff --git a/lib/virtio-mmio.h b/lib/virtio-mmio.h
> > index 250f28a0..73ddbd23 100644
> > --- a/lib/virtio-mmio.h
> > +++ b/lib/virtio-mmio.h
> > @@ -60,6 +60,4 @@ struct virtio_mmio_device {
> >   	void *base;
> >   };
> > -extern struct virtio_device *virtio_mmio_bind(u32 devid);
> > -
> >   #endif /* _VIRTIO_MMIO_H_ */
> > diff --git a/lib/virtio.c b/lib/virtio.c
> > index 69054757..e10153b9 100644
> > --- a/lib/virtio.c
> > +++ b/lib/virtio.c
> > @@ -123,8 +123,3 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
> >   	return ret;
> >   }
> > -
> > -struct virtio_device *virtio_bind(u32 devid)
> > -{
> > -	return virtio_mmio_bind(devid);
> > -}
> > 
> 
> I agree that this needs to be improved somehow, but I'm not sure whether
> moving the function to virtio-mmio.c is the right solution. I guess the
> original idea was that virtio_bind() could cope with multiple transports,
> i.e. when there is support for virtio-pci, it could choose between mmio and
> pci on ARM, or between CCW and PCI on s390x.

That's right. If we wanted to use virtio-pci on ARM, then, after
implementing virtio_pci_bind(), we'd change this to

  struct virtio_device *virtio_bind(u32 devid)  
  {
      struct virtio_device *dev = virtio_mmio_bind(devid);

      if (!dev)
          dev = virtio_pci_bind(devid);

      return dev;
  }

Then, we'd use config selection logic in the test harness to decide how to
construct the QEMU command line in order to choose between mmio and pci.

> 
> So maybe this should rather get an "#if defined(__arm__) ||
> defined(__aarch64__)" instead? Drew, what's your opinion here?

Yup, but I think I'd prefer we do it in the header, like below, and
then also implement something like the above for virtio_bind().

diff --git a/lib/virtio-mmio.h b/lib/virtio-mmio.h
index 250f28a0d300..a0a3bf827156 100644
--- a/lib/virtio-mmio.h
+++ b/lib/virtio-mmio.h
@@ -60,6 +60,13 @@ struct virtio_mmio_device {
        void *base;
 };
 
+#if defined(__arm__) || defined(__aarch64__)
 extern struct virtio_device *virtio_mmio_bind(u32 devid);
+#else
+static inline struct virtio_device *virtio_mmio_bind(u32 devid)
+{
+        return NULL;
+}
+#endif
 
 #endif /* _VIRTIO_MMIO_H_ */


Thanks,
drew

Re: [kvm-unit-tests PATCH 1/7] arm: virtio: move VIRTIO transport initialization inside virtio-mmio

[Pierre Morel]

On 11/3/21 08:41, Andrew Jones wrote:
> On Wed, Nov 03, 2021 at 08:00:09AM +0100, Thomas Huth wrote:
>> Sorry for the late reply - still trying to get my Inbox under control again ...
>>
>> On 27/08/2021 12.17, Pierre Morel wrote:
>>> To be able to use different VIRTIO transport in the future we need
>>> the initialisation entry call of the transport to be inside the
>>> transport file and keep the VIRTIO level transport agnostic.
>>>
>>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>>> ---
>>>    lib/virtio-mmio.c | 2 +-
>>>    lib/virtio-mmio.h | 2 --
>>>    lib/virtio.c      | 5 -----
>>>    3 files changed, 1 insertion(+), 8 deletions(-)
>>>
>>> diff --git a/lib/virtio-mmio.c b/lib/virtio-mmio.c
>>> index e5e8f660..fb8a86a3 100644
>>> --- a/lib/virtio-mmio.c
>>> +++ b/lib/virtio-mmio.c
>>> @@ -173,7 +173,7 @@ static struct virtio_device *virtio_mmio_dt_bind(u32 devid)
>>>    	return &vm_dev->vdev;
>>>    }
>>> -struct virtio_device *virtio_mmio_bind(u32 devid)
>>> +struct virtio_device *virtio_bind(u32 devid)
>>>    {
>>>    	return virtio_mmio_dt_bind(devid);
>>>    }
>>> diff --git a/lib/virtio-mmio.h b/lib/virtio-mmio.h
>>> index 250f28a0..73ddbd23 100644
>>> --- a/lib/virtio-mmio.h
>>> +++ b/lib/virtio-mmio.h
>>> @@ -60,6 +60,4 @@ struct virtio_mmio_device {
>>>    	void *base;
>>>    };
>>> -extern struct virtio_device *virtio_mmio_bind(u32 devid);
>>> -
>>>    #endif /* _VIRTIO_MMIO_H_ */
>>> diff --git a/lib/virtio.c b/lib/virtio.c
>>> index 69054757..e10153b9 100644
>>> --- a/lib/virtio.c
>>> +++ b/lib/virtio.c
>>> @@ -123,8 +123,3 @@ void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len)
>>>    	return ret;
>>>    }
>>> -
>>> -struct virtio_device *virtio_bind(u32 devid)
>>> -{
>>> -	return virtio_mmio_bind(devid);
>>> -}
>>>
>>
>> I agree that this needs to be improved somehow, but I'm not sure whether
>> moving the function to virtio-mmio.c is the right solution. I guess the
>> original idea was that virtio_bind() could cope with multiple transports,
>> i.e. when there is support for virtio-pci, it could choose between mmio and
>> pci on ARM, or between CCW and PCI on s390x.
> 
> That's right. If we wanted to use virtio-pci on ARM, then, after
> implementing virtio_pci_bind(), we'd change this to
> 
>    struct virtio_device *virtio_bind(u32 devid)
>    {
>        struct virtio_device *dev = virtio_mmio_bind(devid);
> 
>        if (!dev)
>            dev = virtio_pci_bind(devid);
> 
>        return dev;
>    }
> 
> Then, we'd use config selection logic in the test harness to decide how to
> construct the QEMU command line in order to choose between mmio and pci.

OK, I understand.

> 
>>
>> So maybe this should rather get an "#if defined(__arm__) ||
>> defined(__aarch64__)" instead? Drew, what's your opinion here?
> 
> Yup, but I think I'd prefer we do it in the header, like below, and
> then also implement something like the above for virtio_bind().
> 
> diff --git a/lib/virtio-mmio.h b/lib/virtio-mmio.h
> index 250f28a0d300..a0a3bf827156 100644
> --- a/lib/virtio-mmio.h
> +++ b/lib/virtio-mmio.h
> @@ -60,6 +60,13 @@ struct virtio_mmio_device {
>          void *base;
>   };
>   
> +#if defined(__arm__) || defined(__aarch64__)
>   extern struct virtio_device *virtio_mmio_bind(u32 devid);
> +#else
> +static inline struct virtio_device *virtio_mmio_bind(u32 devid)
> +{
> +        return NULL;
> +}
> +#endif
>   
>   #endif /* _VIRTIO_MMIO_H_ */
> 
> 
> Thanks,
> drew
> 

Thanks, I will modify accordingly.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH 2/7] s390x: css: add callback for emnumeration

[Pierre Morel]

We will need to look for a device inside the channel subsystem
based upon device specificities.

Let's provide a callback for an upper layer to be called during
the enumeration of the channel subsystem.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h     | 3 ++-
 lib/s390x/css_lib.c | 4 +++-
 s390x/css.c         | 2 +-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index d644971f..2005f4d7 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -278,7 +278,8 @@ void dump_irb(struct irb *irbp);
 void dump_pmcw(struct pmcw *p);
 void dump_orb(struct orb *op);
 
-int css_enumerate(void);
+typedef int (enumerate_cb_t)(int);
+int css_enumerate(enumerate_cb_t *f);
 #define MAX_ENABLE_RETRIES      5
 
 #define IO_SCH_ISC      3
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index efc70576..484f9c41 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -117,7 +117,7 @@ bool get_chsc_scsc(void)
  * On success return the first subchannel ID found.
  * On error return an invalid subchannel ID containing cc
  */
-int css_enumerate(void)
+int css_enumerate(enumerate_cb_t *f)
 {
 	struct pmcw *pmcw = &schib.pmcw;
 	int scn_found = 0;
@@ -153,6 +153,8 @@ int css_enumerate(void)
 			schid = scn | SCHID_ONE;
 		report_info("Found subchannel %08x", scn | SCHID_ONE);
 		dev_found++;
+		if (f)
+			f(scn | SCHID_ONE);
 	}
 
 out:
diff --git a/s390x/css.c b/s390x/css.c
index c340c539..b50fbc67 100644
--- a/s390x/css.c
+++ b/s390x/css.c
@@ -29,7 +29,7 @@ struct ccw1 *ccw;
 
 static void test_enumerate(void)
 {
-	test_device_sid = css_enumerate();
+	test_device_sid = css_enumerate(NULL);
 	if (test_device_sid & SCHID_ONE) {
 		report(1, "Schid of first I/O device: 0x%08x", test_device_sid);
 		return;
-- 
2.25.1

Re: [kvm-unit-tests PATCH 2/7] s390x: css: add callback for emnumeration

[Thomas Huth]

On 27/08/2021 12.17, Pierre Morel wrote:
> We will need to look for a device inside the channel subsystem
> based upon device specificities.
> 
> Let's provide a callback for an upper layer to be called during
> the enumeration of the channel subsystem.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     | 3 ++-
>   lib/s390x/css_lib.c | 4 +++-
>   s390x/css.c         | 2 +-
>   3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index d644971f..2005f4d7 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -278,7 +278,8 @@ void dump_irb(struct irb *irbp);
>   void dump_pmcw(struct pmcw *p);
>   void dump_orb(struct orb *op);
>   
> -int css_enumerate(void);
> +typedef int (enumerate_cb_t)(int);
> +int css_enumerate(enumerate_cb_t *f);
>   #define MAX_ENABLE_RETRIES      5
>   
>   #define IO_SCH_ISC      3
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index efc70576..484f9c41 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -117,7 +117,7 @@ bool get_chsc_scsc(void)
>    * On success return the first subchannel ID found.
>    * On error return an invalid subchannel ID containing cc
>    */
> -int css_enumerate(void)
> +int css_enumerate(enumerate_cb_t *f)

I'd maybe call it "cb" instead of "f" ... but that's just my personal taste, 
I guess.

>   {
>   	struct pmcw *pmcw = &schib.pmcw;
>   	int scn_found = 0;
> @@ -153,6 +153,8 @@ int css_enumerate(void)
>   			schid = scn | SCHID_ONE;
>   		report_info("Found subchannel %08x", scn | SCHID_ONE);
>   		dev_found++;
> +		if (f)
> +			f(scn | SCHID_ONE);
>   	}
>   
>   out:
> diff --git a/s390x/css.c b/s390x/css.c
> index c340c539..b50fbc67 100644
> --- a/s390x/css.c
> +++ b/s390x/css.c
> @@ -29,7 +29,7 @@ struct ccw1 *ccw;
>   
>   static void test_enumerate(void)
>   {
> -	test_device_sid = css_enumerate();
> +	test_device_sid = css_enumerate(NULL);
>   	if (test_device_sid & SCHID_ONE) {
>   		report(1, "Schid of first I/O device: 0x%08x", test_device_sid);
>   		return;
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH 2/7] s390x: css: add callback for emnumeration

[Pierre Morel]

On 11/3/21 08:29, Thomas Huth wrote:
> On 27/08/2021 12.17, Pierre Morel wrote:
>> We will need to look for a device inside the channel subsystem
>> based upon device specificities.
>>
>> Let's provide a callback for an upper layer to be called during
>> the enumeration of the channel subsystem.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     | 3 ++-
>>   lib/s390x/css_lib.c | 4 +++-
>>   s390x/css.c         | 2 +-
>>   3 files changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index d644971f..2005f4d7 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -278,7 +278,8 @@ void dump_irb(struct irb *irbp);
>>   void dump_pmcw(struct pmcw *p);
>>   void dump_orb(struct orb *op);
>> -int css_enumerate(void);
>> +typedef int (enumerate_cb_t)(int);
>> +int css_enumerate(enumerate_cb_t *f);
>>   #define MAX_ENABLE_RETRIES      5
>>   #define IO_SCH_ISC      3
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index efc70576..484f9c41 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -117,7 +117,7 @@ bool get_chsc_scsc(void)
>>    * On success return the first subchannel ID found.
>>    * On error return an invalid subchannel ID containing cc
>>    */
>> -int css_enumerate(void)
>> +int css_enumerate(enumerate_cb_t *f)
> 
> I'd maybe call it "cb" instead of "f" ... but that's just my personal 
> taste, I guess.

I can use cb, looks better.

> 
>>   {
>>       struct pmcw *pmcw = &schib.pmcw;
>>       int scn_found = 0;
>> @@ -153,6 +153,8 @@ int css_enumerate(void)
>>               schid = scn | SCHID_ONE;
>>           report_info("Found subchannel %08x", scn | SCHID_ONE);
>>           dev_found++;
>> +        if (f)
>> +            f(scn | SCHID_ONE);
>>       }
>>   out:
>> diff --git a/s390x/css.c b/s390x/css.c
>> index c340c539..b50fbc67 100644
>> --- a/s390x/css.c
>> +++ b/s390x/css.c
>> @@ -29,7 +29,7 @@ struct ccw1 *ccw;
>>   static void test_enumerate(void)
>>   {
>> -    test_device_sid = css_enumerate();
>> +    test_device_sid = css_enumerate(NULL);
>>       if (test_device_sid & SCHID_ONE) {
>>           report(1, "Schid of first I/O device: 0x%08x", 
>> test_device_sid);
>>           return;
>>
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH 3/7] s390x: virtio: CCW transport implementation

[Pierre Morel]

This is the implementation of the virtio-ccw transport level.

We only support VIRTIO revision 0.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/virtio-ccw.c | 374 +++++++++++++++++++++++++++++++++++++++++
 lib/s390x/virtio-ccw.h | 111 ++++++++++++
 lib/virtio-config.h    |  30 ++++
 s390x/Makefile         |   2 +
 4 files changed, 517 insertions(+)
 create mode 100644 lib/s390x/virtio-ccw.c
 create mode 100644 lib/s390x/virtio-ccw.h
 create mode 100644 lib/virtio-config.h

diff --git a/lib/s390x/virtio-ccw.c b/lib/s390x/virtio-ccw.c
new file mode 100644
index 00000000..cf447de6
--- /dev/null
+++ b/lib/s390x/virtio-ccw.c
@@ -0,0 +1,374 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Virtio CCW Library
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ */
+
+#include <libcflat.h>
+#include <alloc_page.h>
+#include <asm/page.h>
+#include <string.h>
+#include <interrupt.h>
+#include <asm/arch_def.h>
+#include <asm/facility.h>
+#include <asm/uv.h>
+
+#include <css.h>
+#include <virtio.h>
+#include <virtio-config.h>
+#include <virtio-ccw.h>
+#include <malloc_io.h>
+
+static struct linked_list vcdev_list = {
+	.prev = &vcdev_list,
+	.next = &vcdev_list
+};
+
+static inline uint32_t swap16(uint32_t b)
+{
+		return (((b & 0xff00U) <<  8) |
+		((b & 0x00ff) >>  8));
+}
+
+static inline uint32_t swap32(uint32_t b)
+{
+	return (((b & 0x000000ffU) << 24) |
+		((b & 0x0000ff00U) <<  8) |
+		((b & 0x00ff0000U) >>  8) |
+		((b & 0xff000000U) >> 24));
+}
+
+static inline uint64_t swap64(uint64_t x)
+{
+	return (((x & 0x00000000000000ffULL) << 56) |
+		((x & 0x000000000000ff00ULL) << 40) |
+		((x & 0x0000000000ff0000ULL) << 24) |
+		((x & 0x00000000ff000000ULL) <<  8) |
+		((x & 0x000000ff00000000ULL) >>  8) |
+		((x & 0x0000ff0000000000ULL) >> 24) |
+		((x & 0x00ff000000000000ULL) >> 40) |
+		((x & 0xff00000000000000ULL) >> 56));
+}
+
+/*
+ * flags: flags for CCW
+ * Returns !0 on failure
+ * Returns 0 on success
+ */
+int ccw_send(struct virtio_ccw_device *vcdev, int code, void *data, int count,
+	     unsigned char flags)
+{
+	struct ccw1 *ccw;
+	int ret = -1;
+
+	ccw = alloc_io_mem(sizeof(*ccw), 0);
+	if (!ccw)
+		return ret;
+
+	/* Build the CCW chain with a single CCW */
+	ccw->code = code;
+	ccw->flags = flags;
+	ccw->count = count;
+	ccw->data_address = (unsigned long)data;
+
+	ret = start_ccw1_chain(vcdev->schid, ccw);
+	if (!ret)
+		ret = wait_and_check_io_completion(vcdev->schid);
+
+	free_io_mem(ccw, sizeof(*ccw));
+	return ret;
+}
+
+int virtio_ccw_set_revision(struct virtio_ccw_device *vcdev)
+{
+	struct virtio_rev_info *rev_info;
+	int ret = -1;
+
+	rev_info = alloc_io_mem(sizeof(*rev_info), 0);
+	if (!rev_info)
+		return ret;
+
+	rev_info->revision = VIRTIO_CCW_REV_MAX;
+	rev_info->revision = 0;
+	do {
+		ret = ccw_send(vcdev, CCW_CMD_SET_VIRTIO_REV, rev_info,
+			       sizeof(*rev_info), 0);
+	} while (ret && rev_info->revision--);
+
+	free_io_mem(rev_info, sizeof(*rev_info));
+
+	return ret ? -1 : rev_info->revision;
+}
+
+int virtio_ccw_reset(struct virtio_ccw_device *vcdev)
+{
+	return ccw_send(vcdev, CCW_CMD_VDEV_RESET, 0, 0, 0);
+}
+
+int virtio_ccw_read_status(struct virtio_ccw_device *vcdev)
+{
+	return ccw_send(vcdev, CCW_CMD_READ_STATUS, &vcdev->status,
+			sizeof(vcdev->status), 0);
+}
+
+int virtio_ccw_write_status(struct virtio_ccw_device *vcdev)
+{
+	return ccw_send(vcdev, CCW_CMD_WRITE_STATUS, &vcdev->status,
+			sizeof(vcdev->status), 0);
+}
+
+int virtio_ccw_read_features(struct virtio_ccw_device *vcdev, uint64_t *features)
+{
+	struct virtio_feature_desc *f_desc = &vcdev->f_desc;
+
+	f_desc->index = 0;
+	if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
+		return -1;
+	*features = swap32(f_desc->features);
+
+	f_desc->index = 1;
+	if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
+		return -1;
+	*features |= (uint64_t)swap32(f_desc->features) << 32;
+
+	return 0;
+}
+
+int virtio_ccw_write_features(struct virtio_ccw_device *vcdev, uint64_t features)
+{
+	struct virtio_feature_desc *f_desc = &vcdev->f_desc;
+
+	f_desc->index = 0;
+	f_desc->features = swap32((uint32_t)features & 0xffffffff);
+	if (ccw_send(vcdev, CCW_CMD_WRITE_FEAT, &f_desc, sizeof(*f_desc), 0))
+		return -1;
+
+	f_desc->index = 1;
+	f_desc->features = swap32((uint32_t)(features >> 32) & 0xffffffff);
+	if (ccw_send(vcdev, CCW_CMD_WRITE_FEAT, &f_desc, sizeof(*f_desc), 0))
+		return -1;
+
+	return 0;
+}
+
+int virtio_ccw_read_config(struct virtio_ccw_device *vcdev)
+{
+	return ccw_send(vcdev, CCW_CMD_READ_CONF, &vcdev->config,
+			sizeof(vcdev->config), 0);
+}
+
+int virtio_ccw_write_config(struct virtio_ccw_device *vcdev)
+{
+	return ccw_send(vcdev, CCW_CMD_WRITE_CONF, &vcdev->config,
+			sizeof(vcdev->config), 0);
+}
+
+int virtio_ccw_setup_indicators(struct virtio_ccw_device *vcdev)
+{
+	vcdev->ind = alloc_io_mem(sizeof(PAGE_SIZE), 0);
+	if (ccw_send(vcdev, CCW_CMD_SET_IND, &vcdev->ind,
+		     sizeof(vcdev->ind), 0))
+		return -1;
+
+	vcdev->conf_ind = alloc_io_mem(PAGE_SIZE, 0);
+	if (ccw_send(vcdev, CCW_CMD_SET_CONF_IND, &vcdev->conf_ind,
+		     sizeof(vcdev->conf_ind), 0))
+		return -1;
+
+	return 0;
+}
+
+static uint64_t virtio_ccw_notify_host(int schid, int queue, uint64_t cookie)
+{
+	register unsigned long nr asm("1") = 0x03;
+	register unsigned long s asm("2") = schid;
+	register unsigned long q asm("3") = queue;
+	register long rc asm("2");
+	register long c asm("4") = cookie;
+
+	asm volatile ("diag 2,4,0x500\n"
+			: "=d" (rc)
+			: "d" (nr), "d" (s), "d" (q), "d"(c)
+			: "memory", "cc");
+	return rc;
+}
+
+static bool virtio_ccw_notify(struct virtqueue *vq)
+{
+	struct virtio_ccw_device *vcdev = to_vc_device(vq->vdev);
+	struct virtio_ccw_vq_info *info = vq->priv;
+
+	info->cookie = virtio_ccw_notify_host(vcdev->schid, vq->index,
+					      info->cookie);
+	if (info->cookie < 0)
+		return false;
+	return true;
+}
+
+/* allocates a vring_virtqueue but returns a pointer to the
+ * virtqueue inside of it or NULL on error.
+ */
+static struct virtqueue *setup_vq(struct virtio_device *vdev, int index,
+				  void (*callback)(struct virtqueue *vq),
+				  const char *name)
+{
+	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
+	struct virtio_ccw_vq_info *info;
+	struct vring_virtqueue *vq;
+	struct vring *vr;
+	void *queue;
+
+	vq = alloc_io_mem(sizeof(*vq), 0);
+	info = alloc_io_mem(sizeof(*info), 0);
+	queue = alloc_io_mem(4 * PAGE_SIZE, 0);
+	assert(vq && queue && info);
+
+	info->info_block = alloc_io_mem(sizeof(*info->info_block), 0);
+	assert(info->info_block);
+
+	vcdev->vq_conf.index = index;
+	if (ccw_send(vcdev, CCW_CMD_READ_VQ_CONF, &vcdev->vq_conf,
+		     sizeof(vcdev->vq_conf), 0))
+		return NULL;
+
+	vring_init_virtqueue(vq, index, vcdev->vq_conf.max_num, PAGE_SIZE, vdev,
+			     queue, virtio_ccw_notify, callback, name);
+
+	vr = &vq->vring;
+	info->info_block->s.desc = vr->desc;
+	info->info_block->s.index = index;
+	info->info_block->s.num = vr->num;
+	info->info_block->s.avail = vr->avail;
+	info->info_block->s.used = vr->used;
+
+	info->info_block->l.desc = vr->desc;
+	info->info_block->l.index = index;
+	info->info_block->l.num = vr->num;
+	info->info_block->l.align = PAGE_SIZE;
+
+	if (ccw_send(vcdev, CCW_CMD_SET_VQ, info->info_block,
+		     sizeof(info->info_block->l), 0))
+		return NULL;
+
+	info->vq = &vq->vq;
+	vq->vq.priv = info;
+
+	return &vq->vq;
+}
+
+static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
+			       struct virtqueue *vqs[], vq_callback_t *callbacks[],
+			       const char *names[])
+{
+	int i;
+
+	for (i = 0; i < nvqs; ++i) {
+		vqs[i] = setup_vq(vdev, i,
+				  callbacks ? callbacks[i] : NULL,
+				  names ? names[i] : "");
+		if (!vqs[i])
+			return -1;
+	}
+
+	return 0;
+}
+
+static void virtio_ccw_config_get(struct virtio_device *vdev,
+				  unsigned int offset, void *buf,
+				  unsigned int len)
+{
+	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
+
+	if (virtio_ccw_read_config(vcdev))
+		return;
+	memcpy(buf, vcdev->config, len);
+}
+
+static void virtio_ccw_config_set(struct virtio_device *vdev,
+				  unsigned int offset, const void *buf,
+				  unsigned int len)
+{
+	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
+
+	memcpy(vcdev->config, buf, len);
+	virtio_ccw_write_config(vcdev);
+}
+
+static const struct virtio_config_ops virtio_ccw_ops = {
+	.get = virtio_ccw_config_get,
+	.set = virtio_ccw_config_set,
+	.find_vqs = virtio_ccw_find_vqs,
+};
+
+const struct virtio_config_ops *virtio_ccw_register(void)
+{
+	return &virtio_ccw_ops;
+}
+
+static int sense(struct virtio_ccw_device *vcdev)
+{
+	struct senseid *senseid;
+
+	senseid = alloc_io_mem(sizeof(*senseid), 0);
+	assert(senseid);
+
+	assert(!ccw_send(vcdev, CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), 0));
+
+	assert(senseid->reserved == 0xff);
+
+	vcdev->cu_type = senseid->cu_type;
+	vcdev->cu_model = senseid->cu_model;
+	vcdev->dev_type = senseid->dev_type;
+	vcdev->dev_model = senseid->dev_model;
+
+	return 0;
+}
+
+static struct virtio_ccw_device *find_vcdev_by_devid(int devid)
+{
+	struct virtio_ccw_device *dev;
+	struct linked_list *l;
+
+	for (l = vcdev_list.next; l != &vcdev_list; l = l->next) {
+		dev = container_of(l, struct virtio_ccw_device, list);
+		if (dev->cu_model == devid)
+			return dev;
+	}
+	return NULL;
+}
+
+struct virtio_device *virtio_bind(u32 devid)
+{
+	struct virtio_ccw_device *vcdev;
+
+	vcdev = find_vcdev_by_devid(devid);
+
+	return &vcdev->vdev;
+}
+
+static int virtio_enumerate(int schid)
+{
+	struct virtio_ccw_device *vcdev;
+
+	vcdev = alloc_io_mem(sizeof(*vcdev), 0);
+	assert(vcdev);
+	vcdev->schid = schid;
+
+	list_add(&vcdev_list, &vcdev->list);
+
+	assert(css_enable(schid, IO_SCH_ISC) == 0);
+	sense(vcdev);
+
+	return 0;
+}
+
+/* Must get a param */
+bool virtio_ccw_init(void)
+{
+	return css_enumerate(virtio_enumerate) != 0;
+}
diff --git a/lib/s390x/virtio-ccw.h b/lib/s390x/virtio-ccw.h
new file mode 100644
index 00000000..961d8bed
--- /dev/null
+++ b/lib/s390x/virtio-ccw.h
@@ -0,0 +1,111 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * VIRTIO-CCW definitions
+ *
+ * Copyright IBM, Corp. 2020
+ * Author: Pierre Morel <pmorel@linux.ibm.com>
+ *
+ */
+
+#ifndef VIRTIO_CCW_H
+#define VIRTIO_CCW_H
+
+#define CCW_CMD_WRITE_FEAT	0x11
+#define CCW_CMD_READ_FEAT	0x12
+#define CCW_CMD_SET_VQ		0x13
+#define CCW_CMD_WRITE_CONF	0x21
+#define CCW_CMD_READ_CONF	0x22
+#define CCW_CMD_WRITE_STATUS	0x31
+#define CCW_CMD_READ_VQ_CONF	0x32
+#define CCW_CMD_VDEV_RESET	0x33
+#define CCW_CMD_SET_IND		0x43
+#define CCW_CMD_SET_CONF_IND	0x53
+#define CCW_CMD_READ_STATUS	0x72
+#define CCW_CMD_SET_IND_ADAPTER	0x73
+#define CCW_CMD_SET_VIRTIO_REV	0x83
+
+struct virtio_rev_info {
+#define VIRTIO_CCW_REV_MAX 1
+	uint16_t revision;
+	uint16_t length;
+	uint8_t	data[];
+};
+
+struct virtio_feature_desc {
+	uint32_t features;
+	uint8_t index;
+} __attribute__ ((packed));
+
+struct vq_config_block {
+	uint16_t index;
+	uint16_t max_num;
+};
+
+struct vq_info_block_legacy {
+	struct vring_desc *desc;
+	uint32_t align;
+	uint16_t index;
+	uint16_t num;
+} __attribute__ ((packed));
+
+struct vq_info_block {
+	struct vring_desc *desc;
+	uint32_t res0;
+	uint16_t index;
+	uint16_t num;
+	struct vring_avail *avail;
+	struct vring_used *used;
+} __attribute__ ((packed));
+
+#define VIRTIO_CCW_CONFIG_SIZE	0x100
+
+#include <list.h>
+struct virtio_ccw_device {
+	struct virtio_device vdev;
+	uint8_t config[VIRTIO_CCW_CONFIG_SIZE];
+	struct virtio_config_ops *config_ops;
+	struct virtio_feature_desc f_desc;
+	struct vq_config_block vq_conf;
+	struct linked_list list;
+	int schid;
+	uint16_t dev_type;
+	uint8_t dev_model;
+	uint16_t cu_type;
+	uint8_t cu_model;
+	uint8_t status;
+	uint64_t *ind;
+	uint64_t *conf_ind;
+#define VCDEV_INIT	1
+#define VCDEV_ERROR	2
+	int state;
+};
+
+struct virtio_ccw_vq_info {
+	struct virtqueue *vq;
+	int num;
+	union {
+		struct vq_info_block s;
+		struct vq_info_block_legacy l;
+	} *info_block;
+	int bit_nr;
+	long cookie;
+};
+
+#define to_vc_device(vdev_ptr) \
+	container_of(vdev_ptr, struct virtio_ccw_device, vdev)
+
+int ccw_send(struct virtio_ccw_device *vdev, int code, void *data, int count,
+	     unsigned char flags);
+int virtio_ccw_set_revision(struct virtio_ccw_device *vdev);
+int virtio_ccw_reset(struct virtio_ccw_device *vdev);
+int virtio_ccw_read_config(struct virtio_ccw_device *vdev);
+int virtio_ccw_write_config(struct virtio_ccw_device *vdev);
+int virtio_ccw_read_status(struct virtio_ccw_device *vdev);
+int virtio_ccw_write_status(struct virtio_ccw_device *vdev);
+int virtio_ccw_read_features(struct virtio_ccw_device *vdev, uint64_t *f);
+int virtio_ccw_write_features(struct virtio_ccw_device *vdev, uint64_t f);
+int virtio_ccw_setup_indicators(struct virtio_ccw_device *vdev);
+
+bool virtio_ccw_init(void);
+const struct virtio_config_ops *virtio_ccw_register(void);
+#endif
diff --git a/lib/virtio-config.h b/lib/virtio-config.h
new file mode 100644
index 00000000..3507304c
--- /dev/null
+++ b/lib/virtio-config.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _VIRTIO_CONFIG_H_
+#define _VIRTIO_CONFIG_H_
+
+#define VIRTIO_CONFIG_S_ACKNOWLEDGE	0x01
+#define VIRTIO_CONFIG_S_DRIVER		0x02
+#define VIRTIO_CONFIG_S_DRIVER_OK	0x04
+#define VIRTIO_CONFIG_S_FEATURES_OK	0x08
+#define VIRTIO_CONFIG_S_NEEDS_RESET	0x40
+#define VIRTIO_CONFIG_S_FAILED		0x80
+
+/* Generic VIRTIO Features */
+#define VIRTIO_F_NOTIFY_ON_EMPTY	24
+#define VIRTIO_F_ANY_LAYOUT		27
+
+/* Transport specific features */
+#define VIRTIO_TRANSPORT_F_START	28
+#define VIRTIO_F_RING_INDIRECT_DESC	28
+#define VIRTIO_F_RING_EVENT_IDX		29
+#define VIRTIO_TRANSPORT_F_END		38
+
+/* New features starting with version 1 */
+#define VIRTIO_F_VERSION_1		32
+#define VIRTIO_F_IOMMU_PLATFORM		33
+#define VIRTIO_F_RING_PACKED		34
+#define VIRTIO_F_ORDER_PLATFORM		36
+#define VIRTIO_F_SR_IOV			37
+
+#endif /* _VIRTIO_CONFIG_H_ */
diff --git a/s390x/Makefile b/s390x/Makefile
index 6565561b..3f4acc3e 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -71,6 +71,8 @@ cflatobjs += lib/s390x/css_dump.o
 cflatobjs += lib/s390x/css_lib.o
 cflatobjs += lib/s390x/malloc_io.o
 cflatobjs += lib/s390x/uv.o
+cflatobjs += lib/virtio.o
+cflatobjs += lib/s390x/virtio-ccw.o
 
 OBJDIRS += lib/s390x
 
-- 
2.25.1

Re: [kvm-unit-tests PATCH 3/7] s390x: virtio: CCW transport implementation

[Andrew Jones]

On Fri, Aug 27, 2021 at 12:17:16PM +0200, Pierre Morel wrote:
> This is the implementation of the virtio-ccw transport level.
> 
> We only support VIRTIO revision 0.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/s390x/virtio-ccw.c | 374 +++++++++++++++++++++++++++++++++++++++++
>  lib/s390x/virtio-ccw.h | 111 ++++++++++++
>  lib/virtio-config.h    |  30 ++++

I'd probably just drop these config defines into lib/virtio.h.

Thanks,
drew

Re: [kvm-unit-tests PATCH 3/7] s390x: virtio: CCW transport implementation

[Pierre Morel]

On 11/3/21 08:46, Andrew Jones wrote:
> On Fri, Aug 27, 2021 at 12:17:16PM +0200, Pierre Morel wrote:
>> This is the implementation of the virtio-ccw transport level.
>>
>> We only support VIRTIO revision 0.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/virtio-ccw.c | 374 +++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/virtio-ccw.h | 111 ++++++++++++
>>   lib/virtio-config.h    |  30 ++++
> 
> I'd probably just drop these config defines into lib/virtio.h.
> 
> Thanks,
> drew
> 

I will do that.
Thanks,

Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH 3/7] s390x: virtio: CCW transport implementation

[Thomas Huth]

On 27/08/2021 12.17, Pierre Morel wrote:
> This is the implementation of the virtio-ccw transport level.
> 
> We only support VIRTIO revision 0.

That means only legacy virtio? Wouldn't it be better to shoot for modern 
virtio instead?

> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/virtio-ccw.c | 374 +++++++++++++++++++++++++++++++++++++++++
>   lib/s390x/virtio-ccw.h | 111 ++++++++++++
>   lib/virtio-config.h    |  30 ++++
>   s390x/Makefile         |   2 +
>   4 files changed, 517 insertions(+)
>   create mode 100644 lib/s390x/virtio-ccw.c
>   create mode 100644 lib/s390x/virtio-ccw.h
>   create mode 100644 lib/virtio-config.h
> 
> diff --git a/lib/s390x/virtio-ccw.c b/lib/s390x/virtio-ccw.c
> new file mode 100644
> index 00000000..cf447de6
> --- /dev/null
> +++ b/lib/s390x/virtio-ccw.c
> @@ -0,0 +1,374 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Virtio CCW Library
> + *
> + * Copyright (c) 2021 IBM Corp
> + *
> + * Authors:
> + *  Pierre Morel <pmorel@linux.ibm.com>
> + *
> + */
> +
> +#include <libcflat.h>
> +#include <alloc_page.h>
> +#include <asm/page.h>
> +#include <string.h>
> +#include <interrupt.h>
> +#include <asm/arch_def.h>
> +#include <asm/facility.h>
> +#include <asm/uv.h>
> +
> +#include <css.h>
> +#include <virtio.h>
> +#include <virtio-config.h>
> +#include <virtio-ccw.h>
> +#include <malloc_io.h>
> +
> +static struct linked_list vcdev_list = {
> +	.prev = &vcdev_list,
> +	.next = &vcdev_list
> +};
> +
> +static inline uint32_t swap16(uint32_t b)
> +{
> +		return (((b & 0xff00U) <<  8) |
> +		((b & 0x00ff) >>  8));
> +}
> +
> +static inline uint32_t swap32(uint32_t b)
> +{
> +	return (((b & 0x000000ffU) << 24) |
> +		((b & 0x0000ff00U) <<  8) |
> +		((b & 0x00ff0000U) >>  8) |
> +		((b & 0xff000000U) >> 24));
> +}
> +
> +static inline uint64_t swap64(uint64_t x)
> +{
> +	return (((x & 0x00000000000000ffULL) << 56) |
> +		((x & 0x000000000000ff00ULL) << 40) |
> +		((x & 0x0000000000ff0000ULL) << 24) |
> +		((x & 0x00000000ff000000ULL) <<  8) |
> +		((x & 0x000000ff00000000ULL) >>  8) |
> +		((x & 0x0000ff0000000000ULL) >> 24) |
> +		((x & 0x00ff000000000000ULL) >> 40) |
> +		((x & 0xff00000000000000ULL) >> 56));
> +}

We already have macros for swapping in lib/asm-generic/io.h ... could you 
use those instead?

> +/*
> + * flags: flags for CCW
> + * Returns !0 on failure
> + * Returns 0 on success
> + */
> +int ccw_send(struct virtio_ccw_device *vcdev, int code, void *data, int count,
> +	     unsigned char flags)
> +{
> +	struct ccw1 *ccw;
> +	int ret = -1;
> +
> +	ccw = alloc_io_mem(sizeof(*ccw), 0);
> +	if (!ccw)
> +		return ret;
> +
> +	/* Build the CCW chain with a single CCW */
> +	ccw->code = code;
> +	ccw->flags = flags;
> +	ccw->count = count;
> +	ccw->data_address = (unsigned long)data;
> +
> +	ret = start_ccw1_chain(vcdev->schid, ccw);
> +	if (!ret)
> +		ret = wait_and_check_io_completion(vcdev->schid);
> +
> +	free_io_mem(ccw, sizeof(*ccw));
> +	return ret;
> +}
> +
> +int virtio_ccw_set_revision(struct virtio_ccw_device *vcdev)
> +{
> +	struct virtio_rev_info *rev_info;
> +	int ret = -1;
> +
> +	rev_info = alloc_io_mem(sizeof(*rev_info), 0);
> +	if (!rev_info)
> +		return ret;
> +
> +	rev_info->revision = VIRTIO_CCW_REV_MAX;
> +	rev_info->revision = 0;

Either VIRTIO_CCW_REV_MAX or 0, but not both?

> +	do {
> +		ret = ccw_send(vcdev, CCW_CMD_SET_VIRTIO_REV, rev_info,
> +			       sizeof(*rev_info), 0);
> +	} while (ret && rev_info->revision--);
> +
> +	free_io_mem(rev_info, sizeof(*rev_info));
> +
> +	return ret ? -1 : rev_info->revision;
> +}
> +
> +int virtio_ccw_reset(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_VDEV_RESET, 0, 0, 0);
> +}
> +
> +int virtio_ccw_read_status(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_READ_STATUS, &vcdev->status,
> +			sizeof(vcdev->status), 0);
> +}
> +
> +int virtio_ccw_write_status(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_WRITE_STATUS, &vcdev->status,
> +			sizeof(vcdev->status), 0);
> +}
> +
> +int virtio_ccw_read_features(struct virtio_ccw_device *vcdev, uint64_t *features)
> +{
> +	struct virtio_feature_desc *f_desc = &vcdev->f_desc;
> +
> +	f_desc->index = 0;
> +	if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
> +		return -1;
> +	*features = swap32(f_desc->features);
> +
> +	f_desc->index = 1;
> +	if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
> +		return -1;
> +	*features |= (uint64_t)swap32(f_desc->features) << 32;

Weren't the upper feature bits only available for modern virtio anyway?

> +	return 0;
> +}
> +
> +int virtio_ccw_write_features(struct virtio_ccw_device *vcdev, uint64_t features)
> +{
> +	struct virtio_feature_desc *f_desc = &vcdev->f_desc;
> +
> +	f_desc->index = 0;
> +	f_desc->features = swap32((uint32_t)features & 0xffffffff);
> +	if (ccw_send(vcdev, CCW_CMD_WRITE_FEAT, &f_desc, sizeof(*f_desc), 0))
> +		return -1;
> +
> +	f_desc->index = 1;
> +	f_desc->features = swap32((uint32_t)(features >> 32) & 0xffffffff);
> +	if (ccw_send(vcdev, CCW_CMD_WRITE_FEAT, &f_desc, sizeof(*f_desc), 0))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +int virtio_ccw_read_config(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_READ_CONF, &vcdev->config,
> +			sizeof(vcdev->config), 0);
> +}
> +
> +int virtio_ccw_write_config(struct virtio_ccw_device *vcdev)
> +{
> +	return ccw_send(vcdev, CCW_CMD_WRITE_CONF, &vcdev->config,
> +			sizeof(vcdev->config), 0);
> +}
> +
> +int virtio_ccw_setup_indicators(struct virtio_ccw_device *vcdev)
> +{
> +	vcdev->ind = alloc_io_mem(sizeof(PAGE_SIZE), 0);
> +	if (ccw_send(vcdev, CCW_CMD_SET_IND, &vcdev->ind,
> +		     sizeof(vcdev->ind), 0))
> +		return -1;
> +
> +	vcdev->conf_ind = alloc_io_mem(PAGE_SIZE, 0);
> +	if (ccw_send(vcdev, CCW_CMD_SET_CONF_IND, &vcdev->conf_ind,
> +		     sizeof(vcdev->conf_ind), 0))
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static uint64_t virtio_ccw_notify_host(int schid, int queue, uint64_t cookie)
> +{
> +	register unsigned long nr asm("1") = 0x03;
> +	register unsigned long s asm("2") = schid;
> +	register unsigned long q asm("3") = queue;
> +	register long rc asm("2");

Using asm("2") for two variables looks somewhat weird ... but ok, as long as 
it works... (otherwise you could also use only one variable and mark it as 
input + output parameter below).

> +	register long c asm("4") = cookie;
> +
> +	asm volatile ("diag 2,4,0x500\n"
> +			: "=d" (rc)
> +			: "d" (nr), "d" (s), "d" (q), "d"(c)
> +			: "memory", "cc");
> +	return rc;
> +}
> +
> +static bool virtio_ccw_notify(struct virtqueue *vq)
> +{
> +	struct virtio_ccw_device *vcdev = to_vc_device(vq->vdev);
> +	struct virtio_ccw_vq_info *info = vq->priv;
> +
> +	info->cookie = virtio_ccw_notify_host(vcdev->schid, vq->index,
> +					      info->cookie);
> +	if (info->cookie < 0)
> +		return false;
> +	return true;
> +}
> +
> +/* allocates a vring_virtqueue but returns a pointer to the
> + * virtqueue inside of it or NULL on error.
> + */
> +static struct virtqueue *setup_vq(struct virtio_device *vdev, int index,
> +				  void (*callback)(struct virtqueue *vq),
> +				  const char *name)
> +{
> +	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> +	struct virtio_ccw_vq_info *info;
> +	struct vring_virtqueue *vq;
> +	struct vring *vr;
> +	void *queue;
> +
> +	vq = alloc_io_mem(sizeof(*vq), 0);
> +	info = alloc_io_mem(sizeof(*info), 0);
> +	queue = alloc_io_mem(4 * PAGE_SIZE, 0);
> +	assert(vq && queue && info);
> +
> +	info->info_block = alloc_io_mem(sizeof(*info->info_block), 0);
> +	assert(info->info_block);
> +
> +	vcdev->vq_conf.index = index;
> +	if (ccw_send(vcdev, CCW_CMD_READ_VQ_CONF, &vcdev->vq_conf,
> +		     sizeof(vcdev->vq_conf), 0))
> +		return NULL;
> +
> +	vring_init_virtqueue(vq, index, vcdev->vq_conf.max_num, PAGE_SIZE, vdev,
> +			     queue, virtio_ccw_notify, callback, name);
> +
> +	vr = &vq->vring;
> +	info->info_block->s.desc = vr->desc;
> +	info->info_block->s.index = index;
> +	info->info_block->s.num = vr->num;
> +	info->info_block->s.avail = vr->avail;
> +	info->info_block->s.used = vr->used;
> +
> +	info->info_block->l.desc = vr->desc;
> +	info->info_block->l.index = index;
> +	info->info_block->l.num = vr->num;
> +	info->info_block->l.align = PAGE_SIZE;
> +
> +	if (ccw_send(vcdev, CCW_CMD_SET_VQ, info->info_block,
> +		     sizeof(info->info_block->l), 0))
> +		return NULL;
> +
> +	info->vq = &vq->vq;
> +	vq->vq.priv = info;
> +
> +	return &vq->vq;
> +}
> +
> +static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> +			       struct virtqueue *vqs[], vq_callback_t *callbacks[],
> +			       const char *names[])
> +{
> +	int i;
> +
> +	for (i = 0; i < nvqs; ++i) {
> +		vqs[i] = setup_vq(vdev, i,
> +				  callbacks ? callbacks[i] : NULL,
> +				  names ? names[i] : "");
> +		if (!vqs[i])
> +			return -1;
> +	}
> +
> +	return 0;
> +}
> +
> +static void virtio_ccw_config_get(struct virtio_device *vdev,
> +				  unsigned int offset, void *buf,
> +				  unsigned int len)
> +{
> +	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> +
> +	if (virtio_ccw_read_config(vcdev))
> +		return;
> +	memcpy(buf, vcdev->config, len);
> +}
> +
> +static void virtio_ccw_config_set(struct virtio_device *vdev,
> +				  unsigned int offset, const void *buf,
> +				  unsigned int len)
> +{
> +	struct virtio_ccw_device *vcdev = to_vc_device(vdev);
> +
> +	memcpy(vcdev->config, buf, len);
> +	virtio_ccw_write_config(vcdev);
> +}
> +
> +static const struct virtio_config_ops virtio_ccw_ops = {
> +	.get = virtio_ccw_config_get,
> +	.set = virtio_ccw_config_set,
> +	.find_vqs = virtio_ccw_find_vqs,
> +};
> +
> +const struct virtio_config_ops *virtio_ccw_register(void)
> +{
> +	return &virtio_ccw_ops;
> +}
> +
> +static int sense(struct virtio_ccw_device *vcdev)
> +{
> +	struct senseid *senseid;
> +
> +	senseid = alloc_io_mem(sizeof(*senseid), 0);
> +	assert(senseid);
> +
> +	assert(!ccw_send(vcdev, CCW_CMD_SENSE_ID, senseid, sizeof(*senseid), 0));
> +
> +	assert(senseid->reserved == 0xff);
> +
> +	vcdev->cu_type = senseid->cu_type;
> +	vcdev->cu_model = senseid->cu_model;
> +	vcdev->dev_type = senseid->dev_type;
> +	vcdev->dev_model = senseid->dev_model;
> +
> +	return 0;
> +}
> +
> +static struct virtio_ccw_device *find_vcdev_by_devid(int devid)
> +{
> +	struct virtio_ccw_device *dev;
> +	struct linked_list *l;
> +
> +	for (l = vcdev_list.next; l != &vcdev_list; l = l->next) {
> +		dev = container_of(l, struct virtio_ccw_device, list);
> +		if (dev->cu_model == devid)
> +			return dev;
> +	}
> +	return NULL;
> +}
> +
> +struct virtio_device *virtio_bind(u32 devid)
> +{
> +	struct virtio_ccw_device *vcdev;
> +
> +	vcdev = find_vcdev_by_devid(devid);
> +
> +	return &vcdev->vdev;
> +}
> +
> +static int virtio_enumerate(int schid)
> +{
> +	struct virtio_ccw_device *vcdev;
> +
> +	vcdev = alloc_io_mem(sizeof(*vcdev), 0);
> +	assert(vcdev);
> +	vcdev->schid = schid;
> +
> +	list_add(&vcdev_list, &vcdev->list);
> +
> +	assert(css_enable(schid, IO_SCH_ISC) == 0);
> +	sense(vcdev);
> +
> +	return 0;
> +}
> +
> +/* Must get a param */

I don't understand that comment, could you elaborate?

> +bool virtio_ccw_init(void)
> +{
> +	return css_enumerate(virtio_enumerate) != 0;
> +}

  Thomas

Re: [kvm-unit-tests PATCH 3/7] s390x: virtio: CCW transport implementation

[Pierre Morel]

On 11/3/21 08:49, Thomas Huth wrote:
> On 27/08/2021 12.17, Pierre Morel wrote:
>> This is the implementation of the virtio-ccw transport level.
>>
>> We only support VIRTIO revision 0.
> 
> That means only legacy virtio? Wouldn't it be better to shoot for modern 
> virtio instead?

Yes but can we do it in a second series?
We will need more chqnges in the comon libraries.


> 
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/virtio-ccw.c | 374 +++++++++++++++++++++++++++++++++++++++++
>>   lib/s390x/virtio-ccw.h | 111 ++++++++++++
>>   lib/virtio-config.h    |  30 ++++
>>   s390x/Makefile         |   2 +
>>   4 files changed, 517 insertions(+)
>>   create mode 100644 lib/s390x/virtio-ccw.c
>>   create mode 100644 lib/s390x/virtio-ccw.h
>>   create mode 100644 lib/virtio-config.h
>>
>> diff --git a/lib/s390x/virtio-ccw.c b/lib/s390x/virtio-ccw.c
>> new file mode 100644
>> index 00000000..cf447de6
>> --- /dev/null
>> +++ b/lib/s390x/virtio-ccw.c
>> @@ -0,0 +1,374 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Virtio CCW Library
>> + *
>> + * Copyright (c) 2021 IBM Corp
>> + *
>> + * Authors:
>> + *  Pierre Morel <pmorel@linux.ibm.com>
>> + *
>> + */
>> +
>> +#include <libcflat.h>
>> +#include <alloc_page.h>
>> +#include <asm/page.h>
>> +#include <string.h>
>> +#include <interrupt.h>
>> +#include <asm/arch_def.h>
>> +#include <asm/facility.h>
>> +#include <asm/uv.h>
>> +
>> +#include <css.h>
>> +#include <virtio.h>
>> +#include <virtio-config.h>
>> +#include <virtio-ccw.h>
>> +#include <malloc_io.h>
>> +
>> +static struct linked_list vcdev_list = {
>> +    .prev = &vcdev_list,
>> +    .next = &vcdev_list
>> +};
>> +
>> +static inline uint32_t swap16(uint32_t b)
>> +{
>> +        return (((b & 0xff00U) <<  8) |
>> +        ((b & 0x00ff) >>  8));
>> +}
>> +
>> +static inline uint32_t swap32(uint32_t b)
>> +{
>> +    return (((b & 0x000000ffU) << 24) |
>> +        ((b & 0x0000ff00U) <<  8) |
>> +        ((b & 0x00ff0000U) >>  8) |
>> +        ((b & 0xff000000U) >> 24));
>> +}
>> +
>> +static inline uint64_t swap64(uint64_t x)
>> +{
>> +    return (((x & 0x00000000000000ffULL) << 56) |
>> +        ((x & 0x000000000000ff00ULL) << 40) |
>> +        ((x & 0x0000000000ff0000ULL) << 24) |
>> +        ((x & 0x00000000ff000000ULL) <<  8) |
>> +        ((x & 0x000000ff00000000ULL) >>  8) |
>> +        ((x & 0x0000ff0000000000ULL) >> 24) |
>> +        ((x & 0x00ff000000000000ULL) >> 40) |
>> +        ((x & 0xff00000000000000ULL) >> 56));
>> +}
> 
> We already have macros for swapping in lib/asm-generic/io.h ... could 
> you use those instead?

Yes, I will.

> 
>> +/*
>> + * flags: flags for CCW
>> + * Returns !0 on failure
>> + * Returns 0 on success
>> + */
>> +int ccw_send(struct virtio_ccw_device *vcdev, int code, void *data, 
>> int count,
>> +         unsigned char flags)
>> +{
>> +    struct ccw1 *ccw;
>> +    int ret = -1;
>> +
>> +    ccw = alloc_io_mem(sizeof(*ccw), 0);
>> +    if (!ccw)
>> +        return ret;
>> +
>> +    /* Build the CCW chain with a single CCW */
>> +    ccw->code = code;
>> +    ccw->flags = flags;
>> +    ccw->count = count;
>> +    ccw->data_address = (unsigned long)data;
>> +
>> +    ret = start_ccw1_chain(vcdev->schid, ccw);
>> +    if (!ret)
>> +        ret = wait_and_check_io_completion(vcdev->schid);
>> +
>> +    free_io_mem(ccw, sizeof(*ccw));
>> +    return ret;
>> +}
>> +
>> +int virtio_ccw_set_revision(struct virtio_ccw_device *vcdev)
>> +{
>> +    struct virtio_rev_info *rev_info;
>> +    int ret = -1;
>> +
>> +    rev_info = alloc_io_mem(sizeof(*rev_info), 0);
>> +    if (!rev_info)
>> +        return ret;
>> +
>> +    rev_info->revision = VIRTIO_CCW_REV_MAX;
>> +    rev_info->revision = 0;
> 
> Either VIRTIO_CCW_REV_MAX or 0, but not both?

:D
Seems the second line has been forgotten.


> 
>> +    do {
>> +        ret = ccw_send(vcdev, CCW_CMD_SET_VIRTIO_REV, rev_info,
>> +                   sizeof(*rev_info), 0);
>> +    } while (ret && rev_info->revision--);
>> +
>> +    free_io_mem(rev_info, sizeof(*rev_info));
>> +
>> +    return ret ? -1 : rev_info->revision;
>> +}
>> +
>> +int virtio_ccw_reset(struct virtio_ccw_device *vcdev)
>> +{
>> +    return ccw_send(vcdev, CCW_CMD_VDEV_RESET, 0, 0, 0);
>> +}
>> +
>> +int virtio_ccw_read_status(struct virtio_ccw_device *vcdev)
>> +{
>> +    return ccw_send(vcdev, CCW_CMD_READ_STATUS, &vcdev->status,
>> +            sizeof(vcdev->status), 0);
>> +}
>> +
>> +int virtio_ccw_write_status(struct virtio_ccw_device *vcdev)
>> +{
>> +    return ccw_send(vcdev, CCW_CMD_WRITE_STATUS, &vcdev->status,
>> +            sizeof(vcdev->status), 0);
>> +}
>> +
>> +int virtio_ccw_read_features(struct virtio_ccw_device *vcdev, 
>> uint64_t *features)
>> +{
>> +    struct virtio_feature_desc *f_desc = &vcdev->f_desc;
>> +
>> +    f_desc->index = 0;
>> +    if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
>> +        return -1;
>> +    *features = swap32(f_desc->features);
>> +
>> +    f_desc->index = 1;
>> +    if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
>> +        return -1;
>> +    *features |= (uint64_t)swap32(f_desc->features) << 32;
> 
> Weren't the upper feature bits only available for modern virtio anyway?

Yes.
I have the intention to upgrade to Rev. 1 when I get enough time for it.
Should I remove this? It does not induce problem does it?

> 
>> +    return 0;
>> +}
>> +
>> +int virtio_ccw_write_features(struct virtio_ccw_device *vcdev, 
>> uint64_t features)
>> +{
>> +    struct virtio_feature_desc *f_desc = &vcdev->f_desc;
>> +
>> +    f_desc->index = 0;
>> +    f_desc->features = swap32((uint32_t)features & 0xffffffff);
>> +    if (ccw_send(vcdev, CCW_CMD_WRITE_FEAT, &f_desc, sizeof(*f_desc), 
>> 0))
>> +        return -1;
>> +
>> +    f_desc->index = 1;
>> +    f_desc->features = swap32((uint32_t)(features >> 32) & 0xffffffff);
>> +    if (ccw_send(vcdev, CCW_CMD_WRITE_FEAT, &f_desc, sizeof(*f_desc), 
>> 0))
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +int virtio_ccw_read_config(struct virtio_ccw_device *vcdev)
>> +{
>> +    return ccw_send(vcdev, CCW_CMD_READ_CONF, &vcdev->config,
>> +            sizeof(vcdev->config), 0);
>> +}
>> +
>> +int virtio_ccw_write_config(struct virtio_ccw_device *vcdev)
>> +{
>> +    return ccw_send(vcdev, CCW_CMD_WRITE_CONF, &vcdev->config,
>> +            sizeof(vcdev->config), 0);
>> +}
>> +
>> +int virtio_ccw_setup_indicators(struct virtio_ccw_device *vcdev)
>> +{
>> +    vcdev->ind = alloc_io_mem(sizeof(PAGE_SIZE), 0);
>> +    if (ccw_send(vcdev, CCW_CMD_SET_IND, &vcdev->ind,
>> +             sizeof(vcdev->ind), 0))
>> +        return -1;
>> +
>> +    vcdev->conf_ind = alloc_io_mem(PAGE_SIZE, 0);
>> +    if (ccw_send(vcdev, CCW_CMD_SET_CONF_IND, &vcdev->conf_ind,
>> +             sizeof(vcdev->conf_ind), 0))
>> +        return -1;
>> +
>> +    return 0;
>> +}
>> +
>> +static uint64_t virtio_ccw_notify_host(int schid, int queue, uint64_t 
>> cookie)
>> +{
>> +    register unsigned long nr asm("1") = 0x03;
>> +    register unsigned long s asm("2") = schid;
>> +    register unsigned long q asm("3") = queue;
>> +    register long rc asm("2");
> 
> Using asm("2") for two variables looks somewhat weird ... but ok, as 
> long as it works... (otherwise you could also use only one variable and 
> mark it as input + output parameter below).

OK

> 
>> +    register long c asm("4") = cookie;
>> +
>> +    asm volatile ("diag 2,4,0x500\n"
>> +            : "=d" (rc)
>> +            : "d" (nr), "d" (s), "d" (q), "d"(c)
>> +            : "memory", "cc");
>> +    return rc;
>> +}
>> +
>> +static bool virtio_ccw_notify(struct virtqueue *vq)
>> +{
>> +    struct virtio_ccw_device *vcdev = to_vc_device(vq->vdev);
>> +    struct virtio_ccw_vq_info *info = vq->priv;
>> +
>> +    info->cookie = virtio_ccw_notify_host(vcdev->schid, vq->index,
>> +                          info->cookie);
>> +    if (info->cookie < 0)
>> +        return false;
>> +    return true;
>> +}
>> +
>> +/* allocates a vring_virtqueue but returns a pointer to the
>> + * virtqueue inside of it or NULL on error.
>> + */
>> +static struct virtqueue *setup_vq(struct virtio_device *vdev, int index,
>> +                  void (*callback)(struct virtqueue *vq),
>> +                  const char *name)
>> +{
>> +    struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>> +    struct virtio_ccw_vq_info *info;
>> +    struct vring_virtqueue *vq;
>> +    struct vring *vr;
>> +    void *queue;
>> +
>> +    vq = alloc_io_mem(sizeof(*vq), 0);
>> +    info = alloc_io_mem(sizeof(*info), 0);
>> +    queue = alloc_io_mem(4 * PAGE_SIZE, 0);
>> +    assert(vq && queue && info);
>> +
>> +    info->info_block = alloc_io_mem(sizeof(*info->info_block), 0);
>> +    assert(info->info_block);
>> +
>> +    vcdev->vq_conf.index = index;
>> +    if (ccw_send(vcdev, CCW_CMD_READ_VQ_CONF, &vcdev->vq_conf,
>> +             sizeof(vcdev->vq_conf), 0))
>> +        return NULL;
>> +
>> +    vring_init_virtqueue(vq, index, vcdev->vq_conf.max_num, 
>> PAGE_SIZE, vdev,
>> +                 queue, virtio_ccw_notify, callback, name);
>> +
>> +    vr = &vq->vring;
>> +    info->info_block->s.desc = vr->desc;
>> +    info->info_block->s.index = index;
>> +    info->info_block->s.num = vr->num;
>> +    info->info_block->s.avail = vr->avail;
>> +    info->info_block->s.used = vr->used;
>> +
>> +    info->info_block->l.desc = vr->desc;
>> +    info->info_block->l.index = index;
>> +    info->info_block->l.num = vr->num;
>> +    info->info_block->l.align = PAGE_SIZE;
>> +
>> +    if (ccw_send(vcdev, CCW_CMD_SET_VQ, info->info_block,
>> +             sizeof(info->info_block->l), 0))
>> +        return NULL;
>> +
>> +    info->vq = &vq->vq;
>> +    vq->vq.priv = info;
>> +
>> +    return &vq->vq;
>> +}
>> +
>> +static int virtio_ccw_find_vqs(struct virtio_device *vdev, unsigned 
>> int nvqs,
>> +                   struct virtqueue *vqs[], vq_callback_t *callbacks[],
>> +                   const char *names[])
>> +{
>> +    int i;
>> +
>> +    for (i = 0; i < nvqs; ++i) {
>> +        vqs[i] = setup_vq(vdev, i,
>> +                  callbacks ? callbacks[i] : NULL,
>> +                  names ? names[i] : "");
>> +        if (!vqs[i])
>> +            return -1;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void virtio_ccw_config_get(struct virtio_device *vdev,
>> +                  unsigned int offset, void *buf,
>> +                  unsigned int len)
>> +{
>> +    struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>> +
>> +    if (virtio_ccw_read_config(vcdev))
>> +        return;
>> +    memcpy(buf, vcdev->config, len);
>> +}
>> +
>> +static void virtio_ccw_config_set(struct virtio_device *vdev,
>> +                  unsigned int offset, const void *buf,
>> +                  unsigned int len)
>> +{
>> +    struct virtio_ccw_device *vcdev = to_vc_device(vdev);
>> +
>> +    memcpy(vcdev->config, buf, len);
>> +    virtio_ccw_write_config(vcdev);
>> +}
>> +
>> +static const struct virtio_config_ops virtio_ccw_ops = {
>> +    .get = virtio_ccw_config_get,
>> +    .set = virtio_ccw_config_set,
>> +    .find_vqs = virtio_ccw_find_vqs,
>> +};
>> +
>> +const struct virtio_config_ops *virtio_ccw_register(void)
>> +{
>> +    return &virtio_ccw_ops;
>> +}
>> +
>> +static int sense(struct virtio_ccw_device *vcdev)
>> +{
>> +    struct senseid *senseid;
>> +
>> +    senseid = alloc_io_mem(sizeof(*senseid), 0);
>> +    assert(senseid);
>> +
>> +    assert(!ccw_send(vcdev, CCW_CMD_SENSE_ID, senseid, 
>> sizeof(*senseid), 0));
>> +
>> +    assert(senseid->reserved == 0xff);
>> +
>> +    vcdev->cu_type = senseid->cu_type;
>> +    vcdev->cu_model = senseid->cu_model;
>> +    vcdev->dev_type = senseid->dev_type;
>> +    vcdev->dev_model = senseid->dev_model;
>> +
>> +    return 0;
>> +}
>> +
>> +static struct virtio_ccw_device *find_vcdev_by_devid(int devid)
>> +{
>> +    struct virtio_ccw_device *dev;
>> +    struct linked_list *l;
>> +
>> +    for (l = vcdev_list.next; l != &vcdev_list; l = l->next) {
>> +        dev = container_of(l, struct virtio_ccw_device, list);
>> +        if (dev->cu_model == devid)
>> +            return dev;
>> +    }
>> +    return NULL;
>> +}
>> +
>> +struct virtio_device *virtio_bind(u32 devid)
>> +{
>> +    struct virtio_ccw_device *vcdev;
>> +
>> +    vcdev = find_vcdev_by_devid(devid);
>> +
>> +    return &vcdev->vdev;
>> +}
>> +
>> +static int virtio_enumerate(int schid)
>> +{
>> +    struct virtio_ccw_device *vcdev;
>> +
>> +    vcdev = alloc_io_mem(sizeof(*vcdev), 0);
>> +    assert(vcdev);
>> +    vcdev->schid = schid;
>> +
>> +    list_add(&vcdev_list, &vcdev->list);
>> +
>> +    assert(css_enable(schid, IO_SCH_ISC) == 0);
>> +    sense(vcdev);
>> +
>> +    return 0;
>> +}
>> +
>> +/* Must get a param */
> 
> I don't understand that comment, could you elaborate?

No, sorry.
I forgot what I wanted to say here.
May be it comes back when I will work on the comments from you and Andrew.


Thanks,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH 3/7] s390x: virtio: CCW transport implementation

[Thomas Huth]

On 08/11/2021 13.34, Pierre Morel wrote:
> 
> 
> On 11/3/21 08:49, Thomas Huth wrote:
>> On 27/08/2021 12.17, Pierre Morel wrote:
>>> This is the implementation of the virtio-ccw transport level.
>>>
>>> We only support VIRTIO revision 0.
>>
>> That means only legacy virtio? Wouldn't it be better to shoot for modern 
>> virtio instead?
> 
> Yes but can we do it in a second series?

Sure.

>>> +int virtio_ccw_read_features(struct virtio_ccw_device *vcdev, uint64_t 
>>> *features)
>>> +{
>>> +    struct virtio_feature_desc *f_desc = &vcdev->f_desc;
>>> +
>>> +    f_desc->index = 0;
>>> +    if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
>>> +        return -1;
>>> +    *features = swap32(f_desc->features);
>>> +
>>> +    f_desc->index = 1;
>>> +    if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 0))
>>> +        return -1;
>>> +    *features |= (uint64_t)swap32(f_desc->features) << 32;
>>
>> Weren't the upper feature bits only available for modern virtio anyway?
> 
> Yes.
> I have the intention to upgrade to Rev. 1 when I get enough time for it.
> Should I remove this? It does not induce problem does it?

No problem - maybe simply add a comment that the upper bits are for virtio 
1.0 and later.

  Thomas

Re: [kvm-unit-tests PATCH 3/7] s390x: virtio: CCW transport implementation

[Pierre Morel]

On 11/9/21 08:01, Thomas Huth wrote:
> On 08/11/2021 13.34, Pierre Morel wrote:
>>
>>
>> On 11/3/21 08:49, Thomas Huth wrote:
>>> On 27/08/2021 12.17, Pierre Morel wrote:
>>>> This is the implementation of the virtio-ccw transport level.
>>>>
>>>> We only support VIRTIO revision 0.
>>>
>>> That means only legacy virtio? Wouldn't it be better to shoot for 
>>> modern virtio instead?
>>
>> Yes but can we do it in a second series?
> 
> Sure.
> 
>>>> +int virtio_ccw_read_features(struct virtio_ccw_device *vcdev, 
>>>> uint64_t *features)
>>>> +{
>>>> +    struct virtio_feature_desc *f_desc = &vcdev->f_desc;
>>>> +
>>>> +    f_desc->index = 0;
>>>> +    if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 
>>>> 0))
>>>> +        return -1;
>>>> +    *features = swap32(f_desc->features);
>>>> +
>>>> +    f_desc->index = 1;
>>>> +    if (ccw_send(vcdev, CCW_CMD_READ_FEAT, f_desc, sizeof(*f_desc), 
>>>> 0))
>>>> +        return -1;
>>>> +    *features |= (uint64_t)swap32(f_desc->features) << 32;
>>>
>>> Weren't the upper feature bits only available for modern virtio anyway?
>>
>> Yes.
>> I have the intention to upgrade to Rev. 1 when I get enough time for it.
>> Should I remove this? It does not induce problem does it?
> 
> No problem - maybe simply add a comment that the upper bits are for 
> virtio 1.0 and later.

OK,
Thanks,

Pierre

> 
>   Thomas
> 

-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH 4/7] s390x: css: registering IRQ

[Pierre Morel]

Registering IRQ for the CSS level.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/s390x/css.h     | 21 +++++++++++++++++++++
 lib/s390x/css_lib.c | 27 +++++++++++++++++++++++++--
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/lib/s390x/css.h b/lib/s390x/css.h
index 2005f4d7..0422f2e7 100644
--- a/lib/s390x/css.h
+++ b/lib/s390x/css.h
@@ -402,4 +402,25 @@ struct measurement_block_format1 {
 	uint32_t irq_prio_delay_time;
 };
 
+#include <asm/arch_def.h>
+static inline void disable_io_irq(void)
+{
+	uint64_t mask;
+
+	mask = extract_psw_mask();
+	mask &= ~PSW_MASK_IO;
+	load_psw_mask(mask);
+}
+
+static inline void enable_io_irq(void)
+{
+	uint64_t mask;
+
+	mask = extract_psw_mask();
+	mask |= PSW_MASK_IO;
+	load_psw_mask(mask);
+}
+
+int register_css_irq_func(void (*f)(void));
+int unregister_css_irq_func(void (*f)(void));
 #endif
diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
index 484f9c41..a89fc93c 100644
--- a/lib/s390x/css_lib.c
+++ b/lib/s390x/css_lib.c
@@ -350,8 +350,29 @@ bool css_disable_mb(int schid)
 	return retry_count > 0;
 }
 
-static struct irb irb;
+static void (*css_irq_func)(void);
+
+int register_css_irq_func(void (*f)(void))
+{
+	if (css_irq_func)
+		return -1;
+	css_irq_func = f;
+	assert(register_io_int_func(css_irq_io) == 0);
+	enable_io_isc(0x80 >> IO_SCH_ISC);
+	return 0;
+}
 
+int unregister_css_irq_func(void (*f)(void))
+{
+	if (css_irq_func != f)
+		return -1;
+	enable_io_isc(0);
+	unregister_io_int_func(css_irq_io);
+	css_irq_func = NULL;
+	return 0;
+}
+
+static struct irb irb;
 void css_irq_io(void)
 {
 	int ret = 0;
@@ -386,7 +407,9 @@ void css_irq_io(void)
 		report(0, "tsch reporting sch %08x as not operational", sid);
 		break;
 	case 0:
-		/* Stay humble on success */
+		/* Call upper level IRQ routine */
+		if (css_irq_func)
+			css_irq_func();
 		break;
 	}
 pop:
-- 
2.25.1

Re: [kvm-unit-tests PATCH 4/7] s390x: css: registering IRQ

[Thomas Huth]

On 27/08/2021 12.17, Pierre Morel wrote:
> Registering IRQ for the CSS level.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>   lib/s390x/css.h     | 21 +++++++++++++++++++++
>   lib/s390x/css_lib.c | 27 +++++++++++++++++++++++++--
>   2 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
> index 2005f4d7..0422f2e7 100644
> --- a/lib/s390x/css.h
> +++ b/lib/s390x/css.h
> @@ -402,4 +402,25 @@ struct measurement_block_format1 {
>   	uint32_t irq_prio_delay_time;
>   };
>   
> +#include <asm/arch_def.h>
> +static inline void disable_io_irq(void)
> +{
> +	uint64_t mask;
> +
> +	mask = extract_psw_mask();
> +	mask &= ~PSW_MASK_IO;
> +	load_psw_mask(mask);
> +}
> +
> +static inline void enable_io_irq(void)
> +{
> +	uint64_t mask;
> +
> +	mask = extract_psw_mask();
> +	mask |= PSW_MASK_IO;
> +	load_psw_mask(mask);
> +}
> +
> +int register_css_irq_func(void (*f)(void));
> +int unregister_css_irq_func(void (*f)(void));
>   #endif
> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
> index 484f9c41..a89fc93c 100644
> --- a/lib/s390x/css_lib.c
> +++ b/lib/s390x/css_lib.c
> @@ -350,8 +350,29 @@ bool css_disable_mb(int schid)
>   	return retry_count > 0;
>   }
>   
> -static struct irb irb;
> +static void (*css_irq_func)(void);
> +
> +int register_css_irq_func(void (*f)(void))
> +{
> +	if (css_irq_func)
> +		return -1;
> +	css_irq_func = f;
> +	assert(register_io_int_func(css_irq_io) == 0);

It's unlikely that we ever disable assert() in the k-u-t, but anyway, I'd 
prefer not to put function calls within assert() statements, just in case. 
So could you please replace this with:

	rc = register_io_int_func(css_irq_io);
	assert(rc == 0);

?

> +	enable_io_isc(0x80 >> IO_SCH_ISC);
> +	return 0;
> +}
>   
> +int unregister_css_irq_func(void (*f)(void))
> +{
> +	if (css_irq_func != f)
> +		return -1;
> +	enable_io_isc(0);
> +	unregister_io_int_func(css_irq_io);
> +	css_irq_func = NULL;
> +	return 0;
> +}
> +
> +static struct irb irb;
>   void css_irq_io(void)
>   {
>   	int ret = 0;
> @@ -386,7 +407,9 @@ void css_irq_io(void)
>   		report(0, "tsch reporting sch %08x as not operational", sid);
>   		break;
>   	case 0:
> -		/* Stay humble on success */
> +		/* Call upper level IRQ routine */
> +		if (css_irq_func)
> +			css_irq_func();
>   		break;
>   	}
>   pop:
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [kvm-unit-tests PATCH 4/7] s390x: css: registering IRQ

[Pierre Morel]

On 11/3/21 09:01, Thomas Huth wrote:
> On 27/08/2021 12.17, Pierre Morel wrote:
>> Registering IRQ for the CSS level.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/s390x/css.h     | 21 +++++++++++++++++++++
>>   lib/s390x/css_lib.c | 27 +++++++++++++++++++++++++--
>>   2 files changed, 46 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/s390x/css.h b/lib/s390x/css.h
>> index 2005f4d7..0422f2e7 100644
>> --- a/lib/s390x/css.h
>> +++ b/lib/s390x/css.h
>> @@ -402,4 +402,25 @@ struct measurement_block_format1 {
>>       uint32_t irq_prio_delay_time;
>>   };
>> +#include <asm/arch_def.h>
>> +static inline void disable_io_irq(void)
>> +{
>> +    uint64_t mask;
>> +
>> +    mask = extract_psw_mask();
>> +    mask &= ~PSW_MASK_IO;
>> +    load_psw_mask(mask);
>> +}
>> +
>> +static inline void enable_io_irq(void)
>> +{
>> +    uint64_t mask;
>> +
>> +    mask = extract_psw_mask();
>> +    mask |= PSW_MASK_IO;
>> +    load_psw_mask(mask);
>> +}
>> +
>> +int register_css_irq_func(void (*f)(void));
>> +int unregister_css_irq_func(void (*f)(void));
>>   #endif
>> diff --git a/lib/s390x/css_lib.c b/lib/s390x/css_lib.c
>> index 484f9c41..a89fc93c 100644
>> --- a/lib/s390x/css_lib.c
>> +++ b/lib/s390x/css_lib.c
>> @@ -350,8 +350,29 @@ bool css_disable_mb(int schid)
>>       return retry_count > 0;
>>   }
>> -static struct irb irb;
>> +static void (*css_irq_func)(void);
>> +
>> +int register_css_irq_func(void (*f)(void))
>> +{
>> +    if (css_irq_func)
>> +        return -1;
>> +    css_irq_func = f;
>> +    assert(register_io_int_func(css_irq_io) == 0);
> 
> It's unlikely that we ever disable assert() in the k-u-t, but anyway, 
> I'd prefer not to put function calls within assert() statements, just in 
> case. So could you please replace this with:
> 
>      rc = register_io_int_func(css_irq_io);
>      assert(rc == 0);
>  > ?

OK

> 
>> +    enable_io_isc(0x80 >> IO_SCH_ISC);
>> +    return 0;
>> +}
>> +int unregister_css_irq_func(void (*f)(void))
>> +{
>> +    if (css_irq_func != f)
>> +        return -1;
>> +    enable_io_isc(0);
>> +    unregister_io_int_func(css_irq_io);
>> +    css_irq_func = NULL;
>> +    return 0;
>> +}
>> +
>> +static struct irb irb;
>>   void css_irq_io(void)
>>   {
>>       int ret = 0;
>> @@ -386,7 +407,9 @@ void css_irq_io(void)
>>           report(0, "tsch reporting sch %08x as not operational", sid);
>>           break;
>>       case 0:
>> -        /* Stay humble on success */
>> +        /* Call upper level IRQ routine */
>> +        if (css_irq_func)
>> +            css_irq_func();
>>           break;
>>       }
>>   pop:
>>
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH 5/7] virtio: implement the virtio_add_inbuf routine

[Pierre Morel]

To communicate in both directions with a VIRTIO device we need
to add the incoming communication to the VIRTIO level.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 lib/virtio.c | 32 ++++++++++++++++++++++++++++++++
 lib/virtio.h |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/lib/virtio.c b/lib/virtio.c
index e10153b9..b84bc680 100644
--- a/lib/virtio.c
+++ b/lib/virtio.c
@@ -47,6 +47,38 @@ void vring_init_virtqueue(struct vring_virtqueue *vq, unsigned index,
 	vq->data[i] = NULL;
 }
 
+int virtqueue_add_inbuf(struct virtqueue *_vq, char *buf, unsigned int len)
+{
+	struct vring_virtqueue *vq = to_vvq(_vq);
+	unsigned int avail;
+	int head;
+
+	assert(buf);
+	assert(len);
+
+	if (!vq->vq.num_free)
+		return -1;
+
+	--vq->vq.num_free;
+
+	head = vq->free_head;
+
+	vq->vring.desc[head].flags = 0;
+	vq->vring.desc[head].addr = virt_to_phys(buf);
+	vq->vring.desc[head].len = len;
+
+	vq->free_head = vq->vring.desc[head].next;
+
+	vq->data[head] = buf;
+
+	avail = (vq->vring.avail->idx & (vq->vring.num - 1));
+	vq->vring.avail->ring[avail] = head;
+	wmb();	/* be sure to update the ring before updating the idx */
+	vq->vring.avail->idx++;
+	vq->num_added++;
+
+	return 0;
+}
 int virtqueue_add_outbuf(struct virtqueue *_vq, char *buf, unsigned int len)
 {
 	struct vring_virtqueue *vq = to_vvq(_vq);
diff --git a/lib/virtio.h b/lib/virtio.h
index 2c31fdc7..44b727f8 100644
--- a/lib/virtio.h
+++ b/lib/virtio.h
@@ -141,6 +141,8 @@ extern void vring_init_virtqueue(struct vring_virtqueue *vq, unsigned index,
 				 const char *name);
 extern int virtqueue_add_outbuf(struct virtqueue *vq, char *buf,
 				unsigned int len);
+extern int virtqueue_add_inbuf(struct virtqueue *vq, char *buf,
+			       unsigned int len);
 extern bool virtqueue_kick(struct virtqueue *vq);
 extern void detach_buf(struct vring_virtqueue *vq, unsigned head);
 extern void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len);
-- 
2.25.1

Re: [kvm-unit-tests PATCH 5/7] virtio: implement the virtio_add_inbuf routine

[Andrew Jones]

On Fri, Aug 27, 2021 at 12:17:18PM +0200, Pierre Morel wrote:
> To communicate in both directions with a VIRTIO device we need
> to add the incoming communication to the VIRTIO level.
> 
> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
> ---
>  lib/virtio.c | 32 ++++++++++++++++++++++++++++++++
>  lib/virtio.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/lib/virtio.c b/lib/virtio.c
> index e10153b9..b84bc680 100644
> --- a/lib/virtio.c
> +++ b/lib/virtio.c
> @@ -47,6 +47,38 @@ void vring_init_virtqueue(struct vring_virtqueue *vq, unsigned index,
>  	vq->data[i] = NULL;
>  }
>  
> +int virtqueue_add_inbuf(struct virtqueue *_vq, char *buf, unsigned int len)
> +{
> +	struct vring_virtqueue *vq = to_vvq(_vq);
> +	unsigned int avail;
> +	int head;
> +
> +	assert(buf);
> +	assert(len);
> +
> +	if (!vq->vq.num_free)
> +		return -1;
> +
> +	--vq->vq.num_free;
> +
> +	head = vq->free_head;
> +
> +	vq->vring.desc[head].flags = 0;
> +	vq->vring.desc[head].addr = virt_to_phys(buf);
> +	vq->vring.desc[head].len = len;
> +
> +	vq->free_head = vq->vring.desc[head].next;
> +
> +	vq->data[head] = buf;
> +
> +	avail = (vq->vring.avail->idx & (vq->vring.num - 1));
> +	vq->vring.avail->ring[avail] = head;
> +	wmb();	/* be sure to update the ring before updating the idx */
> +	vq->vring.avail->idx++;
> +	vq->num_added++;
> +
> +	return 0;
> +}
>  int virtqueue_add_outbuf(struct virtqueue *_vq, char *buf, unsigned int len)
>  {
>  	struct vring_virtqueue *vq = to_vvq(_vq);
> diff --git a/lib/virtio.h b/lib/virtio.h
> index 2c31fdc7..44b727f8 100644
> --- a/lib/virtio.h
> +++ b/lib/virtio.h
> @@ -141,6 +141,8 @@ extern void vring_init_virtqueue(struct vring_virtqueue *vq, unsigned index,
>  				 const char *name);
>  extern int virtqueue_add_outbuf(struct virtqueue *vq, char *buf,
>  				unsigned int len);
> +extern int virtqueue_add_inbuf(struct virtqueue *vq, char *buf,
> +			       unsigned int len);
>  extern bool virtqueue_kick(struct virtqueue *vq);
>  extern void detach_buf(struct vring_virtqueue *vq, unsigned head);
>  extern void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len);
> -- 
> 2.25.1
>

Reviewed-by: Andrew Jones <drjones@redhat.com>

Re: [kvm-unit-tests PATCH 5/7] virtio: implement the virtio_add_inbuf routine

[Pierre Morel]

On 11/3/21 08:51, Andrew Jones wrote:
> On Fri, Aug 27, 2021 at 12:17:18PM +0200, Pierre Morel wrote:
>> To communicate in both directions with a VIRTIO device we need
>> to add the incoming communication to the VIRTIO level.
>>
>> Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
>> ---
>>   lib/virtio.c | 32 ++++++++++++++++++++++++++++++++
>>   lib/virtio.h |  2 ++
>>   2 files changed, 34 insertions(+)
>>
>> diff --git a/lib/virtio.c b/lib/virtio.c
>> index e10153b9..b84bc680 100644
>> --- a/lib/virtio.c
>> +++ b/lib/virtio.c
>> @@ -47,6 +47,38 @@ void vring_init_virtqueue(struct vring_virtqueue *vq, unsigned index,
>>   	vq->data[i] = NULL;
>>   }
>>   
>> +int virtqueue_add_inbuf(struct virtqueue *_vq, char *buf, unsigned int len)
>> +{
>> +	struct vring_virtqueue *vq = to_vvq(_vq);
>> +	unsigned int avail;
>> +	int head;
>> +
>> +	assert(buf);
>> +	assert(len);
>> +
>> +	if (!vq->vq.num_free)
>> +		return -1;
>> +
>> +	--vq->vq.num_free;
>> +
>> +	head = vq->free_head;
>> +
>> +	vq->vring.desc[head].flags = 0;
>> +	vq->vring.desc[head].addr = virt_to_phys(buf);
>> +	vq->vring.desc[head].len = len;
>> +
>> +	vq->free_head = vq->vring.desc[head].next;
>> +
>> +	vq->data[head] = buf;
>> +
>> +	avail = (vq->vring.avail->idx & (vq->vring.num - 1));
>> +	vq->vring.avail->ring[avail] = head;
>> +	wmb();	/* be sure to update the ring before updating the idx */
>> +	vq->vring.avail->idx++;
>> +	vq->num_added++;
>> +
>> +	return 0;
>> +}
>>   int virtqueue_add_outbuf(struct virtqueue *_vq, char *buf, unsigned int len)
>>   {
>>   	struct vring_virtqueue *vq = to_vvq(_vq);
>> diff --git a/lib/virtio.h b/lib/virtio.h
>> index 2c31fdc7..44b727f8 100644
>> --- a/lib/virtio.h
>> +++ b/lib/virtio.h
>> @@ -141,6 +141,8 @@ extern void vring_init_virtqueue(struct vring_virtqueue *vq, unsigned index,
>>   				 const char *name);
>>   extern int virtqueue_add_outbuf(struct virtqueue *vq, char *buf,
>>   				unsigned int len);
>> +extern int virtqueue_add_inbuf(struct virtqueue *vq, char *buf,
>> +			       unsigned int len);
>>   extern bool virtqueue_kick(struct virtqueue *vq);
>>   extern void detach_buf(struct vring_virtqueue *vq, unsigned head);
>>   extern void *virtqueue_get_buf(struct virtqueue *_vq, unsigned int *len);
>> -- 
>> 2.25.1
>>
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 

Thanks,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH 6/7] s390x: virtio tests setup

[Pierre Morel]

This patch can be squatch with the next one, "s390x: virtio data
transfer" and is separated to ease review.

In this patch we initialize the VIRTIO device.
There are currently no error insertion, the goal is to get an
initialized device to check data transfer within the next patch.

Future development will include error response checks.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/Makefile      |   1 +
 s390x/unittests.cfg |   4 +
 s390x/virtio_pong.c | 208 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 213 insertions(+)
 create mode 100644 s390x/virtio_pong.c

diff --git a/s390x/Makefile b/s390x/Makefile
index 3f4acc3e..633e1af1 100644
--- a/s390x/Makefile
+++ b/s390x/Makefile
@@ -24,6 +24,7 @@ tests += $(TEST_DIR)/mvpg.elf
 tests += $(TEST_DIR)/uv-host.elf
 tests += $(TEST_DIR)/edat.elf
 tests += $(TEST_DIR)/mvpg-sie.elf
+tests += $(TEST_DIR)/virtio_pong.elf
 
 tests_binary = $(patsubst %.elf,%.bin,$(tests))
 ifneq ($(HOST_KEY_DOCUMENT),)
diff --git a/s390x/unittests.cfg b/s390x/unittests.cfg
index 9e1802fd..dd84ed28 100644
--- a/s390x/unittests.cfg
+++ b/s390x/unittests.cfg
@@ -109,3 +109,7 @@ file = edat.elf
 
 [mvpg-sie]
 file = mvpg-sie.elf
+
+[virtio-pong]
+file = uv-virtio.elf
+extra_params = -device virtio-pong-cww
diff --git a/s390x/virtio_pong.c b/s390x/virtio_pong.c
new file mode 100644
index 00000000..1e050a4d
--- /dev/null
+++ b/s390x/virtio_pong.c
@@ -0,0 +1,208 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Channel Subsystem tests
+ *
+ * Copyright (c) 2021 IBM Corp
+ *
+ * Authors:
+ *  Pierre Morel <pmorel@linux.ibm.com>
+ *
+ */
+#include <libcflat.h>
+#include <alloc_page.h>
+#include <asm/page.h>
+#include <string.h>
+#include <interrupt.h>
+#include <asm/arch_def.h>
+#include <asm/facility.h>
+#include <asm/uv.h>
+
+#include <css.h>
+#include <virtio.h>
+#include <virtio-config.h>
+#include <virtio-ccw.h>
+
+#include <malloc_io.h>
+#include <asm/time.h>
+
+#define VIRTIO_ID_PONG         30 /* virtio pong */
+
+#define VIRTIO_F_PONG_CKSUM	1
+
+#define SUPPORTED_FEATURES (1UL << VIRTIO_F_RING_INDIRECT_DESC	| \
+			    1UL << VIRTIO_F_RING_EVENT_IDX	| \
+			    1UL << VIRTIO_F_NOTIFY_ON_EMPTY	| \
+			    1UL << VIRTIO_F_ANY_LAYOUT	| \
+			    1UL << VIRTIO_F_PONG_CKSUM)
+
+static struct virtio_ccw_device *vcdev;
+static struct virtqueue *out_vq;
+static struct virtqueue *in_vq;
+
+static void test_find_vqs(void)
+{
+	struct virtio_device *vdev = &vcdev->vdev;
+	static const char *io_names[] = {"pong_input", "pong_output"};
+	struct virtqueue *vqs[2];
+	int ret;
+
+	if (vcdev->state != VCDEV_INIT) {
+		report_skip("Device non initialized");
+		vcdev->state = VCDEV_ERROR;
+		return;
+	}
+
+	ret = vdev->config->find_vqs(vdev, 2, vqs, NULL, io_names);
+	if (!ret) {
+		in_vq = vqs[0];
+		out_vq = vqs[1];
+	}
+	report(!ret, "Find virtqueues");
+}
+
+static int virtio_ccw_init_dev(struct virtio_ccw_device *vcdev)
+{
+	uint64_t features;
+	uint64_t unsupported_feat;
+	int ret;
+
+	ret = virtio_ccw_set_revision(vcdev);
+	report(!ret, "Revision 0");
+	if (ret)
+		return VCDEV_ERROR;
+
+	ret = virtio_ccw_reset(vcdev);
+	report(!ret, "RESET");
+	if (ret)
+		return VCDEV_ERROR;
+
+	ret = virtio_ccw_read_status(vcdev);
+	report(!ret && vcdev->status == 0, "Read Status : 0x%08x", vcdev->status);
+	if (ret)
+		return VCDEV_ERROR;
+
+	vcdev->status = VIRTIO_CONFIG_S_ACKNOWLEDGE | VIRTIO_CONFIG_S_DRIVER;
+	ret = virtio_ccw_write_status(vcdev);
+	report(!ret, "Write ACKNOWLEDGE and DRIVER Status: 0x%08x", vcdev->status);
+	if (ret)
+		return VCDEV_ERROR;
+
+	/* Checking features */
+	ret = virtio_ccw_read_features(vcdev, &features);
+	report(!ret, "Read features : 0x%016lx", features);
+	if (ret)
+		return VCDEV_ERROR;
+
+	report(features & 1UL << VIRTIO_F_RING_INDIRECT_DESC,
+	       "Feature: RING_INDIRECT_DESC");
+
+	report(features & 1UL << VIRTIO_F_RING_EVENT_IDX,
+	       "Feature: RING_EVENT_IDX");
+
+	unsupported_feat = features & ~SUPPORTED_FEATURES;
+	report(!unsupported_feat, "Features supported: 0x%016lx got 0x%016lx",
+	       SUPPORTED_FEATURES, features);
+	if (unsupported_feat)
+		return VCDEV_ERROR;
+
+	/* Accept supported features */
+	features &= SUPPORTED_FEATURES;
+	ret = virtio_ccw_write_features(vcdev, features);
+	report(!ret, "Write features: 0x%016lx", features);
+	if (ret)
+		return VCDEV_ERROR;
+
+	vcdev->status |= VIRTIO_CONFIG_S_FEATURES_OK;
+	ret = virtio_ccw_write_status(vcdev);
+	report(!ret, "Write FEATURES_OK Status: 0x%08x", vcdev->status);
+	if (ret)
+		return VCDEV_ERROR;
+
+	ret = virtio_ccw_read_status(vcdev);
+	report(!ret, "Read Status : 0x%08x", vcdev->status);
+	if (ret)
+		return VCDEV_ERROR;
+	report(vcdev->status & VIRTIO_CONFIG_S_FEATURES_OK, "Status: FEATURES_OK");
+	if (!(vcdev->status & VIRTIO_CONFIG_S_FEATURES_OK))
+		return VCDEV_ERROR;
+
+	ret = virtio_ccw_setup_indicators(vcdev);
+	report(!ret, "Setup indicators");
+	if (ret)
+		return VCDEV_ERROR;
+
+	vcdev->vdev.config = virtio_ccw_register();
+	if (!vcdev->vdev.config)
+		return VCDEV_ERROR;
+
+	vcdev->status |= VIRTIO_CONFIG_S_DRIVER_OK;
+	ret = virtio_ccw_write_status(vcdev);
+	report(!ret, "Write DRIVER_OK Status: 0x%08x", vcdev->status);
+	if (ret)
+		return VCDEV_ERROR;
+
+	return VCDEV_INIT;
+}
+
+static void test_virtio_device_init(void)
+{
+	struct virtio_device *vdev;
+
+	vdev = virtio_bind(VIRTIO_ID_PONG);
+	if (!vdev) {
+		report_abort("virtio_bind failed");
+		return;
+	}
+
+	vcdev = to_vc_device(vdev);
+	vcdev->state = virtio_ccw_init_dev(vcdev);
+	report(vcdev->state == VCDEV_INIT, "Initialization");
+}
+
+static void test_virtio_ccw_bus(void)
+{
+	report(virtio_ccw_init(), "Initialisation");
+}
+
+static void virtio_irq(void)
+{
+	/*
+	 * Empty function currently needed to setup IRQ by providing
+	 * an address to register_css_irq_func().
+	 * Will be use in the future to check parallel I/O.
+	 */
+}
+
+static int css_init(void)
+{
+	assert(register_css_irq_func(virtio_irq) == 0);
+	return 0;
+}
+
+static struct {
+	const char *name;
+	void (*func)(void);
+} tests[] = {
+	{ "CCW Bus", test_virtio_ccw_bus },
+	{ "CCW Device", test_virtio_device_init },
+	{ "Queues setup", test_find_vqs },
+	{ NULL, NULL }
+};
+
+int main(int argc, char *argv[])
+{
+	int i;
+
+	report_prefix_push("Virtio");
+
+	css_init();
+
+	for (i = 0; tests[i].name; i++) {
+		report_prefix_push(tests[i].name);
+		tests[i].func();
+		report_prefix_pop();
+	}
+	report_prefix_pop();
+
+	return report_summary();
+}
-- 
2.25.1

Re: [kvm-unit-tests PATCH 6/7] s390x: virtio tests setup

[Andrew Jones]

On Fri, Aug 27, 2021 at 12:17:19PM +0200, Pierre Morel wrote:
> +
> +#define VIRTIO_ID_PONG         30 /* virtio pong */

I take it this is a virtio test device that ping-pong's I/O. It sounds
useful for other VIRTIO transports too. Can it be ported? Hmm, I can't
find it in QEMU at all?

Thanks,
drew

Re: [kvm-unit-tests PATCH 6/7] s390x: virtio tests setup

[Thomas Huth]

On 03/11/2021 08.56, Andrew Jones wrote:
> On Fri, Aug 27, 2021 at 12:17:19PM +0200, Pierre Morel wrote:
>> +
>> +#define VIRTIO_ID_PONG         30 /* virtio pong */
> 
> I take it this is a virtio test device that ping-pong's I/O. It sounds
> useful for other VIRTIO transports too. Can it be ported? Hmm, I can't
> find it in QEMU at all?

I also wonder whether we could do testing with an existing device instead? 
E.g. do a loopback with a virtio-serial device? Or use two virtio-net 
devices, connect them to a QEMU hub and send a packet from one device to the 
other? ... that would be a little bit more complicated here, but would not 
require a PONG device upstream first, so it could also be used for testing 
older versions of QEMU...

  Thomas

Re: [kvm-unit-tests PATCH 6/7] s390x: virtio tests setup

[Pierre Morel]

On 11/3/21 09:14, Thomas Huth wrote:
> On 03/11/2021 08.56, Andrew Jones wrote:
>> On Fri, Aug 27, 2021 at 12:17:19PM +0200, Pierre Morel wrote:
>>> +
>>> +#define VIRTIO_ID_PONG         30 /* virtio pong */
>>
>> I take it this is a virtio test device that ping-pong's I/O. It sounds
>> useful for other VIRTIO transports too. Can it be ported? Hmm, I can't
>> find it in QEMU at all?
> 
> I also wonder whether we could do testing with an existing device 
> instead? E.g. do a loopback with a virtio-serial device? Or use two 
> virtio-net devices, connect them to a QEMU hub and send a packet from 
> one device to the other? ... that would be a little bit more complicated 
> here, but would not require a PONG device upstream first, so it could 
> also be used for testing older versions of QEMU...
> 
>   Thomas
> 
> 

Yes having a dedicated device has the drawback that we need it in QEMU.
On the other hand using a specific device, serial or network, wouldn't 
we get trapped with a reduce set of test possibilities?

The idea was to have a dedicated test device, which could be flexible 
and extended to test all VIRTIO features, even the current 
implementation is yet far from it.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH 6/7] s390x: virtio tests setup

[Thomas Huth]

On 08/11/2021 14.00, Pierre Morel wrote:
> 
> 
> On 11/3/21 09:14, Thomas Huth wrote:
>> On 03/11/2021 08.56, Andrew Jones wrote:
>>> On Fri, Aug 27, 2021 at 12:17:19PM +0200, Pierre Morel wrote:
>>>> +
>>>> +#define VIRTIO_ID_PONG         30 /* virtio pong */
>>>
>>> I take it this is a virtio test device that ping-pong's I/O. It sounds
>>> useful for other VIRTIO transports too. Can it be ported? Hmm, I can't
>>> find it in QEMU at all?
>>
>> I also wonder whether we could do testing with an existing device instead? 
>> E.g. do a loopback with a virtio-serial device? Or use two virtio-net 
>> devices, connect them to a QEMU hub and send a packet from one device to 
>> the other? ... that would be a little bit more complicated here, but would 
>> not require a PONG device upstream first, so it could also be used for 
>> testing older versions of QEMU...
>>
>>   Thomas
>>
>>
> 
> Yes having a dedicated device has the drawback that we need it in QEMU.
> On the other hand using a specific device, serial or network, wouldn't we 
> get trapped with a reduce set of test possibilities?
> 
> The idea was to have a dedicated test device, which could be flexible and 
> extended to test all VIRTIO features, even the current implementation is yet 
> far from it.

Do you have anything in the works that could only be tested with a dedicated 
test device? If not, I'd rather go with the loopback via virtio-net, I think 
(you can peek into the s390-ccw bios sources to see how to send packets via 
virtio-net, shouldn't be too hard to do, I think).

The pong device could later be added on top for additional tests that are 
not possible with virtio-net. And having some basic tests with virito-net 
has also the advantage that the k-u-t work with QEMU binaries where the pong 
device is not available, e.g. older versions and downstream versions that 
only enable the bare minimum of devices to keep the attack surface small.

  Thomas

Re: [kvm-unit-tests PATCH 6/7] s390x: virtio tests setup

[Andrew Jones]

On Tue, Nov 09, 2021 at 08:10:34AM +0100, Thomas Huth wrote:
> On 08/11/2021 14.00, Pierre Morel wrote:
> > 
> > 
> > On 11/3/21 09:14, Thomas Huth wrote:
> > > On 03/11/2021 08.56, Andrew Jones wrote:
> > > > On Fri, Aug 27, 2021 at 12:17:19PM +0200, Pierre Morel wrote:
> > > > > +
> > > > > +#define VIRTIO_ID_PONG         30 /* virtio pong */
> > > > 
> > > > I take it this is a virtio test device that ping-pong's I/O. It sounds
> > > > useful for other VIRTIO transports too. Can it be ported? Hmm, I can't
> > > > find it in QEMU at all?
> > > 
> > > I also wonder whether we could do testing with an existing device
> > > instead? E.g. do a loopback with a virtio-serial device? Or use two
> > > virtio-net devices, connect them to a QEMU hub and send a packet
> > > from one device to the other? ... that would be a little bit more
> > > complicated here, but would not require a PONG device upstream
> > > first, so it could also be used for testing older versions of
> > > QEMU...
> > > 
> > >   Thomas
> > > 
> > > 
> > 
> > Yes having a dedicated device has the drawback that we need it in QEMU.
> > On the other hand using a specific device, serial or network, wouldn't
> > we get trapped with a reduce set of test possibilities?
> > 
> > The idea was to have a dedicated test device, which could be flexible
> > and extended to test all VIRTIO features, even the current
> > implementation is yet far from it.
> 
> Do you have anything in the works that could only be tested with a dedicated
> test device? If not, I'd rather go with the loopback via virtio-net, I think
> (you can peek into the s390-ccw bios sources to see how to send packets via
> virtio-net, shouldn't be too hard to do, I think).
> 
> The pong device could later be added on top for additional tests that are
> not possible with virtio-net. And having some basic tests with virito-net
> has also the advantage that the k-u-t work with QEMU binaries where the pong
> device is not available, e.g. older versions and downstream versions that
> only enable the bare minimum of devices to keep the attack surface small.
>

I'd also like to see the testdev we already have, qemu:chardev/testdev.c,
get more functions, but I'm not sure virtio-serial will allow you to
exercise all the virtio functionality that you'd like to.

Thanks,
drew

Re: [kvm-unit-tests PATCH 6/7] s390x: virtio tests setup

[Pierre Morel]

On 11/9/21 09:42, Andrew Jones wrote:
> On Tue, Nov 09, 2021 at 08:10:34AM +0100, Thomas Huth wrote:
>> On 08/11/2021 14.00, Pierre Morel wrote:
>>>
>>>
>>> On 11/3/21 09:14, Thomas Huth wrote:
>>>> On 03/11/2021 08.56, Andrew Jones wrote:
>>>>> On Fri, Aug 27, 2021 at 12:17:19PM +0200, Pierre Morel wrote:
>>>>>> +
>>>>>> +#define VIRTIO_ID_PONG         30 /* virtio pong */
>>>>>
>>>>> I take it this is a virtio test device that ping-pong's I/O. It sounds
>>>>> useful for other VIRTIO transports too. Can it be ported? Hmm, I can't
>>>>> find it in QEMU at all?
>>>>
>>>> I also wonder whether we could do testing with an existing device
>>>> instead? E.g. do a loopback with a virtio-serial device? Or use two
>>>> virtio-net devices, connect them to a QEMU hub and send a packet
>>>> from one device to the other? ... that would be a little bit more
>>>> complicated here, but would not require a PONG device upstream
>>>> first, so it could also be used for testing older versions of
>>>> QEMU...
>>>>
>>>>    Thomas
>>>>
>>>>
>>>
>>> Yes having a dedicated device has the drawback that we need it in QEMU.
>>> On the other hand using a specific device, serial or network, wouldn't
>>> we get trapped with a reduce set of test possibilities?
>>>
>>> The idea was to have a dedicated test device, which could be flexible
>>> and extended to test all VIRTIO features, even the current
>>> implementation is yet far from it.
>>
>> Do you have anything in the works that could only be tested with a dedicated
>> test device? If not, I'd rather go with the loopback via virtio-net, I think
>> (you can peek into the s390-ccw bios sources to see how to send packets via
>> virtio-net, shouldn't be too hard to do, I think).
>>
>> The pong device could later be added on top for additional tests that are
>> not possible with virtio-net. And having some basic tests with virito-net
>> has also the advantage that the k-u-t work with QEMU binaries where the pong
>> device is not available, e.g. older versions and downstream versions that
>> only enable the bare minimum of devices to keep the attack surface small.
>>
> 
> I'd also like to see the testdev we already have, qemu:chardev/testdev.c,
> get more functions, but I'm not sure virtio-serial will allow you to
> exercise all the virtio functionality that you'd like to.
> 
> Thanks,
> drew
> 

Yes, that is why I did not used it first.
But OK, I understand what you both want and will build something in that 
direction, virtio-net, virtio-serial and come back later to something 
independent of existing devices if we find it does have a purpose.
Thanks for the comments.

Regards,
Pierre

-- 
Pierre Morel
IBM Lab Boeblingen

Re: [kvm-unit-tests PATCH 6/7] s390x: virtio tests setup

[Pierre Morel]

On 11/3/21 08:56, Andrew Jones wrote:
> On Fri, Aug 27, 2021 at 12:17:19PM +0200, Pierre Morel wrote:
>> +
>> +#define VIRTIO_ID_PONG         30 /* virtio pong */
> 
> I take it this is a virtio test device that ping-pong's I/O. It sounds
> useful for other VIRTIO transports too. Can it be ported? Hmm, I can't
> find it in QEMU at all?
> 
> Thanks,
> drew
> 

It could certainly be ported, I will study the question.

I sent a first version of the QEMU part here:

https://marc.info/?l=kvm&m=163006146622427&w=3


Regards,
Pierre


-- 
Pierre Morel
IBM Lab Boeblingen

[kvm-unit-tests PATCH 7/7] s390x: virtio data transfer

[Pierre Morel]

We use a test device, "PONG" to transfer chunks of data of different
size and alignment and compare a checksum calculated before the sending
with the VIRTIO device checksum response.

Signed-off-by: Pierre Morel <pmorel@linux.ibm.com>
---
 s390x/virtio_pong.c | 107 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/s390x/virtio_pong.c b/s390x/virtio_pong.c
index 1e050a4d..94d6acdc 100644
--- a/s390x/virtio_pong.c
+++ b/s390x/virtio_pong.c
@@ -39,6 +39,112 @@ static struct virtio_ccw_device *vcdev;
 static struct virtqueue *out_vq;
 static struct virtqueue *in_vq;
 
+static bool virtio_read(struct virtqueue *q, char **buf, unsigned int *len)
+{
+	int ret;
+	char *p;
+
+	ret = virtqueue_add_inbuf(q, *buf, *len);
+	if (ret < 0)
+		return false;
+
+	disable_io_irq();
+	virtqueue_kick(q);
+	wait_for_interrupt(PSW_MASK_IO);
+
+	do {
+		p = virtqueue_get_buf(q, len);
+	} while (!p);
+
+	*buf = (void *)p;
+
+	return true;
+}
+
+static bool virtio_write(struct virtqueue *q, char *buf, unsigned int len)
+{
+	int ret;
+
+	ret = virtqueue_add_outbuf(q, buf, len);
+	if (ret < 0)
+		return false;
+
+	virtqueue_kick(q);
+	while (!virtqueue_get_buf(q, &len))
+		;
+
+	return true;
+}
+
+static unsigned int simple_checksum(char *buf, unsigned int len)
+{
+	unsigned int sum = 0;
+
+	while (len--) {
+		sum += *buf * *buf + 7 * *buf + 3;
+		buf++;
+	}
+
+	return sum;
+}
+
+static void pong_write(char *buf, unsigned int len)
+{
+	unsigned int cksum;
+	unsigned int cksum_ret;
+	char *ret_ptr = (char *)&cksum_ret;
+
+	cksum = simple_checksum(buf, len);
+	report(virtio_write(out_vq, buf, len), "Sending data: %08x", cksum);
+
+	len = sizeof(cksum_ret);
+	report(virtio_read(in_vq, &ret_ptr, &len),
+	       "Receiving checksum: %08x", cksum_ret);
+
+	report(cksum == cksum_ret, "Verifying checksum");
+}
+
+static struct {
+	const char *name;
+	int size;
+	int offset;
+} chunks[] = {
+	{ "Small buffer", 3, 0 },
+	{ "Page aligned", 4096, 0 },
+	{ "Large page aligned", 0x00100000, 0 },
+	{ "Page unaligned", 4096, 0x107 },
+	{ "Random data", 5119, 0x107 },
+	{ NULL, 0, 0 }
+};
+
+static void test_pong_data(void)
+{
+	char *buf;
+	char *p;
+	int len;
+	int i;
+
+	if (vcdev->state != VCDEV_INIT) {
+		report_skip("Device non initialized");
+		return;
+	}
+
+	for (i = 0; chunks[i].name; i++) {
+		report_prefix_push(chunks[i].name);
+
+		len = chunks[i].size + chunks[i].offset;
+		buf = alloc_io_mem(len, 0);
+
+		p = buf + chunks[i].offset;
+		memset(p, 0xA5, chunks[i].size);
+		pong_write(p, chunks[i].size);
+
+		free_io_mem(buf, len);
+
+		report_prefix_pop();
+	}
+}
+
 static void test_find_vqs(void)
 {
 	struct virtio_device *vdev = &vcdev->vdev;
@@ -186,6 +292,7 @@ static struct {
 	{ "CCW Bus", test_virtio_ccw_bus },
 	{ "CCW Device", test_virtio_device_init },
 	{ "Queues setup", test_find_vqs },
+	{ "Data transfer", test_pong_data },
 	{ NULL, NULL }
 };
 
-- 
2.25.1