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

[Thomas Huth <thuth@redhat.com>]

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