[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH] hw/virtio/vhost: re-factor vhost-section and allow DIRTY
From: |
Alex Bennée |
Subject: |
Re: [RFC PATCH] hw/virtio/vhost: re-factor vhost-section and allow DIRTY_MEMORY_CODE |
Date: |
Thu, 04 Jun 2020 14:50:14 +0100 |
User-agent: |
mu4e 1.5.1; emacs 28.0.50 |
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
Re: [RFC PATCH] hw/virtio/vhost: re-factor vhost-section and allow DIRTY_MEMORY_CODE, Dr. David Alan Gilbert, 2020/06/04