On Fri, 20 Jan 2023 at 14:42, Darren Kenny <darren.ke...@oracle.com> wrote: > Generally, this looks good, but I do have a comment below... > > On Thursday, 2023-01-19 at 02:00:02 -05, Alexander Bulekov wrote: > > Add a flag to the DeviceState, when a device is engaged in PIO/MMIO/DMA. > > This flag is set/checked prior to calling a device's MemoryRegion > > handlers, and set when device code initiates DMA. The purpose of this > > flag is to prevent two types of DMA-based reentrancy issues:
> > diff --git a/softmmu/memory.c b/softmmu/memory.c > > index e05332d07f..90ffaaa4f5 100644 > > --- a/softmmu/memory.c > > +++ b/softmmu/memory.c > > @@ -533,6 +533,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) { > > @@ -542,6 +543,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); > > I don't know how likely this is to happen, but according to: > > - https://qemu-project.gitlab.io/qemu/devel/qom.html#c.object_dynamic_cast > > it is possible for the object_dynamic_cast() function to return NULL, > so it might make sense to wrap the subsequent calls in a test of dev != > NULL. Yes. This came up in a previous version of this: https://lore.kernel.org/qemu-devel/CAFEAcA8E4nDoAWcj-v-dED-0hDtXGjJNSp3A=kdgf8uocw0...@mail.gmail.com/ It's generally a bug to call object_dynamic_cast() and then not check the return value. thanks -- PMM