Michael S. Tsirkin <m...@redhat.com> writes:
> On Thu, Jun 04, 2020 at 12:49:17PM +0100, Alex Bennée wrote: >> >> Michael S. Tsirkin <m...@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.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. >> >> The only real point of the enum is to give a little more detailed >> information to the trace point to expose why a section wasn't included. >> In a previous iteration I just had the tracepoint at the bottom before a >> return true where all other legs had returned false. We could switch to >> just having the tracepoint hit for explicit inclusions? > > I didn't notice. Yes, ok more tracepoints IMHO. I can simplify to two: trace_vhost_section(mr->name) trace_vhost_reject_section(mr->name, int reason) Not sure if it's worth defining a enum outside just for the purposes of the trace though. Do we have the concept of per-trace event enum codes? >> > Also why force OK to 0? >> >> Personal preference where 0 indicates success and !0 indicates failure >> of various kinds. Again we can drop if we don't want the information in >> the tracepoint. > > So in that case we need to set all values so people can decode them > from the trace. But I think it's best to just have more trace points > or drop it from the trace. > >> > And I prefer an explicit "else result = NOT_RAM" below >> > instead of initializing it here. >> >> Ok. >> >> > >> >> + >> >> + 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) { >> .. 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. > > vhost does not know how to track writes to specific regions. It can either > track all writes to memory (which slows it down quite a bit) > or no writes. So can vhost interfere with dirty tracking itself in the kernel by trapping the writes? I guess there is no way this can happen with vhost-user? (I wonder what would happen if a vhost-user daemon did an mprotect() on RAM from it's shared view?) > It never actually *needs* to write to VGA, > so we do a hack and just skip these and then if that's the > only thing we need to track then we don't need to enable > its dirty tracking. > > I don't really know what is DIRTY_MEMORY_CODE and when it's set. We use it softmmu do any pages that have code in them always force the slow-path into cputlb for writes to those pages. This allows us to detect self-modifying code. The kernel would never get involved but I don't think vhost and TCG is compatible anyway. I'm only really interested in vhost-user and it's interaction with TCG. I'll spin a v2 now. -- Alex Bennée