[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data
From: |
Gabriel L. Somlo |
Subject: |
Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes |
Date: |
Mon, 16 Mar 2015 14:41:47 -0400 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Mon, Mar 16, 2015 at 06:02:45PM +0100, Laszlo Ersek wrote:
> On 03/16/15 15:15, Gabriel L. Somlo wrote:
> > The fw_cfg device allowed guest-side data writes to overwrite the
> > selected entry in place, without allowing modification to the size
> > of the entry, and with the ability to invoke a callback each time
> > the entry was overwritten completely.
> >
> > To date, we are not aware of any use case which relies on the guest's
> > ability to (over)write any given fw_cfg entry, and QEMU does not
> > register any fw_cfg write callbacks.
> >
> > This patch removes the unused code path for registering fw_cfg write
> > callbacks, and also removes support for guest-side data writes to the
> > fw_cfg device.
> >
> > Signed-off-by: Gabriel Somlo <address@hidden>
>
> - The code hunks seem okay to me. But, you also remove a trace call
> (trace_fw_cfg_write), so the corresponding trace point should be dropped
> from the file "trace-events".
>
> - I can't tell of the top of my head what happens if the guest attempts
> an fw_cfg data write nonetheless. I vaguely recall some unassigned-io
> stuff from MemoryRegion land, which ultimately renders the guest access
> effect-less. Can anyone please test / confirm / explain that?
>
> Hm, the new validity checks should catch those:
>
> memory_region_dispatch_write()
> memory_region_access_valid()
> mr->ops->valid.accepts()
> unassigned_mem_write()
> cpu_unassigned_access()
> cc->do_unassigned_access()
>
> Seems to land in the CPUClass.do_unassigned_access() member, if there is
> one.
>
> Hm. The intersection between "has non-NULL do_unassigned_access()" and
> "uses fw_cfg" seems to SPARC. See:
> - sparc_cpu_unassigned_access() in "target-sparc/ldst_helper.c",
> - fw_cfg_init_mem() in "hw/sparc/sun4m.c",
> - fw_cfg_init_io() in "hw/sparc64/sun4u.c".
>
> I don't have the slightest clue if we should care, but *theoretically*,
> this change could turn guest code (ie. fw_cfg data writes) that used to
> do "nothing" into traps. Someone else will have to chime in on that.
>
> Oh why is nothing simple. :(
Another possibility is to leave the write_ops in there, but turn them
into no-ops. Or just keep support for writing data to fw_cfg -- I don't
have strong feelings in this regard, it only came up during the Q&A
about fw_cfg internals I started a bit earlier... :)
If turning fw_cfg writes into traps is unacceptable for some reason,
and we still collectively think it's a good idea to remove write
support, I also have nothing against trying to wrap my head around
do_unassigned_access(), of which I know nothing right now.
So I echo Laszlo's sentiment that we need more feedback on this. While
I totally welcome opportunities to learn about QEMU, I'd also rather not
go off on too many tangents unless there's a good technical motivation
to do so w.r.t. my original goal (in this case, that's to allow fw_cfg
blobs to be added from the command line).
Thanks much,
--Gabriel
- [Qemu-devel] [PATCH 1/6] fw_cfg: add documentation file (docs/specs/fw_cfg.txt), (continued)
- [Qemu-devel] [PATCH 1/6] fw_cfg: add documentation file (docs/specs/fw_cfg.txt), Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 4/6] fw_cfg: exit with error when dupe fw_cfg file name inserted, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 5/6] fw_cfg: insert fw_cfg file blobs via qemu cmdline, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 6/6] qga: RFC: guest-side retrieval of fw_cfg file, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 3/6] fw_cfg: assertion to detect memory leak when adding new data blob, Gabriel L. Somlo, 2015/03/16
- [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes, Gabriel L. Somlo, 2015/03/16
- Re: [Qemu-devel] [PATCH 0/6] fw-cfg: documentation, cleanup, and proposed feature, Patchew Tool, 2015/03/16