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 <so...@cmu.edu> > > - 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
