* [PATCH] memory: Do not allow subregion out of the parent region range @ 2019-12-14 16:02 Philippe Mathieu-Daudé 2019-12-16 13:08 ` Paolo Bonzini 0 siblings, 1 reply; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-14 16:02 UTC (permalink / raw) To: qemu-devel; +Cc: Paolo Bonzini, Philippe Mathieu-Daudé, Peter Maydell If a subregion is mapped out of the parent region range, it will never get accessed. Since this is a bug, abort to help the developer notice the mistake. Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- memory.c | 1 + 1 file changed, 1 insertion(+) diff --git a/memory.c b/memory.c index 06484c2bff..61f355dcd5 100644 --- a/memory.c +++ b/memory.c @@ -2390,6 +2390,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr, { assert(!subregion->container); subregion->container = mr; + assert(offset + memory_region_size(subregion) <= memory_region_size(mr)); subregion->addr = offset; memory_region_update_container_subregions(subregion); } -- 2.21.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-14 16:02 [PATCH] memory: Do not allow subregion out of the parent region range Philippe Mathieu-Daudé @ 2019-12-16 13:08 ` Paolo Bonzini 2019-12-16 17:46 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2019-12-16 13:08 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel; +Cc: Peter Maydell On 14/12/19 17:02, Philippe Mathieu-Daudé wrote: > If a subregion is mapped out of the parent region range, it > will never get accessed. Since this is a bug, abort to help > the developer notice the mistake. > > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > memory.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/memory.c b/memory.c > index 06484c2bff..61f355dcd5 100644 > --- a/memory.c > +++ b/memory.c > @@ -2390,6 +2390,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr, > { > assert(!subregion->container); > subregion->container = mr; > + assert(offset + memory_region_size(subregion) <= memory_region_size(mr)); > subregion->addr = offset; > memory_region_update_container_subregions(subregion); > } > I think in some cases this could be intentional, for example if you have different models with different BAR sizes and you organize this with the same tree of MemoryRegion and different sizes for the parent. I'm not saying this happens in the current devices we support, I'm just wondering if it should be a reason not to apply the patch. I suppose you did spend some time debugging something where the patch would have been useful; what was that something? Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-16 13:08 ` Paolo Bonzini @ 2019-12-16 17:46 ` Philippe Mathieu-Daudé 2019-12-17 10:51 ` Paolo Bonzini 0 siblings, 1 reply; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-16 17:46 UTC (permalink / raw) To: Paolo Bonzini, qemu-devel, Peter Maydell, Alexey Kardashevskiy, Michael S. Tsirkin, Alex Williamson On 12/16/19 2:08 PM, Paolo Bonzini wrote: > On 14/12/19 17:02, Philippe Mathieu-Daudé wrote: >> If a subregion is mapped out of the parent region range, it >> will never get accessed. Since this is a bug, abort to help >> the developer notice the mistake. >> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> --- >> memory.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/memory.c b/memory.c >> index 06484c2bff..61f355dcd5 100644 >> --- a/memory.c >> +++ b/memory.c >> @@ -2390,6 +2390,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr, >> { >> assert(!subregion->container); >> subregion->container = mr; >> + assert(offset + memory_region_size(subregion) <= memory_region_size(mr)); >> subregion->addr = offset; >> memory_region_update_container_subregions(subregion); >> } >> > > I think in some cases this could be intentional, for example if you have > different models with different BAR sizes and you organize this with the > same tree of MemoryRegion and different sizes for the parent. But if a child is outside of the parent range, it can't be reached, right? I'm confused, maybe AddressSpace can do that, but MemoryRegion shouldn't? In the PCI case, you will simply alias a subregion with memory_region_init_alias(..., size), and size has to be <= parent size. But you won't add the PCI region, you'll add the alias, so the assert won't fire. > I'm not > saying this happens in the current devices we support, I'm just > wondering if it should be a reason not to apply the patch. I suppose > you did spend some time debugging something where the patch would have > been useful; what was that something? I'm updating some devices to use relative base address, instead of absolute one. This is useful when a subdevice is reused in another device but mapped at a different location. One case is the Raspberry Pi: $ git grep BCM2835_VC_PERI_BASE hw/arm/bcm2835_peripherals.c:20:#define BCM2835_VC_PERI_BASE 0x7e000000 hw/arm/bcm2835_peripherals.c:156: memory_region_add_subregion_overlap(&s->gpu_bus_mr, BCM2835_VC_PERI_BASE, The GPU physical address space is 1GiB, and virtual is 4GiB. Currently the device is mapped in virtual space, not respecting the GPU cache mappings. If we move the chipset base address (and correct the cache mappings) this device ends out of the GPU physical address space. To have it working I had to correct the physical address to 0x3e000000. Maybe 'info mtree' is more explicit: before: address-space: bcm2835-dma-memory 0000000000000000-00000000ffffffff (prio 0, i/o): bcm2835-gpu 0000000000000000-000000003fffffff (prio 0, i/o): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff 0000000040000000-000000007fffffff (prio 0, i/o): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff 000000007e000000-000000007effffff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals 0000000000000000-0000000000ffffff 0000000080000000-00000000bfffffff (prio 0, i/o): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff 00000000c0000000-00000000ffffffff (prio 0, i/o): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff after: address-space: bcm2835-dma-memory 0000000000000000-00000000ffffffff (prio 0, i/o): gpu-bus 0000000000000000-000000003fffffff (prio 0, i/o): alias l1-l2-cached @videocore 0000000000000000-000000003fffffff 0000000040000000-000000007fffffff (prio 0, i/o): alias l2-cached-coherent @videocore 0000000000000000-000000003fffffff 0000000080000000-00000000bfffffff (prio 0, i/o): alias l2-cached @videocore 0000000000000000-000000003fffffff 00000000c0000000-00000000ffffffff (prio 0, i/o): alias direct-uncached @videocore 0000000000000000-000000003fffffff memory-region: videocore 0000000000000000-000000003fffffff (prio 0, i/o): videocore 0000000000000000-000000003fffffff (prio 0, i/o): alias vc-ram @ram 0000000000000000-000000003fffffff 000000003e000000-000000003effffff (prio 1, i/o): alias vc-peripherals @bcm2835-peripherals 0000000000000000-0000000000ffffff Even clearer, a diff of 'info mtree -f': (qemu) info mtree -f FlatView #0 AS "bcm2835-fb-memory", root: gpu-bus AS "bcm2835-property-memory", root: gpu-bus AS "bcm2835-dma-memory", root: gpu-bus Root memory region: gpu-bus - 0000000000000000-000000003fffffff (prio 0, ram): ram + 0000000000000000-000000003e002fff (prio 0, ram): ram + 000000003e003000-000000003e00301f (prio 0, i/o): bcm2835-sys-timer + 000000003e003020-000000003e006fff (prio 0, ram): ram @000000003e003020 + 000000003e007000-000000003e007fff (prio 0, i/o): bcm2835-dma + 000000003e008000-000000003e00b1ff (prio 0, ram): ram @000000003e008000 + 000000003e00b200-000000003e00b3ff (prio 0, i/o): bcm2835-ic + 000000003e00b400-000000003e00b43f (prio -1000, i/o): bcm2835-sp804 + 000000003e00b440-000000003e00b7ff (prio 0, ram): ram @000000003e00b440 + 000000003e00b800-000000003e00bbff (prio 0, i/o): bcm2835-mbox + 000000003e00bc00-000000003e0fffff (prio 0, ram): ram @000000003e00bc00 + 000000003e100000-000000003e100fff (prio -1000, i/o): bcm2835-cprman + 000000003e101000-000000003e101fff (prio 0, ram): ram @000000003e101000 + 000000003e102000-000000003e102fff (prio -1000, i/o): bcm2835-a2w + 000000003e103000-000000003e103fff (prio 0, ram): ram @000000003e103000 + 000000003e104000-000000003e10400f (prio 0, i/o): bcm2835-rng + 000000003e104010-000000003e1fffff (prio 0, ram): ram @000000003e104010 + 000000003e200000-000000003e200fff (prio 0, i/o): bcm2835_gpio + 000000003e201000-000000003e201fff (prio 0, i/o): pl011 + 000000003e202000-000000003e202fff (prio 0, i/o): bcm2835-sdhost + 000000003e203000-000000003e2030ff (prio -1000, i/o): bcm2835-i2s + 000000003e203100-000000003e203fff (prio 0, ram): ram @000000003e203100 + 000000003e204000-000000003e20401f (prio -1000, i/o): bcm2835-spi0 + 000000003e204020-000000003e204fff (prio 0, ram): ram @000000003e204020 + 000000003e205000-000000003e20501f (prio -1000, i/o): bcm2835-i2c0 + 000000003e205020-000000003e20efff (prio 0, ram): ram @000000003e205020 + 000000003e20f000-000000003e20f07f (prio -1000, i/o): bcm2835-otp + 000000003e20f080-000000003e211fff (prio 0, ram): ram @000000003e20f080 + 000000003e212000-000000003e212007 (prio 0, i/o): bcm2835-thermal + 000000003e212008-000000003e213fff (prio 0, ram): ram @000000003e212008 + 000000003e214000-000000003e2140ff (prio -1000, i/o): bcm2835-spis + 000000003e214100-000000003e214fff (prio 0, ram): ram @000000003e214100 + 000000003e215000-000000003e2150ff (prio 0, i/o): bcm2835-aux + 000000003e215100-000000003e2fffff (prio 0, ram): ram @000000003e215100 + 000000003e300000-000000003e3000ff (prio 0, i/o): sdhci + 000000003e300100-000000003e5fffff (prio 0, ram): ram @000000003e300100 + 000000003e600000-000000003e6000ff (prio -1000, i/o): bcm2835-smi + 000000003e600100-000000003e803fff (prio 0, ram): ram @000000003e600100 + 000000003e804000-000000003e80401f (prio -1000, i/o): bcm2835-i2c1 + 000000003e804020-000000003e804fff (prio 0, ram): ram @000000003e804020 + 000000003e805000-000000003e80501f (prio -1000, i/o): bcm2835-i2c2 + 000000003e805020-000000003e8fffff (prio 0, ram): ram @000000003e805020 + 000000003e900000-000000003e907fff (prio -1000, i/o): bcm2835-dbus + 000000003e908000-000000003e90ffff (prio 0, ram): ram @000000003e908000 + 000000003e910000-000000003e917fff (prio -1000, i/o): bcm2835-ave0 + 000000003e918000-000000003e97ffff (prio 0, ram): ram @000000003e918000 + 000000003e980000-000000003e980fff (prio -1000, i/o): dwc-usb2 + 000000003e981000-000000003edfffff (prio 0, ram): ram @000000003e981000 + 000000003ee00000-000000003ee000ff (prio -1000, i/o): bcm2835-sdramc + 000000003ee00100-000000003ee04fff (prio 0, ram): ram @000000003ee00100 + 000000003ee05000-000000003ee050ff (prio 0, i/o): bcm2835-dma-chan15 + 000000003ee05100-000000003fffffff (prio 0, ram): ram @000000003ee05100 0000000040000000-000000007e002fff (prio 0, ram): ram 000000007e003000-000000007e00301f (prio 0, i/o): bcm2835-sys-timer 000000007e003020-000000007e006fff (prio 0, ram): ram @000000003e003020 @@ -56,5 +106,105 @@ 000000007ee00100-000000007ee04fff (prio 0, ram): ram @000000003ee00100 000000007ee05000-000000007ee050ff (prio 0, i/o): bcm2835-dma-chan15 000000007ee05100-000000007fffffff (prio 0, ram): ram @000000003ee05100 - 0000000080000000-00000000bfffffff (prio 0, ram): ram - 00000000c0000000-00000000ffffffff (prio 0, ram): ram + 0000000080000000-00000000be002fff (prio 0, ram): ram + 00000000be003000-00000000be00301f (prio 0, i/o): bcm2835-sys-timer + 00000000be003020-00000000be006fff (prio 0, ram): ram @000000003e003020 + 00000000be007000-00000000be007fff (prio 0, i/o): bcm2835-dma + 00000000be008000-00000000be00b1ff (prio 0, ram): ram @000000003e008000 + 00000000be00b200-00000000be00b3ff (prio 0, i/o): bcm2835-ic + 00000000be00b400-00000000be00b43f (prio -1000, i/o): bcm2835-sp804 + 00000000be00b440-00000000be00b7ff (prio 0, ram): ram @000000003e00b440 + 00000000be00b800-00000000be00bbff (prio 0, i/o): bcm2835-mbox + 00000000be00bc00-00000000be0fffff (prio 0, ram): ram @000000003e00bc00 + 00000000be100000-00000000be100fff (prio -1000, i/o): bcm2835-cprman + 00000000be101000-00000000be101fff (prio 0, ram): ram @000000003e101000 + 00000000be102000-00000000be102fff (prio -1000, i/o): bcm2835-a2w + 00000000be103000-00000000be103fff (prio 0, ram): ram @000000003e103000 + 00000000be104000-00000000be10400f (prio 0, i/o): bcm2835-rng + 00000000be104010-00000000be1fffff (prio 0, ram): ram @000000003e104010 + 00000000be200000-00000000be200fff (prio 0, i/o): bcm2835_gpio + 00000000be201000-00000000be201fff (prio 0, i/o): pl011 + 00000000be202000-00000000be202fff (prio 0, i/o): bcm2835-sdhost + 00000000be203000-00000000be2030ff (prio -1000, i/o): bcm2835-i2s + 00000000be203100-00000000be203fff (prio 0, ram): ram @000000003e203100 + 00000000be204000-00000000be20401f (prio -1000, i/o): bcm2835-spi0 + 00000000be204020-00000000be204fff (prio 0, ram): ram @000000003e204020 + 00000000be205000-00000000be20501f (prio -1000, i/o): bcm2835-i2c0 + 00000000be205020-00000000be20efff (prio 0, ram): ram @000000003e205020 + 00000000be20f000-00000000be20f07f (prio -1000, i/o): bcm2835-otp + 00000000be20f080-00000000be211fff (prio 0, ram): ram @000000003e20f080 + 00000000be212000-00000000be212007 (prio 0, i/o): bcm2835-thermal + 00000000be212008-00000000be213fff (prio 0, ram): ram @000000003e212008 + 00000000be214000-00000000be2140ff (prio -1000, i/o): bcm2835-spis + 00000000be214100-00000000be214fff (prio 0, ram): ram @000000003e214100 + 00000000be215000-00000000be2150ff (prio 0, i/o): bcm2835-aux + 00000000be215100-00000000be2fffff (prio 0, ram): ram @000000003e215100 + 00000000be300000-00000000be3000ff (prio 0, i/o): sdhci + 00000000be300100-00000000be5fffff (prio 0, ram): ram @000000003e300100 + 00000000be600000-00000000be6000ff (prio -1000, i/o): bcm2835-smi + 00000000be600100-00000000be803fff (prio 0, ram): ram @000000003e600100 + 00000000be804000-00000000be80401f (prio -1000, i/o): bcm2835-i2c1 + 00000000be804020-00000000be804fff (prio 0, ram): ram @000000003e804020 + 00000000be805000-00000000be80501f (prio -1000, i/o): bcm2835-i2c2 + 00000000be805020-00000000be8fffff (prio 0, ram): ram @000000003e805020 + 00000000be900000-00000000be907fff (prio -1000, i/o): bcm2835-dbus + 00000000be908000-00000000be90ffff (prio 0, ram): ram @000000003e908000 + 00000000be910000-00000000be917fff (prio -1000, i/o): bcm2835-ave0 + 00000000be918000-00000000be97ffff (prio 0, ram): ram @000000003e918000 + 00000000be980000-00000000be980fff (prio -1000, i/o): dwc-usb2 + 00000000be981000-00000000bedfffff (prio 0, ram): ram @000000003e981000 + 00000000bee00000-00000000bee000ff (prio -1000, i/o): bcm2835-sdramc + 00000000bee00100-00000000bee04fff (prio 0, ram): ram @000000003ee00100 + 00000000bee05000-00000000bee050ff (prio 0, i/o): bcm2835-dma-chan15 + 00000000bee05100-00000000bfffffff (prio 0, ram): ram @000000003ee05100 + 00000000c0000000-00000000fe002fff (prio 0, ram): ram + 00000000fe003000-00000000fe00301f (prio 0, i/o): bcm2835-sys-timer + 00000000fe003020-00000000fe006fff (prio 0, ram): ram @000000003e003020 + 00000000fe007000-00000000fe007fff (prio 0, i/o): bcm2835-dma + 00000000fe008000-00000000fe00b1ff (prio 0, ram): ram @000000003e008000 + 00000000fe00b200-00000000fe00b3ff (prio 0, i/o): bcm2835-ic + 00000000fe00b400-00000000fe00b43f (prio -1000, i/o): bcm2835-sp804 + 00000000fe00b440-00000000fe00b7ff (prio 0, ram): ram @000000003e00b440 + 00000000fe00b800-00000000fe00bbff (prio 0, i/o): bcm2835-mbox + 00000000fe00bc00-00000000fe0fffff (prio 0, ram): ram @000000003e00bc00 + 00000000fe100000-00000000fe100fff (prio -1000, i/o): bcm2835-cprman + 00000000fe101000-00000000fe101fff (prio 0, ram): ram @000000003e101000 + 00000000fe102000-00000000fe102fff (prio -1000, i/o): bcm2835-a2w + 00000000fe103000-00000000fe103fff (prio 0, ram): ram @000000003e103000 + 00000000fe104000-00000000fe10400f (prio 0, i/o): bcm2835-rng + 00000000fe104010-00000000fe1fffff (prio 0, ram): ram @000000003e104010 + 00000000fe200000-00000000fe200fff (prio 0, i/o): bcm2835_gpio + 00000000fe201000-00000000fe201fff (prio 0, i/o): pl011 + 00000000fe202000-00000000fe202fff (prio 0, i/o): bcm2835-sdhost + 00000000fe203000-00000000fe2030ff (prio -1000, i/o): bcm2835-i2s + 00000000fe203100-00000000fe203fff (prio 0, ram): ram @000000003e203100 + 00000000fe204000-00000000fe20401f (prio -1000, i/o): bcm2835-spi0 + 00000000fe204020-00000000fe204fff (prio 0, ram): ram @000000003e204020 + 00000000fe205000-00000000fe20501f (prio -1000, i/o): bcm2835-i2c0 + 00000000fe205020-00000000fe20efff (prio 0, ram): ram @000000003e205020 + 00000000fe20f000-00000000fe20f07f (prio -1000, i/o): bcm2835-otp + 00000000fe20f080-00000000fe211fff (prio 0, ram): ram @000000003e20f080 + 00000000fe212000-00000000fe212007 (prio 0, i/o): bcm2835-thermal + 00000000fe212008-00000000fe213fff (prio 0, ram): ram @000000003e212008 + 00000000fe214000-00000000fe2140ff (prio -1000, i/o): bcm2835-spis + 00000000fe214100-00000000fe214fff (prio 0, ram): ram @000000003e214100 + 00000000fe215000-00000000fe2150ff (prio 0, i/o): bcm2835-aux + 00000000fe215100-00000000fe2fffff (prio 0, ram): ram @000000003e215100 + 00000000fe300000-00000000fe3000ff (prio 0, i/o): sdhci + 00000000fe300100-00000000fe5fffff (prio 0, ram): ram @000000003e300100 + 00000000fe600000-00000000fe6000ff (prio -1000, i/o): bcm2835-smi + 00000000fe600100-00000000fe803fff (prio 0, ram): ram @000000003e600100 + 00000000fe804000-00000000fe80401f (prio -1000, i/o): bcm2835-i2c1 + 00000000fe804020-00000000fe804fff (prio 0, ram): ram @000000003e804020 + 00000000fe805000-00000000fe80501f (prio -1000, i/o): bcm2835-i2c2 + 00000000fe805020-00000000fe8fffff (prio 0, ram): ram @000000003e805020 + 00000000fe900000-00000000fe907fff (prio -1000, i/o): bcm2835-dbus + 00000000fe908000-00000000fe90ffff (prio 0, ram): ram @000000003e908000 + 00000000fe910000-00000000fe917fff (prio -1000, i/o): bcm2835-ave0 + 00000000fe918000-00000000fe97ffff (prio 0, ram): ram @000000003e918000 + 00000000fe980000-00000000fe980fff (prio -1000, i/o): dwc-usb2 + 00000000fe981000-00000000fedfffff (prio 0, ram): ram @000000003e981000 + 00000000fee00000-00000000fee000ff (prio -1000, i/o): bcm2835-sdramc + 00000000fee00100-00000000fee04fff (prio 0, ram): ram @000000003ee00100 + 00000000fee05000-00000000fee050ff (prio 0, i/o): bcm2835-dma-chan15 + 00000000fee05100-00000000ffffffff (prio 0, ram): ram @000000003ee05100 This is for the raspi2, but there is a mmap schema for the raspi1: https://www.cnx-software.com/wp-content/uploads/2012/02/BCM2835-Memory-Map-Large.png I found another discrepancy in the Bonito north bridge, which maps in KSEG1 instead of physical: $ git grep 'sysbus_mmio_map(.*, 0xb' hw/pci-host/bonito.c:645: sysbus_mmio_map(sysbus, 3, 0xbfe00200); hw/pci-host/bonito.c:650: sysbus_mmio_map(sysbus, 4, 0xbfe00300); We can put this patch on hold for now, I might came back to this thread later with more use cases. Regards, Phil. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-16 17:46 ` Philippe Mathieu-Daudé @ 2019-12-17 10:51 ` Paolo Bonzini 2019-12-17 11:58 ` Christophe de Dinechin 0 siblings, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2019-12-17 10:51 UTC (permalink / raw) To: Philippe Mathieu-Daudé, qemu-devel, Peter Maydell, Alexey Kardashevskiy, Michael S. Tsirkin, Alex Williamson On 16/12/19 18:46, Philippe Mathieu-Daudé wrote: >>> >> >> I think in some cases this could be intentional, for example if you have >> different models with different BAR sizes and you organize this with the >> same tree of MemoryRegion and different sizes for the parent. > > But if a child is outside of the parent range, it can't be reached, > right? I'm confused, maybe AddressSpace can do that, but MemoryRegion > shouldn't? Yes, the idea is that you could have for one version of the device parent 0x000-0x7ff stuff 0x000-0x3ff morestuff 0x400-0x7ff and for another parent 0x000-0x3ff stuff 0x000-0x3ff morestuff 0x400-0x7ff where parent is the BAR, and you can share the code to generate the tree underneath parent. > In the PCI case, you will simply alias a subregion with > memory_region_init_alias(..., size), and size has to be <= parent size. > But you won't add the PCI region, you'll add the alias, so the assert > won't fire. Yes, this is a workaround though. You shouldn't need the alias. I can see a case for your patch but I can also see one for the current behavior... Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-17 10:51 ` Paolo Bonzini @ 2019-12-17 11:58 ` Christophe de Dinechin 2019-12-17 16:57 ` Richard Henderson 0 siblings, 1 reply; 10+ messages in thread From: Christophe de Dinechin @ 2019-12-17 11:58 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Michael S. Tsirkin, Alexey Kardashevskiy, qemu-devel, Alex Williamson, Philippe Mathieu-Daudé > On 17 Dec 2019, at 11:51, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 16/12/19 18:46, Philippe Mathieu-Daudé wrote: >>>> >>> >>> I think in some cases this could be intentional, for example if you have >>> different models with different BAR sizes and you organize this with the >>> same tree of MemoryRegion and different sizes for the parent. >> >> But if a child is outside of the parent range, it can't be reached, >> right? I'm confused, maybe AddressSpace can do that, but MemoryRegion >> shouldn't? > > Yes, the idea is that you could have for one version of the device > > parent 0x000-0x7ff > stuff 0x000-0x3ff > morestuff 0x400-0x7ff > > and for another > > parent 0x000-0x3ff > stuff 0x000-0x3ff > morestuff 0x400-0x7ff > > where parent is the BAR, and you can share the code to generate the tree > underneath parent. I can see why you would have code reuse reasons to do that, but frankly it looks buggy and confusing. In the rare cases where this is indented, maybe add a flag making it explicit? > >> In the PCI case, you will simply alias a subregion with >> memory_region_init_alias(..., size), and size has to be <= parent size. >> But you won't add the PCI region, you'll add the alias, so the assert >> won't fire. > > Yes, this is a workaround though. You shouldn't need the alias. > > I can see a case for your patch but I can also see one for the current > behavior... > > Paolo > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-17 11:58 ` Christophe de Dinechin @ 2019-12-17 16:57 ` Richard Henderson 2019-12-17 18:17 ` Peter Maydell 0 siblings, 1 reply; 10+ messages in thread From: Richard Henderson @ 2019-12-17 16:57 UTC (permalink / raw) To: Christophe de Dinechin, Paolo Bonzini Cc: Peter Maydell, Michael S. Tsirkin, Alexey Kardashevskiy, qemu-devel, Alex Williamson, Philippe Mathieu-Daudé On 12/17/19 1:58 AM, Christophe de Dinechin wrote: > > >> On 17 Dec 2019, at 11:51, Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> On 16/12/19 18:46, Philippe Mathieu-Daudé wrote: >>>>> >>>> >>>> I think in some cases this could be intentional, for example if you have >>>> different models with different BAR sizes and you organize this with the >>>> same tree of MemoryRegion and different sizes for the parent. >>> >>> But if a child is outside of the parent range, it can't be reached, >>> right? I'm confused, maybe AddressSpace can do that, but MemoryRegion >>> shouldn't? >> >> Yes, the idea is that you could have for one version of the device >> >> parent 0x000-0x7ff >> stuff 0x000-0x3ff >> morestuff 0x400-0x7ff >> >> and for another >> >> parent 0x000-0x3ff >> stuff 0x000-0x3ff >> morestuff 0x400-0x7ff >> >> where parent is the BAR, and you can share the code to generate the tree >> underneath parent. > > I can see why you would have code reuse reasons to do that, > but frankly it looks buggy and confusing. In the rare cases > where this is indented, maybe add a flag making it explicit? The guest OS is programming the BAR, producing a configuration that, while it doesn't make sense, is also legal per PCI. QEMU cannot abort for this configuration. r~ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-17 16:57 ` Richard Henderson @ 2019-12-17 18:17 ` Peter Maydell 2019-12-17 18:31 ` Paolo Bonzini 0 siblings, 1 reply; 10+ messages in thread From: Peter Maydell @ 2019-12-17 18:17 UTC (permalink / raw) To: Richard Henderson Cc: Michael S. Tsirkin, Alexey Kardashevskiy, QEMU Developers, Alex Williamson, Christophe de Dinechin, Paolo Bonzini, Philippe Mathieu-Daudé On Tue, 17 Dec 2019 at 16:57, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 12/17/19 1:58 AM, Christophe de Dinechin wrote: > > > > > >> On 17 Dec 2019, at 11:51, Paolo Bonzini <pbonzini@redhat.com> wrote: > >> Yes, the idea is that you could have for one version of the device > >> > >> parent 0x000-0x7ff > >> stuff 0x000-0x3ff > >> morestuff 0x400-0x7ff > >> > >> and for another > >> > >> parent 0x000-0x3ff > >> stuff 0x000-0x3ff > >> morestuff 0x400-0x7ff > >> > >> where parent is the BAR, and you can share the code to generate the tree > >> underneath parent. > > > > I can see why you would have code reuse reasons to do that, > > but frankly it looks buggy and confusing. In the rare cases > > where this is indented, maybe add a flag making it explicit? > > The guest OS is programming the BAR, producing a configuration that, while it > doesn't make sense, is also legal per PCI. QEMU cannot abort for this > configuration. Does guest programming of the PCI BAR size actually change the size of the 'parent' region, or does it just result in the creation of an appropriately sized alias into 'parent' ? thanks -- PMM ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-17 18:17 ` Peter Maydell @ 2019-12-17 18:31 ` Paolo Bonzini 2019-12-17 18:52 ` Alex Williamson 0 siblings, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2019-12-17 18:31 UTC (permalink / raw) To: Peter Maydell, Richard Henderson Cc: Michael S. Tsirkin, Alexey Kardashevskiy, QEMU Developers, Alex Williamson, Christophe de Dinechin, Philippe Mathieu-Daudé On 17/12/19 19:17, Peter Maydell wrote: > On Tue, 17 Dec 2019 at 16:57, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 12/17/19 1:58 AM, Christophe de Dinechin wrote: >>> >>> >>>> On 17 Dec 2019, at 11:51, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>> Yes, the idea is that you could have for one version of the device >>>> >>>> parent 0x000-0x7ff >>>> stuff 0x000-0x3ff >>>> morestuff 0x400-0x7ff >>>> >>>> and for another >>>> >>>> parent 0x000-0x3ff >>>> stuff 0x000-0x3ff >>>> morestuff 0x400-0x7ff >>>> >>>> where parent is the BAR, and you can share the code to generate the tree >>>> underneath parent. >>> >>> I can see why you would have code reuse reasons to do that, >>> but frankly it looks buggy and confusing. In the rare cases >>> where this is indented, maybe add a flag making it explicit? >> >> The guest OS is programming the BAR, producing a configuration that, while it >> doesn't make sense, is also legal per PCI. QEMU cannot abort for this >> configuration. > > Does guest programming of the PCI BAR size actually change the size > of the 'parent' region, or does it just result in the creation > of an appropriately sized alias into 'parent' ? Resizable BARs are not handled by the PCI host bridge but rather from the device itself, so the device is free to handle them either way. Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-17 18:31 ` Paolo Bonzini @ 2019-12-17 18:52 ` Alex Williamson 2019-12-17 19:17 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 10+ messages in thread From: Alex Williamson @ 2019-12-17 18:52 UTC (permalink / raw) To: Paolo Bonzini Cc: Peter Maydell, Michael S. Tsirkin, Alexey Kardashevskiy, Richard Henderson, QEMU Developers, Christophe de Dinechin, Philippe Mathieu-Daudé On Tue, 17 Dec 2019 19:31:41 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 17/12/19 19:17, Peter Maydell wrote: > > On Tue, 17 Dec 2019 at 16:57, Richard Henderson > > <richard.henderson@linaro.org> wrote: > >> > >> On 12/17/19 1:58 AM, Christophe de Dinechin wrote: > >>> > >>> > >>>> On 17 Dec 2019, at 11:51, Paolo Bonzini <pbonzini@redhat.com> wrote: > >>>> Yes, the idea is that you could have for one version of the device > >>>> > >>>> parent 0x000-0x7ff > >>>> stuff 0x000-0x3ff > >>>> morestuff 0x400-0x7ff > >>>> > >>>> and for another > >>>> > >>>> parent 0x000-0x3ff > >>>> stuff 0x000-0x3ff > >>>> morestuff 0x400-0x7ff > >>>> > >>>> where parent is the BAR, and you can share the code to generate the tree > >>>> underneath parent. > >>> > >>> I can see why you would have code reuse reasons to do that, > >>> but frankly it looks buggy and confusing. In the rare cases > >>> where this is indented, maybe add a flag making it explicit? > >> > >> The guest OS is programming the BAR, producing a configuration that, while it > >> doesn't make sense, is also legal per PCI. QEMU cannot abort for this > >> configuration. > > > > Does guest programming of the PCI BAR size actually change the size > > of the 'parent' region, or does it just result in the creation > > of an appropriately sized alias into 'parent' ? > > Resizable BARs are not handled by the PCI host bridge but rather from > the device itself, so the device is free to handle them either way. More specifically, it's the responsibility of drivers within the guest to resize the parent bridge aperture to make the extent of the BAR accessible. This does seem like an interesting way to implement a resizable BAR in QEMU though. Thanks, Alex ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] memory: Do not allow subregion out of the parent region range 2019-12-17 18:52 ` Alex Williamson @ 2019-12-17 19:17 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 10+ messages in thread From: Philippe Mathieu-Daudé @ 2019-12-17 19:17 UTC (permalink / raw) To: Alex Williamson, Paolo Bonzini Cc: Peter Maydell, Michael S. Tsirkin, Alexey Kardashevskiy, Mark Cave-Ayland, Richard Henderson, QEMU Developers, Hervé Poussineau, Christophe de Dinechin On 12/17/19 7:52 PM, Alex Williamson wrote: > On Tue, 17 Dec 2019 19:31:41 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 17/12/19 19:17, Peter Maydell wrote: >>> On Tue, 17 Dec 2019 at 16:57, Richard Henderson >>> <richard.henderson@linaro.org> wrote: >>>> >>>> On 12/17/19 1:58 AM, Christophe de Dinechin wrote: >>>>> >>>>> >>>>>> On 17 Dec 2019, at 11:51, Paolo Bonzini <pbonzini@redhat.com> wrote: >>>>>> Yes, the idea is that you could have for one version of the device >>>>>> >>>>>> parent 0x000-0x7ff >>>>>> stuff 0x000-0x3ff >>>>>> morestuff 0x400-0x7ff >>>>>> >>>>>> and for another >>>>>> >>>>>> parent 0x000-0x3ff >>>>>> stuff 0x000-0x3ff >>>>>> morestuff 0x400-0x7ff >>>>>> >>>>>> where parent is the BAR, and you can share the code to generate the tree >>>>>> underneath parent. >>>>> >>>>> I can see why you would have code reuse reasons to do that, >>>>> but frankly it looks buggy and confusing. In the rare cases >>>>> where this is indented, maybe add a flag making it explicit? >>>> >>>> The guest OS is programming the BAR, producing a configuration that, while it >>>> doesn't make sense, is also legal per PCI. QEMU cannot abort for this >>>> configuration. >>> >>> Does guest programming of the PCI BAR size actually change the size >>> of the 'parent' region, or does it just result in the creation >>> of an appropriately sized alias into 'parent' ? >> >> Resizable BARs are not handled by the PCI host bridge but rather from >> the device itself, so the device is free to handle them either way. > > More specifically, it's the responsibility of drivers within the guest > to resize the parent bridge aperture to make the extent of the BAR > accessible. This does seem like an interesting way to implement a > resizable BAR in QEMU though. Thanks, This is something I'm thinking about since some time, as I observed this behavior in 3 different MIPS boards with different northbridge chipset (Malta with the GT64120, Fuloong2E with the Bonito). The firmware sets one layout, Linux (or other) reinit & reorder all the memory layout. I guess Mark hit the same issue with his sparc64 based boards. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-12-17 19:19 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-14 16:02 [PATCH] memory: Do not allow subregion out of the parent region range Philippe Mathieu-Daudé 2019-12-16 13:08 ` Paolo Bonzini 2019-12-16 17:46 ` Philippe Mathieu-Daudé 2019-12-17 10:51 ` Paolo Bonzini 2019-12-17 11:58 ` Christophe de Dinechin 2019-12-17 16:57 ` Richard Henderson 2019-12-17 18:17 ` Peter Maydell 2019-12-17 18:31 ` Paolo Bonzini 2019-12-17 18:52 ` Alex Williamson 2019-12-17 19:17 ` Philippe Mathieu-Daudé
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.
