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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Alex Bennée (alex.bennee@linaro.org) 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

The problem with VGA is that a VGA page can become mapped and unmapped
under the control of the guest; somewhere in a low address.  This tends
to break hugepage mappings.
For vhost-user, and in particular vhost-user-postcopy this means it
fails the mapping on the vhost-user client.

However the other problem is that with vhost-user, the vhost-user client
is changing memory; and won't mark the pages as dirty - except for
migration (I'm not clear if vhost kernel does this).
So TCG won't notice a page that's been changed by the driver; now in
most cases it's rare for a device to be writing directly into a page
you're going to execute out of, but it's not unknown.

So, as it is, any area that's expecting to get non-migration dirty
notifications is going to be disappointed by a vhost-user backend.

Dave

> 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;
> +
> +    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);
> +        }
> +        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
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK