On 12/08/14 20:39, Laszlo Ersek wrote: > On 12/08/14 15:41, Peter Maydell wrote: >> On 8 December 2014 at 14:01, Laszlo Ersek <ler...@redhat.com> wrote: >>> Peter, can we introduce a second, 64-bit wide, data register, for >>> fw_cfg? (Or even two -- Drew suggested the LDP instruction for the guest.) >> >> I don't have an in-principle objection. I do require that >> whatever mechanism we provide is usable by 32 bit guests >> as well as 64 bit guests. > > The idea is that in the following: > > fw_cfg_data_mem_read() > fw_cfg_read() > > fw_cfg_data_mem_read() doesn't care about either "addr" or "size", which > happens to be the best for the current problem. > > According to the documentation on "MemoryRegionOps" in > "include/exec/memory.h" (and also to my testing in practice), if you set > .valid.max_access_size to something large (definitely up to 8, but maybe > even up to 16?), but keep .impl.max_access_size at something smaller, > then "memory.c" will split up the access for you automatically. > (Ultimately turning the bigger access to several sequential calls to > fw_cfg_read(), which happens to be correct.) > > So, if a 32-bit guest never "submits" an access wider than 32-bit, then > that's the largest that "memory.c" will slice and dice. > >> You'll also want the access instruction >> to be one where the hardware provides instruction syndrome information >> to KVM about the faulting load/store, > > Yes, I definitely remembered this; I just didn't know how to name it > precisely. > >> which means you can only use >> AArch64 loads and stores of a single general-purpose register, and >> AArch32 instructions LDR, LDA, LDRT, LDRSH, LDRSHT, LDRH, LDAH, LDRHT, >> LDRSB, LDRSBT, LDRB, LDAB, LDRBT, STR, STL, STRT, STRH, STLH, STRHT, >> STRB, STLB, or STRBT. >> >> Note that this rules out LDP. > > Yes, I was concerned about this (Drew can confirm :)) Thank you very > much for making this clear; this will definitely help with the guest > code. Also we don't have to figure out if .valid.max_access_size=16 > would work at all. > >> We also need to make sure it works with TCG QEMU. (64-bit access >> to devices is something we've needed previously in ARM QEMU, so >> though in theory it should work it would need testing.) > > (Factoring in the typo correction from your next mail:) > > Good point. I just tested TCG. While the speedup is similar, the data > that is transferred looks corrupt.
Okay, TCG is not the culprit, it seems to work okay. I messed up the memory region registration in the previous patch. On x86_64 the 8-byte access is broken up into two 4-byte accesses, and the second one of those is not accepted as a valid access, and it qualifies as an unassigned read. So the following in addition makes it work on TCG (x86_64) too: ----------------- diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c index 7147fea..c2bc44c 100644 --- a/hw/nvram/fw_cfg.c +++ b/hw/nvram/fw_cfg.c @@ -31,7 +31,7 @@ #include "qemu/config-file.h" #define FW_CFG_SIZE 2 -#define FW_CFG_DATA_SIZE 1 +#define FW_CFG_DATA_SIZE 8 #define TYPE_FW_CFG "fw_cfg" #define FW_CFG_NAME "fw_cfg" #define FW_CFG_PATH "/machine/" FW_CFG_NAME ----------------- It affects the memory_region_init_io() call in fw_cfg_initfn(). I hope to submit a small v3 series soon. Thanks, Laszlo