| Message ID | 20140825134516.2361.32987.stgit@aravindap |
|---|---|
| State | New |
| Headers | show |
On Mon, Aug 25, 2014 at 07:15:16PM +0530, Aravinda Prasad wrote: > Extend rtas-blob to accommodate error log. Error log > structure is saved in rtas space upon a machine check > exception. Hrm. Putting the reserved space into the actual firmware image file seems really clumsy to me. Wouldn't it be better to reserve the space directly from qemu, then just paste the 20 bytes of code into that area.
On Tuesday 26 August 2014 11:08 AM, David Gibson wrote: > On Mon, Aug 25, 2014 at 07:15:16PM +0530, Aravinda Prasad wrote: >> Extend rtas-blob to accommodate error log. Error log >> structure is saved in rtas space upon a machine check >> exception. > > Hrm. Putting the reserved space into the actual firmware image file > seems really clumsy to me. Wouldn't it be better to reserve the space > directly from qemu, then just paste the 20 bytes of code into that > area. Hmm. It is possible to it from qemu. In that case we can get rid of spapr-rtas.S. Let me think about that. Regards, Aravinda > --
On Tue, Aug 26, 2014 at 12:04:27PM +0530, Aravinda Prasad wrote: > > > On Tuesday 26 August 2014 11:08 AM, David Gibson wrote: > > On Mon, Aug 25, 2014 at 07:15:16PM +0530, Aravinda Prasad wrote: > >> Extend rtas-blob to accommodate error log. Error log > >> structure is saved in rtas space upon a machine check > >> exception. > > > > Hrm. Putting the reserved space into the actual firmware image file > > seems really clumsy to me. Wouldn't it be better to reserve the space > > directly from qemu, then just paste the 20 bytes of code into that > > area. > > Hmm. It is possible to it from qemu. In that case we can get rid of > spapr-rtas.S. Let me think about that. I think getting rid of spapr-rtas.S is probably a good idea. It complicates building things for the sake of 5 instructions. Especially since in your new code you construct the vector instructions directly, it makes sense to do the same thing for spapr-rtas.
On 25.08.14 15:45, 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 <aravinda@linux.vnet.ibm.com> I can't say I'm a big fan of this patch. Can we somehow separate that NMI page from the RTAS blob? Also I'd definitely prefer if we keep rtas_entry == rtas_addr - if nothing else for the sake of backwards compatibility. So how about we lay out the structure in memory like this: [ spapr-rtas.bin ] [ padding to 4k boundary or whatever sPAPR requires ] [ 4k NMI region ] Then the only thing we'd have to really change internally is the size information of the rtas blob. Alex
On Thursday 28 August 2014 04:10 PM, Alexander Graf wrote: > > > On 25.08.14 15:45, 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 <aravinda@linux.vnet.ibm.com> > > I can't say I'm a big fan of this patch. Can we somehow separate that > NMI page from the RTAS blob? Also I'd definitely prefer if we keep > rtas_entry == rtas_addr - if nothing else for the sake of backwards > compatibility. > > So how about we lay out the structure in memory like this: > > [ spapr-rtas.bin ] > [ padding to 4k boundary or whatever sPAPR requires ] > [ 4k NMI region ] > > Then the only thing we'd have to really change internally is the size > information of the rtas blob. Either we can have it like this or completely eliminate spapr-rtas.bin (and spapr-rtas.S) by simply allocating required space in QEMU and then patching the 5 instructions at rtas-entry as earlier discussed with David. http://lists.nongnu.org/archive/html/qemu-ppc/2014-08/msg00251.html Regards, Aravinda > > > Alex >
On 28.08.14 20:20, Aravinda Prasad wrote: > > > On Thursday 28 August 2014 04:10 PM, Alexander Graf wrote: >> >> >> On 25.08.14 15:45, 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 <aravinda@linux.vnet.ibm.com> >> >> I can't say I'm a big fan of this patch. Can we somehow separate that >> NMI page from the RTAS blob? Also I'd definitely prefer if we keep >> rtas_entry == rtas_addr - if nothing else for the sake of backwards >> compatibility. >> >> So how about we lay out the structure in memory like this: >> >> [ spapr-rtas.bin ] >> [ padding to 4k boundary or whatever sPAPR requires ] >> [ 4k NMI region ] >> >> Then the only thing we'd have to really change internally is the size >> information of the rtas blob. > > Either we can have it like this or completely eliminate spapr-rtas.bin > (and spapr-rtas.S) by simply allocating required space in QEMU and then > patching the 5 instructions at rtas-entry as earlier discussed with David. > > http://lists.nongnu.org/archive/html/qemu-ppc/2014-08/msg00251.html > I strongly disagree with David. Legally there is no difference between a .bin file that contains code and an array made of instructions. And the more target code we can keep outside of QEMU the better. Alex
On Fri, 2014-08-29 at 00:18 +0200, Alexander Graf wrote: > I strongly disagree with David. Legally there is no difference between a > .bin file that contains code and an array made of instructions. And the > more target code we can keep outside of QEMU the better. Ugh ... having a separate file with it's own build system etc... for 5 instructions is borderline ridiculous too :-) Cheers, Ben.
On 29.08.14 00:25, Benjamin Herrenschmidt wrote: > On Fri, 2014-08-29 at 00:18 +0200, Alexander Graf wrote: >> I strongly disagree with David. Legally there is no difference between a >> .bin file that contains code and an array made of instructions. And the >> more target code we can keep outside of QEMU the better. > > Ugh ... having a separate file with it's own build system etc... for 5 > instructions is borderline ridiculous too :-) Then don't use it - nobody's forcing you to do so :). Just use the provided binary. I fail to see the difference between uint32_t rtas_blob[] = { INST_SC1, INST_BLR }; and uint32_t *rtas_blob; load_file_from_disk(rtas_blob); except that we're using an actual assembler ;). Alex
On Fri, 2014-08-29 at 02:40 +0200, Alexander Graf wrote: > > On 29.08.14 00:25, Benjamin Herrenschmidt wrote: > > On Fri, 2014-08-29 at 00:18 +0200, Alexander Graf wrote: > >> I strongly disagree with David. Legally there is no difference between a > >> .bin file that contains code and an array made of instructions. And the > >> more target code we can keep outside of QEMU the better. > > > > Ugh ... having a separate file with it's own build system etc... for 5 > > instructions is borderline ridiculous too :-) > > Then don't use it - nobody's forcing you to do so :). Just use the > provided binary. I fail to see the difference between > > uint32_t rtas_blob[] = { INST_SC1, INST_BLR }; > > and > > uint32_t *rtas_blob; > load_file_from_disk(rtas_blob); > > except that we're using an actual assembler ;). So you fail to see the difference with an array of 5 words vs. running through thousands of instructions & syscall to read those same 5 words from disk ? :-) Cheers, Ben.
> Am 29.08.2014 um 03:06 schrieb Benjamin Herrenschmidt <benh@au1.ibm.com>: > >> On Fri, 2014-08-29 at 02:40 +0200, Alexander Graf wrote: >> >>> On 29.08.14 00:25, Benjamin Herrenschmidt wrote: >>>> On Fri, 2014-08-29 at 00:18 +0200, Alexander Graf wrote: >>>> I strongly disagree with David. Legally there is no difference between a >>>> .bin file that contains code and an array made of instructions. And the >>>> more target code we can keep outside of QEMU the better. >>> >>> Ugh ... having a separate file with it's own build system etc... for 5 >>> instructions is borderline ridiculous too :-) >> >> Then don't use it - nobody's forcing you to do so :). Just use the >> provided binary. I fail to see the difference between >> >> uint32_t rtas_blob[] = { INST_SC1, INST_BLR }; >> >> and >> >> uint32_t *rtas_blob; >> load_file_from_disk(rtas_blob); >> >> except that we're using an actual assembler ;). > > So you fail to see the difference with an array of 5 words vs. running > through thousands of instructions & syscall to read those same 5 words > from disk ? :-) I fail to see a problem, yeah :). Imagine the same thing on x86 with its completely messed up instruction set. Would you still advocate for in-qemu code or would you prefer to have a compiler between you and the ugly opcodes? Alex
On Fri, 2014-08-29 at 03:33 +0200, Alexander Graf wrote: > I fail to see a problem, yeah :). Imagine the same thing on x86 with > its completely messed up instruction set. Would you still advocate for > in-qemu code or would you prefer to have a compiler between you and > the ugly opcodes? You mean an assembler ? :-) Cheers, Ben.
On Fri, Aug 29, 2014 at 03:33:59AM +0200, Alexander Graf wrote: > > > > Am 29.08.2014 um 03:06 schrieb Benjamin Herrenschmidt <benh@au1.ibm.com>: > > > >> On Fri, 2014-08-29 at 02:40 +0200, Alexander Graf wrote: > >> > >>> On 29.08.14 00:25, Benjamin Herrenschmidt wrote: > >>>> On Fri, 2014-08-29 at 00:18 +0200, Alexander Graf wrote: > >>>> I strongly disagree with David. Legally there is no difference between a > >>>> .bin file that contains code and an array made of instructions. And the > >>>> more target code we can keep outside of QEMU the better. > >>> > >>> Ugh ... having a separate file with it's own build system etc... for 5 > >>> instructions is borderline ridiculous too :-) > >> > >> Then don't use it - nobody's forcing you to do so :). Just use the > >> provided binary. I fail to see the difference between > >> > >> uint32_t rtas_blob[] = { INST_SC1, INST_BLR }; > >> > >> and > >> > >> uint32_t *rtas_blob; > >> load_file_from_disk(rtas_blob); > >> > >> except that we're using an actual assembler ;). > > > > So you fail to see the difference with an array of 5 words vs. running > > through thousands of instructions & syscall to read those same 5 words > > from disk ? :-) > > I fail to see a problem, yeah :). Imagine the same thing on x86 with > its completely messed up instruction set. Would you still advocate > for in-qemu code or would you prefer to have a compiler between you > and the ugly opcodes? If it was only 20 bytes worth, and by it's nature unlikely to need changing, then, yes, I would, even with the extra x86 ugliness. Assuming it was well commented, obviously.
On Fri, Aug 29, 2014 at 12:18:44AM +0200, Alexander Graf wrote: > > > On 28.08.14 20:20, Aravinda Prasad wrote: > > > > > > On Thursday 28 August 2014 04:10 PM, Alexander Graf wrote: > >> > >> > >> On 25.08.14 15:45, 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 <aravinda@linux.vnet.ibm.com> > >> > >> I can't say I'm a big fan of this patch. Can we somehow separate that > >> NMI page from the RTAS blob? Also I'd definitely prefer if we keep > >> rtas_entry == rtas_addr - if nothing else for the sake of backwards > >> compatibility. > >> > >> So how about we lay out the structure in memory like this: > >> > >> [ spapr-rtas.bin ] > >> [ padding to 4k boundary or whatever sPAPR requires ] > >> [ 4k NMI region ] > >> > >> Then the only thing we'd have to really change internally is the size > >> information of the rtas blob. > > > > Either we can have it like this or completely eliminate spapr-rtas.bin > > (and spapr-rtas.S) by simply allocating required space in QEMU and then > > patching the 5 instructions at rtas-entry as earlier discussed with David. > > > > http://lists.nongnu.org/archive/html/qemu-ppc/2014-08/msg00251.html > > > > I strongly disagree with David. Legally there is no difference between a > .bin file that contains code and an array made of instructions. And the > more target code we can keep outside of QEMU the better. What do legalities have to do with it?
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 mr 4,3 lis 3,KVMPPC_H_RTAS@h ori 3,3,KVMPPC_H_RTAS@l
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 <aravinda@linux.vnet.ibm.com> --- 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(-)