[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Mark Cave-Ayland
Subject: Re: [Qemu-devel] [PATCH] memory region: check the old.mmio.read status
Date: Thu, 13 Sep 2018 05:31:14 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

On 13/09/18 01:31, Peter Maydell wrote:

> 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. (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.)

This sounds familiar to me. I remember whilst working on the Mac
uninorth patches I couldn't quite figure out why a simple change to the
PCI bridge IO address space started to cause some accesses to fail: it
was because the guest was issuing a periodic read to an address without
a MemoryRegion which was now failing with MEMTX_ERROR rather than the
returning 0 which was the previous behaviour.

The fix was ultimately to add an unassigned_io_ops to the parent
MemoryRegion which I found out was being sneakily added by the old code,
although it certainly had me scratching my head for a while...



reply via email to

[Prev in Thread] Current Thread [Next in Thread]