Message ID | 20210417103028.601124-1-f4bug@amsat.org |
---|---|
Headers | show |
Series | memory: Forbid mapping AddressSpace root MemoryRegion | expand |
Hello, On 4/17/21 12:30 PM, Philippe Mathieu-Daudé wrote: > Hi, > > This series is the result of a long thread with Peter: > https://www.mail-archive.com/qemu-devel@nongnu.org/msg788366.html > and IRC chats... > > AddressSpace are physical address view and shouldn't be using > non-zero base address. The correct way to map a MR used as AS > root is to use a MR alias. > > Fix the current incorrect uses, then forbid further use. > > Since v1: > - Split the Raven patch in multiple changes, easier to follow/review > (https://www.mail-archive.com/qemu-devel@nongnu.org/msg791116.html) > > Note, the Aspeed patches are already queued in Cédric tree. I had > to cherry-pick them from his tree to have the series pass CI. So I will wait for this patchset to be merged before sending the aspeed queue. Are there any blocking aspects to it ? Thanks, C.
On 4/19/21 9:17 AM, Cédric Le Goater wrote: > On 4/17/21 12:30 PM, Philippe Mathieu-Daudé wrote: >> This series is the result of a long thread with Peter: >> https://www.mail-archive.com/qemu-devel@nongnu.org/msg788366.html >> and IRC chats... >> >> AddressSpace are physical address view and shouldn't be using >> non-zero base address. The correct way to map a MR used as AS >> root is to use a MR alias. >> >> Fix the current incorrect uses, then forbid further use. >> >> Note, the Aspeed patches are already queued in Cédric tree. I had >> to cherry-pick them from his tree to have the series pass CI. > > So I will wait for this patchset to be merged before sending the > aspeed queue. Are there any blocking aspects to it ? I think it is easier for both of us the other way around ;) Your aspeed queue being merged first. Nothing from mine blocks yours, and mine depends of a maintainer willing to take it, which has yet to happen :) Regards, Phil.
On Sat, Apr 17, 2021 at 12:30:17PM +0200, Philippe Mathieu-Daudé wrote: > AddressSpace are physical address view and shouldn't be using > non-zero base address. The correct way to map a MR used as AS > root is to use a MR alias. Today when I rethink this, I figured another way (maybe easier?) to fix the issue. The major problem so far we had is that mr->addr can be anything for a root mr if it's added as subregion of another mr. E.g. in current implementation of mtree_print_mr() MR.addr is constantly used as an offset value: cur_start = base + mr->addr; However afaict mr->addr is defined as "relative offset of this mr to its container", as in memory_region_add_subregion_common(). Say, mr->addr is undefined from that pov if mr->container==NULL, as this MR belongs to nobody. And if it's defined, it's only meaning is in its container's context (or say, address space) only. That said, when we do mtree_print_mr(), another solution could be as simple as, not referencing mr->addr if we _know_ we're working on the root mr, as this is definitely _not_ in the context of the mr's container, even if it has one container after all: ---8<--- diff --git a/softmmu/memory.c b/softmmu/memory.c index d4493ef9e43..d71fb8ecc89 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2940,7 +2940,7 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level, return; } - cur_start = base + mr->addr; + cur_start = base + (level == 1) ? 0 : mr->addr; cur_end = cur_start + MR_SIZE(mr->size); /* ---8<--- Phil, do you think it'll work too to fix the strange offset value dumped in "info mtree"? I don't know (even if it works, perhaps I've missed something) which is better, as current series seems cleaner, then any mr will either belong to a AS or a MR (never both!), but just raise it up. Thanks, -- Peter Xu