From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34532) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZrOC9-0007un-IO for qemu-devel@nongnu.org; Wed, 28 Oct 2015 06:43:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZrOC6-0000hT-B2 for qemu-devel@nongnu.org; Wed, 28 Oct 2015 06:43:05 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39945) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZrOC6-0000hF-3O for qemu-devel@nongnu.org; Wed, 28 Oct 2015 06:43:02 -0400 References: <55FC99F6.4050005@redhat.com> <20151022212210.GA1979@HEDWIG.INI.CMU.EDU> <20151026104808.GE20111@stefanha-x1.localdomain> <20151026124919.GE1979@HEDWIG.INI.CMU.EDU> <562E2CC3.9060100@redhat.com> <1445944285.4397.37.camel@redhat.com> <562F717B.6070501@redhat.com> <20151028011217.GA22084@GLSMBP.INI.CMU.EDU> From: Laszlo Ersek Message-ID: <5630A6B1.8080009@redhat.com> Date: Wed, 28 Oct 2015 11:42:57 +0100 MIME-Version: 1.0 In-Reply-To: <20151028011217.GA22084@GLSMBP.INI.CMU.EDU> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 4/5] Enable fw_cfg DMA interface for ARM List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Gabriel L. Somlo" Cc: peter.maydell@linaro.org, Stefan Hajnoczi , qemu-devel@nongnu.org, Gerd Hoffmann , jordan.l.justen@intel.com, pbonzini@redhat.com, markmb@redhat.com On 10/28/15 02:12, Gabriel L. Somlo wrote: > On Tue, Oct 27, 2015 at 01:43:39PM +0100, Laszlo Ersek wrote: >> On 10/27/15 12:11, Gerd Hoffmann wrote: >>> Hi, >>> >>>>> My hypothesis (which I guess I'm volunteering to verify, unless we >>>>> end up rejecting this immediately as a bad idea, for some reason that >>>>> I have missed), is that current functionality wouldn't change, given >>>>> the way existing callbacks work right now, and that we could run the >>>>> callback each time a blob is *selected*, rather than hooking into the >>>>> (dma/mmio/pio) read methods. >>>> >>>> Callback executed on first read only sounds okay to me, callback >>>> executed on selection... hm... don't like it. :) >>> >>> Care to explain why? >>> >>> I think callback on selection would be better. Interface is more clear >>> then, I don't like read having different behavior depending on hidden >>> state (current offset). >> >>> And in practice selection and read will always >>> be called together, >> >> This is what I think you cannot guarantee on the host side, without >> auditing all guest code. The behavior of callbacks has been specified >> under fw_cfg_add_file_callback(), in docs/specs/fw_cfg.txt, and guest >> code is allowed to work off that. >> >>> so there shouldn't be a difference in practice ... >> >> I guess I have no choice but to audit all QemuFwCfgSelectItem calls in >> edk2... >> >> Right, here's what I've had in the back of my mind: see the >> DetectSmbiosVersion() function in >> "OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c". It selects >> the key that belongs to the "etc/smbios/smbios-anchor" fw_cfg file, but >> the switch statement right after it can jump to the "default" label, and >> under that label *nothing* is read from fw_cfg. >> >> This is valid guest code according to the current specs. Its behavior >> would change (however obscurely) if there was a callback on the >> "etc/smbios/smbios-anchor" file, and the callback was executed on >> selection, not read. > > OK, but none of "etc/smbios/*" blobs actually have a callback at all. > > After some grepping, the only places inserting callback-equipped blobs > are: > > - hw/i386/acpi-build.c (via rom_add_blob or directly by calling > fw_cfg_add_file_callback) > > - files added are > "/etc/acpi/rsdp", > "/etc/acpi/tables", and > "/etc/table-loader". > > - all using the same callback: acpi_build_update() > > - hw/arm/virt-acpi-build.c (via rom_add_blob only) > > - same three files as on i386 > > - all using the same callback: virt_acpi_build_update() > > Both of these callbacks are a one-shot deal, i.e. they both > contain something along these lines: > > /* No state to update or already patched? Nothing to do. */ > if (!build_state || build_state->patched) { > return; > } > build_state->patched = 1; > > So, they do something *once* before the first byte is ever read, and > never again after that. > >> ... This one instance wouldn't be particularly hard to patch in edk2, >> but in general our specs are useless if we don't stick to them. > > OK, so I was proposing to amend the specs (now, while externally > visible behavior won't be affected), and *THEN* stick to them :) > > We're already giving up on the letter of the specs (right now, they > say once per byte read, but DMA is only doing once per chunk transfered, > which in practice amounts to once each time a whole blob is read). Good point. > Of course, if you (or anyone else with much more clue than me) expect > a future scenario where we'd need the opportunity to run the callback > more than once *before* reading anything from the blob, or (as is the > case with smbios) wish to select (but not read from) blobs, and the > blobs will be callback-enabled, but running the callback will be a bad > thing when no read follows, then by all means, let's stick with > hooking into each individual read operation. > > As it is right now, the ammended spec I'm proposing (if set, callback > runs on select, whether a read follows or not) is a NOP w.r.t. currently > visible behavior. It allows simplifying things, at the price of removing > theoretical future flexibility (but also unnecessary slowness as well). Okay. Please go ahead with the change, as far as I'm concerned. Thanks Laszlo > Thanks for helping me think this through ! > > --Gabriel >