[v2] arm: add fw_cfg to "virt" board

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 <lersek@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:

-----------------
-----------------

It affects the memory_region_init_io() call in fw_cfg_initfn().

I hope to submit a small v3 series soon.

Thanks,
Laszlo

On 8 December 2014 at 21:19, Laszlo Ersek <lersek@redhat.com> wrote:
> 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.

If you do that don't you now try to define an ioport on
x86 that's 8 bytes wide? You probably also need to check
whether the ppc and sparc boards that use mmio fw_cfg
can handle the wider data register.

-- PMM

On 12/08/14 22:34, Peter Maydell wrote:
> On 8 December 2014 at 21:19, Laszlo Ersek <lersek@redhat.com> wrote:
>> 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.
> 
> If you do that don't you now try to define an ioport on
> x86 that's 8 bytes wide? You probably also need to check
> whether the ppc and sparc boards that use mmio fw_cfg
> can handle the wider data register.

The above was just a quick PoC. The series I'm about to post takes care
not to change anything (not even gdb experience) for clients that don't
request the wider register.

Thanks
Laszlo