Re: [Qemu-devel] [PATCH v2] arm: add fw_cfg to "virt" board

[Laszlo Ersek]

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 <address@hidden> 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