Re: [Qemu-devel] [Qemu-ppc] [PATCH 4/5] target-ppc: Handle ibm, nmi-register RTAS call

[Alexander Graf <agraf@suse.de>]

On 06.10.14 08:32, Aravinda Prasad wrote:
> 
> 
> On Monday 08 September 2014 02:17 AM, Alexander Graf wrote:
>>
>>
>> On 25.08.14 15:45, Aravinda Prasad wrote:
>>> This patch adds FWNMI support in qemu for powerKVM
>>> guests by handling the ibm,nmi-register rtas call.
>>> Whenever OS issues ibm,nmi-register RTAS call, the
>>> machine check notification address is saved and the
>>> machine check interrupt vector 0x200 is patched to
>>> issue a private hcall.
>>>
>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>>> ---
>>>  hw/ppc/spapr_rtas.c    |   91 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  include/hw/ppc/spapr.h |    8 ++++
>>>  2 files changed, 98 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>> index 02ddbf9..1135d2b 100644
>>> --- a/hw/ppc/spapr_rtas.c
>>> +++ b/hw/ppc/spapr_rtas.c
>>> @@ -277,6 +277,91 @@ static void rtas_ibm_set_system_parameter(PowerPCCPU *cpu,
>>>      rtas_st(rets, 0, ret);
>>>  }
>>>  
>>> +static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
>>> +                                  sPAPREnvironment *spapr,
>>> +                                  uint32_t token, uint32_t nargs,
>>> +                                  target_ulong args,
>>> +                                  uint32_t nret, target_ulong rets)
>>> +{
>>> +    int i;
>>> +    uint32_t branch_inst = 0x48000002;
>>> +    target_ulong guest_machine_check_addr;
>>> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>> +    /*
>>> +     * Trampoline saves r3 in sprg2 and issues private hcall
>>> +     * to request qemu to build error log. QEMU builds the
>>> +     * error log, copies to rtas-blob and returns the address.
>>> +     * The initial 16 bytes in rtas-blob consists of saved srr0
>>> +     * and srr1 which we restore and pass on the actual error
>>> +     * log address to OS handled mcachine check notification
>>> +     * routine
>>> +     */
>>> +    uint32_t trampoline[] = {
>>> +        0x7c7243a6,    /* mtspr   SPRN_SPRG2,r3 */
>>> +        0x38600000,    /* li      r3,0   */
>>> +        /* 0xf004 is the KVMPPC_H_REPORT_ERR private HCALL */
>>> +        0x6063f004,    /* ori     r3,r3,f004  */
>>> +        /* Issue H_CALL */
>>> +        0x44000022,    /*  sc      1     */
>>> +        0x7c9243a6,    /* mtspr r4 sprg2 */
>>> +        0xe8830000,    /* ld r4, 0(r3) */
>>> +        0x7c9a03a6,    /* mtspr r4, srr0 */
>>> +        0xe8830008,    /* ld r4, 8(r3) */
>>> +        0x7c9b03a6,    /* mtspr r4, srr1 */
>>> +        0x38630010,    /* addi r3,r3,16 */
>>> +        0x7c9242a6,    /* mfspr r4 sprg2 */
>>> +        0x48000002,    /* Branch to address registered
>>> +                        * by OS. The branch address is
>>> +                        * patched below */
>>> +        0x48000000,    /* b . */
>>
>> So how about we just completely change the layout of the RTAS blob?
>>
>> Imagine something like the following (completely untested):
>>
>> ----
>>
>> /**** index table ****/
>> .long rtas_entry
>> .long nmi_register
>> .long nmi_register_final_branch
>> .long nmi_data
>>
>> /**** RTAS handling code ****/
>> .align 1024
>> rtas_entry:
>> 	...
>> nmi_register:
>> 	...
>> nmi_register_final_branch:
>> 	ba .
>>
>> /**** RTAS data regions ****/
>> .align 4096
>> nmi_data:
>> .long 0
>> .align 4096
>>
>> ----
>>
>> With this we should be able to have a nice hybrid between easily tunable
>> asm code and an easy to load and handle blob.
> 
> Sorry, I was out of office hence could not respond.
> 
> Yes, even I prefer something like this.
> 
> BTB, did you intend to have this in spapr-rtas.S? The spapr-rtas.S is
> compiled into a binary and is read into spapr->rtas_blob. If we want to
> have rtas-blob layout something similar to above then we may need to
> link the object file of spapr-rtas.S to QEMU so that the symbols in
> "index table" and other places could be resolved inside QEMU.
> 
> If this is fine I will include it in v3.

You can't link against the object file with QEMU, since QEMU could be
executed on an x86 machine which can't compile the rtas blob.

In my proposal above, you would maintain a table of entry points at well
known locations in the binary blob (starting from 0 is probably the
easiest).


Alex