[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob
From: |
Aravinda Prasad |
Subject: |
Re: [Qemu-devel] [PATCH 1/5] target-ppc: Extend rtas-blob |
Date: |
Tue, 02 Sep 2014 12:37:22 +0530 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130510 Thunderbird/17.0.6 |
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.
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);
>>> [...]
>>>
>>>
>>>
>>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>> mr 4,3
>>>>>>>> lis 3,address@hidden
>>>>>>>> ori 3,3,address@hidden
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>
--
Regards,
Aravinda
- 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 <=
- 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