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

[Andrew Jones <drjones@xxxxxxxxxx>]

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@xxxxxxxxxxxxx>
> > ---
> >   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