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,
>