| Message ID | 20180405012241.25714-2-f4bug@amsat.org |
|---|---|
| State | New |
| Headers | show |
| Series | memory: fix access_with_adjusted_size() and misc | expand |
On 5 April 2018 at 02:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > If an user creates a RAM region smaller than TARGET_PAGE_SIZE, > this region will be handled as a subpage. > While the subpage behavior can be noticed by an experienced QEMU > developper, it might takes hours to a novice to figure it out. > To save time to novices, do not allow subpage creation via the > memory_region_init_ram_*() functions. This commit message doesn't make it clear to me what actually goes wrong. Why doesn't the subpage mechanism do the right thing here? Also, a quick grep revealed at least one caller that's currently creating a less-than-a-page sized RAM block, in hw/pci-host/xilinx-pcie.c. We would need to change all the call sites that rely on creating small RAM blocks before we could make this throw an error. (Better still would be if we could make small lumps of RAM actually work.) thanks -- PMM
On 04/05/2018 06:27 AM, Peter Maydell wrote: > On 5 April 2018 at 02:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> If an user creates a RAM region smaller than TARGET_PAGE_SIZE, >> this region will be handled as a subpage. >> While the subpage behavior can be noticed by an experienced QEMU >> developper, it might takes hours to a novice to figure it out. >> To save time to novices, do not allow subpage creation via the >> memory_region_init_ram_*() functions. > > This commit message doesn't make it clear to me what actually > goes wrong. Why doesn't the subpage mechanism do the right thing > here? Trying to understand a bit more, I think the problem is "you can not _execute_ from a region smaller than TARGET_PAGE_SIZE", however if this region is used for I/O this is not a problem (the xilinx-pcie.c case). In my case I create a 2K SRAM which contains the exception vectors, but instructions are never fetched because it is handled as I/O. I suppose my issue is between these two functions: MemoryRegion *iotlb_to_region(CPUState *cpu, hwaddr index, MemTxAttrs attrs) { int asidx = cpu_asidx_from_attrs(cpu, attrs); CPUAddressSpace *cpuas = &cpu->cpu_ases[asidx]; AddressSpaceDispatch *d = atomic_rcu_read(&cpuas->memory_dispatch); MemoryRegionSection *sections = d->map.sections; return sections[index & ~TARGET_PAGE_MASK].mr; } tb_page_addr_t get_page_addr_code(CPUArchState *env, target_ulong addr) { int mmu_idx, index, pd; void *p; MemoryRegion *mr; CPUState *cpu = ENV_GET_CPU(env); CPUIOTLBEntry *iotlbentry; hwaddr physaddr; index = (addr >> TARGET_PAGE_BITS) & (CPU_TLB_SIZE - 1); mmu_idx = cpu_mmu_index(env, true); if (unlikely(env->tlb_table[mmu_idx][index].addr_code != (addr & (TARGET_PAGE_MASK | TLB_INVALID_MASK)))) { if (!VICTIM_TLB_HIT(addr_read, addr)) { tlb_fill(ENV_GET_CPU(env), addr, 0, MMU_INST_FETCH, mmu_idx, 0); } } iotlbentry = &env->iotlb[mmu_idx][index]; pd = iotlbentry->addr & ~TARGET_PAGE_MASK; mr = iotlb_to_region(cpu, pd, iotlbentry->attrs); if (memory_region_is_unassigned(mr)) { ... > > Also, a quick grep revealed at least one caller that's currently > creating a less-than-a-page sized RAM block, in > hw/pci-host/xilinx-pcie.c. We would need to change all the > call sites that rely on creating small RAM blocks before we > could make this throw an error. (Better still would be if we > could make small lumps of RAM actually work.)
On 5 April 2018 at 13:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: > On 04/05/2018 06:27 AM, Peter Maydell wrote: >> On 5 April 2018 at 02:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> If an user creates a RAM region smaller than TARGET_PAGE_SIZE, >>> this region will be handled as a subpage. >>> While the subpage behavior can be noticed by an experienced QEMU >>> developper, it might takes hours to a novice to figure it out. >>> To save time to novices, do not allow subpage creation via the >>> memory_region_init_ram_*() functions. >> >> This commit message doesn't make it clear to me what actually >> goes wrong. Why doesn't the subpage mechanism do the right thing >> here? > > Trying to understand a bit more, I think the problem is "you can not > _execute_ from a region smaller than TARGET_PAGE_SIZE", however if this > region is used for I/O this is not a problem (the xilinx-pcie.c case). > > In my case I create a 2K SRAM which contains the exception vectors, but > instructions are never fetched because it is handled as I/O. Ah, I wondered if it might be that. Yes, you can't execute from small lumps of memory at the moment. We might be able in theory to fix this for TCG, though I think it's harder to do so for KVM. If we end up implementing small-MPU-region support for ARM v7M/v8M that will have a similar setup. When RTH and I last discussed that: https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00293.html I think the favoured idea was to have a way to say "always take the slow path and do an MPU/MMU check". If we also had a mechanism for taking the slow path for code execution that would effectively also allow execution from subpages, though done only slowly one guest insn per TLB. The two use cases aren't exactly the same but some of the implementation seems similar enough to do the same way. (There's a lot of unresolved detail and irritating corner cases to deal with, though.) thanks -- PMM
On 04/05/2018 10:20 AM, Peter Maydell wrote: > On 5 April 2018 at 13:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >> On 04/05/2018 06:27 AM, Peter Maydell wrote: >>> On 5 April 2018 at 02:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>> If an user creates a RAM region smaller than TARGET_PAGE_SIZE, >>>> this region will be handled as a subpage. >>>> While the subpage behavior can be noticed by an experienced QEMU >>>> developper, it might takes hours to a novice to figure it out. >>>> To save time to novices, do not allow subpage creation via the >>>> memory_region_init_ram_*() functions. >>> >>> This commit message doesn't make it clear to me what actually >>> goes wrong. Why doesn't the subpage mechanism do the right thing >>> here? >> >> Trying to understand a bit more, I think the problem is "you can not >> _execute_ from a region smaller than TARGET_PAGE_SIZE", however if this >> region is used for I/O this is not a problem (the xilinx-pcie.c case). >> >> In my case I create a 2K SRAM which contains the exception vectors, but >> instructions are never fetched because it is handled as I/O. > > Ah, I wondered if it might be that. Yes, you can't execute from > small lumps of memory at the moment. We might be able in theory > to fix this for TCG, though I think it's harder to do so for KVM. > If we end up implementing small-MPU-region support for ARM v7M/v8M > that will have a similar setup. When RTH and I last discussed that: > https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00293.html > I think the favoured idea was to have a way to say "always take the > slow path and do an MPU/MMU check". If we also had a mechanism for > taking the slow path for code execution that would effectively > also allow execution from subpages, though done only slowly one > guest insn per TLB. The two use cases aren't exactly the same but > some of the implementation seems similar enough to do the same way. > (There's a lot of unresolved detail and irritating corner cases to > deal with, though.) Now this makes more sens to me, thanks :) Luckily in my case I can workaround with a 4K SRAM, but it took me hours to track this down until Paolo helped me. Regards, Phil.
On 04/05/2018 03:31 PM, Philippe Mathieu-Daudé wrote: > On 04/05/2018 10:20 AM, Peter Maydell wrote: >> On 5 April 2018 at 13:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>> On 04/05/2018 06:27 AM, Peter Maydell wrote: >>>> On 5 April 2018 at 02:22, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote: >>>>> If an user creates a RAM region smaller than TARGET_PAGE_SIZE, >>>>> this region will be handled as a subpage. >>>>> While the subpage behavior can be noticed by an experienced QEMU >>>>> developper, it might takes hours to a novice to figure it out. >>>>> To save time to novices, do not allow subpage creation via the >>>>> memory_region_init_ram_*() functions. >>>> >>>> This commit message doesn't make it clear to me what actually >>>> goes wrong. Why doesn't the subpage mechanism do the right thing >>>> here? >>> >>> Trying to understand a bit more, I think the problem is "you can not >>> _execute_ from a region smaller than TARGET_PAGE_SIZE", however if this >>> region is used for I/O this is not a problem (the xilinx-pcie.c case). >>> >>> In my case I create a 2K SRAM which contains the exception vectors, but >>> instructions are never fetched because it is handled as I/O. >> >> Ah, I wondered if it might be that. Yes, you can't execute from >> small lumps of memory at the moment. We might be able in theory >> to fix this for TCG, though I think it's harder to do so for KVM. >> If we end up implementing small-MPU-region support for ARM v7M/v8M >> that will have a similar setup. When RTH and I last discussed that: >> https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg00293.html >> I think the favoured idea was to have a way to say "always take the >> slow path and do an MPU/MMU check". If we also had a mechanism for >> taking the slow path for code execution that would effectively >> also allow execution from subpages, though done only slowly one >> guest insn per TLB. The two use cases aren't exactly the same but >> some of the implementation seems similar enough to do the same way. >> (There's a lot of unresolved detail and irritating corner cases to >> deal with, though.) > > Now this makes more sens to me, thanks :) > > Luckily in my case I can workaround with a 4K SRAM, but it took me hours > to track this down until Paolo helped me. > I seems to remember having the same pain with the mmio execution stuff. It triggered a bad ram pointer as far a I remember. Thanks, Fred > Regards, > > Phil. >
On 5 April 2018 at 14:34, KONRAD Frederic <frederic.konrad@adacore.com> wrote: > On 04/05/2018 03:31 PM, Philippe Mathieu-Daudé wrote: >> Luckily in my case I can workaround with a 4K SRAM, but it took me hours >> to track this down until Paolo helped me. > I seems to remember having the same pain with the mmio execution > stuff. It triggered a bad ram pointer as far a I remember. I think this would once have ended up in report_bad_exec() which prints a more-or-less helpful message. However for CPUs which now implement the unassigned-access or transaction-failed hooks you'll get a guest exception instead now, I guess. Incidentally we really ought to fix the interaction of mmio-exec and migration, rather than just leaving it as it is (disabling migration for xilinx_qspips device). thanks -- PMM
diff --git a/memory.c b/memory.c index e70b64b8b9..51d27b7b26 100644 --- a/memory.c +++ b/memory.c @@ -1519,6 +1519,15 @@ void memory_region_init_ram_shared_nomigrate(MemoryRegion *mr, bool share, Error **errp) { + if (size < TARGET_PAGE_SIZE) { + /* Region less than PAGE_SIZE are handled as subpages, which are + * surely not what the caller expects. + * Limit the minimum ram region size to avoid annoying debugging. + */ + error_setg(errp, "Invalid RAM size: %ld (minimum required: %d)", + size, TARGET_PAGE_SIZE); + return; + } memory_region_init(mr, owner, name, size); mr->ram = true; mr->terminates = true;
If an user creates a RAM region smaller than TARGET_PAGE_SIZE, this region will be handled as a subpage. While the subpage behavior can be noticed by an experienced QEMU developper, it might takes hours to a novice to figure it out. To save time to novices, do not allow subpage creation via the memory_region_init_ram_*() functions. Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- memory.c | 9 +++++++++ 1 file changed, 9 insertions(+)