Re: [PATCH v1] vhost-vdpa: Set discarding of RAM broken when initializing the backend

[David Hildenbrand <david@redhat.com>]

On 03.03.21 03:53, Jason Wang wrote:
> 
> On 2021/3/3 12:21 上午, David Hildenbrand wrote:
>> Similar to VFIO, vDPA will go ahead an map+pin all guest memory. Memory
>> that used to be discarded will get re-populated and if we
>> discard+re-access memory after mapping+pinning, the pages mapped into the
>> vDPA IOMMU will go out of sync with the actual pages mapped into the user
>> space page tables.
>>
>> Set discarding of RAM broken such that:
>> - virtio-mem and vhost-vdpa run mutually exclusive
>> - virtio-balloon is inhibited and no memory discards will get issued
>>
>> In the future, we might be able to support coordinated discarding of RAM
>> as used by virtio-mem and as planned for VFIO.
>>
>> Cc: Jason Wang <jasowang@redhat.com>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Cindy Lu <lulu@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> 
>> ---
>>
>> Note: I was not actually able to reproduce/test as I fail to get the
>> vdpa_sim/vdpa_sim_net running on upstream Linux (whetever vdpa, vhost_vdpa,
>> vdpa_sim, vdpa_sim_net modules I probe, and in which order, no vdpa devices
>> appear under /sys/bus/vdpa/devices/ or /dev/).
> 
> 
> The device creation was switched to use vdpa tool that is integrated
> with iproue2[1].
> 
> [1]
> https://git.kernel.org/pub/scm/network/iproute2/iproute2-next.git/commit/?id=143610383da51e1f868c6d5a2a5e2fb552293d18

It would be great to document that somewhere if not already done. I only 
found older RH documentations that were not aware of that. I'll give it 
a try - thanks!

> 
> 
>>
>> ---
>>    hw/virtio/vhost-vdpa.c | 13 +++++++++++++
>>    1 file changed, 13 insertions(+)
>>
>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>> index 01d2101d09..86058d4041 100644
>> --- a/hw/virtio/vhost-vdpa.c
>> +++ b/hw/virtio/vhost-vdpa.c
>> @@ -278,6 +278,17 @@ static int vhost_vdpa_init(struct vhost_dev *dev, void *opaque)
>>        uint64_t features;
>>        assert(dev->vhost_ops->backend_type == VHOST_BACKEND_TYPE_VDPA);
>>        trace_vhost_vdpa_init(dev, opaque);
>> +    int ret;
>> +
>> +    /*
>> +     * Similar to VFIO, we end up pinning all guest memory and have to
>> +     * disable discarding of RAM.
>> +     */
>> +    ret = ram_block_discard_disable(true);
>> +    if (ret) {
>> +        error_report("Cannot set discarding of RAM broken");
>> +        return ret;
>> +    }
> 
> 
> vDPA will support non pinning (shared VM) backend soon[2]. So I guess we
> need a flag to be advertised to usersapce then we can conditionly enable
> the discard here.

I thought that was already the default (because I stumbled over 
enforcing guest IOMMU) but was surprised when I had a look at the 
implementation.

Having a flag sounds good.

BTW: I assume iommu support is not fully working yet, right? I don't see 
special casing for iommu regions, including registering the listener and 
updating the mapping.

-- 
Thanks,

David / dhildenb