Re: [Qemu-devel] [PATCH 3/6] vhost: Update rings information for IOTLB earlier

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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/

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.

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,
Maxime