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

[Laszlo Ersek]

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.

> The other thing you could do is put image and initrd in to guest
> memory as we do currently and have the fw_cfg just pass their
> addresses. That would require that UEFI not have already stomped
> on that part of RAM by the time you get round to looking at it,
> though, and I can imagine that might not be feasible.

Yes, I thought of this. It's not very different from how the initial DTB
is handled right now, theoretically.

The difference is that the DTB is small. It is placed at 0x4000_0000
(1GB, base of DRAM). The earliest phase of the firmare (SEC and first
half of PEI) uses memory at the fixed address 0x4007_c000 (1GB + 496
KB). I would really not want to change that in the firmware.

http://thread.gmane.org/gmane.comp.bios.tianocore.devel/8969/focus=9029

We could place the kernel image and the initrd elsewhere, but (a)
placing stuff in the DRAM in advance puts minimum DRAM size requirements
on the guest (*), which is hard to verify / enforce in the firmware with
blobs of wildly varying size, and more importantly, (b) auditing the
safety of the initial DTB was already the hardest part of the firmware
patchset review by far. (No doubt it was one of the hardest things to
implement for Ard too.)

(*) See later parts in the same message, wrt. "second world". Later
phases need a good chunk of memory counting downwards from the top. See
also the SIZE_128MB assert near
<https://github.com/tianocore/edk2/blob/master/ArmPlatformPkg/ArmVirtualizationPkg/Library/ArmVirtualizationPlatformLib/Virt.c#L174>.

The nice thing about fw_cfg is that it lives in the "128MB..256MB [...]
miscellaneous device I/O" space. It's basically impossible to collide
with that in UEFI, yet it can give you various blobs (and qemu can even
compute those dynamically, see ACPI rebuild after PCI enumeration on
x86). I'd *really* like to stick with that.

I'll try to figure out what's wrong with TCG and come up with a patch
for the second data register.

Thanks!
Laszlo