Re: [Qemu-devel] [PATCH v2] ppc: allow certain HV interrupts to be delivered to guests

[Cédric Le Goater]

On 10/21/2016 06:35 AM, Nicholas Piggin wrote:
> On Fri, 21 Oct 2016 12:09:54 +1100
> David Gibson <address@hidden> wrote:
> 
>> On Fri, Oct 21, 2016 at 12:40:58AM +1100, Nicholas Piggin wrote:
>>> On Thu, 20 Oct 2016 15:08:07 +0200
>>> Cédric Le Goater <address@hidden> wrote:
>>>   
>>>> On 10/20/2016 08:59 AM, Nicholas Piggin wrote:  
>>>>> Signed-off-by: Nicholas Piggin <address@hidden>
>>>>> ---
>>>>>  target-ppc/excp_helper.c | 8 ++++++--
>>>>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
>>>>> index 53c4075..477af10 100644
>>>>> --- a/target-ppc/excp_helper.c
>>>>> +++ b/target-ppc/excp_helper.c
>>>>> @@ -390,9 +390,13 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
>>>>> excp_model, int excp)
>>>>>              /* indicate that we resumed from power save mode */
>>>>>              msr |= 0x10000;
>>>>>              new_msr |= ((target_ulong)1 << MSR_ME);
>>>>> +            new_msr |= (target_ulong)MSR_HVB;
>>>>> +        } else {
>>>>> +     /* The ISA specifies the HV bit is set when the hardware interrupt
>>>>> +      * is raised, however when hypervisors deliver the exception to
>>>>> +      * guests, it should not be set.
>>>>> +      */
>>>>>          }
>>>>> -
>>>>> -        new_msr |= (target_ulong)MSR_HVB;
>>>>>          ail = 0;
>>>>>          break;
>>>>>      case POWERPC_EXCP_DSEG:      /* Data segment exception               
>>>>>     */
>>>>>     
>>>>
>>>> should not that be cleared later on in powerpc_excp() by :
>>>>
>>>>     env->msr = new_msr & env->msr_mask;
>>>>
>>>> ? but the routine is rather long so I might be missing a branch.  
>>>
>>> No you're right, so it can't leak into the guest, phew!
>>>
>>> The problem I get is the interrupt code doing some things differently
>>> depending on on the HV bit. For example what I noticed is the guest
>>> losing its LE bit upon entry.
>>>
>>> Perhaps a cleaner way is for the system reset case to set new_msr
>>> according to the ISA, and then apply the msr_mask (or at least mask
>>> out HV) before calculating the exception model? Any preference?  
>>
>> I think the proposed revision makes sense.
>>
> 
> What do you think of this version? This fixes up machine check guest
> delivery as well. I'm sending this ahead of the new hcall patch, because
> it's a bugfix for existing code. I'll get around to the hcall again next
> week.
> 
> Thanks,
> Nick
> 
> 
> ppc hypervisors have delivered system reset and machine check exception
> interrupts to guests in some situations (e.g., see FWNMI feature of LoPAPR,
> or NMI injection in QEMU).
> 
> These exceptions are architected to set the HV bit in hardware, however
> when injected into a guest, the HV bit should be cleared. Current code
> masks off the HV bit before setting the new MSR, however this happens after
> the interrupt delivery model has calculated delivery mode for the exception.
> This can result in the guest's MSR LE bit being lost.
> 
> Provide a new flag for HV exceptions to allow delivery to guests. The
> exception model masks out the HV bit.
> 
> Also add another sanity check to ensure other such exceptions don't try
> to set HV in guest without setting guest_hv_excp
> 
> Signed-off-by: Nicholas Piggin <address@hidden>

Looks good to me and I gave it a try on the pnv platform which runs in
HV mode.

Reviewed-by: Cédric Le Goater <address@hidden>

Thanks,

C. 

> ---
>  target-ppc/excp_helper.c | 25 ++++++++++++++++++++++---
>  1 file changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
> index 53c4075..1b18433 100644
> --- a/target-ppc/excp_helper.c
> +++ b/target-ppc/excp_helper.c
> @@ -77,7 +77,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      target_ulong msr, new_msr, vector;
> -    int srr0, srr1, asrr0, asrr1, lev, ail;
> +    int srr0, srr1, asrr0, asrr1, lev, ail, guest_hv_excp;
>      bool lpes0;
>  
>      qemu_log_mask(CPU_LOG_INT, "Raise exception at " TARGET_FMT_lx
> @@ -149,6 +149,10 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>       *
>       * AIL is initialized here but can be cleared by
>       * selected exceptions
> +     *
> +     * guest_hv_excp is a provision to raise HV architected
> +     * exceptions in guests by delivering them with HV bit
> +     * clear. System reset and machine check use this.
>       */
>  #if defined(TARGET_PPC64)
>      if (excp_model == POWERPC_EXCP_POWER7 ||
> @@ -165,6 +169,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>          lpes0 = true;
>          ail = 0;
>      }
> +    guest_hv_excp = 0;
>  
>      /* Hypervisor emulation assistance interrupt only exists on server
>       * arch 2.05 server or later. We also don't want to generate it if
> @@ -214,6 +219,7 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>              cs->interrupt_request |= CPU_INTERRUPT_EXITTB;
>          }
>          new_msr |= (target_ulong)MSR_HVB;
> +        guest_hv_excp = 1;
>          ail = 0;
>  
>          /* machine check exceptions don't have ME set */
> @@ -391,8 +397,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>              msr |= 0x10000;
>              new_msr |= ((target_ulong)1 << MSR_ME);
>          }
> -
>          new_msr |= (target_ulong)MSR_HVB;
> +        guest_hv_excp = 1;
>          ail = 0;
>          break;
>      case POWERPC_EXCP_DSEG:      /* Data segment exception                   
> */
> @@ -610,10 +616,23 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>  
>      /* Sanity check */
>      if (!(env->msr_mask & MSR_HVB) && (srr0 == SPR_HSRR0)) {
> -        cpu_abort(cs, "Trying to deliver HV exception %d with "
> +        cpu_abort(cs, "Trying to deliver HV exception (HSRR) %d with "
>                    "no HV support\n", excp);
>      }
>  
> +    /* The ISA specifies the HV bit is set when the hardware interrupt
> +     * is raised, however in some cases hypervisors deliver the exception
> +     * to guests. HV should be cleared in that case.
> +     */
> +    if (!(env->msr_mask & MSR_HVB) && (new_msr & MSR_HVB)) {
> +        if (guest_hv_excp) {
> +            new_msr &= ~MSR_HVB;
> +        } else {
> +            cpu_abort(cs, "Trying to deliver HV exception (MSR) %d with "
> +                      "no HV support\n", excp);
> +        }
> +    }
> +
>      /* If any alternate SRR register are defined, duplicate saved values */
>      if (asrr0 != -1) {
>          env->spr[asrr0] = env->spr[srr0];
>