From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58098) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuLb5-000425-Tp for qemu-devel@nongnu.org; Thu, 05 Nov 2015 09:33:05 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZuLax-0001hK-L8 for qemu-devel@nongnu.org; Thu, 05 Nov 2015 09:33:03 -0500 Received: from relay-05.andrew.cmu.edu ([128.2.157.12]:52839 helo=relay.andrew.cmu.edu) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZuLax-0001hG-GU for qemu-devel@nongnu.org; Thu, 05 Nov 2015 09:32:55 -0500 From: "Gabriel L. Somlo" Date: Thu, 5 Nov 2015 09:32:51 -0500 Message-Id: <1446733972-1602-6-git-send-email-somlo@cmu.edu> In-Reply-To: <1446733972-1602-1-git-send-email-somlo@cmu.edu> References: <1446733972-1602-1-git-send-email-somlo@cmu.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: [Qemu-devel] [PATCH v5 5/6] fw_cfg: add generic non-DMA read method List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, jordan.l.justen@intel.com, armbru@redhat.com, kraxel@redhat.com, pbonzini@redhat.com, markmb@redhat.com, lersek@redhat.com Introduce fw_cfg_data_read(), a generic read method which works on all access widths (1 through 8 bytes, inclusive), and can be used during both IOPort and MMIO read accesses. To maintain legibility, only fw_cfg_data_mem_read() (the MMIO data read method) is replaced by this patch. The new method essentially unwinds the fw_cfg_data_mem_read() + fw_cfg_read() combo, but without unnecessarily repeating all the validity checks performed by the latter on each byte being read. This patch also modifies the trace_fw_cfg_read prototype to accept a 64-bit value argument, allowing it to work properly with the new read method, but also remain backward compatible with existing call sites. Cc: Laszlo Ersek Cc: Gerd Hoffmann Cc: Marc Mar=C3=AD Signed-off-by: Gabriel Somlo --- I think in this case I'll stick with a single assert for the whole bounds check. In the long run, "easy to read" might be just a bit more helpful than "in the VERY unlikely event of actually triggering this, it will also tell us which side of the interval we fell out on"... Thanks again for all the advice and feedback! --Gabriel hw/nvram/fw_cfg.c | 45 +++++++++++++++++++++++++++++++-------------- trace-events | 2 +- 2 files changed, 32 insertions(+), 15 deletions(-) diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 046fa74..8f566f6 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -274,6 +274,36 @@ static int fw_cfg_select(FWCfgState *s, uint16_t key= ) return ret; } =20 +static uint64_t fw_cfg_data_read(void *opaque, hwaddr addr, unsigned siz= e) +{ + FWCfgState *s =3D opaque; + int arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); + FWCfgEntry *e =3D (s->cur_entry =3D=3D FW_CFG_INVALID) ? NULL : + &s->entries[arch][s->cur_entry & FW_CFG_ENTRY_MASK]; + uint64_t value =3D 0; + + assert(size > 0 && size <=3D sizeof(value)); + if (s->cur_entry !=3D FW_CFG_INVALID && e->data && s->cur_offset < e= ->len) { + /* The least significant 'size' bytes of the return value are + * expected to contain a string preserving portion of the item + * data, padded with zeros on the right in case we run out early. + * In technical terms, we're composing the host-endian represent= ation + * of the big endian interpretation of the fw_cfg string. + */ + do { + value =3D (value << 8) | e->data[s->cur_offset++]; + } while (--size && s->cur_offset < e->len); + /* If size is still not zero, we *did* run out early, so continu= e + * left-shifting, to add the appropriate number of padding zeros + * on the right. + */ + value <<=3D 8 * size; + } + + trace_fw_cfg_read(s, value); + return value; +} + static uint8_t fw_cfg_read(FWCfgState *s) { int arch =3D !!(s->cur_entry & FW_CFG_ARCH_LOCAL); @@ -291,19 +321,6 @@ static uint8_t fw_cfg_read(FWCfgState *s) return ret; } =20 -static uint64_t fw_cfg_data_mem_read(void *opaque, hwaddr addr, - unsigned size) -{ - FWCfgState *s =3D opaque; - uint64_t value =3D 0; - unsigned i; - - for (i =3D 0; i < size; ++i) { - value =3D (value << 8) | fw_cfg_read(s); - } - return value; -} - static void fw_cfg_data_mem_write(void *opaque, hwaddr addr, uint64_t value, unsigned size) { @@ -485,7 +502,7 @@ static const MemoryRegionOps fw_cfg_ctl_mem_ops =3D { }; =20 static const MemoryRegionOps fw_cfg_data_mem_ops =3D { - .read =3D fw_cfg_data_mem_read, + .read =3D fw_cfg_data_read, .write =3D fw_cfg_data_mem_write, .endianness =3D DEVICE_BIG_ENDIAN, .valid =3D { diff --git a/trace-events b/trace-events index 17fbddf..f2f2ce4 100644 --- a/trace-events +++ b/trace-events @@ -196,7 +196,7 @@ ecc_diag_mem_readb(uint64_t addr, uint32_t ret) "Read= diagnostic %"PRId64"=3D %02x =20 # hw/nvram/fw_cfg.c fw_cfg_select(void *s, uint16_t key, int ret) "%p key %d =3D %d" -fw_cfg_read(void *s, uint8_t ret) "%p =3D %d" +fw_cfg_read(void *s, uint64_t ret) "%p =3D %"PRIx64 fw_cfg_add_file(void *s, int index, char *name, size_t len) "%p #%d: %s = (%zd bytes)" =20 # hw/block/hd-geometry.c --=20 2.4.3