[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: |
Michael S. Tsirkin |
Subject: |
Re: [RFC PATCH] hw/virtio/vhost: re-factor vhost-section and allow DIRTY_MEMORY_CODE |
Date: |
Thu, 4 Jun 2020 07:24:31 -0400 |
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(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index aff98a0ede5..f81fc87e74c 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -27,6 +27,7 @@
> #include "migration/blocker.h"
> #include "migration/qemu-file-types.h"
> #include "sysemu/dma.h"
> +#include "sysemu/tcg.h"
> #include "trace.h"
>
> /* enabled until disconnected backend stabilizes */
> @@ -403,26 +404,43 @@ static int vhost_verify_ring_mappings(struct vhost_dev
> *dev,
> return r;
> }
>
> +/*
> + * vhost_section: identify sections needed for vhost access
> + *
> + * We only care about RAM sections here (where virtqueue can live). If
> + * we find one we still allow the backend to potentially filter it out
> + * of our list.
> + */
> static bool vhost_section(struct vhost_dev *dev, MemoryRegionSection
> *section)
> {
> - bool result;
> - bool log_dirty = memory_region_get_dirty_log_mask(section->mr) &
> - ~(1 << DIRTY_MEMORY_MIGRATION);
> - result = memory_region_is_ram(section->mr) &&
> - !memory_region_is_rom(section->mr);
> -
> - /* Vhost doesn't handle any block which is doing dirty-tracking other
> - * than migration; this typically fires on VGA areas.
> - */
> - result &= !log_dirty;
> + enum { OK = 0, NOT_RAM, DIRTY, FILTERED } result = NOT_RAM;
I'm not sure what does this enum buy us as compared to bool.
Also why force OK to 0?
And I prefer an explicit "else result = NOT_RAM" below
instead of initializing it here.
> +
> + 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.
> + if (dirty_mask & ~handled_dirty) {
> + result = DIRTY;
> + } else if (dev->vhost_ops->vhost_backend_mem_section_filter &&
> + !dev->vhost_ops->vhost_backend_mem_section_filter(dev, section))
> {
> + result = FILTERED;
> + } else {
> + result = OK;
> + }
> }
>
> trace_vhost_section(section->mr->name, result);
> - return result;
> + return result == OK;
> }
>
> static void vhost_begin(MemoryListener *listener)
> --
> 2.20.1
Re: [RFC PATCH] hw/virtio/vhost: re-factor vhost-section and allow DIRTY_MEMORY_CODE, Dr. David Alan Gilbert, 2020/06/04