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.ben...@linaro.org> > Cc: Michael S. Tsirkin <m...@redhat.com> > Cc: Dr. David Alan Gilbert <dgilb...@redhat.com> > Cc: Stefan Hajnoczi <stefa...@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