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

[Alex Bennée]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> 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))"?

Yeah - that's what I meant... I've left it as the other form in v2
though.

>
>>      .. 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?
>> 


-- 
Alex Bennée