[PATCH] system/physmem: Fix cpu_memory_rw_debug for armv7m MPU

[PATCH] system/physmem: Fix cpu_memory_rw_debug for armv7m MPU

[Xiong Nandi]

The actual page size (region size for MPU) of armv7m may
smaller than TARGET_PAGE_SIZE (2^5 vs 2^10). So we should
use the actual virtual address to get the phys page address.

Since address_space_rw do not check the region protection, so
if we have some region like [0x0020 ~ 0x003F rw], [0x0040 ~ 0x007F --],
we will be able to read out the whole [0x0020 ~ 0x007F] region.
As a debug function, this seems acceptable.

I have make a minimal reproducible demo here:
https://gist.github.com/xndcn/3c534818b6486ecd2414d1cc7925c372

after building main.elf, run:
qemu-system-arm -machine stm32vldiscovery -kernel main.elf -s & 
gdb-multiarch main.elf
(gdb) target remote :1234
(gdb) disas main
Dump of assembler code for function main:
   0x08000040 <+0>:	Cannot access memory at address 0x8000040

[PATCH] system/physmem: Fix cpu_memory_rw_debug for armv7m MPU

[Xiong Nandi]

The actual page size (region size for MPU) of armv7m may
smaller than TARGET_PAGE_SIZE (2^5 vs 2^10). So we should
use the actual virtual address to get the phys page address.

Signed-off-by: Xiong Nandi <xndchn@gmail.com>
---
 system/physmem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/system/physmem.c b/system/physmem.c
index dc1db3a384..a76b305130 100644
--- a/system/physmem.c
+++ b/system/physmem.c
@@ -3564,11 +3564,12 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
         MemTxResult res;
 
         page = addr & TARGET_PAGE_MASK;
-        phys_addr = cpu_get_phys_page_attrs_debug(cpu, page, &attrs);
+        phys_addr = cpu_get_phys_page_attrs_debug(cpu, addr, &attrs);
         asidx = cpu_asidx_from_attrs(cpu, attrs);
         /* if no physical page mapped, return an error */
         if (phys_addr == -1)
             return -1;
+        phys_addr &= TARGET_PAGE_MASK;
         l = (page + TARGET_PAGE_SIZE) - addr;
         if (l > len)
             l = len;
-- 
2.25.1

Re: [PATCH] system/physmem: Fix cpu_memory_rw_debug for armv7m MPU

[Richard Henderson]

On 11/20/24 09:15, Xiong Nandi wrote:
> The actual page size (region size for MPU) of armv7m may
> smaller than TARGET_PAGE_SIZE (2^5 vs 2^10). So we should
> use the actual virtual address to get the phys page address.
> 
> Signed-off-by: Xiong Nandi <xndchn@gmail.com>
> ---
>   system/physmem.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index dc1db3a384..a76b305130 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3564,11 +3564,12 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
>           MemTxResult res;
>   
>           page = addr & TARGET_PAGE_MASK;
> -        phys_addr = cpu_get_phys_page_attrs_debug(cpu, page, &attrs);
> +        phys_addr = cpu_get_phys_page_attrs_debug(cpu, addr, &attrs);
>           asidx = cpu_asidx_from_attrs(cpu, attrs);
>           /* if no physical page mapped, return an error */
>           if (phys_addr == -1)
>               return -1;
> +        phys_addr &= TARGET_PAGE_MASK;
>           l = (page + TARGET_PAGE_SIZE) - addr;
>           if (l > len)
>               l = len;

So... I guess this might accidentally work, but L is definitely incorrect under the 
circumstances.  So we could easily be exchanging one set of bugs for another.

We really need to be returning the range of addresses under which the address translation 
is valid.  One solution could be passing in 'l = len, &l' to be modified so that (addr, l) 
translates to (phys_addr, l) after the call; iterate for sum l < len as we're currently doing.


r~

Re: [PATCH] system/physmem: Fix cpu_memory_rw_debug for armv7m MPU

[xndcn]

Thanks. This patch at least guarantees normal read/write access to
addresses with r/w flags, although there is still a risk of
misidentifying accessible regions within continuous address spaces.

Actually, initially I did write a patch with a modified page size as
an argument, but I soon found that the current implementation of
armv7a (pmsav7) will return the page size (lg_page_size) as 0 in many
situations (such as overlapping regions).
Maybe we can simply make the page size as 2^5 (minimal page size) when
the returned lg_page_size == 0?

On Sat, Nov 23, 2024 at 4:32 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/20/24 09:15, Xiong Nandi wrote:
> > The actual page size (region size for MPU) of armv7m may
> > smaller than TARGET_PAGE_SIZE (2^5 vs 2^10). So we should
> > use the actual virtual address to get the phys page address.
> >
> > Signed-off-by: Xiong Nandi <xndchn@gmail.com>
> > ---
> >   system/physmem.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/system/physmem.c b/system/physmem.c
> > index dc1db3a384..a76b305130 100644
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -3564,11 +3564,12 @@ int cpu_memory_rw_debug(CPUState *cpu, vaddr addr,
> >           MemTxResult res;
> >
> >           page = addr & TARGET_PAGE_MASK;
> > -        phys_addr = cpu_get_phys_page_attrs_debug(cpu, page, &attrs);
> > +        phys_addr = cpu_get_phys_page_attrs_debug(cpu, addr, &attrs);
> >           asidx = cpu_asidx_from_attrs(cpu, attrs);
> >           /* if no physical page mapped, return an error */
> >           if (phys_addr == -1)
> >               return -1;
> > +        phys_addr &= TARGET_PAGE_MASK;
> >           l = (page + TARGET_PAGE_SIZE) - addr;
> >           if (l > len)
> >               l = len;
>
> So... I guess this might accidentally work, but L is definitely incorrect under the
> circumstances.  So we could easily be exchanging one set of bugs for another.
>
> We really need to be returning the range of addresses under which the address translation
> is valid.  One solution could be passing in 'l = len, &l' to be modified so that (addr, l)
> translates to (phys_addr, l) after the call; iterate for sum l < len as we're currently doing.
>
>
> r~

Re: [PATCH] system/physmem: Fix cpu_memory_rw_debug for armv7m MPU

[Richard Henderson]

On 11/23/24 22:30, xndcn wrote:
> Thanks. This patch at least guarantees normal read/write access to
> addresses with r/w flags, although there is still a risk of
> misidentifying accessible regions within continuous address spaces.
> 
> Actually, initially I did write a patch with a modified page size as
> an argument, but I soon found that the current implementation of
> armv7a (pmsav7) will return the page size (lg_page_size) as 0 in many
> situations (such as overlapping regions).

0 was arbitrarily chosen as "anything less than TARGET_PAGE_BITS".

> Maybe we can simply make the page size as 2^5 (minimal page size) when
> the returned lg_page_size == 0?

No, don't lie, or make unhelpful guesses at the generic level. The only thing you could do 
for arm m-profile is give accurate information, which for now will be immediately 
discarded as *still* being less than TARGET_PAGE_BITS.  But I do have plans for that.

Anyway, nothing you do with page sizes is helpful along the debug path.  For that, we need 
to swap away from "pages" to "ranges", where some ranges are in fact pages, but others 
aren't.  This means changing the API for cpu_get_phys_<something>_debug.


r~

Re: [PATCH] system/physmem: Fix cpu_memory_rw_debug for armv7m MPU

[xndcn]

Got it, thanks.
It seems like there is still a lot of work surrounding the API, so I
create a issue to track it
https://gitlab.com/qemu-project/qemu/-/issues/2697

On Mon, Nov 25, 2024 at 5:23 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 11/23/24 22:30, xndcn wrote:
> > Thanks. This patch at least guarantees normal read/write access to
> > addresses with r/w flags, although there is still a risk of
> > misidentifying accessible regions within continuous address spaces.
> >
> > Actually, initially I did write a patch with a modified page size as
> > an argument, but I soon found that the current implementation of
> > armv7a (pmsav7) will return the page size (lg_page_size) as 0 in many
> > situations (such as overlapping regions).
>
> 0 was arbitrarily chosen as "anything less than TARGET_PAGE_BITS".
>
> > Maybe we can simply make the page size as 2^5 (minimal page size) when
> > the returned lg_page_size == 0?
>
> No, don't lie, or make unhelpful guesses at the generic level. The only thing you could do
> for arm m-profile is give accurate information, which for now will be immediately
> discarded as *still* being less than TARGET_PAGE_BITS.  But I do have plans for that.
>
> Anyway, nothing you do with page sizes is helpful along the debug path.  For that, we need
> to swap away from "pages" to "ranges", where some ranges are in fact pages, but others
> aren't.  This means changing the API for cpu_get_phys_<something>_debug.
>
>
> r~