[RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

[RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

[Li Qiang]

Currently the qemu device fuzzer find some DMA to MMIO issue. If the
device handling MMIO currently trigger a DMA which the address is MMIO,
this will reenter the device MMIO handler. As some of the device doesn't
consider this it will sometimes crash the qemu.

This patch tries to solve this by adding a per-device flag 'in_mmio'.
When the memory core dispatch MMIO it will check/set this flag and when
it leaves it will clean this flag.


Li Qiang (4):
  memory: add memory_region_init_io_with_dev interface
  memory: avoid reenter the device's MMIO handler while processing MMIO
  e1000e: use the new memory_region_init_io_with_dev interface
  hcd-xhci: use the new memory_region_init_io_with_dev interface

 hw/net/e1000e.c        |  8 ++++----
 hw/usb/hcd-xhci.c      | 25 ++++++++++++++---------
 include/exec/memory.h  |  9 +++++++++
 include/hw/qdev-core.h |  1 +
 softmmu/memory.c       | 46 +++++++++++++++++++++++++++++++++++++++---
 5 files changed, 72 insertions(+), 17 deletions(-)

-- 
2.17.1

[RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Li Qiang]

Currently the MR is not explicitly connecting with its device instead of
a opaque. In most situation this opaque is the deivce but it is not an
enforcement. This patch adds a DeviceState member of to MemoryRegion
we will use it in later patch.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 include/exec/memory.h |  9 +++++++++
 softmmu/memory.c      | 15 +++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 0cfe987ab4..620fb12d9b 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -404,6 +404,7 @@ struct MemoryRegion {
     const char *name;
     unsigned ioeventfd_nb;
     MemoryRegionIoeventfd *ioeventfds;
+    DeviceState *dev;
 };
 
 struct IOMMUMemoryRegion {
@@ -794,6 +795,14 @@ void memory_region_init_io(MemoryRegion *mr,
                            const char *name,
                            uint64_t size);
 
+void memory_region_init_io_with_dev(MemoryRegion *mr,
+                           struct Object *owner,
+                           const MemoryRegionOps *ops,
+                           void *opaque,
+                           const char *name,
+                           uint64_t size,
+                           DeviceState *dev);
+
 /**
  * memory_region_init_ram_nomigrate:  Initialize RAM memory region.  Accesses
  *                                    into the region will modify memory
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 70b93104e8..2628c9d2d9 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1490,6 +1490,21 @@ void memory_region_init_io(MemoryRegion *mr,
     mr->terminates = true;
 }
 
+void memory_region_init_io_with_dev(MemoryRegion *mr,
+                           Object *owner,
+                           const MemoryRegionOps *ops,
+                           void *opaque,
+                           const char *name,
+                           uint64_t size,
+                           DeviceState *dev)
+{
+    memory_region_init(mr, owner, name, size);
+    mr->ops = ops ? ops : &unassigned_mem_ops;
+    mr->opaque = opaque;
+    mr->terminates = true;
+    mr->dev = dev;
+}
+
 void memory_region_init_ram_nomigrate(MemoryRegion *mr,
                                       Object *owner,
                                       const char *name,
-- 
2.17.1

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Jason Wang]

On 2020/9/9 上午12:41, Li Qiang wrote:
> Currently the MR is not explicitly connecting with its device instead of
> a opaque. In most situation this opaque is the deivce but it is not an
> enforcement. This patch adds a DeviceState member of to MemoryRegion
> we will use it in later patch.


I don't have a deep investigation. But I wonder whether we could make 
sure of owner instead of adding a new field here.

Thanks


>
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>   include/exec/memory.h |  9 +++++++++
>   softmmu/memory.c      | 15 +++++++++++++++
>   2 files changed, 24 insertions(+)
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 0cfe987ab4..620fb12d9b 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -404,6 +404,7 @@ struct MemoryRegion {
>       const char *name;
>       unsigned ioeventfd_nb;
>       MemoryRegionIoeventfd *ioeventfds;
> +    DeviceState *dev;
>   };
>   
>   struct IOMMUMemoryRegion {
> @@ -794,6 +795,14 @@ void memory_region_init_io(MemoryRegion *mr,
>                              const char *name,
>                              uint64_t size);
>   
> +void memory_region_init_io_with_dev(MemoryRegion *mr,
> +                           struct Object *owner,
> +                           const MemoryRegionOps *ops,
> +                           void *opaque,
> +                           const char *name,
> +                           uint64_t size,
> +                           DeviceState *dev);
> +
>   /**
>    * memory_region_init_ram_nomigrate:  Initialize RAM memory region.  Accesses
>    *                                    into the region will modify memory
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index 70b93104e8..2628c9d2d9 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -1490,6 +1490,21 @@ void memory_region_init_io(MemoryRegion *mr,
>       mr->terminates = true;
>   }
>   
> +void memory_region_init_io_with_dev(MemoryRegion *mr,
> +                           Object *owner,
> +                           const MemoryRegionOps *ops,
> +                           void *opaque,
> +                           const char *name,
> +                           uint64_t size,
> +                           DeviceState *dev)
> +{
> +    memory_region_init(mr, owner, name, size);
> +    mr->ops = ops ? ops : &unassigned_mem_ops;
> +    mr->opaque = opaque;
> +    mr->terminates = true;
> +    mr->dev = dev;
> +}
> +
>   void memory_region_init_ram_nomigrate(MemoryRegion *mr,
>                                         Object *owner,
>                                         const char *name,

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Li Qiang]

Jason Wang <jasowang@redhat.com> 于2020年9月9日周三 上午10:16写道：
>
>
> On 2020/9/9 上午12:41, Li Qiang wrote:
> > Currently the MR is not explicitly connecting with its device instead of
> > a opaque. In most situation this opaque is the deivce but it is not an
> > enforcement. This patch adds a DeviceState member of to MemoryRegion
> > we will use it in later patch.
>
>
> I don't have a deep investigation. But I wonder whether we could make
> sure of owner instead of adding a new field here.

I have did some investigation.

void memory_region_init_io(MemoryRegion *mr,
struct Object *owner,
const MemoryRegionOps *ops,
void *opaque,
const char *name,
uint64_t size);


memory_region_init_io now mostly connects to the device with an opaque member.
But it has no guaranteen that this should be true. So we can't assume this.

The 'owner' is just in the 'object' context.

For the MR itself, MR may have sub-MR and alias. This will complicated
the issue.

As the device emulation and MR has a clear relation. I think add such
field is reasonable.


Thanks,
Li Qiang

>
> Thanks
>
>
> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
> > ---
> >   include/exec/memory.h |  9 +++++++++
> >   softmmu/memory.c      | 15 +++++++++++++++
> >   2 files changed, 24 insertions(+)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 0cfe987ab4..620fb12d9b 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -404,6 +404,7 @@ struct MemoryRegion {
> >       const char *name;
> >       unsigned ioeventfd_nb;
> >       MemoryRegionIoeventfd *ioeventfds;
> > +    DeviceState *dev;
> >   };
> >
> >   struct IOMMUMemoryRegion {
> > @@ -794,6 +795,14 @@ void memory_region_init_io(MemoryRegion *mr,
> >                              const char *name,
> >                              uint64_t size);
> >
> > +void memory_region_init_io_with_dev(MemoryRegion *mr,
> > +                           struct Object *owner,
> > +                           const MemoryRegionOps *ops,
> > +                           void *opaque,
> > +                           const char *name,
> > +                           uint64_t size,
> > +                           DeviceState *dev);
> > +
> >   /**
> >    * memory_region_init_ram_nomigrate:  Initialize RAM memory region.  Accesses
> >    *                                    into the region will modify memory
> > diff --git a/softmmu/memory.c b/softmmu/memory.c
> > index 70b93104e8..2628c9d2d9 100644
> > --- a/softmmu/memory.c
> > +++ b/softmmu/memory.c
> > @@ -1490,6 +1490,21 @@ void memory_region_init_io(MemoryRegion *mr,
> >       mr->terminates = true;
> >   }
> >
> > +void memory_region_init_io_with_dev(MemoryRegion *mr,
> > +                           Object *owner,
> > +                           const MemoryRegionOps *ops,
> > +                           void *opaque,
> > +                           const char *name,
> > +                           uint64_t size,
> > +                           DeviceState *dev)
> > +{
> > +    memory_region_init(mr, owner, name, size);
> > +    mr->ops = ops ? ops : &unassigned_mem_ops;
> > +    mr->opaque = opaque;
> > +    mr->terminates = true;
> > +    mr->dev = dev;
> > +}
> > +
> >   void memory_region_init_ram_nomigrate(MemoryRegion *mr,
> >                                         Object *owner,
> >                                         const char *name,
>

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Gerd Hoffmann]

On Wed, Sep 09, 2020 at 10:15:47AM +0800, Jason Wang wrote:
> 
> On 2020/9/9 上午12:41, Li Qiang wrote:
> > Currently the MR is not explicitly connecting with its device instead of
> > a opaque. In most situation this opaque is the deivce but it is not an
> > enforcement. This patch adds a DeviceState member of to MemoryRegion
> > we will use it in later patch.
> 
> 
> I don't have a deep investigation. But I wonder whether we could make sure
> of owner instead of adding a new field here.

Should be possible.  There is object_dynamic_cast() which can be used to
figure whenever a given owner object is a device.

take care,
  Gerd

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Li Qiang]

Gerd Hoffmann <kraxel@redhat.com> 于2020年9月9日周三 下午12:49写道：
>
> On Wed, Sep 09, 2020 at 10:15:47AM +0800, Jason Wang wrote:
> >
> > On 2020/9/9 上午12:41, Li Qiang wrote:
> > > Currently the MR is not explicitly connecting with its device instead of
> > > a opaque. In most situation this opaque is the deivce but it is not an
> > > enforcement. This patch adds a DeviceState member of to MemoryRegion
> > > we will use it in later patch.
> >
> >
> > I don't have a deep investigation. But I wonder whether we could make sure
> > of owner instead of adding a new field here.
>
> Should be possible.  There is object_dynamic_cast() which can be used to
> figure whenever a given owner object is a device.
>

I found most caller of 'memory_region_init_io' will set the owner to
the device object.
But some of them will just set it to NULL. Do will have a clear rule
that the device's MR
'owner' should be the device object? If yes, we can use this field.

Thanks,
Li Qiang

> take care,
>   Gerd
>

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Alexander Bulekov]

On 200909 1258, Li Qiang wrote:
> Gerd Hoffmann <kraxel@redhat.com> 于2020年9月9日周三 下午12:49写道：
> >
> > On Wed, Sep 09, 2020 at 10:15:47AM +0800, Jason Wang wrote:
> > >
> > > On 2020/9/9 上午12:41, Li Qiang wrote:
> > > > Currently the MR is not explicitly connecting with its device instead of
> > > > a opaque. In most situation this opaque is the deivce but it is not an
> > > > enforcement. This patch adds a DeviceState member of to MemoryRegion
> > > > we will use it in later patch.
> > >
> > >
> > > I don't have a deep investigation. But I wonder whether we could make sure
> > > of owner instead of adding a new field here.
> >
> > Should be possible.  There is object_dynamic_cast() which can be used to
> > figure whenever a given owner object is a device.
> >
> 
> I found most caller of 'memory_region_init_io' will set the owner to
> the device object.
> But some of them will just set it to NULL. Do will have a clear rule
> that the device's MR
> 'owner' should be the device object? If yes, we can use this field.
> 

Those seem to be devices that havent't been QOM-imfied yet?  Maybe those
devices are unlikely to be affected by these issues, though... 

For i386, it seems like parallel, port80, portF0, pckbd, and xen_pvdevice .. ?
I'm guessing none of these do DMA.

+CC Stefan, since he replied to the other thread.

> Thanks,
> Li Qiang
> 
> > take care,
> >   Gerd
> >

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Li Qiang]

Alexander Bulekov <alxndr@bu.edu> 于2020年9月9日周三 下午10:28写道：
>
> On 200909 1258, Li Qiang wrote:
> > Gerd Hoffmann <kraxel@redhat.com> 于2020年9月9日周三 下午12:49写道：
> > >
> > > On Wed, Sep 09, 2020 at 10:15:47AM +0800, Jason Wang wrote:
> > > >
> > > > On 2020/9/9 上午12:41, Li Qiang wrote:
> > > > > Currently the MR is not explicitly connecting with its device instead of
> > > > > a opaque. In most situation this opaque is the deivce but it is not an
> > > > > enforcement. This patch adds a DeviceState member of to MemoryRegion
> > > > > we will use it in later patch.
> > > >
> > > >
> > > > I don't have a deep investigation. But I wonder whether we could make sure
> > > > of owner instead of adding a new field here.
> > >
> > > Should be possible.  There is object_dynamic_cast() which can be used to
> > > figure whenever a given owner object is a device.
> > >
> >
> > I found most caller of 'memory_region_init_io' will set the owner to
> > the device object.
> > But some of them will just set it to NULL. Do will have a clear rule
> > that the device's MR
> > 'owner' should be the device object? If yes, we can use this field.
> >
>
> Those seem to be devices that havent't been QOM-imfied yet?  Maybe those
> devices are unlikely to be affected by these issues, though...
>

No it seems not related QOM-ified.

> For i386, it seems like parallel, port80, portF0, pckbd, and xen_pvdevice .. ?
> I'm guessing none of these do DMA.
>

In fact xen_pvdevice is MMIO but the handlers does nothing.

There are some other example than i386 such as the riscv in
hw/riscv/sifive_uart.c

If we have a rule to force the 'MR's owner to the device then we can
fix these NULL owner MR.

Thanks,
Li Qiang

> +CC Stefan, since he replied to the other thread.
>
> > Thanks,
> > Li Qiang
> >
> > > take care,
> > >   Gerd
> > >

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Jason Wang]

On 2020/9/10 下午10:37, Li Qiang wrote:
> Alexander Bulekov <alxndr@bu.edu> 于2020年9月9日周三 下午10:28写道：
>> On 200909 1258, Li Qiang wrote:
>>> Gerd Hoffmann <kraxel@redhat.com> 于2020年9月9日周三 下午12:49写道：
>>>> On Wed, Sep 09, 2020 at 10:15:47AM +0800, Jason Wang wrote:
>>>>> On 2020/9/9 上午12:41, Li Qiang wrote:
>>>>>> Currently the MR is not explicitly connecting with its device instead of
>>>>>> a opaque. In most situation this opaque is the deivce but it is not an
>>>>>> enforcement. This patch adds a DeviceState member of to MemoryRegion
>>>>>> we will use it in later patch.
>>>>>
>>>>> I don't have a deep investigation. But I wonder whether we could make sure
>>>>> of owner instead of adding a new field here.
>>>> Should be possible.  There is object_dynamic_cast() which can be used to
>>>> figure whenever a given owner object is a device.
>>>>
>>> I found most caller of 'memory_region_init_io' will set the owner to
>>> the device object.
>>> But some of them will just set it to NULL. Do will have a clear rule
>>> that the device's MR
>>> 'owner' should be the device object? If yes, we can use this field.
>>>
>> Those seem to be devices that havent't been QOM-imfied yet?  Maybe those
>> devices are unlikely to be affected by these issues, though...
>>
> No it seems not related QOM-ified.
>
>> For i386, it seems like parallel, port80, portF0, pckbd, and xen_pvdevice .. ?
>> I'm guessing none of these do DMA.
>>
> In fact xen_pvdevice is MMIO but the handlers does nothing.
>
> There are some other example than i386 such as the riscv in
> hw/riscv/sifive_uart.c
>
> If we have a rule to force the 'MR's owner to the device then we can
> fix these NULL owner MR.
>
> Thanks,
> Li Qiang


I guess maybe we can start from the ones whose owner is a device and 
convert the rest on top (if necessary)?

Thanks


>
>> +CC Stefan, since he replied to the other thread.
>>
>>> Thanks,
>>> Li Qiang
>>>
>>>> take care,
>>>>    Gerd
>>>>

Re: [RFC 1/4] memory: add memory_region_init_io_with_dev interface

[Paolo Bonzini]

On 08/09/20 18:41, Li Qiang wrote:
> Currently the MR is not explicitly connecting with its device instead of
> a opaque. In most situation this opaque is the deivce but it is not an
> enforcement. This patch adds a DeviceState member of to MemoryRegion
> we will use it in later patch.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  include/exec/memory.h |  9 +++++++++
>  softmmu/memory.c      | 15 +++++++++++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 0cfe987ab4..620fb12d9b 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -404,6 +404,7 @@ struct MemoryRegion {
>      const char *name;
>      unsigned ioeventfd_nb;
>      MemoryRegionIoeventfd *ioeventfds;
> +    DeviceState *dev;

There is already an owner field for this.

Paolo

[RFC 2/4] memory: avoid reenter the device's MMIO handler while processing MMIO

[Li Qiang]

This patch adds a 'in_mmio' flag to 'DeviceState' to indicate that the
device is doing MMIO path. This can avoid the malicious guest do
DMA to MMIO and crash the qemu.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 include/hw/qdev-core.h |  1 +
 softmmu/memory.c       | 31 ++++++++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/include/hw/qdev-core.h b/include/hw/qdev-core.h
index ea3f73a282..c6f4ebba9e 100644
--- a/include/hw/qdev-core.h
+++ b/include/hw/qdev-core.h
@@ -177,6 +177,7 @@ struct DeviceState {
     char *canonical_path;
     bool realized;
     bool pending_deleted_event;
+    bool in_mmio;
     QemuOpts *opts;
     int hotplugged;
     bool allow_unplug_during_migration;
diff --git a/softmmu/memory.c b/softmmu/memory.c
index 2628c9d2d9..7be44f7175 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -1410,8 +1410,20 @@ MemTxResult memory_region_dispatch_read(MemoryRegion *mr,
         return MEMTX_DECODE_ERROR;
     }
 
+    if (mr->dev) {
+        if (mr->dev->in_mmio) {
+            return MEMTX_ERROR;
+        } else {
+            mr->dev->in_mmio = true;
+        }
+    }
+
     r = memory_region_dispatch_read1(mr, addr, pval, size, attrs);
     adjust_endianness(mr, pval, op);
+
+    if (mr->dev) {
+        mr->dev->in_mmio = false;
+    }
     return r;
 }
 
@@ -1448,6 +1460,7 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
                                          MemTxAttrs attrs)
 {
     unsigned size = memop_size(op);
+    MemTxResult ret;
 
     if (!memory_region_access_valid(mr, addr, size, true, attrs)) {
         unassigned_mem_write(mr, addr, data, size);
@@ -1461,20 +1474,32 @@ MemTxResult memory_region_dispatch_write(MemoryRegion *mr,
         return MEMTX_OK;
     }
 
+    if (mr->dev) {
+        if (mr->dev->in_mmio) {
+            return MEMTX_ERROR;
+        } else {
+            mr->dev->in_mmio = true;
+        }
+    }
+
     if (mr->ops->write) {
-        return access_with_adjusted_size(addr, &data, size,
+        ret = access_with_adjusted_size(addr, &data, size,
                                          mr->ops->impl.min_access_size,
                                          mr->ops->impl.max_access_size,
                                          memory_region_write_accessor, mr,
                                          attrs);
     } else {
-        return
-            access_with_adjusted_size(addr, &data, size,
+        ret = access_with_adjusted_size(addr, &data, size,
                                       mr->ops->impl.min_access_size,
                                       mr->ops->impl.max_access_size,
                                       memory_region_write_with_attrs_accessor,
                                       mr, attrs);
     }
+    if (mr->dev) {
+        mr->dev->in_mmio = false;
+    }
+
+    return ret;
 }
 
 void memory_region_init_io(MemoryRegion *mr,
-- 
2.17.1

[RFC 3/4] e1000e: use the new memory_region_init_io_with_dev interface

[Li Qiang]

This can avoid the DMA to MMIO issue here:

https://bugs.launchpad.net/qemu/+bug/1886362
Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/net/e1000e.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
index fda34518c9..0aac5cea2e 100644
--- a/hw/net/e1000e.c
+++ b/hw/net/e1000e.c
@@ -439,8 +439,8 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
     s->subsys_used = s->subsys;
 
     /* Define IO/MMIO regions */
-    memory_region_init_io(&s->mmio, OBJECT(s), &mmio_ops, s,
-                          "e1000e-mmio", E1000E_MMIO_SIZE);
+    memory_region_init_io_with_dev(&s->mmio, OBJECT(s), &mmio_ops, s,
+                          "e1000e-mmio", E1000E_MMIO_SIZE, &pci_dev->qdev);
     pci_register_bar(pci_dev, E1000E_MMIO_IDX,
                      PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mmio);
 
@@ -453,8 +453,8 @@ static void e1000e_pci_realize(PCIDevice *pci_dev, Error **errp)
     pci_register_bar(pci_dev, E1000E_FLASH_IDX,
                      PCI_BASE_ADDRESS_SPACE_MEMORY, &s->flash);
 
-    memory_region_init_io(&s->io, OBJECT(s), &io_ops, s,
-                          "e1000e-io", E1000E_IO_SIZE);
+    memory_region_init_io_with_dev(&s->io, OBJECT(s), &io_ops, s,
+                          "e1000e-io", E1000E_IO_SIZE, &pci_dev->qdev);
     pci_register_bar(pci_dev, E1000E_IO_IDX,
                      PCI_BASE_ADDRESS_SPACE_IO, &s->io);
 
-- 
2.17.1

[RFC 4/4] hcd-xhci: use the new memory_region_init_io_with_dev interface

[Li Qiang]

This can avoid the DMA to MMIO issue here:

https://bugs.launchpad.net/qemu/+bug/1891354
Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/usb/hcd-xhci.c | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 46a2186d91..1954ae2ae7 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -3437,14 +3437,18 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
     xhci->mfwrap_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, xhci_mfwrap_timer, xhci);
 
     memory_region_init(&xhci->mem, OBJECT(xhci), "xhci", LEN_REGS);
-    memory_region_init_io(&xhci->mem_cap, OBJECT(xhci), &xhci_cap_ops, xhci,
-                          "capabilities", LEN_CAP);
-    memory_region_init_io(&xhci->mem_oper, OBJECT(xhci), &xhci_oper_ops, xhci,
-                          "operational", 0x400);
-    memory_region_init_io(&xhci->mem_runtime, OBJECT(xhci), &xhci_runtime_ops, xhci,
-                          "runtime", LEN_RUNTIME);
-    memory_region_init_io(&xhci->mem_doorbell, OBJECT(xhci), &xhci_doorbell_ops, xhci,
-                          "doorbell", LEN_DOORBELL);
+    memory_region_init_io_with_dev(&xhci->mem_cap, OBJECT(xhci),
+                                   &xhci_cap_ops, xhci,
+                                   "capabilities", LEN_CAP, &dev->qdev);
+    memory_region_init_io_with_dev(&xhci->mem_oper, OBJECT(xhci),
+                                   &xhci_oper_ops, xhci,
+                                   "operational", 0x400, &dev->qdev);
+    memory_region_init_io_with_dev(&xhci->mem_runtime, OBJECT(xhci),
+                                   &xhci_runtime_ops, xhci,
+                                   "runtime", LEN_RUNTIME, &dev->qdev);
+    memory_region_init_io_with_dev(&xhci->mem_doorbell, OBJECT(xhci),
+                                   &xhci_doorbell_ops, xhci,
+                                   "doorbell", LEN_DOORBELL, &dev->qdev);
 
     memory_region_add_subregion(&xhci->mem, 0,            &xhci->mem_cap);
     memory_region_add_subregion(&xhci->mem, OFF_OPER,     &xhci->mem_oper);
@@ -3455,8 +3459,9 @@ static void usb_xhci_realize(struct PCIDevice *dev, Error **errp)
         XHCIPort *port = &xhci->ports[i];
         uint32_t offset = OFF_OPER + 0x400 + 0x10 * i;
         port->xhci = xhci;
-        memory_region_init_io(&port->mem, OBJECT(xhci), &xhci_port_ops, port,
-                              port->name, 0x10);
+        memory_region_init_io_with_dev(&port->mem, OBJECT(xhci),
+                                       &xhci_port_ops, port,
+                                       port->name, 0x10, &dev->qdev);
         memory_region_add_subregion(&xhci->mem, offset, &port->mem);
     }
 
-- 
2.17.1

Re: [RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

[Jason Wang]

On 2020/9/9 上午12:41, Li Qiang wrote:
> Currently the qemu device fuzzer find some DMA to MMIO issue. If the
> device handling MMIO currently trigger a DMA which the address is MMIO,
> this will reenter the device MMIO handler. As some of the device doesn't
> consider this it will sometimes crash the qemu.
>
> This patch tries to solve this by adding a per-device flag 'in_mmio'.
> When the memory core dispatch MMIO it will check/set this flag and when
> it leaves it will clean this flag.


What's the plan for fixing the irq issues pointed out by Peter?

Thanks


>
>
> Li Qiang (4):
>    memory: add memory_region_init_io_with_dev interface
>    memory: avoid reenter the device's MMIO handler while processing MMIO
>    e1000e: use the new memory_region_init_io_with_dev interface
>    hcd-xhci: use the new memory_region_init_io_with_dev interface
>
>   hw/net/e1000e.c        |  8 ++++----
>   hw/usb/hcd-xhci.c      | 25 ++++++++++++++---------
>   include/exec/memory.h  |  9 +++++++++
>   include/hw/qdev-core.h |  1 +
>   softmmu/memory.c       | 46 +++++++++++++++++++++++++++++++++++++++---
>   5 files changed, 72 insertions(+), 17 deletions(-)
>

Re: [RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

[Li Qiang]

Jason Wang <jasowang@redhat.com> 于2020年9月9日周三 上午10:17写道：
>
>
> On 2020/9/9 上午12:41, Li Qiang wrote:
> > Currently the qemu device fuzzer find some DMA to MMIO issue. If the
> > device handling MMIO currently trigger a DMA which the address is MMIO,
> > this will reenter the device MMIO handler. As some of the device doesn't
> > consider this it will sometimes crash the qemu.
> >
> > This patch tries to solve this by adding a per-device flag 'in_mmio'.
> > When the memory core dispatch MMIO it will check/set this flag and when
> > it leaves it will clean this flag.
>
>
> What's the plan for fixing the irq issues pointed out by Peter?
>

Just have a basic idea. Just like this we can add a per-device flag,
'in_mmio' or 'in_emulation'
or some other names. The device need solve the irq handler/mmio and
anything other reenter issue by themself
or they can just check/set/clean this flag. This way we may can define
a principle which Peter mentioned that the device emulation should
obey.



Thanks,
Li Qiang


> Thanks
>
>
> >
> >
> > Li Qiang (4):
> >    memory: add memory_region_init_io_with_dev interface
> >    memory: avoid reenter the device's MMIO handler while processing MMIO
> >    e1000e: use the new memory_region_init_io_with_dev interface
> >    hcd-xhci: use the new memory_region_init_io_with_dev interface
> >
> >   hw/net/e1000e.c        |  8 ++++----
> >   hw/usb/hcd-xhci.c      | 25 ++++++++++++++---------
> >   include/exec/memory.h  |  9 +++++++++
> >   include/hw/qdev-core.h |  1 +
> >   softmmu/memory.c       | 46 +++++++++++++++++++++++++++++++++++++++---
> >   5 files changed, 72 insertions(+), 17 deletions(-)
> >
>

Re: [RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

[Paolo Bonzini]

On 08/09/20 18:41, Li Qiang wrote:
> Currently the qemu device fuzzer find some DMA to MMIO issue. If the
> device handling MMIO currently trigger a DMA which the address is MMIO,
> this will reenter the device MMIO handler. As some of the device doesn't
> consider this it will sometimes crash the qemu.
> 
> This patch tries to solve this by adding a per-device flag 'in_mmio'.
> When the memory core dispatch MMIO it will check/set this flag and when
> it leaves it will clean this flag.
> 
> 
> Li Qiang (4):
>   memory: add memory_region_init_io_with_dev interface
>   memory: avoid reenter the device's MMIO handler while processing MMIO
>   e1000e: use the new memory_region_init_io_with_dev interface
>   hcd-xhci: use the new memory_region_init_io_with_dev interface
> 
>  hw/net/e1000e.c        |  8 ++++----
>  hw/usb/hcd-xhci.c      | 25 ++++++++++++++---------
>  include/exec/memory.h  |  9 +++++++++
>  include/hw/qdev-core.h |  1 +
>  softmmu/memory.c       | 46 +++++++++++++++++++++++++++++++++++++++---
>  5 files changed, 72 insertions(+), 17 deletions(-)
> 

I don't think this is a good solution.  These are device bugs and they
need to be fixed.

Paolo

Re: [RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

[Peter Maydell]

On Sun, 20 Sep 2020 at 08:56, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 08/09/20 18:41, Li Qiang wrote:
> > Currently the qemu device fuzzer find some DMA to MMIO issue. If the
> > device handling MMIO currently trigger a DMA which the address is MMIO,
> > this will reenter the device MMIO handler. As some of the device doesn't
> > consider this it will sometimes crash the qemu.

> I don't think this is a good solution.  These are device bugs and they
> need to be fixed.

Do you have an opinion on what the right approach to fixing them is?
It seems like a hard problem to me; my brain has been too full of
cotton wool recently and I haven't felt up to sitting down and
trying to think through whether there's a clean way to handle the
reentrancy-into-device-code problem in the general case...

thanks
-- PMM

Re: [RFC 0/4] Add a 'in_mmio' device flag to avoid the DMA to MMIO

[Li Qiang]

Paolo Bonzini <pbonzini@redhat.com> 于2020年9月20日周日 下午3:56写道：
>
> On 08/09/20 18:41, Li Qiang wrote:
> > Currently the qemu device fuzzer find some DMA to MMIO issue. If the
> > device handling MMIO currently trigger a DMA which the address is MMIO,
> > this will reenter the device MMIO handler. As some of the device doesn't
> > consider this it will sometimes crash the qemu.
> >
> > This patch tries to solve this by adding a per-device flag 'in_mmio'.
> > When the memory core dispatch MMIO it will check/set this flag and when
> > it leaves it will clean this flag.
> >
> >
> > Li Qiang (4):
> >   memory: add memory_region_init_io_with_dev interface
> >   memory: avoid reenter the device's MMIO handler while processing MMIO
> >   e1000e: use the new memory_region_init_io_with_dev interface
> >   hcd-xhci: use the new memory_region_init_io_with_dev interface
> >
> >  hw/net/e1000e.c        |  8 ++++----
> >  hw/usb/hcd-xhci.c      | 25 ++++++++++++++---------
> >  include/exec/memory.h  |  9 +++++++++
> >  include/hw/qdev-core.h |  1 +
> >  softmmu/memory.c       | 46 +++++++++++++++++++++++++++++++++++++++---
> >  5 files changed, 72 insertions(+), 17 deletions(-)
> >
>
> I don't think this is a good solution.  These are device bugs and they
> need to be fixed.

I agree with this the device should finally handle their cases. But we
can do something in a pattern if the device hasn't
do that.
I have posted a patchset:
-->https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg00906.html

This is add flags in the Device State. And check/record the flag when
doing reentrancy code.
Once the device has fixed the reentrancy issue, they can remove this flag.

Thanks,
Li QIang

>
> Paolo
>