Re: [Qemu-devel] [PATCH 2/6] fw_cfg: remove support for guest-side data writes

[Gabriel L. Somlo]

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