Re: [PATCH] Add vhost-user-spi and vhost-user-spi-pci devices

["Alex Bennée" <alex.bennee@linaro.org>]

Haixu Cui <quic_haixcui@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_haixcui@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_haixcui@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.bennee@linaro.org>
>> 

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro