On 12/17/21 04:08, Alexander Bulekov wrote: > Here's my shot at fixing dma-reentracy issues. This patch adds a flag to > the DeviceState, which is set/checked when we call an accessor > associated with the device's IO MRs.
Your approach is exactly what Gerd suggested: https://www.mail-archive.com/[email protected]/msg831437.html > The problem, in short, as I understand it: For the vast majority of > cases, we want to prevent a device from accessing it's own PIO/MMIO > regions over DMA. > > This patch/solution is based on some assumptions: > 1. DMA accesses that hit mmio regions are only dangerous if they end up > interacting with memory-regions belonging to the device initiating the > DMA. > Not dangerous: sdhci_pio->dma_write->e1000_mmio > Dangerous: sdhci_pio->dma_write->sdhci_mmio It doesn't have to be dangerous, see Paolo's example which invalidated my previous attempt and forced me to write 24 patches in multiples series to keep the "niche" cases working: https://www.mail-archive.com/[email protected]/msg72939.html > > 2. Most devices do not interact with their own PIO/MMIO memory-regions > using DMA. > > 3. There is no way for there to be multiple simultaneous accesses to a > device's PIO/MMIO memory-regions. > > 4. All devices are QOMified :-) > > With this patch, I wasn't able to reproduce the issues being tracked > here, with QTest reproducers: > https://gitlab.com/qemu-project/qemu/-/issues/556 > > This passes the i386 qos/qtests for me and I was able to boot some > linux/windows > VMs with basic devices configured, without any apparent problems. > > Cc: Philippe Mathieu-Daudé <[email protected]> > Cc: Mauro Matteo Cascella <[email protected]> > Cc: Qiuhao Li <[email protected]> > Cc: Peter Xu <[email protected]> > Cc: Jason Wang <[email protected]> > Cc: David Hildenbrand <[email protected]> > Cc: Gerd Hoffmann <[email protected]> > Cc: Peter Maydell <[email protected]> > Cc: Li Qiang <[email protected]> > Cc: Thomas Huth <[email protected]> > Cc: Laurent Vivier <[email protected]> > Cc: Bandan Das <[email protected]> > Cc: Edgar E. Iglesias <[email protected]> > Cc: Darren Kenny <[email protected]> > Cc: Bin Meng <[email protected]> > Cc: Paolo Bonzini <[email protected]> > Cc: Stefan Hajnoczi <[email protected]> > > Signed-off-by: Alexander Bulekov <[email protected]> > --- > include/hw/qdev-core.h | 1 + > softmmu/memory.c | 15 +++++++++++++++ > softmmu/trace-events | 1 + > 3 files changed, 17 insertions(+) > > diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h > index 20d3066595..32f7c779ab 100644 > --- a/include/hw/qdev-core.h > +++ b/include/hw/qdev-core.h > @@ -193,6 +193,7 @@ struct DeviceState { > int instance_id_alias; > int alias_required_for_version; > ResettableState reset; > + int engaged_in_direct_io; > }; > > struct DeviceListener { > diff --git a/softmmu/memory.c b/softmmu/memory.c > index 7340e19ff5..255c3c602f 100644 > --- a/softmmu/memory.c > +++ b/softmmu/memory.c > @@ -532,6 +532,7 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > uint64_t access_mask; > unsigned access_size; > unsigned i; > + DeviceState *dev = NULL; > MemTxResult r = MEMTX_OK; > > if (!access_size_min) { > @@ -541,6 +542,17 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > access_size_max = 4; > } > > + /* Do not allow more than one simultanous access to a device's IO > Regions */ > + if (mr->owner && > + !mr->ram_device && !mr->ram && !mr->rom_device && !mr->readonly) { > + dev = (DeviceState *) object_dynamic_cast(mr->owner, TYPE_DEVICE); > + if (dev->engaged_in_direct_io) { > + trace_memory_region_reentrant_io(get_cpu_index(), mr, addr, > size); > + return MEMTX_ERROR; > + } > + dev->engaged_in_direct_io = true; > + } > + > /* FIXME: support unaligned access? */ > access_size = MAX(MIN(size, access_size_max), access_size_min); > access_mask = MAKE_64BIT_MASK(0, access_size * 8); > @@ -555,6 +567,9 @@ static MemTxResult access_with_adjusted_size(hwaddr addr, > access_mask, attrs); > } > } > + if (dev) { > + dev->engaged_in_direct_io = false; > + } > return r; > } > > diff --git a/softmmu/trace-events b/softmmu/trace-events > index 9c88887b3c..d7228316db 100644 > --- a/softmmu/trace-events > +++ b/softmmu/trace-events > @@ -13,6 +13,7 @@ memory_region_ops_read(int cpu_index, void *mr, uint64_t > addr, uint64_t value, u > memory_region_ops_write(int cpu_index, void *mr, uint64_t addr, uint64_t > value, unsigned size, const char *name) "cpu %d mr %p addr 0x%"PRIx64" value > 0x%"PRIx64" size %u name '%s'" > memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, > uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value > 0x%"PRIx64" size %u" > memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, > uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value > 0x%"PRIx64" size %u" > +memory_region_reentrant_io(int cpu_index, void *mr, uint64_t offset, > unsigned size) "cpu %d mr %p offset 0x%"PRIx64" size %u" > memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, > uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value > 0x%"PRIx64" size %u" > memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, > uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value > 0x%"PRIx64" size %u" > memory_region_sync_dirty(const char *mr, const char *listener, int global) > "mr '%s' listener '%s' synced (global=%d)" >
