Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices

[Peter Maydell]

On Wed, 8 Jan 2025 at 20:10, David Hildenbrand <david@redhat.com> wrote:
>
> On 08.01.25 19:35, Stefan Zabka wrote:
> > On 21/12/2024 15:55, David Hildenbrand wrote:
> >   > Let's wait for opinions from others first.
> >
> > <https://www.qemu.org/docs/master/devel/submitting-a-patch.html#if-your-patch-seems-to-have-been-ignored>
> > states that two weeks is a reasonable amount of time for follow-up.
> >
> > Should I also ping the original patch? I thought pinging the thread
> > would be more appropriate, as it contains relevant information.
> >
>
> I just pushed a compiling version of the attrs.debug approach to:
>
>         https://github.com/davidhildenbrand/qemu/tree/debug_access

I think this approach (having a 'debug' attribute in the MemTxAttrs
seems reasonable. I do note that if we allow this kind of access
to write to MMIO devices then we are also permitting ELF (and other)
image loads to write to MMIO devices where currently we ignore those.
That means there's probably a class of guest images (of dubious
correctness) which will start writing junk (likely zeroes) into
device model registers; we previously would silently ignore any
such bogus ELF sections.

Q: should we suggest behaviour for device models if they see a
'debug = 1' transaction, e.g. "don't update your internal state
for a debug read if you have clear-on-read or similar kinds of
register fields" ?

> With two preparation patches, the relevant patch is:
>
>
>  From 2e85cb1724385e4b8640364415832c030e5c5e6d Mon Sep 17 00:00:00 2001
> From: David Hildenbrand <david@redhat.com>
> Date: Wed, 8 Jan 2025 20:58:00 +0100
> Subject: [PATCH] physmem: allow cpu_memory_rw_debug to write to MMIO devices
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
>   hw/core/cpu-sysemu.c    | 13 +++++++++----
>   include/exec/memattrs.h |  5 ++++-
>   include/exec/memory.h   |  2 ++
>   system/physmem.c        |  9 ++-------
>   4 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/hw/core/cpu-sysemu.c b/hw/core/cpu-sysemu.c
> index 2a9a2a4eb5..0aa0a569e4 100644
> --- a/hw/core/cpu-sysemu.c
> +++ b/hw/core/cpu-sysemu.c
> @@ -51,13 +51,18 @@ hwaddr cpu_get_phys_page_attrs_debug(CPUState *cpu, vaddr 
> addr,
>                                        MemTxAttrs *attrs)
>   {
>       CPUClass *cc = CPU_GET_CLASS(cpu);
> +    hwaddr paddr;
>
>       if (cc->sysemu_ops->get_phys_page_attrs_debug) {
> -        return cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
> +        paddr = cc->sysemu_ops->get_phys_page_attrs_debug(cpu, addr, attrs);
> +    } else {
> +        /* Fallback for CPUs which don't implement the _attrs_ hook */
> +        *attrs = MEMTXATTRS_UNSPECIFIED;
> +        paddr = cc->sysemu_ops->get_phys_page_debug(cpu, addr);
>       }
> -    /* Fallback for CPUs which don't implement the _attrs_ hook */
> -    *attrs = MEMTXATTRS_UNSPECIFIED;
> -    return cc->sysemu_ops->get_phys_page_debug(cpu, addr);
> +    /* Indicate that this is a debug access. */
> +    attrs->debug = 1;
> +    return paddr;
>   }
>
>   hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
> diff --git a/include/exec/memattrs.h b/include/exec/memattrs.h
> index e27c18f3dc..14e0edaa58 100644
> --- a/include/exec/memattrs.h
> +++ b/include/exec/memattrs.h
> @@ -26,7 +26,8 @@ typedef struct MemTxAttrs {
>       /* Bus masters which don't specify any attributes will get this
>        * (via the MEMTXATTRS_UNSPECIFIED constant), so that we can
>        * distinguish "all attributes deliberately clear" from
> -     * "didn't specify" if necessary.
> +     * "didn't specify" if necessary. "debug" can be set alongside
> +     * "unspecified".
>        */

This feels like one of those changes which is "*probably* OK,
but breaks a thing that was previously an invariant" ones.
But trying to get all targets to support reporting real
attributes is probably too long a side-quest (we can't really
do it in cpu_get_phys_page_attrs_debug() because I don't think
we have a pre-existing way to ask the target "are you in usermode
now"; the rest of the fields we could validly leave 0).

>       unsigned int unspecified:1;
>       /*
> @@ -50,6 +51,8 @@ typedef struct MemTxAttrs {
>        * (see MEMTX_ACCESS_ERROR).
>        */
>       unsigned int memory:1;
> +    /* Debug access that can even write to ROM. */
> +    unsigned int debug:1;
>       /* Requester ID (for MSI for example) */
>       unsigned int requester_id:16;
>
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index 3dcadcf3a2..7458082455 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -2990,6 +2990,8 @@ static inline bool memory_access_is_direct(MemoryRegion 
> *mr, bool is_write,
>                                              MemTxAttrs attrs)
>   {
>       if (is_write) {
> +        if (attrs.debug)
> +            return memory_region_is_ram(mr) || memory_region_is_romd(mr);

It's a bit weird that the condition for "debug access can write"
is not the same as the one for "debug access can read"...

>           return memory_region_is_ram(mr) && !mr->readonly &&
>                  !mr->rom_device && !memory_region_is_ram_device(mr);
>       } else {

-- PMM