Re: [Qemu-devel] [PATCH] memory region: check the old.mmio.read status

[Li Qiang]

Peter Maydell <address@hidden> 于2018年9月13日周四 上午8:31写道：

> On 12 September 2018 at 18:43, Laszlo Ersek <address@hidden> wrote:
> > On 09/12/18 14:54, Peter Maydell wrote:
> >> There's patches on-list which drop the old_mmio field from the
> MemoryRegion
> >> struct entirely, so I think this patch as it stands is obsolete.
> >>
> >> Currently our semantics are "you must provide both read and write, even
> >> if one of them just always returns 0 / does nothing / returns an error".
> >
> > That's new to me. Has this always been the case?
>
> Pretty sure it has, yes, because the code assumes that if you can
> get a guest read then your MemoryRegion provides an accessor for it.
> If your guest never actually tries to do a read then of course we'll
> never notice...
>
> > There are several
> > static MemoryRegionOps structures that don't conform. (See the end of my
> > other email:
> > <
> http://mid.mail-archive.com/address@hidden
> >.)
> > Beyond the one that Li Qiang reported directly ("fw_cfg_ctl_mem_read").
> >
> > Are all of those ops guest-triggerable QEMU crashers?
>
> Some of them are special cases like the notdirty-memory one where
> reads always go to host RAM rather than taking the slow path via
> the io accessor. But others are probably guest crashers.
>
> >> We could probably reasonably assert this at the point when the
> >> MemoryRegionOps is registered.
> >
> > Apparently, we should have...
>
> Yeah. Or we could define a default for if there's no read function,
> which I guess should be the same as what we do if
> memory_region_access_valid() fails.

If we want that then the
> simplest thing is for memory_region_access_valid() itself to
> check that at least one of the accessor functions exists and
> return false if none do.


I thinks this proposal makes sense as every memory region write/read will
go to this path and also the device code can make no change.

Thanks,
Li Qiang

(But as I mention above we should get
> all the "old_mmio is going away" patches in first and base the
> change on that, or there'll be a conflict.)
>
>



> thanks
> -- PMM
>