From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35033) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhhU9-0000K4-7I for qemu-devel@nongnu.org; Thu, 01 Oct 2015 13:17:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZhhU6-00040R-1j for qemu-devel@nongnu.org; Thu, 01 Oct 2015 13:17:37 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54065) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZhhU5-00040M-Rq for qemu-devel@nongnu.org; Thu, 01 Oct 2015 13:17:34 -0400 References: <1443701677-13629-1-git-send-email-markmb@redhat.com> <1443701819-13855-1-git-send-email-markmb@redhat.com> <1443701819-13855-8-git-send-email-markmb@redhat.com> <560D5A2A.7090403@redhat.com> <20151001170213.GA10605@morn.lan> From: Laszlo Ersek Message-ID: <560D6AAA.6010001@redhat.com> Date: Thu, 1 Oct 2015 19:17:30 +0200 MIME-Version: 1.0 In-Reply-To: <20151001170213.GA10605@morn.lan> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v4 7/7] fw_cfg: Define a static signature to be returned on DMA port reads List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin O'Connor Cc: Drew , Stefan Hajnoczi , "Gabriel L. Somlo" , qemu-devel@nongnu.org, Gerd Hoffmann , =?UTF-8?Q?Marc_Mar=c3=ad?= On 10/01/15 19:02, Kevin O'Connor wrote: > On Thu, Oct 01, 2015 at 06:07:06PM +0200, Laszlo Ersek wrote: >> On 10/01/15 14:16, Marc Mar=ED wrote: >>> From: Kevin O'Connor >>> >>> Return a static signature ("QEMU CFG") if the guest does a read to th= e >>> DMA address io register. >>> >>> Signed-off-by: Kevin O'Connor >>> --- >>> docs/specs/fw_cfg.txt | 4 ++++ >>> hw/nvram/fw_cfg.c | 13 +++++++++++-- >>> 2 files changed, 15 insertions(+), 2 deletions(-) >>> >>> diff --git a/docs/specs/fw_cfg.txt b/docs/specs/fw_cfg.txt >>> index 2d6b2da..249b99e 100644 >>> --- a/docs/specs/fw_cfg.txt >>> +++ b/docs/specs/fw_cfg.txt >>> @@ -93,6 +93,10 @@ by selecting the "signature" item using key 0x0000= (FW_CFG_SIGNATURE), >>> and reading four bytes from the data register. If the fw_cfg device = is >>> present, the four bytes read will contain the characters "QEMU". >>> =20 >>> +Additionaly, if the DMA interface is available then a read to the DM= A >>> +Address will return 0x51454d5520434647 ("QEMU CFG" in big-endian >>> +format). >> >> I suggest: >> >> Additionaly, if the DMA interface is available, then a read into the >> DMA Address register will return the matching substring of the >> "QEMU CFG" string. Characters that are at lower offsets of the >> substring being read will be stored at lower addresses in the guest. >=20 > The interface only supports 32bit and 64bit writes, so I would suggest > the document not encourage 8bit reads. The implementation happens to > works with 8bit reads, but that was just a side-effect. Okay, I don't insist. :) >=20 > [...] >>> @@ -393,6 +395,12 @@ static void fw_cfg_dma_transfer(FWCfgState *s) >>> trace_fw_cfg_read(s, 0); >>> } >>> =20 >>> +static uint64_t fw_cfg_dma_mem_read(void *opaque, hwaddr addr, >>> + unsigned size) >>> +{ >>> + return FW_CFG_DMA_SIGNATURE >> ((8 - addr - size) * 8); >>> +} >>> + >> >> After re-reading the message of QEMU commit 36b62ae6a5, I think this i= s >> mostly correct. >> >> (I think *technically* it is fully correct, due to the truncation that >> is implicit in the argument passing to bswapXX(), in the >> adjust_endianness() function [memory.c].) >> >> However, for the human reader's sake, I'd like to see the >> over-significant bits masked off explicitly here, after the shift. >=20 > The value has to be truncated, as a guest inl() or readl() can't > possibly return more than 32 bits. >=20 > I think the code's harder to read with the mask in place. Are there > any helper macros or functions in QEMU to help with register > read/writes of smaller/larger sizes than expected? Maybe the > .impl.min_access_size should just be set to 4. I'd be fine even with just a comment then. :) >=20 > [...] >>> @@ -416,8 +424,8 @@ static void fw_cfg_dma_mem_write(void *opaque, hw= addr addr, >>> static bool fw_cfg_dma_mem_valid(void *opaque, hwaddr addr, >>> unsigned size, bool is_write) >>> { >>> - return is_write && ((size =3D=3D 4 && (addr =3D=3D 0 || addr =3D= =3D 4)) || >>> - (size =3D=3D 8 && addr =3D=3D 0)); >>> + return !is_write || ((size =3D=3D 4 && (addr =3D=3D 0 || addr =3D= =3D 4)) || >>> + (size =3D=3D 8 && addr =3D=3D 0)); >>> } >> >> This is not good enough I think; it's possible for the guest to do an >> "inl" at offset 7 into the register, which will cause undefined behavi= or >> in fw_cfg_dma_mem_read(). (The shift count being negative.) >=20 > The function never gets called on unaligned accesses (.impl.unaligned > !=3D true), so that can't happen. That may be true, but -- for me at least -- that's too much to remember (or to look up all the time). Plus, I find an explicit check more robust, should the alignment restriction be lifted at some point. In any case, I don't insist. Thanks Laszlo >=20 > -Kevin >=20