[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob
From: |
Alexey Kardashevskiy |
Subject: |
Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob |
Date: |
Tue, 02 Sep 2014 23:17:54 +1000 |
User-agent: |
Mozilla/5.0 (X11; Linux i686 on x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.0 |
On 09/02/2014 07:30 PM, Aravinda Prasad wrote:
>
>
> On Tuesday 02 September 2014 02:10 PM, Alexey Kardashevskiy wrote:
>> On 09/02/2014 05:07 PM, Aravinda Prasad wrote:
>>>
>>>
>>> On Tuesday 02 September 2014 12:04 PM, Alexey Kardashevskiy wrote:
>>>> On 09/02/2014 03:56 PM, Aravinda Prasad wrote:
>>>>>
>>>>>
>>>>> On Tuesday 02 September 2014 11:19 AM, Alexey Kardashevskiy wrote:
>>>>>> On 09/02/2014 03:25 PM, Aravinda Prasad wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Tuesday 02 September 2014 09:39 AM, Alexey Kardashevskiy wrote:
>>>>>>>> On 09/01/2014 09:23 PM, Aravinda Prasad wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Monday 01 September 2014 01:16 PM, Alexey Kardashevskiy wrote:
>>>>>>>>>> On 08/25/2014 11:45 PM, Aravinda Prasad wrote:
>>>>>>>>>>> Extend rtas-blob to accommodate error log. Error log
>>>>>>>>>>> structure is saved in rtas space upon a machine check
>>>>>>>>>>> exception.
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Aravinda Prasad <address@hidden>
>>>>>>>>>>> ---
>>>>>>>>>>> hw/ppc/spapr.c | 13 ++++++++++---
>>>>>>>>>>> hw/ppc/spapr_rtas.c | 4 ++--
>>>>>>>>>>> include/hw/ppc/spapr.h | 2 +-
>>>>>>>>>>> pc-bios/spapr-rtas/spapr-rtas.S | 12 ++++++++++++
>>>>>>>>>>> 4 files changed, 25 insertions(+), 6 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
>>>>>>>>>>> index d01978f..1120988 100644
>>>>>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>>>>>> @@ -85,6 +85,12 @@
>>>>>>>>>>>
>>>>>>>>>>> #define HTAB_SIZE(spapr) (1ULL << ((spapr)->htab_shift))
>>>>>>>>>>>
>>>>>>>>>>> +/*
>>>>>>>>>>> + * The rtas-entry-offset should match the value specified in
>>>>>>>>>>> + * spapr-rtas.S
>>>>>>>>>>> + */
>>>>>>>>>>> +#define RTAS_ENTRY_OFFSET 0x1000
>>>>>>>>>>> +
>>>>>>>>>>> typedef struct sPAPRMachineState sPAPRMachineState;
>>>>>>>>>>>
>>>>>>>>>>> #define TYPE_SPAPR_MACHINE "spapr-machine"
>>>>>>>>>>> @@ -670,7 +676,8 @@ static int
>>>>>>>>>>> spapr_populate_memory(sPAPREnvironment *spapr, void *fdt)
>>>>>>>>>>> static void spapr_finalize_fdt(sPAPREnvironment *spapr,
>>>>>>>>>>> hwaddr fdt_addr,
>>>>>>>>>>> hwaddr rtas_addr,
>>>>>>>>>>> - hwaddr rtas_size)
>>>>>>>>>>> + hwaddr rtas_size,
>>>>>>>>>>> + hwaddr rtas_entry)
>>>>>>>>>>> {
>>>>>>>>>>> int ret, i;
>>>>>>>>>>> size_t cb = 0;
>>>>>>>>>>> @@ -705,7 +712,7 @@ static void spapr_finalize_fdt(sPAPREnvironment
>>>>>>>>>>> *spapr,
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> /* RTAS */
>>>>>>>>>>> - ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size);
>>>>>>>>>>> + ret = spapr_rtas_device_tree_setup(fdt, rtas_addr, rtas_size,
>>>>>>>>>>> rtas_entry);
>>>>>>>>>>> if (ret < 0) {
>>>>>>>>>>> fprintf(stderr, "Couldn't set up RTAS device tree
>>>>>>>>>>> properties\n");
>>>>>>>>>>> }
>>>>>>>>>>> @@ -808,7 +815,7 @@ static void ppc_spapr_reset(void)
>>>>>>>>>>>
>>>>>>>>>>> /* Load the fdt */
>>>>>>>>>>> spapr_finalize_fdt(spapr, spapr->fdt_addr, spapr->rtas_addr,
>>>>>>>>>>> - spapr->rtas_size);
>>>>>>>>>>> + spapr->rtas_size, spapr->rtas_addr +
>>>>>>>>>>> RTAS_ENTRY_OFFSET);
>>>>>>>>>>>
>>>>>>>>>>> /* Set up the entry state */
>>>>>>>>>>> first_ppc_cpu = POWERPC_CPU(first_cpu);
>>>>>>>>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>>>>>>>>> index 9ba1ba6..02ddbf9 100644
>>>>>>>>>>> --- a/hw/ppc/spapr_rtas.c
>>>>>>>>>>> +++ b/hw/ppc/spapr_rtas.c
>>>>>>>>>>> @@ -328,7 +328,7 @@ void spapr_rtas_register(int token, const char
>>>>>>>>>>> *name, spapr_rtas_fn fn)
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>>>>>>>>>> - hwaddr rtas_size)
>>>>>>>>>>> + hwaddr rtas_size, hwaddr
>>>>>>>>>>> rtas_entry)
>>>>>>>>>>> {
>>>>>>>>>>> int ret;
>>>>>>>>>>> int i;
>>>>>>>>>>> @@ -349,7 +349,7 @@ int spapr_rtas_device_tree_setup(void *fdt,
>>>>>>>>>>> hwaddr rtas_addr,
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> ret = qemu_fdt_setprop_cell(fdt, "/rtas", "linux,rtas-entry",
>>>>>>>>>>> - rtas_addr);
>>>>>>>>>>> + rtas_entry);
>>>>>>>>>>> if (ret < 0) {
>>>>>>>>>>> fprintf(stderr, "Couldn't add linux,rtas-entry property:
>>>>>>>>>>> %s\n",
>>>>>>>>>>> fdt_strerror(ret));
>>>>>>>>>>> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
>>>>>>>>>>> index bbba51a..dedfa67 100644
>>>>>>>>>>> --- a/include/hw/ppc/spapr.h
>>>>>>>>>>> +++ b/include/hw/ppc/spapr.h
>>>>>>>>>>> @@ -436,7 +436,7 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu,
>>>>>>>>>>> sPAPREnvironment *spapr,
>>>>>>>>>>> uint32_t token, uint32_t nargs,
>>>>>>>>>>> target_ulong args,
>>>>>>>>>>> uint32_t nret, target_ulong rets);
>>>>>>>>>>> int spapr_rtas_device_tree_setup(void *fdt, hwaddr rtas_addr,
>>>>>>>>>>> - hwaddr rtas_size);
>>>>>>>>>>> + hwaddr rtas_size, hwaddr
>>>>>>>>>>> rtas_entry);
>>>>>>>>>>>
>>>>>>>>>>> #define SPAPR_TCE_PAGE_SHIFT 12
>>>>>>>>>>> #define SPAPR_TCE_PAGE_SIZE (1ULL << SPAPR_TCE_PAGE_SHIFT)
>>>>>>>>>>> diff --git a/pc-bios/spapr-rtas/spapr-rtas.S
>>>>>>>>>>> b/pc-bios/spapr-rtas/spapr-rtas.S
>>>>>>>>>>> index 903bec2..8c9b17e 100644
>>>>>>>>>>> --- a/pc-bios/spapr-rtas/spapr-rtas.S
>>>>>>>>>>> +++ b/pc-bios/spapr-rtas/spapr-rtas.S
>>>>>>>>>>> @@ -30,6 +30,18 @@
>>>>>>>>>>>
>>>>>>>>>>> .globl _start
>>>>>>>>>>> _start:
>>>>>>>>>>> + /*
>>>>>>>>>>> + * Reserve space for error log in RTAS blob.
>>>>>>>>>>> + *
>>>>>>>>>>> + * Either we can reserve initial bytes for error log followed by
>>>>>>>>>>> + * rtas-entry or space can be reserved after rtas-entry. I
>>>>>>>>>>> prefer
>>>>>>>>>>> + * former, as we already have rtas-base and rtas-entry
>>>>>>>>>>> (currently
>>>>>>>>>>> + * both pointing to rtas-base) defined in qemu and we can update
>>>>>>>>>>> + * rtas-entry to point to an offset from rtas-base. This avoids
>>>>>>>>>>> + * unnecessary definition of rtas-error-offset while keeping
>>>>>>>>>>> + * rtas-entry redundant.
>>>>>>>>>>> + */
>>>>>>>>>>> + . = 0x1000
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Why not this (and not changing spapr-rtas.S)?
>>>>>>>>>>
>>>>>>>>>> --- a/hw/ppc/spapr.c
>>>>>>>>>> +++ b/hw/ppc/spapr.c
>>>>>>>>>> @@ -875,7 +875,8 @@ static void ppc_spapr_reset(void)
>>>>>>>>>> spapr->rtas_size);
>>>>>>>>>>
>>>>>>>>>> /* Copy RTAS over */
>>>>>>>>>> - cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
>>>>>>>>>> + cpu_physical_memory_write(spapr->rtas_addr + RTAS_ENTRY_OFFSET,
>>>>>>>>>> + spapr->rtas_blob,
>>>>>>>>>> spapr->rtas_size);
>>>>>>>>>
>>>>>>>>> This is possible, however requires suitable adjustment to make sure
>>>>>>>>> spapr->rtas_addr has enough space allocated.
>>>>>>>>
>>>>>>>>
>>>>>>>> How is adding RTAS_ENTRY_OFFSET not enough to make sure that is has
>>>>>>>> enough
>>>>>>>> space? QEMU copies RTAS to guest memory, QEMU makes up rtas_addr/entry
>>>>>>>> properties.
>>>>>>>
>>>>>>> QEMU adds spapr-rtas.bin as a rom, with rom->addr set to
>>>>>>> spapr->rtas_addr, rom->datasize set to 20 bytes (the size of current
>>>>>>> spapr-rtas.bin) and contents of spapr-rtas.bin read into rom->data
>>>>>>> (malloc-ed region).
>>>>>>>
>>>>>>> I think, access to spapr->rtas_addr is mapped to this rom. Hence it is
>>>>>>> necessary to have rtas_addr and rtas_size consistent with the Rom
>>>>>>> struct. If we use spapr->rtas_addr + RTAS_ENTRY_OFFSET then we are
>>>>>>> trying to access an invalid offset in rom region.
>>>>>>
>>>>>>
>>>>>> What is that "rom" struct you are referring to? In upstream QEMU, I can
>>>>>> only see:
>>>>>>
>>>>>> ppc_spapr_init():
>>>>>> [...]
>>>>>> filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, "spapr-rtas.bin")
>>>>>
>>>>> In ppc_spapr_init() just after qemu_find_file() we have:
>>>>>
>>>>> spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr, ...);
>>>>
>>>>
>>>> What tree has this? Mine does not.
>>>>
>>>> ka1:~/p/qemu$ grep load_image_targphys hw/ppc/spapr.c
>>>> initrd_size = load_image_targphys(initrd_filename, initrd_base,
>>>> fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
>>>> ka1:~/p/qemu$
>>>>
>>>
>>> I am using git://git.qemu.org/qemu.git, master branch.
>>
>> Ah. I see. My bad, I recommended the wrong tree then, sorry about it...
>>
>> This one is the current queue of ppc patches and I believe this patchset
>> will go through it:
>> git://github.com/agraf/qemu.git branch: ppc-next
>>
>> And it does not have load_image_targphys() for rtas.
>
> Hmm.. ok. Let me have a look at it.
>
> So my v2 should be based on git://github.com/agraf/qemu.git branch:
> ppc-next right?
Yes since this branch is closer to upstream and affects what you do.
>
>>
>>
>>>
>>> I have 88e89a57f9 as the latest commit and I see:
>>>
>>> $ grep load_image_targphys hw/ppc/spapr.c
>>> spapr->rtas_size = load_image_targphys(filename, spapr->rtas_addr,
>>> initrd_size = load_image_targphys(initrd_filename, initrd_base,
>>> fw_size = load_image_targphys(filename, 0, FW_MAX_SIZE);
>>>
>>>
>>> This was introduced in commit a3467baa.
>>>
>>> Regards,
>>> Aravinda
>>>
>>>>
>>>>
>>>>>
>>>>> load_image_targphys() -> rom_add_file_fixed() -> rom_add_file(), where
>>>>> Rom is initialized.
>>>>>
>>>>>> spapr->rtas_size = get_image_size(filename);
>>>>>> spapr->rtas_blob = g_malloc(spapr->rtas_size);
>>>>>> if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) {
>>>>>> [...]
>>>>>>
>>>>>> and then
>>>>>>
>>>>>> ppc_spapr_reset():
>>>>>> [...]
>>>>>> spapr->rtas_addr = rtas_limit - RTAS_MAX_SIZE
>>>>>> [...]
>>>>>> cpu_physical_memory_write(spapr->rtas_addr, spapr->rtas_blob,
>>>>>> spapr->rtas_size);
>>>>>> [...]
--
Alexey
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Alexey Kardashevskiy, 2014/09/01
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Aravinda Prasad, 2014/09/01
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Alexey Kardashevskiy, 2014/09/02
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Aravinda Prasad, 2014/09/02
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Alexey Kardashevskiy, 2014/09/02
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Aravinda Prasad, 2014/09/02
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Alexey Kardashevskiy, 2014/09/02
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Aravinda Prasad, 2014/09/02
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Alexey Kardashevskiy, 2014/09/02
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob, Aravinda Prasad, 2014/09/02
- Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob,
Alexey Kardashevskiy <=