Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE
On Mon, 22 Apr 2019 12:33:26 +0530 Aravinda Prasad wrote: > Upon a machine check exception (MCE) in a guest address space, > KVM causes a guest exit to enable QEMU to build and pass the > error to the guest in the PAPR defined rtas error log format. > > This patch builds the rtas error log, copies it to the rtas_addr > and then invokes the guest registered machine check handler. The > handler in the guest takes suitable action(s) depending on the type > and criticality of the error. For example, if an error is > unrecoverable memory corruption in an application inside the > guest, then the guest kernel sends a SIGBUS to the application. > For recoverable errors, the guest performs recovery actions and > logs the error. > > Signed-off-by: Aravinda Prasad > --- > hw/ppc/spapr.c |4 + > hw/ppc/spapr_events.c | 245 > > include/hw/ppc/spapr.h |4 + > 3 files changed, 253 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 2779efe..ffd1715 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2918,6 +2918,10 @@ static void spapr_machine_init(MachineState *machine) > error_report("Could not get size of LPAR rtas '%s'", filename); > exit(1); > } > + > +/* Resize blob to accommodate error log. */ > +spapr->rtas_size = spapr_get_rtas_size(spapr->rtas_size); > + This is the only user for spapr_get_rtas_size(), which is trivial. I suggest you simply open-code it here. But also, spapr->rtas_size is a guest visible thing, "rtas-size" prop in the DT. Since existing machine types don't do that, I guess we should only use the new size if cap-fwnmi-mce=on for the sake of compatibility. > spapr->rtas_blob = g_malloc(spapr->rtas_size); > if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) { > error_report("Could not load LPAR rtas '%s'", filename); > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > index 9922a23..4032db0 100644 > --- a/hw/ppc/spapr_events.c > +++ b/hw/ppc/spapr_events.c > @@ -212,6 +212,106 @@ struct hp_extended_log { > struct rtas_event_log_v6_hp hp; > } QEMU_PACKED; > > +struct rtas_event_log_v6_mc { Even if the rest of the code in this file seems to ignore CODING_STYLE, maybe it's time to start using CamelCase. David ? > +#define RTAS_LOG_V6_SECTION_ID_MC 0x4D43 /* MC */ > +struct rtas_event_log_v6_section_header hdr; > +uint32_t fru_id; > +uint32_t proc_id; > +uint8_t error_type; > +#define RTAS_LOG_V6_MC_TYPE_UE 0 > +#define RTAS_LOG_V6_MC_TYPE_SLB 1 > +#define RTAS_LOG_V6_MC_TYPE_ERAT 2 > +#define RTAS_LOG_V6_MC_TYPE_TLB 4 > +#define RTAS_LOG_V6_MC_TYPE_D_CACHE 5 > +#define RTAS_LOG_V6_MC_TYPE_I_CACHE 7 > +uint8_t sub_err_type; > +#define RTAS_LOG_V6_MC_UE_INDETERMINATE 0 > +#define RTAS_LOG_V6_MC_UE_IFETCH 1 > +#define RTAS_LOG_V6_MC_UE_PAGE_TABLE_WALK_IFETCH 2 > +#define RTAS_LOG_V6_MC_UE_LOAD_STORE 3 > +#define RTAS_LOG_V6_MC_UE_PAGE_TABLE_WALK_LOAD_STORE 4 > +#define RTAS_LOG_V6_MC_SLB_PARITY0 > +#define RTAS_LOG_V6_MC_SLB_MULTIHIT 1 > +#define RTAS_LOG_V6_MC_SLB_INDETERMINATE 2 > +#define RTAS_LOG_V6_MC_ERAT_PARITY 1 > +#define RTAS_LOG_V6_MC_ERAT_MULTIHIT 2 > +#define RTAS_LOG_V6_MC_ERAT_INDETERMINATE3 > +#define RTAS_LOG_V6_MC_TLB_PARITY1 > +#define RTAS_LOG_V6_MC_TLB_MULTIHIT 2 > +#define RTAS_LOG_V6_MC_TLB_INDETERMINATE 3 > +uint8_t reserved_1[6]; > +uint64_t effective_address; > +uint64_t logical_address; > +} QEMU_PACKED; > + > +struct mc_extended_log { > +struct rtas_event_log_v6 v6hdr; > +struct rtas_event_log_v6_mc mc; > +} QEMU_PACKED; > + > +struct MC_ierror_table { > +unsigned long srr1_mask; > +unsigned long srr1_value; > +bool nip_valid; /* nip is a valid indicator of faulting address */ > +uint8_t error_type; > +uint8_t error_subtype; > +unsigned int initiator; > +unsigned int severity; > +}; > + > +static const struct MC_ierror_table mc_ierror_table[] = { > +{ 0x081c, 0x0004, true, > + RTAS_LOG_V6_MC_TYPE_UE, RTAS_LOG_V6_MC_UE_IFETCH, > + RTAS_LOG_INITIATOR_CPU, RTAS_LOG_SEVERITY_ERROR_SYNC, }, > +{ 0x081c, 0x0008, true, > + RTAS_LOG_V6_MC_TYPE_SLB, RTAS_LOG_V6_MC_SLB_PARITY, > + RTAS_LOG_INITIATOR_CPU, RTAS_LOG_SEVERITY_ERROR_SYNC, }, > +{ 0x081c, 0x000c, true, > + RTAS_LOG_V6_MC_TYPE_SLB, RTAS_LOG_V6_MC_SLB_MULTIHIT, > + RTAS_LOG_INITIATOR_CPU, RTAS_LOG_SEVERITY_ERROR_SYNC, }, > +{ 0x081c, 0x0010, true, > + RTAS_LOG_
Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE
On Mon, May 13, 2019 at 01:30:53PM +0200, Greg Kurz wrote: > On Mon, 22 Apr 2019 12:33:26 +0530 > Aravinda Prasad wrote: > > > Upon a machine check exception (MCE) in a guest address space, > > KVM causes a guest exit to enable QEMU to build and pass the > > error to the guest in the PAPR defined rtas error log format. > > > > This patch builds the rtas error log, copies it to the rtas_addr > > and then invokes the guest registered machine check handler. The > > handler in the guest takes suitable action(s) depending on the type > > and criticality of the error. For example, if an error is > > unrecoverable memory corruption in an application inside the > > guest, then the guest kernel sends a SIGBUS to the application. > > For recoverable errors, the guest performs recovery actions and > > logs the error. > > > > Signed-off-by: Aravinda Prasad > > --- > > hw/ppc/spapr.c |4 + > > hw/ppc/spapr_events.c | 245 > > > > include/hw/ppc/spapr.h |4 + > > 3 files changed, 253 insertions(+) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 2779efe..ffd1715 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2918,6 +2918,10 @@ static void spapr_machine_init(MachineState *machine) > > error_report("Could not get size of LPAR rtas '%s'", filename); > > exit(1); > > } > > + > > +/* Resize blob to accommodate error log. */ > > +spapr->rtas_size = spapr_get_rtas_size(spapr->rtas_size); > > + > > This is the only user for spapr_get_rtas_size(), which is trivial. > I suggest you simply open-code it here. I agree. > But also, spapr->rtas_size is a guest visible thing, "rtas-size" prop in the > DT. Since existing machine types don't do that, I guess we should only use > the new size if cap-fwnmi-mce=on for the sake of compatibility. Yes, that's a good idea. Changing this is very unlikely to break a guest, but it's easy to be safe here so let's do it. > > > spapr->rtas_blob = g_malloc(spapr->rtas_size); > > if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) > > { > > error_report("Could not load LPAR rtas '%s'", filename); > > diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c > > index 9922a23..4032db0 100644 > > --- a/hw/ppc/spapr_events.c > > +++ b/hw/ppc/spapr_events.c > > @@ -212,6 +212,106 @@ struct hp_extended_log { > > struct rtas_event_log_v6_hp hp; > > } QEMU_PACKED; > > > > +struct rtas_event_log_v6_mc { > > Even if the rest of the code in this file seems to ignore CODING_STYLE, > maybe it's time to start using CamelCase. > > David ? Out of scope here, I think. > > +#define RTAS_LOG_V6_SECTION_ID_MC 0x4D43 /* MC */ > > +struct rtas_event_log_v6_section_header hdr; > > +uint32_t fru_id; > > +uint32_t proc_id; > > +uint8_t error_type; > > +#define RTAS_LOG_V6_MC_TYPE_UE 0 > > +#define RTAS_LOG_V6_MC_TYPE_SLB 1 > > +#define RTAS_LOG_V6_MC_TYPE_ERAT 2 > > +#define RTAS_LOG_V6_MC_TYPE_TLB 4 > > +#define RTAS_LOG_V6_MC_TYPE_D_CACHE 5 > > +#define RTAS_LOG_V6_MC_TYPE_I_CACHE 7 > > +uint8_t sub_err_type; > > +#define RTAS_LOG_V6_MC_UE_INDETERMINATE 0 > > +#define RTAS_LOG_V6_MC_UE_IFETCH 1 > > +#define RTAS_LOG_V6_MC_UE_PAGE_TABLE_WALK_IFETCH 2 > > +#define RTAS_LOG_V6_MC_UE_LOAD_STORE 3 > > +#define RTAS_LOG_V6_MC_UE_PAGE_TABLE_WALK_LOAD_STORE 4 > > +#define RTAS_LOG_V6_MC_SLB_PARITY0 > > +#define RTAS_LOG_V6_MC_SLB_MULTIHIT 1 > > +#define RTAS_LOG_V6_MC_SLB_INDETERMINATE 2 > > +#define RTAS_LOG_V6_MC_ERAT_PARITY 1 > > +#define RTAS_LOG_V6_MC_ERAT_MULTIHIT 2 > > +#define RTAS_LOG_V6_MC_ERAT_INDETERMINATE3 > > +#define RTAS_LOG_V6_MC_TLB_PARITY1 > > +#define RTAS_LOG_V6_MC_TLB_MULTIHIT 2 > > +#define RTAS_LOG_V6_MC_TLB_INDETERMINATE 3 > > +uint8_t reserved_1[6]; > > +uint64_t effective_address; > > +uint64_t logical_address; > > +} QEMU_PACKED; > > + > > +struct mc_extended_log { > > +struct rtas_event_log_v6 v6hdr; > > +struct rtas_event_log_v6_mc mc; > > +} QEMU_PACKED; > > + > > +struct MC_ierror_table { > > +unsigned long srr1_mask; > > +unsigned long srr1_value; > > +bool nip_valid; /* nip is a valid indicator of faulting address */ > > +uint8_t error_type; > > +uint8_t error_subtype; > > +unsigned int initiator; > > +unsigned int severity; > > +}; > > + > > +static const struct MC_ierror_table mc_ierror_table[] = { > > +{ 0x081c, 0x0004, true, > > + RTAS_LOG_V6_MC_TYPE_UE, RTAS_LOG_V6_MC_UE_IFETCH, > > + RTAS_LOG_INITIATOR
Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE
On Tuesday 14 May 2019 05:38 AM, David Gibson wrote: > On Mon, May 13, 2019 at 01:30:53PM +0200, Greg Kurz wrote: >> On Mon, 22 Apr 2019 12:33:26 +0530 >> Aravinda Prasad wrote: >> >>> Upon a machine check exception (MCE) in a guest address space, >>> KVM causes a guest exit to enable QEMU to build and pass the >>> error to the guest in the PAPR defined rtas error log format. >>> >>> This patch builds the rtas error log, copies it to the rtas_addr >>> and then invokes the guest registered machine check handler. The >>> handler in the guest takes suitable action(s) depending on the type >>> and criticality of the error. For example, if an error is >>> unrecoverable memory corruption in an application inside the >>> guest, then the guest kernel sends a SIGBUS to the application. >>> For recoverable errors, the guest performs recovery actions and >>> logs the error. >>> >>> Signed-off-by: Aravinda Prasad >>> --- >>> hw/ppc/spapr.c |4 + >>> hw/ppc/spapr_events.c | 245 >>> >>> include/hw/ppc/spapr.h |4 + >>> 3 files changed, 253 insertions(+) >>> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>> index 2779efe..ffd1715 100644 >>> --- a/hw/ppc/spapr.c >>> +++ b/hw/ppc/spapr.c >>> @@ -2918,6 +2918,10 @@ static void spapr_machine_init(MachineState *machine) >>> error_report("Could not get size of LPAR rtas '%s'", filename); >>> exit(1); >>> } >>> + >>> +/* Resize blob to accommodate error log. */ >>> +spapr->rtas_size = spapr_get_rtas_size(spapr->rtas_size); >>> + >> >> This is the only user for spapr_get_rtas_size(), which is trivial. >> I suggest you simply open-code it here. > > I agree. Sure. > >> But also, spapr->rtas_size is a guest visible thing, "rtas-size" prop in the >> DT. Since existing machine types don't do that, I guess we should only use >> the new size if cap-fwnmi-mce=on for the sake of compatibility. > > Yes, that's a good idea. Changing this is very unlikely to break a > guest, but it's easy to be safe here so let's do it. I did it like that because the rtas_blob is allocated based on rtas_size in spapr_machine_init(). During spapr_machine_init() it is not know if the guest calls "ibm, nmi-register". So if we want to use the new size only when cap_fwnmi=on, then we have to realloc the blob in "ibm, nmi-register". > >> >>> spapr->rtas_blob = g_malloc(spapr->rtas_size); >>> if (load_image_size(filename, spapr->rtas_blob, spapr->rtas_size) < 0) >>> { >>> error_report("Could not load LPAR rtas '%s'", filename); >>> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c >>> index 9922a23..4032db0 100644 >>> --- a/hw/ppc/spapr_events.c >>> +++ b/hw/ppc/spapr_events.c >>> @@ -212,6 +212,106 @@ struct hp_extended_log { >>> struct rtas_event_log_v6_hp hp; >>> } QEMU_PACKED; >>> >>> +struct rtas_event_log_v6_mc { >> >> Even if the rest of the code in this file seems to ignore CODING_STYLE, >> maybe it's time to start using CamelCase. >> >> David ? > > Out of scope here, I think. > >>> +#define RTAS_LOG_V6_SECTION_ID_MC 0x4D43 /* MC */ >>> +struct rtas_event_log_v6_section_header hdr; >>> +uint32_t fru_id; >>> +uint32_t proc_id; >>> +uint8_t error_type; >>> +#define RTAS_LOG_V6_MC_TYPE_UE 0 >>> +#define RTAS_LOG_V6_MC_TYPE_SLB 1 >>> +#define RTAS_LOG_V6_MC_TYPE_ERAT 2 >>> +#define RTAS_LOG_V6_MC_TYPE_TLB 4 >>> +#define RTAS_LOG_V6_MC_TYPE_D_CACHE 5 >>> +#define RTAS_LOG_V6_MC_TYPE_I_CACHE 7 >>> +uint8_t sub_err_type; >>> +#define RTAS_LOG_V6_MC_UE_INDETERMINATE 0 >>> +#define RTAS_LOG_V6_MC_UE_IFETCH 1 >>> +#define RTAS_LOG_V6_MC_UE_PAGE_TABLE_WALK_IFETCH 2 >>> +#define RTAS_LOG_V6_MC_UE_LOAD_STORE 3 >>> +#define RTAS_LOG_V6_MC_UE_PAGE_TABLE_WALK_LOAD_STORE 4 >>> +#define RTAS_LOG_V6_MC_SLB_PARITY0 >>> +#define RTAS_LOG_V6_MC_SLB_MULTIHIT 1 >>> +#define RTAS_LOG_V6_MC_SLB_INDETERMINATE 2 >>> +#define RTAS_LOG_V6_MC_ERAT_PARITY 1 >>> +#define RTAS_LOG_V6_MC_ERAT_MULTIHIT 2 >>> +#define RTAS_LOG_V6_MC_ERAT_INDETERMINATE3 >>> +#define RTAS_LOG_V6_MC_TLB_PARITY1 >>> +#define RTAS_LOG_V6_MC_TLB_MULTIHIT 2 >>> +#define RTAS_LOG_V6_MC_TLB_INDETERMINATE 3 >>> +uint8_t reserved_1[6]; >>> +uint64_t effective_address; >>> +uint64_t logical_address; >>> +} QEMU_PACKED; >>> + >>> +struct mc_extended_log { >>> +struct rtas_event_log_v6 v6hdr; >>> +struct rtas_event_log_v6_mc mc; >>> +} QEMU_PACKED; >>> + >>> +struct MC_ierror_table { >>> +unsigned long srr1_mask; >>> +unsigned long srr1_value; >>> +bool nip_vali
Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE
On Tue, May 14, 2019 at 09:56:41AM +0530, Aravinda Prasad wrote: > > > On Tuesday 14 May 2019 05:38 AM, David Gibson wrote: > > On Mon, May 13, 2019 at 01:30:53PM +0200, Greg Kurz wrote: > >> On Mon, 22 Apr 2019 12:33:26 +0530 > >> Aravinda Prasad wrote: > >> > >>> Upon a machine check exception (MCE) in a guest address space, > >>> KVM causes a guest exit to enable QEMU to build and pass the > >>> error to the guest in the PAPR defined rtas error log format. > >>> > >>> This patch builds the rtas error log, copies it to the rtas_addr > >>> and then invokes the guest registered machine check handler. The > >>> handler in the guest takes suitable action(s) depending on the type > >>> and criticality of the error. For example, if an error is > >>> unrecoverable memory corruption in an application inside the > >>> guest, then the guest kernel sends a SIGBUS to the application. > >>> For recoverable errors, the guest performs recovery actions and > >>> logs the error. > >>> > >>> Signed-off-by: Aravinda Prasad > >>> --- > >>> hw/ppc/spapr.c |4 + > >>> hw/ppc/spapr_events.c | 245 > >>> > >>> include/hw/ppc/spapr.h |4 + > >>> 3 files changed, 253 insertions(+) > >>> > >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > >>> index 2779efe..ffd1715 100644 > >>> --- a/hw/ppc/spapr.c > >>> +++ b/hw/ppc/spapr.c > >>> @@ -2918,6 +2918,10 @@ static void spapr_machine_init(MachineState > >>> *machine) > >>> error_report("Could not get size of LPAR rtas '%s'", filename); > >>> exit(1); > >>> } > >>> + > >>> +/* Resize blob to accommodate error log. */ > >>> +spapr->rtas_size = spapr_get_rtas_size(spapr->rtas_size); > >>> + > >> > >> This is the only user for spapr_get_rtas_size(), which is trivial. > >> I suggest you simply open-code it here. > > > > I agree. > > Sure. > > > > >> But also, spapr->rtas_size is a guest visible thing, "rtas-size" prop in > >> the > >> DT. Since existing machine types don't do that, I guess we should only use > >> the new size if cap-fwnmi-mce=on for the sake of compatibility. > > > > Yes, that's a good idea. Changing this is very unlikely to break a > > guest, but it's easy to be safe here so let's do it. > > I did it like that because the rtas_blob is allocated based on rtas_size > in spapr_machine_init(). During spapr_machine_init() it is not know if > the guest calls "ibm, nmi-register". So if we want to use the new size > only when cap_fwnmi=on, then we have to realloc the blob in "ibm, > nmi-register". What? Just always allocate the necessary space in spapr_machine_init() if cap_fwnmi=on, it'll be wasted if ibm,nmi-register is never called, but it's not that much space so we don't really care. -- 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 signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE
On Tuesday 14 May 2019 10:10 AM, David Gibson wrote: > On Tue, May 14, 2019 at 09:56:41AM +0530, Aravinda Prasad wrote: >> >> >> On Tuesday 14 May 2019 05:38 AM, David Gibson wrote: >>> On Mon, May 13, 2019 at 01:30:53PM +0200, Greg Kurz wrote: On Mon, 22 Apr 2019 12:33:26 +0530 Aravinda Prasad wrote: > Upon a machine check exception (MCE) in a guest address space, > KVM causes a guest exit to enable QEMU to build and pass the > error to the guest in the PAPR defined rtas error log format. > > This patch builds the rtas error log, copies it to the rtas_addr > and then invokes the guest registered machine check handler. The > handler in the guest takes suitable action(s) depending on the type > and criticality of the error. For example, if an error is > unrecoverable memory corruption in an application inside the > guest, then the guest kernel sends a SIGBUS to the application. > For recoverable errors, the guest performs recovery actions and > logs the error. > > Signed-off-by: Aravinda Prasad > --- > hw/ppc/spapr.c |4 + > hw/ppc/spapr_events.c | 245 > > include/hw/ppc/spapr.h |4 + > 3 files changed, 253 insertions(+) > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 2779efe..ffd1715 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -2918,6 +2918,10 @@ static void spapr_machine_init(MachineState > *machine) > error_report("Could not get size of LPAR rtas '%s'", filename); > exit(1); > } > + > +/* Resize blob to accommodate error log. */ > +spapr->rtas_size = spapr_get_rtas_size(spapr->rtas_size); > + This is the only user for spapr_get_rtas_size(), which is trivial. I suggest you simply open-code it here. >>> >>> I agree. >> >> Sure. >> >>> But also, spapr->rtas_size is a guest visible thing, "rtas-size" prop in the DT. Since existing machine types don't do that, I guess we should only use the new size if cap-fwnmi-mce=on for the sake of compatibility. >>> >>> Yes, that's a good idea. Changing this is very unlikely to break a >>> guest, but it's easy to be safe here so let's do it. >> >> I did it like that because the rtas_blob is allocated based on rtas_size >> in spapr_machine_init(). During spapr_machine_init() it is not know if >> the guest calls "ibm, nmi-register". So if we want to use the new size >> only when cap_fwnmi=on, then we have to realloc the blob in "ibm, >> nmi-register". > > What? Just always allocate the necessary space in > spapr_machine_init() if cap_fwnmi=on, it'll be wasted if > ibm,nmi-register is never called, but it's not that much space so we > don't really care. Yes, not that much space, and ibm,nmi-register is called when the Linux kernel boots. I guess, even though other OSes might not call ibm,nmi-register, they do not constitute significant QEMU on Power users. So I think, I will keep the code as is. > -- Regards, Aravinda
Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE
On Tuesday 23 April 2019 08:08 PM, Fabiano Rosas wrote: > Aravinda Prasad writes: > >> +/* >> + * Properly set bits in MSR before we invoke the handler. >> + * SRR0/1, DAR and DSISR are properly set by KVM >> + */ >> +if (!(*pcc->interrupts_big_endian)(cpu)) { >> +msr |= (1ULL << MSR_LE); >> +} >> + >> +if (env->msr && (1ULL << MSR_SF)) { > > Don't you mean & instead of &&? Ah.. yes.. Thanks for pointing out. > >> +msr |= (1ULL << MSR_SF); >> +} > > -- Regards, Aravinda
Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE
On Tue, May 14, 2019 at 10:36:17AM +0530, Aravinda Prasad wrote: > > > On Tuesday 14 May 2019 10:10 AM, David Gibson wrote: > > On Tue, May 14, 2019 at 09:56:41AM +0530, Aravinda Prasad wrote: > >> > >> > >> On Tuesday 14 May 2019 05:38 AM, David Gibson wrote: > >>> On Mon, May 13, 2019 at 01:30:53PM +0200, Greg Kurz wrote: > On Mon, 22 Apr 2019 12:33:26 +0530 > Aravinda Prasad wrote: > > > Upon a machine check exception (MCE) in a guest address space, > > KVM causes a guest exit to enable QEMU to build and pass the > > error to the guest in the PAPR defined rtas error log format. > > > > This patch builds the rtas error log, copies it to the rtas_addr > > and then invokes the guest registered machine check handler. The > > handler in the guest takes suitable action(s) depending on the type > > and criticality of the error. For example, if an error is > > unrecoverable memory corruption in an application inside the > > guest, then the guest kernel sends a SIGBUS to the application. > > For recoverable errors, the guest performs recovery actions and > > logs the error. > > > > Signed-off-by: Aravinda Prasad > > --- > > hw/ppc/spapr.c |4 + > > hw/ppc/spapr_events.c | 245 > > > > include/hw/ppc/spapr.h |4 + > > 3 files changed, 253 insertions(+) > > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > > index 2779efe..ffd1715 100644 > > --- a/hw/ppc/spapr.c > > +++ b/hw/ppc/spapr.c > > @@ -2918,6 +2918,10 @@ static void spapr_machine_init(MachineState > > *machine) > > error_report("Could not get size of LPAR rtas '%s'", filename); > > exit(1); > > } > > + > > +/* Resize blob to accommodate error log. */ > > +spapr->rtas_size = spapr_get_rtas_size(spapr->rtas_size); > > + > > This is the only user for spapr_get_rtas_size(), which is trivial. > I suggest you simply open-code it here. > >>> > >>> I agree. > >> > >> Sure. > >> > >>> > But also, spapr->rtas_size is a guest visible thing, "rtas-size" prop in > the > DT. Since existing machine types don't do that, I guess we should only > use > the new size if cap-fwnmi-mce=on for the sake of compatibility. > >>> > >>> Yes, that's a good idea. Changing this is very unlikely to break a > >>> guest, but it's easy to be safe here so let's do it. > >> > >> I did it like that because the rtas_blob is allocated based on rtas_size > >> in spapr_machine_init(). During spapr_machine_init() it is not know if > >> the guest calls "ibm, nmi-register". So if we want to use the new size > >> only when cap_fwnmi=on, then we have to realloc the blob in "ibm, > >> nmi-register". > > > > What? Just always allocate the necessary space in > > spapr_machine_init() if cap_fwnmi=on, it'll be wasted if > > ibm,nmi-register is never called, but it's not that much space so we > > don't really care. > > Yes, not that much space, and ibm,nmi-register is called when the Linux > kernel boots. I guess, even though other OSes might not call > ibm,nmi-register, they do not constitute significant QEMU on Power users. > > So I think, I will keep the code as is. No, that's not right. It's impractical to change the allocation depending on whether fwnmi is currently active. But you *can* (and should) base the allocation on whether fwnmi is *possible* - that is, the value of the spapr cap. -- 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 signature.asc Description: PGP signature
Re: [Qemu-devel] [Qemu-ppc] [PATCH v8 4/6] target/ppc: Build rtas error log upon an MCE
On Thursday 16 May 2019 07:17 AM, David Gibson wrote: > On Tue, May 14, 2019 at 10:36:17AM +0530, Aravinda Prasad wrote: >> >> >> On Tuesday 14 May 2019 10:10 AM, David Gibson wrote: >>> On Tue, May 14, 2019 at 09:56:41AM +0530, Aravinda Prasad wrote: On Tuesday 14 May 2019 05:38 AM, David Gibson wrote: > On Mon, May 13, 2019 at 01:30:53PM +0200, Greg Kurz wrote: >> On Mon, 22 Apr 2019 12:33:26 +0530 >> Aravinda Prasad wrote: >> >>> Upon a machine check exception (MCE) in a guest address space, >>> KVM causes a guest exit to enable QEMU to build and pass the >>> error to the guest in the PAPR defined rtas error log format. >>> >>> This patch builds the rtas error log, copies it to the rtas_addr >>> and then invokes the guest registered machine check handler. The >>> handler in the guest takes suitable action(s) depending on the type >>> and criticality of the error. For example, if an error is >>> unrecoverable memory corruption in an application inside the >>> guest, then the guest kernel sends a SIGBUS to the application. >>> For recoverable errors, the guest performs recovery actions and >>> logs the error. >>> >>> Signed-off-by: Aravinda Prasad >>> --- >>> hw/ppc/spapr.c |4 + >>> hw/ppc/spapr_events.c | 245 >>> >>> include/hw/ppc/spapr.h |4 + >>> 3 files changed, 253 insertions(+) >>> >>> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c >>> index 2779efe..ffd1715 100644 >>> --- a/hw/ppc/spapr.c >>> +++ b/hw/ppc/spapr.c >>> @@ -2918,6 +2918,10 @@ static void spapr_machine_init(MachineState >>> *machine) >>> error_report("Could not get size of LPAR rtas '%s'", filename); >>> exit(1); >>> } >>> + >>> +/* Resize blob to accommodate error log. */ >>> +spapr->rtas_size = spapr_get_rtas_size(spapr->rtas_size); >>> + >> >> This is the only user for spapr_get_rtas_size(), which is trivial. >> I suggest you simply open-code it here. > > I agree. Sure. > >> But also, spapr->rtas_size is a guest visible thing, "rtas-size" prop in >> the >> DT. Since existing machine types don't do that, I guess we should only >> use >> the new size if cap-fwnmi-mce=on for the sake of compatibility. > > Yes, that's a good idea. Changing this is very unlikely to break a > guest, but it's easy to be safe here so let's do it. I did it like that because the rtas_blob is allocated based on rtas_size in spapr_machine_init(). During spapr_machine_init() it is not know if the guest calls "ibm, nmi-register". So if we want to use the new size only when cap_fwnmi=on, then we have to realloc the blob in "ibm, nmi-register". >>> >>> What? Just always allocate the necessary space in >>> spapr_machine_init() if cap_fwnmi=on, it'll be wasted if >>> ibm,nmi-register is never called, but it's not that much space so we >>> don't really care. >> >> Yes, not that much space, and ibm,nmi-register is called when the Linux >> kernel boots. I guess, even though other OSes might not call >> ibm,nmi-register, they do not constitute significant QEMU on Power users. >> >> So I think, I will keep the code as is. > > No, that's not right. It's impractical to change the allocation > depending on whether fwnmi is currently active. But you *can* (and > should) base the allocation on whether fwnmi is *possible* - that is, > the value of the spapr cap. Sure.. > -- Regards, Aravinda