Re: [Qemu-devel] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

Re: [Qemu-devel] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

[Aravinda Prasad]

On Monday 25 March 2019 11:52 AM, David Gibson wrote:
> On Fri, Mar 22, 2019 at 12:03:58PM +0530, Aravinda Prasad wrote:
>> Memory error such as bit flips that cannot be corrected
>> by hardware are passed on to the kernel for handling.
>> If the memory address in error belongs to guest then
>> the guest kernel is responsible for taking suitable action.
>> Patch [1] enhances KVM to exit guest with exit reason
>> set to KVM_EXIT_NMI in such cases. This patch handles
>> KVM_EXIT_NMI exit.
>>
>> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
>>     (e20bbd3d and related commits)
>>
>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>> ---
>>  hw/ppc/spapr_events.c  |   22 ++++++++++++++++++++++
>>  include/hw/ppc/spapr.h |    1 +
>>  target/ppc/kvm.c       |   16 ++++++++++++++++
>>  target/ppc/kvm_ppc.h   |    2 ++
>>  4 files changed, 41 insertions(+)
>>

[...]

>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>> index 2427c8e..a593448 100644
>> --- a/target/ppc/kvm.c
>> +++ b/target/ppc/kvm.c
>> @@ -1738,6 +1738,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>>          ret = 0;
>>          break;
>>  
>> +    case KVM_EXIT_NMI:
>> +        DPRINTF("handle NMI exception\n");
> 
> tracepoints are generally preferred to new DPRINTFs.

I see DPRINTFs used in all other exit reasons in this function. Do you
want me to change this particular exit case to tracepoints? I think it
is better to keep this DPRINTF as of now and change all the DPRINTFs to
tracepoints in a separate patch set.

Regards,
Aravinda

> 
>> +        ret = kvm_handle_nmi(cpu, run);
>> +        break;
>> +
>>      default:
>>          fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
>>          ret = -1;
>> @@ -2803,6 +2808,17 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
>>      return data & 0xffff;
>>  }
>>  
>> +int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run)
>> +{
>> +    bool recovered = run->flags & KVM_RUN_PPC_NMI_DISP_FULLY_RECOV;
>> +
>> +    cpu_synchronize_state(CPU(cpu));
>> +
>> +    spapr_mce_req_event(cpu, recovered);
>> +
>> +    return 0;
>> +}
>> +
>>  int kvmppc_enable_hwrng(void)
>>  {
>>      if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG)) {
>> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
>> index 2c2ea30..df5e85f 100644
>> --- a/target/ppc/kvm_ppc.h
>> +++ b/target/ppc/kvm_ppc.h
>> @@ -80,6 +80,8 @@ bool kvmppc_hpt_needs_host_contiguous_pages(void);
>>  void kvm_check_mmu(PowerPCCPU *cpu, Error **errp);
>>  void kvmppc_set_reg_ppc_online(PowerPCCPU *cpu, unsigned int online);
>>  
>> +int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run);
>> +
>>  #else
>>  
>>  static inline uint32_t kvmppc_get_tbfreq(void)
>>
> 

-- 
Regards,
Aravinda

Re: [Qemu-devel] [Qemu-ppc] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

[Greg Kurz]

On Thu, 4 Apr 2019 14:40:45 +0530
Aravinda Prasad <aravinda@linux.vnet.ibm.com> wrote:

> On Monday 25 March 2019 11:52 AM, David Gibson wrote:
> > On Fri, Mar 22, 2019 at 12:03:58PM +0530, Aravinda Prasad wrote:  
> >> Memory error such as bit flips that cannot be corrected
> >> by hardware are passed on to the kernel for handling.
> >> If the memory address in error belongs to guest then
> >> the guest kernel is responsible for taking suitable action.
> >> Patch [1] enhances KVM to exit guest with exit reason
> >> set to KVM_EXIT_NMI in such cases. This patch handles
> >> KVM_EXIT_NMI exit.
> >>
> >> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
> >>     (e20bbd3d and related commits)
> >>
> >> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr_events.c  |   22 ++++++++++++++++++++++
> >>  include/hw/ppc/spapr.h |    1 +
> >>  target/ppc/kvm.c       |   16 ++++++++++++++++
> >>  target/ppc/kvm_ppc.h   |    2 ++
> >>  4 files changed, 41 insertions(+)
> >>  
> 
> [...]
> 
> >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >> index 2427c8e..a593448 100644
> >> --- a/target/ppc/kvm.c
> >> +++ b/target/ppc/kvm.c
> >> @@ -1738,6 +1738,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >>          ret = 0;
> >>          break;
> >>  
> >> +    case KVM_EXIT_NMI:
> >> +        DPRINTF("handle NMI exception\n");  
> > 
> > tracepoints are generally preferred to new DPRINTFs.  
> 
> I see DPRINTFs used in all other exit reasons in this function. Do you
> want me to change this particular exit case to tracepoints? I think it
> is better to keep this DPRINTF as of now and change all the DPRINTFs to
> tracepoints in a separate patch set.
> 

git grep shows that all DPRINTFs (25 of them) are in target/ppc/kvm.c. I guess
this can be addressed with a single, mechanical and easy to review preparatory
patch.

> Regards,
> Aravinda
> 
> >   
> >> +        ret = kvm_handle_nmi(cpu, run);
> >> +        break;
> >> +
> >>      default:
> >>          fprintf(stderr, "KVM: unknown exit reason %d\n", run->exit_reason);
> >>          ret = -1;
> >> @@ -2803,6 +2808,17 @@ int kvm_arch_msi_data_to_gsi(uint32_t data)
> >>      return data & 0xffff;
> >>  }
> >>  
> >> +int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run)
> >> +{
> >> +    bool recovered = run->flags & KVM_RUN_PPC_NMI_DISP_FULLY_RECOV;
> >> +
> >> +    cpu_synchronize_state(CPU(cpu));
> >> +
> >> +    spapr_mce_req_event(cpu, recovered);
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>  int kvmppc_enable_hwrng(void)
> >>  {
> >>      if (!kvm_enabled() || !kvm_check_extension(kvm_state, KVM_CAP_PPC_HWRNG)) {
> >> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> >> index 2c2ea30..df5e85f 100644
> >> --- a/target/ppc/kvm_ppc.h
> >> +++ b/target/ppc/kvm_ppc.h
> >> @@ -80,6 +80,8 @@ bool kvmppc_hpt_needs_host_contiguous_pages(void);
> >>  void kvm_check_mmu(PowerPCCPU *cpu, Error **errp);
> >>  void kvmppc_set_reg_ppc_online(PowerPCCPU *cpu, unsigned int online);
> >>  
> >> +int kvm_handle_nmi(PowerPCCPU *cpu, struct kvm_run *run);
> >> +
> >>  #else
> >>  
> >>  static inline uint32_t kvmppc_get_tbfreq(void)
> >>  
> >   
> 

Re: [Qemu-devel] [PATCH v7 5/6] ppc: spapr: Enable FWNMI capability

[Fabiano Rosas]

Aravinda Prasad <aravinda@linux.vnet.ibm.com> writes:

(...)
> diff --git a/target/ppc/kvm_ppc.h b/target/ppc/kvm_ppc.h
> index df5e85f..cf7b24f 100644
> --- a/target/ppc/kvm_ppc.h
> +++ b/target/ppc/kvm_ppc.h
> @@ -27,6 +27,7 @@ void kvmppc_enable_h_page_init(void);
>  void kvmppc_set_papr(PowerPCCPU *cpu);
>  int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu);

Building for all architectures results in:

In file included from qemu/hw/ppc/ppc.c:36:
qemu/target/ppc/kvm_ppc.h:162:5: error: no previous prototype for
‘kvmppc_fwnmi_enable’ [-Werror=missing-prototypes]
 int kvmppc_fwnmi_enable(PowerPCCPU *cpu)           
     ^~~~~~~~~~~~~~~~~~~             

>  int kvmppc_smt_threads(void);
>  void kvmppc_hint_smt_possible(Error **errp);
>  int kvmppc_set_smt_threads(int smt);
> @@ -158,6 +159,11 @@ static inline void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy)
>  {
>  }
>  
> +int kvmppc_fwnmi_enable(PowerPCCPU *cpu)
> +{
> +    return 1;
> +}
> +
>  static inline int kvmppc_smt_threads(void)
>  {
>      return 1;

Re: [Qemu-devel] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

On Thu, Apr 04, 2019 at 02:40:45PM +0530, Aravinda Prasad wrote:
> 
> 
> On Monday 25 March 2019 11:52 AM, David Gibson wrote:
> > On Fri, Mar 22, 2019 at 12:03:58PM +0530, Aravinda Prasad wrote:
> >> Memory error such as bit flips that cannot be corrected
> >> by hardware are passed on to the kernel for handling.
> >> If the memory address in error belongs to guest then
> >> the guest kernel is responsible for taking suitable action.
> >> Patch [1] enhances KVM to exit guest with exit reason
> >> set to KVM_EXIT_NMI in such cases. This patch handles
> >> KVM_EXIT_NMI exit.
> >>
> >> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
> >>     (e20bbd3d and related commits)
> >>
> >> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr_events.c  |   22 ++++++++++++++++++++++
> >>  include/hw/ppc/spapr.h |    1 +
> >>  target/ppc/kvm.c       |   16 ++++++++++++++++
> >>  target/ppc/kvm_ppc.h   |    2 ++
> >>  4 files changed, 41 insertions(+)
> >>
> 
> [...]
> 
> >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >> index 2427c8e..a593448 100644
> >> --- a/target/ppc/kvm.c
> >> +++ b/target/ppc/kvm.c
> >> @@ -1738,6 +1738,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >>          ret = 0;
> >>          break;
> >>  
> >> +    case KVM_EXIT_NMI:
> >> +        DPRINTF("handle NMI exception\n");
> > 
> > tracepoints are generally preferred to new DPRINTFs.
> 
> I see DPRINTFs used in all other exit reasons in this function. Do you
> want me to change this particular exit case to tracepoints? I think it
> is better to keep this DPRINTF as of now and change all the DPRINTFs to
> tracepoints in a separate patch set.

Ah, good point.  Tracepoints are generally preferred, but since
DPRINTFs are in use here, stick with that (at some point it would be
good to change the whole file, but that's out of scope here).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

On Thu, Apr 04, 2019 at 02:40:45PM +0530, Aravinda Prasad wrote:
> 
> 
> On Monday 25 March 2019 11:52 AM, David Gibson wrote:
> > On Fri, Mar 22, 2019 at 12:03:58PM +0530, Aravinda Prasad wrote:
> >> Memory error such as bit flips that cannot be corrected
> >> by hardware are passed on to the kernel for handling.
> >> If the memory address in error belongs to guest then
> >> the guest kernel is responsible for taking suitable action.
> >> Patch [1] enhances KVM to exit guest with exit reason
> >> set to KVM_EXIT_NMI in such cases. This patch handles
> >> KVM_EXIT_NMI exit.
> >>
> >> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
> >>     (e20bbd3d and related commits)
> >>
> >> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> >> ---
> >>  hw/ppc/spapr_events.c  |   22 ++++++++++++++++++++++
> >>  include/hw/ppc/spapr.h |    1 +
> >>  target/ppc/kvm.c       |   16 ++++++++++++++++
> >>  target/ppc/kvm_ppc.h   |    2 ++
> >>  4 files changed, 41 insertions(+)
> >>
> 
> [...]
> 
> >> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >> index 2427c8e..a593448 100644
> >> --- a/target/ppc/kvm.c
> >> +++ b/target/ppc/kvm.c
> >> @@ -1738,6 +1738,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >>          ret = 0;
> >>          break;
> >>  
> >> +    case KVM_EXIT_NMI:
> >> +        DPRINTF("handle NMI exception\n");
> > 
> > tracepoints are generally preferred to new DPRINTFs.
> 
> I see DPRINTFs used in all other exit reasons in this function. Do you
> want me to change this particular exit case to tracepoints? I think it
> is better to keep this DPRINTF as of now and change all the DPRINTFs to
> tracepoints in a separate patch set.

Ah, good point.  Tracepoints are generally preferred, but since
DPRINTFs are in use here, stick with that (at some point it would be
good to change the whole file, but that's out of scope here).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

[Alexey Kardashevskiy]

On 05/04/2019 10:17, David Gibson wrote:
> On Thu, Apr 04, 2019 at 02:40:45PM +0530, Aravinda Prasad wrote:
>>
>>
>> On Monday 25 March 2019 11:52 AM, David Gibson wrote:
>>> On Fri, Mar 22, 2019 at 12:03:58PM +0530, Aravinda Prasad wrote:
>>>> Memory error such as bit flips that cannot be corrected
>>>> by hardware are passed on to the kernel for handling.
>>>> If the memory address in error belongs to guest then
>>>> the guest kernel is responsible for taking suitable action.
>>>> Patch [1] enhances KVM to exit guest with exit reason
>>>> set to KVM_EXIT_NMI in such cases. This patch handles
>>>> KVM_EXIT_NMI exit.
>>>>
>>>> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
>>>>     (e20bbd3d and related commits)
>>>>
>>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/ppc/spapr_events.c  |   22 ++++++++++++++++++++++
>>>>  include/hw/ppc/spapr.h |    1 +
>>>>  target/ppc/kvm.c       |   16 ++++++++++++++++
>>>>  target/ppc/kvm_ppc.h   |    2 ++
>>>>  4 files changed, 41 insertions(+)
>>>>
>>
>> [...]
>>
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index 2427c8e..a593448 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -1738,6 +1738,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>>>>          ret = 0;
>>>>          break;
>>>>  
>>>> +    case KVM_EXIT_NMI:
>>>> +        DPRINTF("handle NMI exception\n");
>>>
>>> tracepoints are generally preferred to new DPRINTFs.
>>
>> I see DPRINTFs used in all other exit reasons in this function. Do you
>> want me to change this particular exit case to tracepoints? I think it
>> is better to keep this DPRINTF as of now and change all the DPRINTFs to
>> tracepoints in a separate patch set.
> 
> Ah, good point. 

imho not. The kvm.c already knows about traces (there are two) and even
if every other trace in kvm_arch_handle_exit() is DPRINTF (enabled all
at once), having at least one which can be enabled without QEMU
recompile and separately from the others is a small but nice bonus
before someone gets rid of DPRINTF.


> Tracepoints are generally preferred, but since
> DPRINTFs are in use here, stick with that (at some point it would be
> good to change the whole file, but that's out of scope here).


-- 
Alexey Kardashevskiy
IBM OzLabs, LTC Team

e-mail: aik@linux.ibm.com

Re: [Qemu-devel] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

[Alexey Kardashevskiy]

On 05/04/2019 10:17, David Gibson wrote:
> On Thu, Apr 04, 2019 at 02:40:45PM +0530, Aravinda Prasad wrote:
>>
>>
>> On Monday 25 March 2019 11:52 AM, David Gibson wrote:
>>> On Fri, Mar 22, 2019 at 12:03:58PM +0530, Aravinda Prasad wrote:
>>>> Memory error such as bit flips that cannot be corrected
>>>> by hardware are passed on to the kernel for handling.
>>>> If the memory address in error belongs to guest then
>>>> the guest kernel is responsible for taking suitable action.
>>>> Patch [1] enhances KVM to exit guest with exit reason
>>>> set to KVM_EXIT_NMI in such cases. This patch handles
>>>> KVM_EXIT_NMI exit.
>>>>
>>>> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
>>>>     (e20bbd3d and related commits)
>>>>
>>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/ppc/spapr_events.c  |   22 ++++++++++++++++++++++
>>>>  include/hw/ppc/spapr.h |    1 +
>>>>  target/ppc/kvm.c       |   16 ++++++++++++++++
>>>>  target/ppc/kvm_ppc.h   |    2 ++
>>>>  4 files changed, 41 insertions(+)
>>>>
>>
>> [...]
>>
>>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
>>>> index 2427c8e..a593448 100644
>>>> --- a/target/ppc/kvm.c
>>>> +++ b/target/ppc/kvm.c
>>>> @@ -1738,6 +1738,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
>>>>          ret = 0;
>>>>          break;
>>>>  
>>>> +    case KVM_EXIT_NMI:
>>>> +        DPRINTF("handle NMI exception\n");
>>>
>>> tracepoints are generally preferred to new DPRINTFs.
>>
>> I see DPRINTFs used in all other exit reasons in this function. Do you
>> want me to change this particular exit case to tracepoints? I think it
>> is better to keep this DPRINTF as of now and change all the DPRINTFs to
>> tracepoints in a separate patch set.
> 
> Ah, good point. 

imho not. The kvm.c already knows about traces (there are two) and even
if every other trace in kvm_arch_handle_exit() is DPRINTF (enabled all
at once), having at least one which can be enabled without QEMU
recompile and separately from the others is a small but nice bonus
before someone gets rid of DPRINTF.


> Tracepoints are generally preferred, but since
> DPRINTFs are in use here, stick with that (at some point it would be
> good to change the whole file, but that's out of scope here).


-- 
Alexey Kardashevskiy
IBM OzLabs, LTC Team

e-mail: aik@linux.ibm.com

Re: [Qemu-devel] [Qemu-ppc] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

[Greg Kurz]

On Fri, 5 Apr 2019 16:04:27 +1100
Alexey Kardashevskiy <aik@linux.ibm.com> wrote:

> On 05/04/2019 10:17, David Gibson wrote:
> > On Thu, Apr 04, 2019 at 02:40:45PM +0530, Aravinda Prasad wrote:  
> >>
> >>
> >> On Monday 25 March 2019 11:52 AM, David Gibson wrote:  
> >>> On Fri, Mar 22, 2019 at 12:03:58PM +0530, Aravinda Prasad wrote:  
> >>>> Memory error such as bit flips that cannot be corrected
> >>>> by hardware are passed on to the kernel for handling.
> >>>> If the memory address in error belongs to guest then
> >>>> the guest kernel is responsible for taking suitable action.
> >>>> Patch [1] enhances KVM to exit guest with exit reason
> >>>> set to KVM_EXIT_NMI in such cases. This patch handles
> >>>> KVM_EXIT_NMI exit.
> >>>>
> >>>> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
> >>>>     (e20bbd3d and related commits)
> >>>>
> >>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> >>>> ---
> >>>>  hw/ppc/spapr_events.c  |   22 ++++++++++++++++++++++
> >>>>  include/hw/ppc/spapr.h |    1 +
> >>>>  target/ppc/kvm.c       |   16 ++++++++++++++++
> >>>>  target/ppc/kvm_ppc.h   |    2 ++
> >>>>  4 files changed, 41 insertions(+)
> >>>>  
> >>
> >> [...]
> >>  
> >>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >>>> index 2427c8e..a593448 100644
> >>>> --- a/target/ppc/kvm.c
> >>>> +++ b/target/ppc/kvm.c
> >>>> @@ -1738,6 +1738,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >>>>          ret = 0;
> >>>>          break;
> >>>>  
> >>>> +    case KVM_EXIT_NMI:
> >>>> +        DPRINTF("handle NMI exception\n");  
> >>>
> >>> tracepoints are generally preferred to new DPRINTFs.  
> >>
> >> I see DPRINTFs used in all other exit reasons in this function. Do you
> >> want me to change this particular exit case to tracepoints? I think it
> >> is better to keep this DPRINTF as of now and change all the DPRINTFs to
> >> tracepoints in a separate patch set.  
> > 
> > Ah, good point.   
> 
> imho not. The kvm.c already knows about traces (there are two) and even
> if every other trace in kvm_arch_handle_exit() is DPRINTF (enabled all
> at once), having at least one which can be enabled without QEMU
> recompile and separately from the others is a small but nice bonus
> before someone gets rid of DPRINTF.
> 

Done.

https://patchwork.ozlabs.org/project/qemu-devel/list/?series=101141

> 
> > Tracepoints are generally preferred, but since
> > DPRINTFs are in use here, stick with that (at some point it would be
> > good to change the whole file, but that's out of scope here).  
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v7 3/6] target/ppc: Handle NMI guest exit

[Greg Kurz]

On Fri, 5 Apr 2019 16:04:27 +1100
Alexey Kardashevskiy <aik@linux.ibm.com> wrote:

> On 05/04/2019 10:17, David Gibson wrote:
> > On Thu, Apr 04, 2019 at 02:40:45PM +0530, Aravinda Prasad wrote:  
> >>
> >>
> >> On Monday 25 March 2019 11:52 AM, David Gibson wrote:  
> >>> On Fri, Mar 22, 2019 at 12:03:58PM +0530, Aravinda Prasad wrote:  
> >>>> Memory error such as bit flips that cannot be corrected
> >>>> by hardware are passed on to the kernel for handling.
> >>>> If the memory address in error belongs to guest then
> >>>> the guest kernel is responsible for taking suitable action.
> >>>> Patch [1] enhances KVM to exit guest with exit reason
> >>>> set to KVM_EXIT_NMI in such cases. This patch handles
> >>>> KVM_EXIT_NMI exit.
> >>>>
> >>>> [1] https://www.spinics.net/lists/kvm-ppc/msg12637.html
> >>>>     (e20bbd3d and related commits)
> >>>>
> >>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> >>>> ---
> >>>>  hw/ppc/spapr_events.c  |   22 ++++++++++++++++++++++
> >>>>  include/hw/ppc/spapr.h |    1 +
> >>>>  target/ppc/kvm.c       |   16 ++++++++++++++++
> >>>>  target/ppc/kvm_ppc.h   |    2 ++
> >>>>  4 files changed, 41 insertions(+)
> >>>>  
> >>
> >> [...]
> >>  
> >>>> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> >>>> index 2427c8e..a593448 100644
> >>>> --- a/target/ppc/kvm.c
> >>>> +++ b/target/ppc/kvm.c
> >>>> @@ -1738,6 +1738,11 @@ int kvm_arch_handle_exit(CPUState *cs, struct kvm_run *run)
> >>>>          ret = 0;
> >>>>          break;
> >>>>  
> >>>> +    case KVM_EXIT_NMI:
> >>>> +        DPRINTF("handle NMI exception\n");  
> >>>
> >>> tracepoints are generally preferred to new DPRINTFs.  
> >>
> >> I see DPRINTFs used in all other exit reasons in this function. Do you
> >> want me to change this particular exit case to tracepoints? I think it
> >> is better to keep this DPRINTF as of now and change all the DPRINTFs to
> >> tracepoints in a separate patch set.  
> > 
> > Ah, good point.   
> 
> imho not. The kvm.c already knows about traces (there are two) and even
> if every other trace in kvm_arch_handle_exit() is DPRINTF (enabled all
> at once), having at least one which can be enabled without QEMU
> recompile and separately from the others is a small but nice bonus
> before someone gets rid of DPRINTF.
> 

Done.

https://patchwork.ozlabs.org/project/qemu-devel/list/?series=101141

> 
> > Tracepoints are generally preferred, but since
> > DPRINTFs are in use here, stick with that (at some point it would be
> > good to change the whole file, but that's out of scope here).  
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v7 5/6] ppc: spapr: Enable FWNMI capability

[Aravinda Prasad]

On Tuesday 26 March 2019 05:03 AM, David Gibson wrote:
> On Mon, Mar 25, 2019 at 02:27:45PM +0530, Aravinda Prasad wrote:
>>
>>
>> On Monday 25 March 2019 12:02 PM, David Gibson wrote:
>>> On Fri, Mar 22, 2019 at 12:04:16PM +0530, Aravinda Prasad wrote:
>>>> Enable the KVM capability KVM_CAP_PPC_FWNMI so that
>>>> the KVM causes guest exit with NMI as exit reason
>>>> when it encounters a machine check exception on the
>>>> address belonging to a guest. Without this capability
>>>> enabled, KVM redirects machine check exceptions to
>>>> guest's 0x200 vector.
>>>>
>>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/ppc/spapr_rtas.c  |   15 +++++++++++++++
>>>>  target/ppc/kvm.c     |   14 ++++++++++++++
>>>>  target/ppc/kvm_ppc.h |    6 ++++++
>>>>  3 files changed, 35 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>> index fb594a4..939f428 100644
>>>> --- a/hw/ppc/spapr_rtas.c
>>>> +++ b/hw/ppc/spapr_rtas.c
>>>> @@ -49,6 +49,7 @@
>>>>  #include "hw/ppc/fdt.h"
>>>>  #include "target/ppc/mmu-hash64.h"
>>>>  #include "target/ppc/mmu-book3s-v3.h"
>>>> +#include "kvm_ppc.h"
>>>>  
>>>>  static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>>>                                     uint32_t token, uint32_t nargs,
>>>> @@ -354,6 +355,20 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
>>>>                                    target_ulong args,
>>>>                                    uint32_t nret, target_ulong rets)
>>>>  {
>>>> +    int ret;
>>>> +
>>>> +    ret = kvmppc_fwnmi_enable(cpu);
>>>> +
>>>> +    if (ret == 1) {
>>>> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
>>>
>>> Urgh, we're here making a guest visible different to the environment
>>> depending on a host (KVM) capability.  What happens if you start a
>>> guest and it registers fwnmi support, then migrate it to a host that
>>> lacks the necessary KVM support?
>>
>> I just checked how such scenarios are handled for other KVM
>> capabilities. Should I need to add an Spapr cap for this?
> 
> Yes, I think that's what we'll need to do.

I was looking into SPAPR Cap, and I am not sure that the following code
will help in handling the migration. I need some help here.

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index edc5ed0..ef96192 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -473,6 +473,15 @@ static void cap_ccf_assist_apply(SpaprMachineState
*spapr, uint8_t val,
     }
 }

+static void cap_machine_check_apply(SpaprMachineState *spapr, uint8_t val,
+                                    Error **errp)
+{
+    if (kvm_enabled()) {
+        if (kvmppc_fwnmi_enable(cpu)) {
+            error_setg(errp, "Unable to enable fwnmi capability");
+    }
+}
+
 SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     [SPAPR_CAP_HTM] = {
         .name = "htm",
@@ -571,6 +580,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
         .type = "bool",
         .apply = cap_ccf_assist_apply,
     },
+    [SPAPR_CAP_MACHINE_CHECK] = {
+        .name = "machine-check",
+        .description = "Handle machine check exceptions",
+        .index = SPAPR_CAP_MACHINE_CHECK,
+        .get = spapr_cap_get_bool,
+        .set = spapr_cap_set_bool,
+        .type = "bool",
+        .apply = cap_machine_check_apply,
+    },
 };

 static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
@@ -706,6 +724,7 @@ SPAPR_CAP_MIG_STATE(ibs, SPAPR_CAP_IBS);
 SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
 SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
 SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
+SPAPR_CAP_MIG_STATE(mce, SPAPR_CAP_MACHINE_CHECK);

 void spapr_caps_init(SpaprMachineState *spapr)
 {


Or is it that just adding SPAPR_CAP_MIG_STATE(mce,
SPAPR_CAP_MACHINE_CHECK); is enough as it is checking or cap values?

> 

-- 
Regards,
Aravinda

Re: [Qemu-devel] [Qemu-ppc] [PATCH v7 5/6] ppc: spapr: Enable FWNMI capability

[Aravinda Prasad]

On Tuesday 26 March 2019 05:03 AM, David Gibson wrote:
> On Mon, Mar 25, 2019 at 02:27:45PM +0530, Aravinda Prasad wrote:
>>
>>
>> On Monday 25 March 2019 12:02 PM, David Gibson wrote:
>>> On Fri, Mar 22, 2019 at 12:04:16PM +0530, Aravinda Prasad wrote:
>>>> Enable the KVM capability KVM_CAP_PPC_FWNMI so that
>>>> the KVM causes guest exit with NMI as exit reason
>>>> when it encounters a machine check exception on the
>>>> address belonging to a guest. Without this capability
>>>> enabled, KVM redirects machine check exceptions to
>>>> guest's 0x200 vector.
>>>>
>>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
>>>> ---
>>>>  hw/ppc/spapr_rtas.c  |   15 +++++++++++++++
>>>>  target/ppc/kvm.c     |   14 ++++++++++++++
>>>>  target/ppc/kvm_ppc.h |    6 ++++++
>>>>  3 files changed, 35 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
>>>> index fb594a4..939f428 100644
>>>> --- a/hw/ppc/spapr_rtas.c
>>>> +++ b/hw/ppc/spapr_rtas.c
>>>> @@ -49,6 +49,7 @@
>>>>  #include "hw/ppc/fdt.h"
>>>>  #include "target/ppc/mmu-hash64.h"
>>>>  #include "target/ppc/mmu-book3s-v3.h"
>>>> +#include "kvm_ppc.h"
>>>>  
>>>>  static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState *spapr,
>>>>                                     uint32_t token, uint32_t nargs,
>>>> @@ -354,6 +355,20 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
>>>>                                    target_ulong args,
>>>>                                    uint32_t nret, target_ulong rets)
>>>>  {
>>>> +    int ret;
>>>> +
>>>> +    ret = kvmppc_fwnmi_enable(cpu);
>>>> +
>>>> +    if (ret == 1) {
>>>> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
>>>
>>> Urgh, we're here making a guest visible different to the environment
>>> depending on a host (KVM) capability.  What happens if you start a
>>> guest and it registers fwnmi support, then migrate it to a host that
>>> lacks the necessary KVM support?
>>
>> I just checked how such scenarios are handled for other KVM
>> capabilities. Should I need to add an Spapr cap for this?
> 
> Yes, I think that's what we'll need to do.

I was looking into SPAPR Cap, and I am not sure that the following code
will help in handling the migration. I need some help here.

diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
index edc5ed0..ef96192 100644
--- a/hw/ppc/spapr_caps.c
+++ b/hw/ppc/spapr_caps.c
@@ -473,6 +473,15 @@ static void cap_ccf_assist_apply(SpaprMachineState
*spapr, uint8_t val,
     }
 }

+static void cap_machine_check_apply(SpaprMachineState *spapr, uint8_t val,
+                                    Error **errp)
+{
+    if (kvm_enabled()) {
+        if (kvmppc_fwnmi_enable(cpu)) {
+            error_setg(errp, "Unable to enable fwnmi capability");
+    }
+}
+
 SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
     [SPAPR_CAP_HTM] = {
         .name = "htm",
@@ -571,6 +580,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
         .type = "bool",
         .apply = cap_ccf_assist_apply,
     },
+    [SPAPR_CAP_MACHINE_CHECK] = {
+        .name = "machine-check",
+        .description = "Handle machine check exceptions",
+        .index = SPAPR_CAP_MACHINE_CHECK,
+        .get = spapr_cap_get_bool,
+        .set = spapr_cap_set_bool,
+        .type = "bool",
+        .apply = cap_machine_check_apply,
+    },
 };

 static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
@@ -706,6 +724,7 @@ SPAPR_CAP_MIG_STATE(ibs, SPAPR_CAP_IBS);
 SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
 SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
 SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
+SPAPR_CAP_MIG_STATE(mce, SPAPR_CAP_MACHINE_CHECK);

 void spapr_caps_init(SpaprMachineState *spapr)
 {


Or is it that just adding SPAPR_CAP_MIG_STATE(mce,
SPAPR_CAP_MACHINE_CHECK); is enough as it is checking or cap values?

> 

-- 
Regards,
Aravinda

Re: [Qemu-devel] [Qemu-ppc] [PATCH v7 5/6] ppc: spapr: Enable FWNMI capability

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4856 bytes --]

On Mon, Apr 15, 2019 at 03:42:46PM +0530, Aravinda Prasad wrote:
> 
> 
> On Tuesday 26 March 2019 05:03 AM, David Gibson wrote:
> > On Mon, Mar 25, 2019 at 02:27:45PM +0530, Aravinda Prasad wrote:
> >>
> >>
> >> On Monday 25 March 2019 12:02 PM, David Gibson wrote:
> >>> On Fri, Mar 22, 2019 at 12:04:16PM +0530, Aravinda Prasad wrote:
> >>>> Enable the KVM capability KVM_CAP_PPC_FWNMI so that
> >>>> the KVM causes guest exit with NMI as exit reason
> >>>> when it encounters a machine check exception on the
> >>>> address belonging to a guest. Without this capability
> >>>> enabled, KVM redirects machine check exceptions to
> >>>> guest's 0x200 vector.
> >>>>
> >>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> >>>> ---
> >>>>  hw/ppc/spapr_rtas.c  |   15 +++++++++++++++
> >>>>  target/ppc/kvm.c     |   14 ++++++++++++++
> >>>>  target/ppc/kvm_ppc.h |    6 ++++++
> >>>>  3 files changed, 35 insertions(+)
> >>>>
> >>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >>>> index fb594a4..939f428 100644
> >>>> --- a/hw/ppc/spapr_rtas.c
> >>>> +++ b/hw/ppc/spapr_rtas.c
> >>>> @@ -49,6 +49,7 @@
> >>>>  #include "hw/ppc/fdt.h"
> >>>>  #include "target/ppc/mmu-hash64.h"
> >>>>  #include "target/ppc/mmu-book3s-v3.h"
> >>>> +#include "kvm_ppc.h"
> >>>>  
> >>>>  static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState *spapr,
> >>>>                                     uint32_t token, uint32_t nargs,
> >>>> @@ -354,6 +355,20 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
> >>>>                                    target_ulong args,
> >>>>                                    uint32_t nret, target_ulong rets)
> >>>>  {
> >>>> +    int ret;
> >>>> +
> >>>> +    ret = kvmppc_fwnmi_enable(cpu);
> >>>> +
> >>>> +    if (ret == 1) {
> >>>> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> >>>
> >>> Urgh, we're here making a guest visible different to the environment
> >>> depending on a host (KVM) capability.  What happens if you start a
> >>> guest and it registers fwnmi support, then migrate it to a host that
> >>> lacks the necessary KVM support?
> >>
> >> I just checked how such scenarios are handled for other KVM
> >> capabilities. Should I need to add an Spapr cap for this?
> > 
> > Yes, I think that's what we'll need to do.
> 
> I was looking into SPAPR Cap, and I am not sure that the following code
> will help in handling the migration. I need some help here.
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index edc5ed0..ef96192 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -473,6 +473,15 @@ static void cap_ccf_assist_apply(SpaprMachineState
> *spapr, uint8_t val,
>      }
>  }
> 
> +static void cap_machine_check_apply(SpaprMachineState *spapr, uint8_t val,
> +                                    Error **errp)
> +{
> +    if (kvm_enabled()) {
> +        if (kvmppc_fwnmi_enable(cpu)) {
> +            error_setg(errp, "Unable to enable fwnmi capability");
> +    }
> +}
> +
>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
>          .name = "htm",
> @@ -571,6 +580,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .apply = cap_ccf_assist_apply,
>      },
> +    [SPAPR_CAP_MACHINE_CHECK] = {
> +        .name = "machine-check",
> +        .description = "Handle machine check exceptions",
> +        .index = SPAPR_CAP_MACHINE_CHECK,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_machine_check_apply,
> +    },
>  };
> 
>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -706,6 +724,7 @@ SPAPR_CAP_MIG_STATE(ibs, SPAPR_CAP_IBS);
>  SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
> +SPAPR_CAP_MIG_STATE(mce, SPAPR_CAP_MACHINE_CHECK);
> 
>  void spapr_caps_init(SpaprMachineState *spapr)
>  {
> 
> 
> Or is it that just adding SPAPR_CAP_MIG_STATE(mce,
> SPAPR_CAP_MACHINE_CHECK); is enough as it is checking or cap values?

No, the change you have above looks mostly good.  You'll also need to
set default values for the cap in spapr_machine_class_init(), and
probably compat values in some of the earlier machine type variants.

I also don't like the cap name: there's at least some kind of handling
of machine checks before this, this is more specifically about the
fwnmi scheme, AIUI.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [Qemu-ppc] [PATCH v7 5/6] ppc: spapr: Enable FWNMI capability

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4856 bytes --]

On Mon, Apr 15, 2019 at 03:42:46PM +0530, Aravinda Prasad wrote:
> 
> 
> On Tuesday 26 March 2019 05:03 AM, David Gibson wrote:
> > On Mon, Mar 25, 2019 at 02:27:45PM +0530, Aravinda Prasad wrote:
> >>
> >>
> >> On Monday 25 March 2019 12:02 PM, David Gibson wrote:
> >>> On Fri, Mar 22, 2019 at 12:04:16PM +0530, Aravinda Prasad wrote:
> >>>> Enable the KVM capability KVM_CAP_PPC_FWNMI so that
> >>>> the KVM causes guest exit with NMI as exit reason
> >>>> when it encounters a machine check exception on the
> >>>> address belonging to a guest. Without this capability
> >>>> enabled, KVM redirects machine check exceptions to
> >>>> guest's 0x200 vector.
> >>>>
> >>>> Signed-off-by: Aravinda Prasad <aravinda@linux.vnet.ibm.com>
> >>>> ---
> >>>>  hw/ppc/spapr_rtas.c  |   15 +++++++++++++++
> >>>>  target/ppc/kvm.c     |   14 ++++++++++++++
> >>>>  target/ppc/kvm_ppc.h |    6 ++++++
> >>>>  3 files changed, 35 insertions(+)
> >>>>
> >>>> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> >>>> index fb594a4..939f428 100644
> >>>> --- a/hw/ppc/spapr_rtas.c
> >>>> +++ b/hw/ppc/spapr_rtas.c
> >>>> @@ -49,6 +49,7 @@
> >>>>  #include "hw/ppc/fdt.h"
> >>>>  #include "target/ppc/mmu-hash64.h"
> >>>>  #include "target/ppc/mmu-book3s-v3.h"
> >>>> +#include "kvm_ppc.h"
> >>>>  
> >>>>  static void rtas_display_character(PowerPCCPU *cpu, SpaprMachineState *spapr,
> >>>>                                     uint32_t token, uint32_t nargs,
> >>>> @@ -354,6 +355,20 @@ static void rtas_ibm_nmi_register(PowerPCCPU *cpu,
> >>>>                                    target_ulong args,
> >>>>                                    uint32_t nret, target_ulong rets)
> >>>>  {
> >>>> +    int ret;
> >>>> +
> >>>> +    ret = kvmppc_fwnmi_enable(cpu);
> >>>> +
> >>>> +    if (ret == 1) {
> >>>> +        rtas_st(rets, 0, RTAS_OUT_NOT_SUPPORTED);
> >>>
> >>> Urgh, we're here making a guest visible different to the environment
> >>> depending on a host (KVM) capability.  What happens if you start a
> >>> guest and it registers fwnmi support, then migrate it to a host that
> >>> lacks the necessary KVM support?
> >>
> >> I just checked how such scenarios are handled for other KVM
> >> capabilities. Should I need to add an Spapr cap for this?
> > 
> > Yes, I think that's what we'll need to do.
> 
> I was looking into SPAPR Cap, and I am not sure that the following code
> will help in handling the migration. I need some help here.
> 
> diff --git a/hw/ppc/spapr_caps.c b/hw/ppc/spapr_caps.c
> index edc5ed0..ef96192 100644
> --- a/hw/ppc/spapr_caps.c
> +++ b/hw/ppc/spapr_caps.c
> @@ -473,6 +473,15 @@ static void cap_ccf_assist_apply(SpaprMachineState
> *spapr, uint8_t val,
>      }
>  }
> 
> +static void cap_machine_check_apply(SpaprMachineState *spapr, uint8_t val,
> +                                    Error **errp)
> +{
> +    if (kvm_enabled()) {
> +        if (kvmppc_fwnmi_enable(cpu)) {
> +            error_setg(errp, "Unable to enable fwnmi capability");
> +    }
> +}
> +
>  SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>      [SPAPR_CAP_HTM] = {
>          .name = "htm",
> @@ -571,6 +580,15 @@ SpaprCapabilityInfo capability_table[SPAPR_CAP_NUM] = {
>          .type = "bool",
>          .apply = cap_ccf_assist_apply,
>      },
> +    [SPAPR_CAP_MACHINE_CHECK] = {
> +        .name = "machine-check",
> +        .description = "Handle machine check exceptions",
> +        .index = SPAPR_CAP_MACHINE_CHECK,
> +        .get = spapr_cap_get_bool,
> +        .set = spapr_cap_set_bool,
> +        .type = "bool",
> +        .apply = cap_machine_check_apply,
> +    },
>  };
> 
>  static SpaprCapabilities default_caps_with_cpu(SpaprMachineState *spapr,
> @@ -706,6 +724,7 @@ SPAPR_CAP_MIG_STATE(ibs, SPAPR_CAP_IBS);
>  SPAPR_CAP_MIG_STATE(nested_kvm_hv, SPAPR_CAP_NESTED_KVM_HV);
>  SPAPR_CAP_MIG_STATE(large_decr, SPAPR_CAP_LARGE_DECREMENTER);
>  SPAPR_CAP_MIG_STATE(ccf_assist, SPAPR_CAP_CCF_ASSIST);
> +SPAPR_CAP_MIG_STATE(mce, SPAPR_CAP_MACHINE_CHECK);
> 
>  void spapr_caps_init(SpaprMachineState *spapr)
>  {
> 
> 
> Or is it that just adding SPAPR_CAP_MIG_STATE(mce,
> SPAPR_CAP_MACHINE_CHECK); is enough as it is checking or cap values?

No, the change you have above looks mostly good.  You'll also need to
set default values for the cap in spapr_machine_class_init(), and
probably compat values in some of the earlier machine type variants.

I also don't like the cap name: there's at least some kind of handling
of machine checks before this, this is more specifically about the
fwnmi scheme, AIUI.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]