Haixu Cui <quic_haix...@quicinc.com> writes:
> Hi Alex,
> Thanks a lot for your comments, please refer to my response below.
>
> On 8/28/2024 1:14 AM, Alex Bennée wrote:
>> Haixu Cui <quic_haix...@quicinc.com> writes:
>> Apologies for the delay in getting to this.
>>
>>> This work is based on the virtio-spi spec, virtio-spi driver introduced by
>>> the following patch series:
>>> - https://github.com/oasis-tcs/virtio-spec/tree/virtio-1.4/device-types/spi
>>> - https://lwn.net/Articles/966715/
>>>
>>> To test with rust-vmm vhost-user-spi daemon, start the vhost-daemon firstly:
>>> vhost-device-spi --socket-path=vspi.sock --socket-count=1 --device
>>> "/dev/spidev0.0"
>> I'm struggling to test this on my main dev box. Are there any dummy
>> SPI
>> modules for the kernel for testing? Otherwise we could consider
>> implementing something similar to "mock_gpio" for the rust-vmm
>> vhost-user-spi backend?
>
> I verified this on my board with physical SPI interface, and I don't
> know if these is dummy SPI module available in kernel. I'll look into
> this.
I'll see if I can boot full Linux into a QEMU machine with SPI devices
which would be another approach.
>>
>>> Then invoke qemu with the following parameters:
>>> qemu-system-aarch64 -m 1G \
>>> -chardev socket,path=/home/root/vspi.sock0,id=vspi \
>>> -device vhost-user-spi-pci,chardev=vspi,id=spi \
>>> -object
>>> memory-backend-file,id=mem,size=1G,mem-path=/dev/shm,share=on \
>>> -numa node,memdev=mem
>>> ...
>>
>>>
>>> Signed-off-by: Haixu Cui <quic_haix...@quicinc.com>
>>> ---
>>> hw/virtio/Kconfig | 5 +
>>> hw/virtio/meson.build | 3 +
>>> hw/virtio/vhost-user-spi-pci.c | 69 ++++++++
>>> hw/virtio/vhost-user-spi.c | 66 +++++++
>>> hw/virtio/virtio.c | 4 +-
>>> include/hw/virtio/vhost-user-spi.h | 25 +++
>>> include/standard-headers/linux/virtio_ids.h | 1 +
>>> include/standard-headers/linux/virtio_spi.h | 186 ++++++++++++++++++++
>>> 8 files changed, 358 insertions(+), 1 deletion(-)
>>> create mode 100644 hw/virtio/vhost-user-spi-pci.c
>>> create mode 100644 hw/virtio/vhost-user-spi.c
>>> create mode 100644 include/hw/virtio/vhost-user-spi.h
>>> create mode 100644 include/standard-headers/linux/virtio_spi.h
>> Generally we want separate headers patches for the importing of
>> headers.
>> Doubly so in this case because I can't see the SPI definitions in the
>> current Linux master. So:
>> - 1/2 - Import headers for SPI (!merge until upstream)
>> - 2/2 - Implement vhost-user stub for virtio-spi
>>
>
> Should I move only virtio_spi.h to the first patch, or all of the
> header files? I don't quite understand here.
Just the kernel headers (include/standard-headers). You should import
the kernel headers from a checked out kernel tree using:
./scripts/update-linux-headers.sh <path/to/linux.git>
and save them as a single commit for the 1st patch. As the SPI code is
not yet upstream just make it clear in the commit to avoid merging. e.g.
linux-headers: update to 6.10-rc5 + SPI patches (!merge)
This imports the headers from the current Linux HEAD + the VirtIO SPI
patches which are not yet upstream. Once the SPI work is up-streamed in
the kernel they will be imported from there.
Just make sure you kernel base is newer than the last import otherwise
you will get a lot of additional noise.
>
> Best Regards
> Thanks
>>
>>>
>>> diff --git a/hw/virtio/Kconfig b/hw/virtio/Kconfig
>>> index aa63ff7fd4..d5857651e5 100644
>>> --- a/hw/virtio/Kconfig
>>> +++ b/hw/virtio/Kconfig
>>> @@ -110,3 +110,8 @@ config VHOST_USER_SCMI
>>> bool
>>> default y
>>> depends on VIRTIO && VHOST_USER
>>> +
>>> +config VHOST_USER_SPI
>>> + bool
>>> + default y
>>> + depends on VIRTIO && VHOST_USER
>>> diff --git a/hw/virtio/meson.build b/hw/virtio/meson.build
>>> index 621fc65454..42296219e5 100644
>>> --- a/hw/virtio/meson.build
>>> +++ b/hw/virtio/meson.build
>>> @@ -26,6 +26,7 @@ if have_vhost
>>> system_virtio_ss.add(when: 'CONFIG_VHOST_USER_RNG', if_true:
>>> files('vhost-user-rng.c'))
>>> system_virtio_ss.add(when: 'CONFIG_VHOST_USER_SND', if_true:
>>> files('vhost-user-snd.c'))
>>> system_virtio_ss.add(when: 'CONFIG_VHOST_USER_INPUT', if_true:
>>> files('vhost-user-input.c'))
>>> + system_virtio_ss.add(when: 'CONFIG_VHOST_USER_SPI', if_true:
>>> files('vhost-user-spi.c'))
>>> # PCI Stubs
>>> system_virtio_ss.add(when: 'CONFIG_VIRTIO_PCI', if_true:
>>> files('vhost-user-device-pci.c'))
>>> @@ -39,6 +40,8 @@ if have_vhost
>>> if_true: files('vhost-user-snd-pci.c'))
>>> system_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI',
>>> 'CONFIG_VHOST_USER_INPUT'],
>>> if_true: files('vhost-user-input-pci.c'))
>>> + system_virtio_ss.add(when: ['CONFIG_VIRTIO_PCI',
>>> 'CONFIG_VHOST_USER_SPI'],
>>> + if_true: files('vhost-user-spi-pci.c'))
>>> endif
>>> if have_vhost_vdpa
>>> system_virtio_ss.add(files('vhost-vdpa.c'))
>>> diff --git a/hw/virtio/vhost-user-spi-pci.c b/hw/virtio/vhost-user-spi-pci.c
>>> new file mode 100644
>>> index 0000000000..3565d526af
>>> --- /dev/null
>>> +++ b/hw/virtio/vhost-user-spi-pci.c
>>> @@ -0,0 +1,69 @@
>>> +/*
>>> + * Vhost-user spi virtio device PCI glue
>>> + *
>>> + * Copyright(c) 2024 Qualcomm Innovation Center, Inc. All Rights Reserved.
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "hw/virtio/vhost-user-spi.h"
>>> +#include "hw/virtio/virtio-pci.h"
>>> +
>>> +struct VHostUserSPIPCI {
>>> + VirtIOPCIProxy parent_obj;
>>> + VHostUserSPI vdev;
>>> +};
>>> +
>>> +typedef struct VHostUserSPIPCI VHostUserSPIPCI;
>>> +
>>> +#define TYPE_VHOST_USER_SPI_PCI "vhost-user-spi-pci-base"
>>> +
>>> +DECLARE_INSTANCE_CHECKER(VHostUserSPIPCI, VHOST_USER_SPI_PCI,
>>> + TYPE_VHOST_USER_SPI_PCI)
>>> +
>>> +static void vhost_user_spi_pci_realize(VirtIOPCIProxy *vpci_dev, Error
>>> **errp)
>>> +{
>>> + VHostUserSPIPCI *dev = VHOST_USER_SPI_PCI(vpci_dev);
>>> + DeviceState *vdev = DEVICE(&dev->vdev);
>>> +
>>> + vpci_dev->nvectors = 1;
>>> + qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
>>> +}
>>> +
>>> +static void vhost_user_spi_pci_class_init(ObjectClass *klass, void *data)
>>> +{
>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>> + VirtioPCIClass *k = VIRTIO_PCI_CLASS(klass);
>>> + PCIDeviceClass *pcidev_k = PCI_DEVICE_CLASS(klass);
>>> + k->realize = vhost_user_spi_pci_realize;
>>> + set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>>> + pcidev_k->vendor_id = PCI_VENDOR_ID_REDHAT_QUMRANET;
>>> + pcidev_k->device_id = 0; /* Set by virtio-pci based on virtio id */
>>> + pcidev_k->revision = 0x00;
>>> + pcidev_k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
>>> +}
>>> +
>>> +static void vhost_user_spi_pci_instance_init(Object *obj)
>>> +{
>>> + VHostUserSPIPCI *dev = VHOST_USER_SPI_PCI(obj);
>>> +
>>> + virtio_instance_init_common(obj, &dev->vdev, sizeof(dev->vdev),
>>> + TYPE_VHOST_USER_SPI);
>>> +}
>>> +
>>> +static const VirtioPCIDeviceTypeInfo vhost_user_spi_pci_info = {
>>> + .base_name = TYPE_VHOST_USER_SPI_PCI,
>>> + .non_transitional_name = "vhost-user-spi-pci",
>>> + .instance_size = sizeof(VHostUserSPIPCI),
>>> + .instance_init = vhost_user_spi_pci_instance_init,
>>> + .class_init = vhost_user_spi_pci_class_init,
>>> +};
>>> +
>>> +static void vhost_user_spi_pci_register(void)
>>> +{
>>> + virtio_pci_types_register(&vhost_user_spi_pci_info);
>>> +}
>>> +
>>> +type_init(vhost_user_spi_pci_register);
>>> diff --git a/hw/virtio/vhost-user-spi.c b/hw/virtio/vhost-user-spi.c
>>> new file mode 100644
>>> index 0000000000..e138b8b53b
>>> --- /dev/null
>>> +++ b/hw/virtio/vhost-user-spi.c
>>> @@ -0,0 +1,66 @@
>>> +/*
>>> + * Vhost-user spi virtio device
>>> + *
>>> + * Copyright(c) 2024 Qualcomm Innovation Center, Inc. All Rights Reserved.
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qapi/error.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "hw/virtio/virtio-bus.h"
>>> +#include "hw/virtio/vhost-user-spi.h"
>>> +#include "qemu/error-report.h"
>>> +#include "standard-headers/linux/virtio_ids.h"
>>> +#include "standard-headers/linux/virtio_spi.h"
>>> +
>>> +static Property vspi_properties[] = {
>>> + DEFINE_PROP_CHR("chardev", VHostUserBase, chardev),
>>> + DEFINE_PROP_END_OF_LIST(),
>>> +};
>>> +
>>> +static void vspi_realize(DeviceState *dev, Error **errp)
>>> +{
>>> + VHostUserBase *vub = VHOST_USER_BASE(dev);
>>> + VHostUserBaseClass *vubc = VHOST_USER_BASE_GET_CLASS(dev);
>>> +
>>> + /* Fixed for SPI */
>>> + vub->virtio_id = VIRTIO_ID_SPI;
>>> + vub->num_vqs = 1;
>>> + vub->vq_size = 4;
>>> + vub->config_size = sizeof(struct virtio_spi_config);
>>> +
>>> + vubc->parent_realize(dev, errp);
>>> +}
>>> +
>>> +static const VMStateDescription vu_spi_vmstate = {
>>> + .name = "vhost-user-spi",
>>> + .unmigratable = 1,
>>> +};
>>> +
>>> +static void vu_spi_class_init(ObjectClass *klass, void *data)
>>> +{
>>> + DeviceClass *dc = DEVICE_CLASS(klass);
>>> + VHostUserBaseClass *vubc = VHOST_USER_BASE_CLASS(klass);
>>> +
>>> + dc->vmsd = &vu_spi_vmstate;
>>> + device_class_set_props(dc, vspi_properties);
>>> + device_class_set_parent_realize(dc, vspi_realize,
>>> + &vubc->parent_realize);
>>> + set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>>> +}
>>> +
>>> +static const TypeInfo vu_spi_info = {
>>> + .name = TYPE_VHOST_USER_SPI,
>>> + .parent = TYPE_VHOST_USER_BASE,
>>> + .instance_size = sizeof(VHostUserSPI),
>>> + .class_init = vu_spi_class_init,
>>> +};
>>> +
>>> +static void vu_spi_register_types(void)
>>> +{
>>> + type_register_static(&vu_spi_info);
>>> +}
>>> +
>>> +type_init(vu_spi_register_types)
>>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
>>> index 583a224163..689e2e21e7 100644
>>> --- a/hw/virtio/virtio.c
>>> +++ b/hw/virtio/virtio.c
>>> @@ -46,6 +46,7 @@
>>> #include "standard-headers/linux/virtio_iommu.h"
>>> #include "standard-headers/linux/virtio_mem.h"
>>> #include "standard-headers/linux/virtio_vsock.h"
>>> +#include "standard-headers/linux/virtio_spi.h"
>>> /*
>>> * Maximum size of virtio device config space
>>> @@ -194,7 +195,8 @@ const char *virtio_device_names[] = {
>>> [VIRTIO_ID_PARAM_SERV] = "virtio-param-serv",
>>> [VIRTIO_ID_AUDIO_POLICY] = "virtio-audio-pol",
>>> [VIRTIO_ID_BT] = "virtio-bluetooth",
>>> - [VIRTIO_ID_GPIO] = "virtio-gpio"
>>> + [VIRTIO_ID_GPIO] = "virtio-gpio",
>>> + [VIRTIO_ID_SPI] = "virtio-spi"
>>> };
>> For the vhost-user-stub bits when split from the headers:
>> Reviewed-by: Alex Bennée <alex.ben...@linaro.org>
>>
--
Alex Bennée
Virtualisation Tech Lead @ Linaro
