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

[Pierre Morel <pmorel@linux.ibm.com>]

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