|
From: | Maxime Coquelin |
Subject: | Re: [Qemu-devel] [PATCH 3/6] vhost: Update rings information for IOTLB earlier |
Date: | Fri, 19 May 2017 11:48:52 +0200 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0 |
On 05/18/2017 05:24 PM, Michael S. Tsirkin wrote:
On Thu, May 18, 2017 at 04:45:23PM +0200, Maxime Coquelin wrote:Hi Michael, On 05/18/2017 09:35 AM, Maxime Coquelin wrote:On 05/17/2017 06:41 PM, Michael S. Tsirkin wrote:On Fri, May 12, 2017 at 01:21:18PM +0200, Maxime Coquelin wrote:On 05/11/2017 07:33 PM, Michael S. Tsirkin wrote:On Thu, May 11, 2017 at 02:32:43PM +0200, Maxime Coquelin wrote:Vhost-kernel backend need to receive IOTLB entries for rings information early, but vhost-user need the same information earlier, before VHOST_USER_SET_VRING_ADDR is sent.Weird. What does VHOST_USER_SET_VRING_ADDR have to do with it? According to Starting and stopping rings in vhost user spec, vhost user does not access anything until ring is started and enabled.This patch also trigger IOTLB miss for all rings informations for robustness, even if in practice these adresses are on the same page.Actually, the DPDK vhost-user backend is compliant with the spec, but when handling VHOST_USER_SET_VRING_ADDR request, it translates the guest addresses into backend VAs, and check they are valid. I will make the commit message clearer about this in next revision. The check could be done later, for example when the ring are started, but it wouldn't change the need to trigger a miss at some point.I think it should be done later, yes. As long as ring is not started addresses should not be interpreted.Ok, then I'll move these addresses translations in the VHOST_USER_SET_VRING_KICK handler.s/VHOST_USER_SET_VRING_KICK/VHOST_USER_SET_VRING_ENABLE/Note that when protocol features are off ring is started in enabled state, but iommu requires protocol features.
OK, I will take care of this. Note that currently in DPDK, the ring is created in enabled state, so it is enabled as soon as started even with protocol features. I have done the patch to fix this, will be posted with the patch that do the ring addresses translations only when starting/enabling the ring. Also, note that disabling VHOST_USER_F_PROTOCOL_FEATURES with latest DPDK and QEMU seems broken. I'll add this to my todo list to understand where the problem is, but this is lower priority.
I just looked at implementing this change, but I'm not convinced this is the right thing to do. On backend side, it means saving temporarily the vhost_vring_addr struct into the vq struct, and moving all what is done currently in SET_VRING_ADDR handler to SET_VRING_ENABLE one.Yes, and this is consistent with what the kernel does.My understanding of the "Starting and stopping rings" chapter of the spec is that the ring must not be processed as long as not started and enabled, not that the addresses passed should not be checked/translated as it is done today both in DPDK and libvhost-user. If the addresses are invalid, isn't it better to know as soon as possible? Cheers, MaximeThere could be valid reasons to set an invalid address temporarily. For example to make sure connection is reset.
Ok. Thanks, Maxime
[Prev in Thread] | Current Thread | [Next in Thread] |