On Fri, 20 Jan 2023 at 14:42, Darren Kenny <[email protected]> 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
