[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devic
|
From: |
David Hildenbrand |
|
Subject: |
Re: [PATCH v2] physmem: allow cpu_memory_rw_debug to write to MMIO devices |
|
Date: |
Fri, 10 Jan 2025 17:02:05 +0100 |
|
User-agent: |
Mozilla Thunderbird |
On 10.01.25 16:44, Peter Maydell wrote:
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.
Right. The "debug" access semantics will change for all users.
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.
Do you have any specific instances in mind that we could try?
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" ?
Not an expert on that, but I assume you mean that we start making use of
this debug flag also to modify *read* behavior, not only *write*
behavior. That would make sense to me.
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).
Yes, and having this as part of MemTxAttrs looked cleaner than passing
yet another flag (bool debug) around.
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"...
True, for now I copied what the write_rom function would have done.
Now I wonder why we don't allow read-access to
memory_region_is_ram_device. Does anybody know why?
Either that one is wrong, or the write_rom function should have also
ignored it.
Likely we should do here
if (attrs.debug) {
return memory_region_is_ram(mr) || memory_region_is_romd(mr);
} else if (is_write) {
...
}
Thanks!
--
Cheers,
David / dhildenb