| Message ID | 20200908164157.47108-2-liq3ea@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a 'in_mmio' device flag to avoid the DMA to MMIO | expand |
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,
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, >
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
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 >
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 > >
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 > > >
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 >>>>
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
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,
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(+)