Re: [RFC PATCH] hw/virtio/vhost: re-factor vhost-section and allow DIRTY_MEMORY_CODE

[Philippe Mathieu-Daudé]

On 6/4/20 1:49 PM, Alex Bennée wrote:
> 
> Michael S. Tsirkin <mst@redhat.com> writes:
> 
>> On Thu, Jun 04, 2020 at 12:13:23PM +0100, Alex Bennée wrote:
>>> The purpose of vhost_section is to identify RAM regions that need to
>>> be made available to a vhost client. However when running under TCG
>>> all RAM sections have DIRTY_MEMORY_CODE set which leads to problems
>>> down the line. The original comment implies VGA regions are a problem
>>> but doesn't explain why vhost has a problem with it.
>>>
>>> Re-factor the code so:
>>>
>>>   - steps are clearer to follow
>>>   - reason for rejection is recorded in the trace point
>>>   - we allow DIRTY_MEMORY_CODE when TCG is enabled
>>>
>>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> Cc: Stefan Hajnoczi <stefanha@redhat.com>
>>> ---
>>>  hw/virtio/vhost.c | 46 ++++++++++++++++++++++++++++++++--------------
>>>  1 file changed, 32 insertions(+), 14 deletions(-)
[...]
>>> +
>>> +    if (memory_region_is_ram(section->mr) && 
>>> !memory_region_is_rom(section->mr)) {
>>> +        uint8_t dirty_mask = memory_region_get_dirty_log_mask(section->mr);
>>> +        uint8_t handled_dirty;
>>>  
>>> -    if (result && dev->vhost_ops->vhost_backend_mem_section_filter) {
>>> -        result &=
>>> -            dev->vhost_ops->vhost_backend_mem_section_filter(dev, section);
>>> +        /*
>>> +         * Vhost doesn't handle any block which is doing dirty-tracking 
>>> other
>>> +         * than migration; this typically fires on VGA areas. However
>>> +         * for TCG we also do dirty code page tracking which shouldn't
>>> +         * get in the way.
>>> +         */
>>> +        handled_dirty = (1 << DIRTY_MEMORY_MIGRATION);
>>> +        if (tcg_enabled()) {
>>> +            handled_dirty |= (1 << DIRTY_MEMORY_CODE);
>>> +        }
>>
>> So DIRTY_MEMORY_CODE is only set by TCG right? Thus I'm guessing
>> we can just allow this unconditionally.
> 
> Which actually makes the test:
> 
>   if (dirty_mask & DIRTY_MEMORY_VGA) {

Eh? Shouldn't this be "if (dirty_mask & (1 << DIRTY_MEMORY_VGA))"?

>      .. fail ..
>   }
> 
> which is more in line with the comment although wouldn't fail if we
> added additional DIRTY_MEMORY flags. This leads to the question what
> exactly is it about DIRTY tracking that vhost doesn't like. Is it really
> only avoiding having virtqueue in video RAM? Does this ever actually
> happen?
> 
> I assume boards with unified memory models where video ram is shared
> with system ram just end up partitioning the memory regions?
>