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

[Peter Maydell]

On Fri, 10 Jan 2025 at 16:55, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Peter Maydell <peter.maydell@linaro.org> writes:
>
> > 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" ?
>
> What do we do for device models that want to know which CPU things are
> coming from, as per:
>
>   https://gitlab.com/qemu-project/qemu/-/issues/124

I think that's an orthogonal issue. We already have a problem that
you can see on reads where because we don't encode the CPU ID
into the txattrs devices have to work around this by using
the current_cpu global, which isn't set for gdbstub accesses
(and so if the device doesn't have handling for 'current_cpu == NULL'
then it might crash).

But this patch only changes the handling of writes, which puts them
in the same situation as read accesses. If the write falls over it's
probably also the case that they were already falling over on reads.

thanks
-- PMM