On Tue, Nov 17, 2020 at 11:11:21AM +0000, Stefan Hajnoczi wrote:
>On Fri, Nov 13, 2020 at 02:47:04PM +0100, Stefano Garzarella wrote:
>> +static void vdpasim_blk_work(struct work_struct *work)
>> +{
>> + struct vdpasim *vdpasim = container_of(work, struct vdpasim, work);
>> + u8 status = VIRTIO_BLK_S_OK;
>> + int i;
>> +
>> + spin_lock(&vdpasim->lock);
>> +
>> + if (!(vdpasim->status & VIRTIO_CONFIG_S_DRIVER_OK))
>> + goto out;
>> +
>> + for (i = 0; i < VDPASIM_BLK_VQ_NUM; i++) {
>> + struct vdpasim_virtqueue *vq = &vdpasim->vqs[i];
>> +
>> + if (!vq->ready)
>> + continue;
>> +
>> + while (vringh_getdesc_iotlb(&vq->vring, &vq->iov, &vq->iov,
>> + &vq->head, GFP_ATOMIC) > 0) {
>> +
>> + int write;
>> +
>> + vq->iov.i = vq->iov.used - 1;
>> + write = vringh_iov_push_iotlb(&vq->vring, &vq->iov, &status, 1);
>> + if (write <= 0)
>> + break;
>
>We're lucky the guest driver doesn't crash after VIRTIO_BLK_T_GET_ID? :)

The crash could happen if the simulator doesn't put the string
terminator, but in virtio_blk.c, the serial_show() initialize the buffer
putting the string terminator in the VIRTIO_BLK_ID_BYTES element:

buf[VIRTIO_BLK_ID_BYTES] = '\0';
err = virtblk_get_id(disk, buf);

This should prevent the issue, right?

However in the last patch of this series I implemented
VIRTIO_BLK_T_GET_ID support :-)

Thanks,
Stefano

2020-11-17 14:30:36

by Stefano Garzarella

[permalink] [raw]

Subject: Re: [PATCH RFC 06/12] vdpa_sim: add struct vdpasim_device to store device properties

On Tue, Nov 17, 2020 at 11:23:05AM +0000, Stefan Hajnoczi wrote:
>On Fri, Nov 13, 2020 at 02:47:06PM +0100, Stefano Garzarella wrote:
>> Move device properties used during the entire life cycle in a new
>> structure to simplify the copy of these fields during the vdpasim
>> initialization.
>>
>> Signed-off-by: Stefano Garzarella <[email protected]>
>> ---
>> drivers/vdpa/vdpa_sim/vdpa_sim.h | 17 ++++++++------
>> drivers/vdpa/vdpa_sim/vdpa_sim.c | 33 ++++++++++++++--------------
>> drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 8 +++++--
>> drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 9 +++++---
>> 4 files changed, 38 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim.h b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> index 6a1267c40d5e..76e642042eb0 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim.h
>> @@ -40,12 +40,17 @@ struct vdpasim_virtqueue {
>> irqreturn_t (*cb)(void *data);
>> };
>>
>> +struct vdpasim_device {
>> + u64 supported_features;
>> + u32 id;
>> + int nvqs;
>> +};
>> +
>> struct vdpasim_init_attr {
>> - u32 device_id;
>> - u64 features;
>> + struct vdpasim_device device;
>
>It's unclear to me what the exact purpose of struct vdpasim_device is.
>At least the name reminds me of struct device, which this is not.
>
>Should this be called just struct vdpasim_attr or struct
>vdpasim_dev_attr? In other words, the attributes that are needed even
>after intialization?

Yes, they are attributes that are needed even after intialization,
so I think vdpasim_dev_attr should be better.

I'll change it and I'll try to write a better commit message.

Thanks,
Stefano

2020-11-17 14:36:34

by Stefano Garzarella

[permalink] [raw]

Subject: Re: [PATCH RFC 12/12] vdpa_sim_blk: implement ramdisk behaviour

On Tue, Nov 17, 2020 at 11:36:36AM +0000, Stefan Hajnoczi wrote:
>On Fri, Nov 13, 2020 at 02:47:12PM +0100, Stefano Garzarella wrote:
>> diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> index 8e41b3ab98d5..68e74383322f 100644
>> --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c
>> @@ -7,6 +7,7 @@
>> */
>>
>> #include <linux/module.h>
>> +#include <linux/blkdev.h>
>> #include <uapi/linux/virtio_blk.h>
>>
>> #include "vdpa_sim.h"
>> @@ -24,10 +25,137 @@
>>
>> static struct vdpasim *vdpasim_blk_dev;
>>
>> +static int vdpasim_blk_handle_req(struct vdpasim *vdpasim,
>> + struct vdpasim_virtqueue *vq)
>
>This function has a non-standard int return value. Please document it.

Yes, I'll do.

>
>> +{
>> + size_t wrote = 0, to_read = 0, to_write = 0;
>> + struct virtio_blk_outhdr hdr;
>> + uint8_t status;
>> + uint32_t type;
>> + ssize_t bytes;
>> + loff_t offset;
>> + int i, ret;
>> +
>> + vringh_kiov_cleanup(&vq->riov);
>> + vringh_kiov_cleanup(&vq->wiov);
>> +
>> + ret = vringh_getdesc_iotlb(&vq->vring, &vq->riov, &vq->wiov,
>> + &vq->head, GFP_ATOMIC);
>> + if (ret != 1)
>> + return ret;
>> +
>> + for (i = 0; i < vq->wiov.used; i++)
>> + to_write += vq->wiov.iov[i].iov_len;
>> + to_write -= 1; /* last byte is the status */
>
>What if vq->wiov.used == 0?

Right, we should discard the descriptor.

>
>> +
>> + for (i = 0; i < vq->riov.used; i++)
>> + to_read += vq->riov.iov[i].iov_len;
>> +
>> + bytes = vringh_iov_pull_iotlb(&vq->vring, &vq->riov, &hdr, sizeof(hdr));
>> + if (bytes != sizeof(hdr))
>> + return 0;
>> +
>> + to_read -= bytes;
>> +
>> + type = le32_to_cpu(hdr.type);
>> + offset = le64_to_cpu(hdr.sector) << SECTOR_SHIFT;
>> + status = VIRTIO_BLK_S_OK;
>> +
>> + switch (type) {
>> + case VIRTIO_BLK_T_IN:
>> + if (offset + to_write > VDPASIM_BLK_CAPACITY << SECTOR_SHIFT) {
>
>Integer overflow is not handled.

I'll fix.

Thanks,
Stefano

2020-11-17 15:07:58

by Stefano Garzarella

[permalink] [raw]

Subject: Re: [PATCH RFC 01/12] vhost-vdpa: add support for vDPA blk devices

On Tue, Nov 17, 2020 at 10:57:09AM +0000, Stefan Hajnoczi wrote:
>On Fri, Nov 13, 2020 at 02:47:01PM +0100, Stefano Garzarella wrote:
>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>> index 2754f3069738..fb0411594963 100644
>> --- a/drivers/vhost/vdpa.c
>> +++ b/drivers/vhost/vdpa.c
>> @@ -22,6 +22,7 @@
>> #include <linux/nospec.h>
>> #include <linux/vhost.h>
>> #include <linux/virtio_net.h>
>> +#include <linux/virtio_blk.h>
>>
>> #include "vhost.h"
>>
>> @@ -194,6 +195,9 @@ static int vhost_vdpa_config_validate(struct vhost_vdpa *v,
>> case VIRTIO_ID_NET:
>> size = sizeof(struct virtio_net_config);
>> break;
>> + case VIRTIO_ID_BLOCK:
>> + size = sizeof(struct virtio_blk_config);
>> + break;
>> }
>>
>> if (c->len == 0)
>
>Can vdpa_config_ops->get/set_config() handle the size check instead of
>hardcoding device-specific knowledge into drivers/vhost/vdpa.c?

I agree that this should be better. For example we already check if the
buffer is large enough in the simulator callbacks, we only need to
return an error in case it is not true.

@Jason, do you think it's okay to add a return value to
vdpa_config_ops->get/set_config() to handle the size check?

Thanks,
Stefano

2020-11-17 16:46:05

On 2020/12/22 下午8:29, Jason Wang wrote:
>
> On 2020/12/22 下午6:57, Stefano Garzarella wrote:
>> On Tue, Dec 22, 2020 at 10:44:48AM +0800, Jason Wang wrote:
>>>
>>> On 2020/12/21 下午7:14, Stefano Garzarella wrote:
>>>> On Mon, Dec 21, 2020 at 11:16:54AM +0800, Jason Wang wrote:
>>>>>
>>>>> On 2020/12/18 下午7:38, Stefano Garzarella wrote:
>>>>>> On Mon, Nov 16, 2020 at 11:37:48AM +0800, Jason Wang wrote:
>>>>>>>
>>>>>>> On 2020/11/13 下午9:47, Stefano Garzarella wrote:
>>>>>>>> Thanks to Max that started this work!
>>>>>>>> I took his patches, and extended the block simulator a bit.
>>>>>>>>
>>>>>>>> This series moves the network device simulator in a new module
>>>>>>>> (vdpa_sim_net) and leaves the generic functions in the vdpa_sim
>>>>>>>> core
>>>>>>>> module, allowing the possibility to add new vDPA device
>>>>>>>> simulators.
>>>>>>>> Then we added a new vdpa_sim_blk module to simulate a block
>>>>>>>> device.
>>>>>>>>
>>>>>>>> I'm not sure about patch 11 ("vringh: allow vringh_iov_xfer()
>>>>>>>> to skip
>>>>>>>> bytes when ptr is NULL"), maybe we can add a new functions
>>>>>>>> instead of
>>>>>>>> modify vringh_iov_xfer().
>>>>>>>>
>>>>>>>> As Max reported, I'm also seeing errors with vdpa_sim_blk
>>>>>>>> related to
>>>>>>>> iotlb and vringh when there is high load, these are some of the
>>>>>>>> error
>>>>>>>> messages I can see randomly:
>>>>>>>>
>>>>>>>> vringh: Failed to access avail idx at 00000000e8deb2cc
>>>>>>>> vringh: Failed to read head: idx 6289 address 00000000e1ad1d50
>>>>>>>> vringh: Failed to get flags at 000000006635d7a3
>>>>>>>>
>>>>>>>> virtio_vdpa vdpa0: vringh_iov_push_iotlb() error: -14 offset:
>>>>>>>> 0x2840000 len: 0x20000
>>>>>>>> virtio_vdpa vdpa0: vringh_iov_pull_iotlb() error: -14 offset:
>>>>>>>> 0x58ee000 len: 0x3000
>>>>>>>>
>>>>>>>> These errors should all be related to the fact that
>>>>>>>> iotlb_translate()
>>>>>>>> fails with -EINVAL, so it seems that we miss some mapping.
>>>>>>>
>>>>>>>
>>>>>>> Is this only reproducible when there's multiple co-current
>>>>>>> accessing of IOTLB? If yes, it's probably a hint that some kind
>>>>>>> of synchronization is still missed somewhere.
>>>>>>>
>>>>>>> It might be useful to log the dma_map/unmp in both virtio_ring
>>>>>>> and vringh to see who is missing the map.
>>>>>>>
>>>>>>
>>>>>> Just an update about these issues with vdpa-sim-blk.
>>>>>> I've been focusing a little bit on these failures over the last
>>>>>> few days and have found two issues related to the IOTLB/IOMMU:
>>>>>>
>>>>>> 1. Some requests coming from the block layer fills the SG list
>>>>>> with multiple buffers that had the same physical address. This
>>>>>> happens for example while using 'mkfs', at some points multiple
>>>>>> sectors are zeroed so multiple SG elements point to the same
>>>>>> physical page that is zeroed.
>>>>>> Since we are using vhost_iotlb_del_range() in the
>>>>>> vdpasim_unmap_page(), this removes all the overlapped ranges. I
>>>>>> fixed removing a single map in vdpasim_unmap_page(), but has an
>>>>>> alternative we can implement some kind of reference counts.
>>>>>
>>>>>
>>>>> I think we need to do what hardware do. So using refcount is
>>>>> probably not a good ida.
>>>>
>>>> Okay, so since we are using for simplicity an identical mapping, we
>>>> are assigning the same dma_addr to multiple pages.
>>>
>>>
>>> I think I get you now. That's the root cause for the failure.
>>
>> Yes, sorry, I didn't explain well previously.
>>
>>>
>>> Then I think we need an simple iova allocator for vdpa simulator,
>>> and it might be useful for VDUSE as well.
>>
>> Okay, I'll work on it.
>> If you have an example to follow or some pointers, they are welcome :-)
>
>
> Kernel had implemented one in iova.c but I'm not sure we need the
> complexity like that. Or we can just use rbtree or idr to implement a
> simpler one.

VDUSE[1] implements another allocator, but it's still complicated since
it needs to track bounce pages. I feel like we'd better start from a
simple one.

Thanks

[1] https://www.spinics.net/lists/linux-mm/msg231576.html

>
> Thanks
>
>
>>
>> Thanks,
>> Stefano
>>

2020-12-22 13:27:36

by Stefano Garzarella

[permalink] [raw]

Subject: Re: [PATCH RFC 00/12] vdpa: generalize vdpa simulator and add block device

On Tue, Dec 22, 2020 at 08:29:20PM +0800, Jason Wang wrote:
>
>On 2020/12/22 下午6:57, Stefano Garzarella wrote:
>>On Tue, Dec 22, 2020 at 10:44:48AM +0800, Jason Wang wrote:
>>>
>>>On 2020/12/21 下午7:14, Stefano Garzarella wrote:
>>>>On Mon, Dec 21, 2020 at 11:16:54AM +0800, Jason Wang wrote:
>>>>>
>>>>>On 2020/12/18 下午7:38, Stefano Garzarella wrote:
>>>>>>On Mon, Nov 16, 2020 at 11:37:48AM +0800, Jason Wang wrote:
>>>>>>>
>>>>>>>On 2020/11/13 下午9:47, Stefano Garzarella wrote:
>>>>>>>>Thanks to Max that started this work!
>>>>>>>>I took his patches, and extended the block simulator a bit.
>>>>>>>>
>>>>>>>>This series moves the network device simulator in a new module
>>>>>>>>(vdpa_sim_net) and leaves the generic functions in the
>>>>>>>>vdpa_sim core
>>>>>>>>module, allowing the possibility to add new vDPA device simulators.
>>>>>>>>Then we added a new vdpa_sim_blk module to simulate a block device.
>>>>>>>>
>>>>>>>>I'm not sure about patch 11 ("vringh: allow
>>>>>>>>vringh_iov_xfer() to skip
>>>>>>>>bytes when ptr is NULL"), maybe we can add a new
>>>>>>>>functions instead of
>>>>>>>>modify vringh_iov_xfer().
>>>>>>>>
>>>>>>>>As Max reported, I'm also seeing errors with
>>>>>>>>vdpa_sim_blk related to
>>>>>>>>iotlb and vringh when there is high load, these are some
>>>>>>>>of the error
>>>>>>>>messages I can see randomly:
>>>>>>>>
>>>>>>>> vringh: Failed to access avail idx at 00000000e8deb2cc
>>>>>>>> vringh: Failed to read head: idx 6289 address 00000000e1ad1d50
>>>>>>>> vringh: Failed to get flags at 000000006635d7a3
>>>>>>>>
>>>>>>>> virtio_vdpa vdpa0: vringh_iov_push_iotlb() error: -14
>>>>>>>>offset: 0x2840000 len: 0x20000
>>>>>>>> virtio_vdpa vdpa0: vringh_iov_pull_iotlb() error: -14
>>>>>>>>offset: 0x58ee000 len: 0x3000
>>>>>>>>
>>>>>>>>These errors should all be related to the fact that
>>>>>>>>iotlb_translate()
>>>>>>>>fails with -EINVAL, so it seems that we miss some mapping.
>>>>>>>
>>>>>>>
>>>>>>>Is this only reproducible when there's multiple co-current
>>>>>>>accessing of IOTLB? If yes, it's probably a hint that some
>>>>>>>kind of synchronization is still missed somewhere.
>>>>>>>
>>>>>>>It might be useful to log the dma_map/unmp in both
>>>>>>>virtio_ring and vringh to see who is missing the map.
>>>>>>>
>>>>>>
>>>>>>Just an update about these issues with vdpa-sim-blk.
>>>>>>I've been focusing a little bit on these failures over the
>>>>>>last few days and have found two issues related to the
>>>>>>IOTLB/IOMMU:
>>>>>>
>>>>>>1. Some requests coming from the block layer fills the SG
>>>>>>list with multiple buffers that had the same physical
>>>>>>address. This happens for example while using 'mkfs', at
>>>>>>some points multiple sectors are zeroed so multiple SG
>>>>>>elements point to the same physical page that is zeroed.
>>>>>>Since we are using vhost_iotlb_del_range() in the
>>>>>>vdpasim_unmap_page(), this removes all the overlapped
>>>>>>ranges. I fixed removing a single map in
>>>>>>vdpasim_unmap_page(), but has an alternative we can
>>>>>>implement some kind of reference counts.
>>>>>
>>>>>
>>>>>I think we need to do what hardware do. So using refcount is
>>>>>probably not a good ida.
>>>>
>>>>Okay, so since we are using for simplicity an identical mapping,
>>>>we are assigning the same dma_addr to multiple pages.
>>>
>>>
>>>I think I get you now. That's the root cause for the failure.
>>
>>Yes, sorry, I didn't explain well previously.
>>
>>>
>>>Then I think we need an simple iova allocator for vdpa simulator,
>>>and it might be useful for VDUSE as well.
>>
>>Okay, I'll work on it.
>>If you have an example to follow or some pointers, they are welcome :-)
>
>
>Kernel had implemented one in iova.c but I'm not sure we need the
>complexity like that. Or we can just use rbtree or idr to implement a
>simpler one.

Yeah, I found it and I started to integrate it in the simulator.
Also if it appears complicated, it seams to me that it should be simple
to integrate.

I'll give it a try, and if it is too complicate, I'll switch to a simple
rbtree.

Thanks,
Stefano