| Message ID | 20190826034434.9590-1-aik@ozlabs.ru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | rtas: Reserve space for FWNMI log | expand |
On 26/08/2019 05.44, Alexey Kardashevskiy wrote: > The Firmware Assisted Non-Maskable Interrupts Option (FWNMI) feature > requires some space for RTAS log which is in the RTAS blob area. > > This expands the RTAS blob size to 2k. > > More details here: https://patchwork.ozlabs.org/patch/1146765/ Ouch, that sounds like an very fragile interface. Is this mandated by sPAPR that we have to put the log into the RTAS region? > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > --- > lib/libhvcall/hvcall.S | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S > index b19f6dbeff2c..adcda0dd905d 100644 > --- a/lib/libhvcall/hvcall.S > +++ b/lib/libhvcall/hvcall.S > @@ -134,6 +134,7 @@ ENTRY(hv_rtas) > ori r3,r3,KVMPPC_H_RTAS@l > HVCALL > blr > + .space 2048 - (. - hv_rtas) > .globl hv_rtas_size > hv_rtas_size: > .long . - hv_rtas; > @@ -144,6 +145,7 @@ ENTRY(hv_rtas_broken_sc1) > ori r3,r3,KVMPPC_H_RTAS@l > .long 0x7c000268 > blr > + .space 2048 - (. - hv_rtas_broken_sc1) > .globl hv_rtas_broken_sc1_size > hv_rtas_broken_sc1_size: > .long . - hv_rtas_broken_sc1; > Can you please at least add some comments to the code which explain the padding - otherwise it's pretty impossible to understand when you look at the code later. Thomas
On 26/08/2019 22:29, Thomas Huth wrote: > On 26/08/2019 05.44, Alexey Kardashevskiy wrote: >> The Firmware Assisted Non-Maskable Interrupts Option (FWNMI) feature >> requires some space for RTAS log which is in the RTAS blob area. >> >> This expands the RTAS blob size to 2k. >> >> More details here: https://patchwork.ozlabs.org/patch/1146765/ > > Ouch, that sounds like an very fragile interface. Is this mandated by > sPAPR that we have to put the log into the RTAS region? This is my understanding, yes. Aravinda can correct me if I am wrong. QEMU uses a fixed offset of 0x30 into this area to decide where the log starts. Not perfect but the frequency of RTAS changes (which a naught) assumes we are quite safe here. > >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >> --- >> lib/libhvcall/hvcall.S | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S >> index b19f6dbeff2c..adcda0dd905d 100644 >> --- a/lib/libhvcall/hvcall.S >> +++ b/lib/libhvcall/hvcall.S >> @@ -134,6 +134,7 @@ ENTRY(hv_rtas) >> ori r3,r3,KVMPPC_H_RTAS@l >> HVCALL >> blr >> + .space 2048 - (. - hv_rtas) >> .globl hv_rtas_size >> hv_rtas_size: >> .long . - hv_rtas; >> @@ -144,6 +145,7 @@ ENTRY(hv_rtas_broken_sc1) >> ori r3,r3,KVMPPC_H_RTAS@l >> .long 0x7c000268 >> blr >> + .space 2048 - (. - hv_rtas_broken_sc1) >> .globl hv_rtas_broken_sc1_size >> hv_rtas_broken_sc1_size: >> .long . - hv_rtas_broken_sc1; >> > > Can you please at least add some comments to the code which explain the > padding - otherwise it's pretty impossible to understand when you look > at the code later. Sure can do that, how about: /* * A space reserved for a RTAS log from Firmware Assisted * Non-Maskable Interrupts Option (FWNMI) feature. * * The QEMU implementation uses 0x30..0x800 for the log. */ ?
On Tue, Aug 27, 2019 at 11:39:21AM +1000, Alexey Kardashevskiy wrote: > > > On 26/08/2019 22:29, Thomas Huth wrote: > > On 26/08/2019 05.44, Alexey Kardashevskiy wrote: > > > The Firmware Assisted Non-Maskable Interrupts Option (FWNMI) feature > > > requires some space for RTAS log which is in the RTAS blob area. > > > > > > This expands the RTAS blob size to 2k. > > > > > > More details here: https://patchwork.ozlabs.org/patch/1146765/ > > > > Ouch, that sounds like an very fragile interface. Is this mandated by > > sPAPR that we have to put the log into the RTAS region? > > This is my understanding, yes. Aravinda can correct me if I am wrong. QEMU > uses a fixed offset of 0x30 into this area to decide where the log starts. > Not perfect but the frequency of RTAS changes (which a naught) assumes we > are quite safe here. This is also my understanding from Aravinda, in the grand tradition of PAPR imposing all the performance penalty of paravirtualization, with none of the abstraction of paravirtualization. I just had a look at PAPR and it's kind of complicated, but amounts to requiring this in practice. The wording implies that the RTAS space should be used. The mechanics don't exactly require this, since the guest does receive an address containing (amongst other things) the error log when it gets an NMI notification. But, AFAICT, there's not really any other option: the page at 0x7000 is reserved for FWNMI stuff, but use of that is deprecated, leaving the RTAS space as basically the only other firmware-owned piece of memory to use for this. There's no way for the guest to supply a buffer, which would be the obvious sensible way of doing this. > > > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> > > > --- > > > lib/libhvcall/hvcall.S | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S > > > index b19f6dbeff2c..adcda0dd905d 100644 > > > --- a/lib/libhvcall/hvcall.S > > > +++ b/lib/libhvcall/hvcall.S > > > @@ -134,6 +134,7 @@ ENTRY(hv_rtas) > > > ori r3,r3,KVMPPC_H_RTAS@l > > > HVCALL > > > blr > > > + .space 2048 - (. - hv_rtas) > > > .globl hv_rtas_size > > > hv_rtas_size: > > > .long . - hv_rtas; > > > @@ -144,6 +145,7 @@ ENTRY(hv_rtas_broken_sc1) > > > ori r3,r3,KVMPPC_H_RTAS@l > > > .long 0x7c000268 > > > blr > > > + .space 2048 - (. - hv_rtas_broken_sc1) > > > .globl hv_rtas_broken_sc1_size > > > hv_rtas_broken_sc1_size: > > > .long . - hv_rtas_broken_sc1; > > > > > > > Can you please at least add some comments to the code which explain the > > padding - otherwise it's pretty impossible to understand when you look > > at the code later. > > Sure can do that, how about: > > /* > * A space reserved for a RTAS log from Firmware Assisted > * Non-Maskable Interrupts Option (FWNMI) feature. > * > * The QEMU implementation uses 0x30..0x800 for the log. > */ > > ? > >
On 27/08/2019 03.39, Alexey Kardashevskiy wrote: > > > On 26/08/2019 22:29, Thomas Huth wrote: >> On 26/08/2019 05.44, Alexey Kardashevskiy wrote: >>> The Firmware Assisted Non-Maskable Interrupts Option (FWNMI) feature >>> requires some space for RTAS log which is in the RTAS blob area. >>> >>> This expands the RTAS blob size to 2k. >>> >>> More details here: https://patchwork.ozlabs.org/patch/1146765/ >> >> Ouch, that sounds like an very fragile interface. Is this mandated by >> sPAPR that we have to put the log into the RTAS region? > > > This is my understanding, yes. Aravinda can correct me if I am wrong. > QEMU uses a fixed offset of 0x30 into this area to decide where the log > starts. Not perfect but the frequency of RTAS changes (which a naught) > assumes we are quite safe here. > >> >>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>> --- >>> lib/libhvcall/hvcall.S | 2 ++ >>> 1 file changed, 2 insertions(+) >>> >>> diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S >>> index b19f6dbeff2c..adcda0dd905d 100644 >>> --- a/lib/libhvcall/hvcall.S >>> +++ b/lib/libhvcall/hvcall.S >>> @@ -134,6 +134,7 @@ ENTRY(hv_rtas) >>> ori r3,r3,KVMPPC_H_RTAS@l >>> HVCALL >>> blr >>> + .space 2048 - (. - hv_rtas) >>> .globl hv_rtas_size >>> hv_rtas_size: >>> .long . - hv_rtas; >>> @@ -144,6 +145,7 @@ ENTRY(hv_rtas_broken_sc1) >>> ori r3,r3,KVMPPC_H_RTAS@l >>> .long 0x7c000268 >>> blr >>> + .space 2048 - (. - hv_rtas_broken_sc1) >>> .globl hv_rtas_broken_sc1_size >>> hv_rtas_broken_sc1_size: >>> .long . - hv_rtas_broken_sc1; >>> >> >> Can you please at least add some comments to the code which explain the >> padding - otherwise it's pretty impossible to understand when you look >> at the code later. > > Sure can do that, how about: > > /* > * A space reserved for a RTAS log from Firmware Assisted > * Non-Maskable Interrupts Option (FWNMI) feature. > * > * The QEMU implementation uses 0x30..0x800 for the log. > */ Sounds good, yes. Thomas
On Tuesday 27 August 2019 09:47 AM, David Gibson wrote: > On Tue, Aug 27, 2019 at 11:39:21AM +1000, Alexey Kardashevskiy wrote: >> >> >> On 26/08/2019 22:29, Thomas Huth wrote: >>> On 26/08/2019 05.44, Alexey Kardashevskiy wrote: >>>> The Firmware Assisted Non-Maskable Interrupts Option (FWNMI) feature >>>> requires some space for RTAS log which is in the RTAS blob area. >>>> >>>> This expands the RTAS blob size to 2k. >>>> >>>> More details here: https://patchwork.ozlabs.org/patch/1146765/ >>> >>> Ouch, that sounds like an very fragile interface. Is this mandated by >>> sPAPR that we have to put the log into the RTAS region? >> >> This is my understanding, yes. Aravinda can correct me if I am wrong. QEMU >> uses a fixed offset of 0x30 into this area to decide where the log starts. >> Not perfect but the frequency of RTAS changes (which a naught) assumes we >> are quite safe here. Yes. QEMU uses a fixed offset 0x30 into the RTAS region, because the first few bytes of the RTAS region is used by pc-bios/spapr-rtas/spapr-rtas.S. > > This is also my understanding from Aravinda, in the grand tradition of > PAPR imposing all the performance penalty of paravirtualization, with > none of the abstraction of paravirtualization. > > I just had a look at PAPR and it's kind of complicated, but amounts to > requiring this in practice. The wording implies that the RTAS space > should be used. The mechanics don't exactly require this, since the > guest does receive an address containing (amongst other things) the > error log when it gets an NMI notification. But, AFAICT, there's not > really any other option: the page at 0x7000 is reserved for FWNMI > stuff, but use of that is deprecated, leaving the RTAS space as > basically the only other firmware-owned piece of memory to use for > this. There's no way for the guest to supply a buffer, which would be > the obvious sensible way of doing this. PAPR (section 7.3.14 FWNMI) mentions that the RTAS space should be used to save FWNMI error logs. The one that allows FWNMI error log to be placed at 0x7000 is deprecated. So RTAS space is the only option if we want to adhere to PAPR. Also the kernel checks if the error log is in the RTAS area before reading it. Regards, Aravinda > >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> >>>> --- >>>> lib/libhvcall/hvcall.S | 2 ++ >>>> 1 file changed, 2 insertions(+) >>>> >>>> diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S >>>> index b19f6dbeff2c..adcda0dd905d 100644 >>>> --- a/lib/libhvcall/hvcall.S >>>> +++ b/lib/libhvcall/hvcall.S >>>> @@ -134,6 +134,7 @@ ENTRY(hv_rtas) >>>> ori r3,r3,KVMPPC_H_RTAS@l >>>> HVCALL >>>> blr >>>> + .space 2048 - (. - hv_rtas) >>>> .globl hv_rtas_size >>>> hv_rtas_size: >>>> .long . - hv_rtas; >>>> @@ -144,6 +145,7 @@ ENTRY(hv_rtas_broken_sc1) >>>> ori r3,r3,KVMPPC_H_RTAS@l >>>> .long 0x7c000268 >>>> blr >>>> + .space 2048 - (. - hv_rtas_broken_sc1) >>>> .globl hv_rtas_broken_sc1_size >>>> hv_rtas_broken_sc1_size: >>>> .long . - hv_rtas_broken_sc1; >>>> >>> >>> Can you please at least add some comments to the code which explain the >>> padding - otherwise it's pretty impossible to understand when you look >>> at the code later. >> >> Sure can do that, how about: >> >> /* >> * A space reserved for a RTAS log from Firmware Assisted >> * Non-Maskable Interrupts Option (FWNMI) feature. >> * >> * The QEMU implementation uses 0x30..0x800 for the log. >> */ >> >> ? >> >> >
diff --git a/lib/libhvcall/hvcall.S b/lib/libhvcall/hvcall.S index b19f6dbeff2c..adcda0dd905d 100644 --- a/lib/libhvcall/hvcall.S +++ b/lib/libhvcall/hvcall.S @@ -134,6 +134,7 @@ ENTRY(hv_rtas) ori r3,r3,KVMPPC_H_RTAS@l HVCALL blr + .space 2048 - (. - hv_rtas) .globl hv_rtas_size hv_rtas_size: .long . - hv_rtas; @@ -144,6 +145,7 @@ ENTRY(hv_rtas_broken_sc1) ori r3,r3,KVMPPC_H_RTAS@l .long 0x7c000268 blr + .space 2048 - (. - hv_rtas_broken_sc1) .globl hv_rtas_broken_sc1_size hv_rtas_broken_sc1_size: .long . - hv_rtas_broken_sc1;
The Firmware Assisted Non-Maskable Interrupts Option (FWNMI) feature requires some space for RTAS log which is in the RTAS blob area. This expands the RTAS blob size to 2k. More details here: https://patchwork.ozlabs.org/patch/1146765/ Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru> --- lib/libhvcall/hvcall.S | 2 ++ 1 file changed, 2 insertions(+)