From: Jason Wang <jasowang@redhat.com>
To: Kishon Vijay Abraham I <kishon@ti.com>,
Cornelia Huck <cohuck@redhat.com>
Cc: Ohad Ben-Cohen <ohad@wizery.com>, Allen Hubbe <allenbh@gmail.com>,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
Dave Jiang <dave.jiang@intel.com>,
kvm@vger.kernel.org, linux-ntb@googlegroups.com,
"Michael S. Tsirkin" <mst@redhat.com>,
linux-pci@vger.kernel.org, linux-doc@vger.kernel.org,
linux-remoteproc@vger.kernel.org, linux-kernel@vger.kernel.org,
Bjorn Andersson <bjorn.andersson@linaro.org>,
netdev@vger.kernel.org, Stefan Hajnoczi <stefanha@redhat.com>,
Jon Mason <jdmason@kudzu.us>, Bjorn Helgaas <bhelgaas@google.com>,
Paolo Bonzini <pbonzini@redhat.com>,
virtualization@lists.linux-foundation.org
Subject: Re: [RFC PATCH 00/22] Enhance VHOST to enable SoC-to-SoC communication
Date: Fri, 18 Sep 2020 12:04:51 +0800 [thread overview]
Message-ID: <dc006fed-c3b6-8925-51d8-5ed3ee8662cd@redhat.com> (raw)
In-Reply-To: <67924594-c70e-390e-ce2e-dda41a94ada1@ti.com>
On 2020/9/16 下午7:47, Kishon Vijay Abraham I wrote:
> Hi Jason,
>
> On 16/09/20 8:40 am, Jason Wang wrote:
>> On 2020/9/15 下午11:47, Kishon Vijay Abraham I wrote:
>>> Hi Jason,
>>>
>>> On 15/09/20 1:48 pm, Jason Wang wrote:
>>>> Hi Kishon:
>>>>
>>>> On 2020/9/14 下午3:23, Kishon Vijay Abraham I wrote:
>>>>>> Then you need something that is functional equivalent to virtio PCI
>>>>>> which is actually the concept of vDPA (e.g vDPA provides
>>>>>> alternatives if
>>>>>> the queue_sel is hard in the EP implementation).
>>>>> Okay, I just tried to compare the 'struct vdpa_config_ops' and 'struct
>>>>> vhost_config_ops' ( introduced in [RFC PATCH 03/22] vhost: Add ops for
>>>>> the VHOST driver to configure VHOST device).
>>>>>
>>>>> struct vdpa_config_ops {
>>>>> /* Virtqueue ops */
>>>>> int (*set_vq_address)(struct vdpa_device *vdev,
>>>>> u16 idx, u64 desc_area, u64 driver_area,
>>>>> u64 device_area);
>>>>> void (*set_vq_num)(struct vdpa_device *vdev, u16 idx, u32 num);
>>>>> void (*kick_vq)(struct vdpa_device *vdev, u16 idx);
>>>>> void (*set_vq_cb)(struct vdpa_device *vdev, u16 idx,
>>>>> struct vdpa_callback *cb);
>>>>> void (*set_vq_ready)(struct vdpa_device *vdev, u16 idx, bool
>>>>> ready);
>>>>> bool (*get_vq_ready)(struct vdpa_device *vdev, u16 idx);
>>>>> int (*set_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>>> const struct vdpa_vq_state *state);
>>>>> int (*get_vq_state)(struct vdpa_device *vdev, u16 idx,
>>>>> struct vdpa_vq_state *state);
>>>>> struct vdpa_notification_area
>>>>> (*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
>>>>> /* vq irq is not expected to be changed once DRIVER_OK is set */
>>>>> int (*get_vq_irq)(struct vdpa_device *vdv, u16 idx);
>>>>>
>>>>> /* Device ops */
>>>>> u32 (*get_vq_align)(struct vdpa_device *vdev);
>>>>> u64 (*get_features)(struct vdpa_device *vdev);
>>>>> int (*set_features)(struct vdpa_device *vdev, u64 features);
>>>>> void (*set_config_cb)(struct vdpa_device *vdev,
>>>>> struct vdpa_callback *cb);
>>>>> u16 (*get_vq_num_max)(struct vdpa_device *vdev);
>>>>> u32 (*get_device_id)(struct vdpa_device *vdev);
>>>>> u32 (*get_vendor_id)(struct vdpa_device *vdev);
>>>>> u8 (*get_status)(struct vdpa_device *vdev);
>>>>> void (*set_status)(struct vdpa_device *vdev, u8 status);
>>>>> void (*get_config)(struct vdpa_device *vdev, unsigned int offset,
>>>>> void *buf, unsigned int len);
>>>>> void (*set_config)(struct vdpa_device *vdev, unsigned int offset,
>>>>> const void *buf, unsigned int len);
>>>>> u32 (*get_generation)(struct vdpa_device *vdev);
>>>>>
>>>>> /* DMA ops */
>>>>> int (*set_map)(struct vdpa_device *vdev, struct vhost_iotlb
>>>>> *iotlb);
>>>>> int (*dma_map)(struct vdpa_device *vdev, u64 iova, u64 size,
>>>>> u64 pa, u32 perm);
>>>>> int (*dma_unmap)(struct vdpa_device *vdev, u64 iova, u64 size);
>>>>>
>>>>> /* Free device resources */
>>>>> void (*free)(struct vdpa_device *vdev);
>>>>> };
>>>>>
>>>>> +struct vhost_config_ops {
>>>>> + int (*create_vqs)(struct vhost_dev *vdev, unsigned int nvqs,
>>>>> + unsigned int num_bufs, struct vhost_virtqueue *vqs[],
>>>>> + vhost_vq_callback_t *callbacks[],
>>>>> + const char * const names[]);
>>>>> + void (*del_vqs)(struct vhost_dev *vdev);
>>>>> + int (*write)(struct vhost_dev *vdev, u64 vhost_dst, void *src,
>>>>> int len);
>>>>> + int (*read)(struct vhost_dev *vdev, void *dst, u64 vhost_src, int
>>>>> len);
>>>>> + int (*set_features)(struct vhost_dev *vdev, u64 device_features);
>>>>> + int (*set_status)(struct vhost_dev *vdev, u8 status);
>>>>> + u8 (*get_status)(struct vhost_dev *vdev);
>>>>> +};
>>>>> +
>>>>> struct virtio_config_ops
>>>>> I think there's some overlap here and some of the ops tries to do the
>>>>> same thing.
>>>>>
>>>>> I think it differs in (*set_vq_address)() and (*create_vqs)().
>>>>> [create_vqs() introduced in struct vhost_config_ops provides
>>>>> complimentary functionality to (*find_vqs)() in struct
>>>>> virtio_config_ops. It seemingly encapsulates the functionality of
>>>>> (*set_vq_address)(), (*set_vq_num)(), (*set_vq_cb)(),..].
>>>>>
>>>>> Back to the difference between (*set_vq_address)() and (*create_vqs)(),
>>>>> set_vq_address() directly provides the virtqueue address to the vdpa
>>>>> device but create_vqs() only provides the parameters of the virtqueue
>>>>> (like the number of virtqueues, number of buffers) but does not
>>>>> directly
>>>>> provide the address. IMO the backend client drivers (like net or vhost)
>>>>> shouldn't/cannot by itself know how to access the vring created on
>>>>> virtio front-end. The vdpa device/vhost device should have logic for
>>>>> that. That will help the client drivers to work with different types of
>>>>> vdpa device/vhost device and can access the vring created by virtio
>>>>> irrespective of whether the vring can be accessed via mmio or kernel
>>>>> space or user space.
>>>>>
>>>>> I think vdpa always works with client drivers in userspace and
>>>>> providing
>>>>> userspace address for vring.
>>>> Sorry for being unclear. What I meant is not replacing vDPA with the
>>>> vhost(bus) you proposed but the possibility of replacing virtio-pci-epf
>>>> with vDPA in:
>>> Okay, so the virtio back-end still use vhost and front end should use
>>> vDPA. I see. So the host side PCI driver for EPF should populate
>>> vdpa_config_ops and invoke vdpa_register_device().
>>
>> Yes.
>>
>>
>>>> My question is basically for the part of virtio_pci_epf_send_command(),
>>>> so it looks to me you have a vendor specific API to replace the
>>>> virtio-pci layout of the BAR:
>>> Even when we use vDPA, we have to use some sort of
>>> virtio_pci_epf_send_command() to communicate with virtio backend right?
>>
>> Right.
>>
>>
>>> Right, the layout is slightly different from the standard layout.
>>>
>>> This is the layout
>>> struct epf_vhost_reg_queue {
>>> u8 cmd;
>>> u8 cmd_status;
>>> u16 status;
>>> u16 num_buffers;
>>> u16 msix_vector;
>>> u64 queue_addr;
>>
>> What's the meaning of queue_addr here?
> Using queue_addr, the virtio front-end communicates the address of the
> allocated memory for virtqueue to the virtio back-end.
>> Does not mean the device expects a contiguous memory for avail/desc/used
>> ring?
> It's contiguous memory. Isn't this similar to other virtio transport
> (both PCI legacy and modern interface)?.
That's only for legacy device, for modern device we don't have such
restriction.
>>
>>> } __packed;
>>>
>>> struct epf_vhost_reg {
>>> u64 host_features;
>>> u64 guest_features;
>>> u16 msix_config;
>>> u16 num_queues;
>>> u8 device_status;
>>> u8 config_generation;
>>> u32 isr;
>>> u8 cmd;
>>> u8 cmd_status;
>>> struct epf_vhost_reg_queue vq[MAX_VQS];
>>> } __packed;
>>>> +static int virtio_pci_epf_send_command(struct virtio_pci_device
>>>> *vp_dev,
>>>> + u32 command)
>>>> +{
>>>> + struct virtio_pci_epf *pci_epf;
>>>> + void __iomem *ioaddr;
>>>> + ktime_t timeout;
>>>> + bool timedout;
>>>> + int ret = 0;
>>>> + u8 status;
>>>> +
>>>> + pci_epf = to_virtio_pci_epf(vp_dev);
>>>> + ioaddr = vp_dev->ioaddr;
>>>> +
>>>> + mutex_lock(&pci_epf->lock);
>>>> + writeb(command, ioaddr + HOST_CMD);
>>>> + timeout = ktime_add_ms(ktime_get(), COMMAND_TIMEOUT);
>>>> + while (1) {
>>>> + timedout = ktime_after(ktime_get(), timeout);
>>>> + status = readb(ioaddr + HOST_CMD_STATUS);
>>>> +
>>>>
>>>> Several questions:
>>>>
>>>> - It's not clear to me how the synchronization is done between the RC
>>>> and EP. E.g how and when the value of HOST_CMD_STATUS can be changed.
>>> The HOST_CMD (commands sent to the EP) is serialized by using mutex.
>>> Once the EP reads the command, it resets the value in HOST_CMD. So
>>> HOST_CMD is less likely an issue.
>>
>> Here's my understanding of the protocol:
>>
>> 1) RC write to HOST_CMD
>> 2) RC wait for HOST_CMD_STATUS to be HOST_CMD_STATUS_OKAY
> That's right!
>> It looks to me what EP should do is
>>
>> 1) EP reset HOST_CMD after reading new command
> That's right! It does.
>> And it looks to me EP should also reset HOST_CMD_STATUS here?
> yeah, that would require RC to send another command to reset the status.
> Didn't see it required in the normal scenario but good to add this.
>> (I thought there should be patch to handle stuffs like this but I didn't
>> find it in this series)
> This is added in [RFC PATCH 19/22] PCI: endpoint: Add EP function driver
> to provide VHOST interface
>
> pci_epf_vhost_cmd_handler() gets commands from RC using "reg->cmd;". On
> the EP side, it is local memory access (mapped to BAR memory exposed to
> the host) and hence accessed using structure member access.
Thanks for the pointer, will have a look at and I think this part need
to be carefully designed and the key to the success of the epf transport.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2020-09-18 4:05 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-02 8:21 [RFC PATCH 00/22] Enhance VHOST to enable SoC-to-SoC communication Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 01/22] vhost: Make _feature_ bits a property of vhost device Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 02/22] vhost: Introduce standard Linux driver model in VHOST Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 03/22] vhost: Add ops for the VHOST driver to configure VHOST device Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 04/22] vringh: Add helpers to access vring in MMIO Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 05/22] vhost: Add MMIO helpers for operations on vhost virtqueue Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 06/22] vhost: Introduce configfs entry for configuring VHOST Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 07/22] virtio_pci: Use request_threaded_irq() instead of request_irq() Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 08/22] rpmsg: virtio_rpmsg_bus: Disable receive virtqueue callback when reading messages Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 09/22] rpmsg: Introduce configfs entry for configuring rpmsg Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 10/22] rpmsg: virtio_rpmsg_bus: Add Address Service Notification support Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 11/22] rpmsg: virtio_rpmsg_bus: Move generic rpmsg structure to rpmsg_internal.h Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 12/22] virtio: Add ops to allocate and free buffer Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 13/22] rpmsg: virtio_rpmsg_bus: Use virtio_alloc_buffer() and virtio_free_buffer() Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 14/22] rpmsg: Add VHOST based remote processor messaging bus Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 15/22] samples/rpmsg: Setup delayed work to send message Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 16/22] samples/rpmsg: Wait for address to be bound to rpdev for sending message Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 17/22] rpmsg.txt: Add Documentation to configure rpmsg using configfs Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 18/22] virtio_pci: Add VIRTIO driver for VHOST on Configurable PCIe Endpoint device Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 19/22] PCI: endpoint: Add EP function driver to provide VHOST interface Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 20/22] NTB: Add a new NTB client driver to implement VIRTIO functionality Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 21/22] NTB: Add a new NTB client driver to implement VHOST functionality Kishon Vijay Abraham I
2020-07-02 8:21 ` [RFC PATCH 22/22] NTB: Describe ntb_virtio and ntb_vhost client in the documentation Kishon Vijay Abraham I
2020-07-02 9:51 ` [RFC PATCH 00/22] Enhance VHOST to enable SoC-to-SoC communication Michael S. Tsirkin
2020-07-02 10:10 ` Jason Wang
2020-07-02 13:35 ` Kishon Vijay Abraham I
2020-07-03 7:16 ` Jason Wang
2020-07-06 9:32 ` Kishon Vijay Abraham I
2020-07-07 9:47 ` Jason Wang
2020-07-07 14:45 ` Kishon Vijay Abraham I
2020-07-08 11:22 ` Jason Wang
2020-07-08 13:13 ` Kishon Vijay Abraham I
2020-07-09 6:26 ` Jason Wang
2020-08-28 10:34 ` Cornelia Huck
[not found] ` <ac8f7e4f-9f46-919a-f5c2-89b07794f0ab@ti.com>
2020-09-01 8:50 ` Jason Wang
2020-09-08 16:37 ` Cornelia Huck
2020-09-09 8:41 ` Jason Wang
[not found] ` <edf25301-93c0-4ba6-aa85-5f04137d0906@ti.com>
2020-09-15 8:18 ` Jason Wang
[not found] ` <181ae83d-edeb-9406-27cc-1195fe29ae95@ti.com>
2020-09-16 3:10 ` Jason Wang
[not found] ` <67924594-c70e-390e-ce2e-dda41a94ada1@ti.com>
2020-09-18 4:04 ` Jason Wang [this message]
2020-07-15 17:15 ` Mathieu Poirier
2020-07-02 10:25 ` Kishon Vijay Abraham I
2020-07-02 17:31 ` Mathieu Poirier
2020-07-03 6:17 ` Kishon Vijay Abraham I
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=dc006fed-c3b6-8925-51d8-5ed3ee8662cd@redhat.com \
--to=jasowang@redhat.com \
--cc=allenbh@gmail.com \
--cc=bhelgaas@google.com \
--cc=bjorn.andersson@linaro.org \
--cc=cohuck@redhat.com \
--cc=dave.jiang@intel.com \
--cc=jdmason@kudzu.us \
--cc=kishon@ti.com \
--cc=kvm@vger.kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-ntb@googlegroups.com \
--cc=linux-pci@vger.kernel.org \
--cc=linux-remoteproc@vger.kernel.org \
--cc=lorenzo.pieralisi@arm.com \
--cc=mst@redhat.com \
--cc=netdev@vger.kernel.org \
--cc=ohad@wizery.com \
--cc=pbonzini@redhat.com \
--cc=stefanha@redhat.com \
--cc=virtualization@lists.linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).