Groups > linux.kernel > #1606046 > unrolled thread

[PATCH V13 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

Started by	Tyler Baicar <tbaicar@codeaurora.org>
First post	2017-03-21 23:50 +0100
Last post	2017-03-22 00:00 +0100
Articles	20 on this page of 25 — 8 participants

Back to article view | Back to linux.kernel

  [PATCH V13 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64 Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
    [PATCH V13 09/10] trace, ras: add ARM processor error trace event Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
      Re: [PATCH V13 09/10] trace, ras: add ARM processor error trace event Xie XiuQi <xiexiuqi@huawei.com> - 2017-03-22 04:00 +0100
        Re: [PATCH V13 09/10] trace, ras: add ARM processor error trace event "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-03-22 04:10 +0100
    [PATCH V13 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1 Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
    [PATCH V13 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
    [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
      Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support Catalin Marinas <catalin.marinas@arm.com> - 2017-03-23 19:00 +0100
      Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support Marc Zyngier <marc.zyngier@arm.com> - 2017-03-24 15:10 +0100
        Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-03-28 18:40 +0200
      Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support Christoffer Dall <cdall@linaro.org> - 2017-03-24 17:10 +0100
        Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support gengdongjiu <gengdongjiu@huawei.com> - 2017-03-28 06:30 +0200
          Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-03-28 18:50 +0200
        Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-03-28 18:50 +0200
      Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support gengdongjiu <gengdongjiu@huawei.com> - 2017-03-28 12:00 +0200
        Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support "Baicar, Tyler" <tbaicar@codeaurora.org> - 2017-03-28 18:50 +0200
      Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support James Morse <james.morse@arm.com> - 2017-03-28 13:00 +0200
    [PATCH V13 04/10] arm64: exception: handle Synchronous External Abort Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
      Re: [PATCH V13 04/10] arm64: exception: handle Synchronous External  Abort Catalin Marinas <catalin.marinas@arm.com> - 2017-03-23 18:10 +0100
    [PATCH V13 07/10] efi: print unrecognized CPER section Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
    [PATCH V13 05/10] acpi: apei: handle SEA notification type for ARMv8 Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
      Re: [PATCH V13 05/10] acpi: apei: handle SEA notification type for  ARMv8 Catalin Marinas <catalin.marinas@arm.com> - 2017-03-23 19:00 +0100
    [PATCH V13 01/10] acpi: apei: read ack upon ghes record consumption Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
    [PATCH V13 06/10] acpi: apei: panic OS with fatal error status block Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-21 23:50 +0100
    [PATCH V13 03/10] efi: parse ARM processor error Tyler Baicar <tbaicar@codeaurora.org> - 2017-03-22 00:00 +0100

Page 1 of 2 [1] 2 Next page →

#1606046 — [PATCH V13 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

From	Tyler Baicar <tbaicar@codeaurora.org>
Date	2017-03-21 23:50 +0100
Subject	[PATCH V13 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64
Message-ID	<tnALf-1nk-1@gated-at.bofh.it>

When a memory error, CPU error, PCIe error, or other type of hardware error
that's covered by RAS occurs, firmware should populate the shared GHES memory
location with the proper GHES structures to notify the OS of the error.
For example, platforms that implement firmware first handling may implement
separate GHES sources for corrected errors and uncorrected errors. If the
error is an uncorrectable error, then the firmware will notify the OS
immediately since the error needs to be handled ASAP. The OS will then be able
to take the appropriate action needed such as offlining a page. If the error
is a corrected error, then the firmware will not interrupt the OS immediately.
Instead, the OS will see and report the error the next time it's GHES timer
expires. The kernel will first parse the GHES structures and report the errors
through the kernel logs and then notify the user space through RAS trace
events. This allows user space applications such as RAS Daemon to see the
errors and report them however the user desires. This patchset extends the
kernel functionality for RAS errors based on updates in the UEFI 2.6 and
ACPI 6.1 specifications.

An example flow from firmware to user space could be:

                 +---------------+
       +-------->|               |
       |         |  GHES polling |--+
+-------------+  |    source     |  |   +---------------+   +------------+
|             |  +---------------+  |   |  Kernel GHES  |   |            |
|  Firmware   |                     +-->|  CPER AER and |-->|  RAS trace |
|             |  +---------------+  |   |  EDAC drivers |   |   event    |
+-------------+  |               |  |   +---------------+   +------------+
       |         |  GHES sci     |--+
       +-------->|   source      |
                 +---------------+

Add support for Generic Hardware Error Source (GHES) v2, which introduces the
capability for the OS to acknowledge the consumption of the error record
generated by the Reliability, Availability and Serviceability (RAS) controller.
This eliminates potential race conditions between the OS and the RAS controller.

Add support for the timestamp field added to the Generic Error Data Entry v3,
allowing the OS to log the time that the error is generated by the firmware,
rather than the time the error is consumed. This improves the correctness of
event sequences when analyzing error logs. The timestamp is added in
ACPI 6.1, reference Table 18-343 Generic Error Data Entry.

Add support for ARMv8 Common Platform Error Record (CPER) per UEFI 2.6
specification. ARMv8 specific processor error information is reported as part of
the CPER records.  This provides more detail on for processor error logs. This
can help describe ARMv8 cache, tlb, and bus errors.

Synchronous External Abort (SEA) represents a specific processor error condition
in ARM systems. A handler is added to recognize SEA errors, and a notifier is
added to parse and report the errors before the process is killed. Refer to
section N.2.1.1 in the Common Platform Error Record appendix of the UEFI 2.6
specification.

Currently the kernel ignores CPER records that are unrecognized.
On the other hand, UEFI spec allows for non-standard (eg. vendor
proprietary) error section type in CPER (Common Platform Error Record),
as defined in section N2.3 of UEFI version 2.5. Therefore, user
is not able to see hardware error data of non-standard section.

If section Type field of Generic Error Data Entry is unrecognized,
prints out the raw data in dmesg buffer, and also adds a tracepoint
for reporting such hardware errors.

Currently even if an error status block's severity is fatal, the kernel
does not honor the severity level and panic. With the firmware first
model, the platform could inform the OS about a fatal hardware error
through the non-NMI GHES notification type. The OS should panic when a
hardware error record is received with this severity.

Add support to handle SEAs that occur while a KVM guest kernel is
running. Currently these are unsupported by the guest abort handling.

V13:Rebase on 4.11rc2
    Print decimal and hex sizes for unknown CPER section errors
    Use proper CONFIG_* when using IS_ENABLED
    Move handle_guest_sea call prior to SEI check
    Add a return value to handle_guest_sea
    Move RCU locking into ghes_notify_sea
    Add valid bit checks to ARM trace event
    Remove GPIO, SEI, and GSIV cases in GHES
    Add ARCH_HAVE_NMI_SAFE_CMPXCHG since we added NMI usage

V12:Remove double quotes from CPER code
    Add helper function to check all SEA cases in KVM patch
    Replace nmi_enter/exit with rcu_read_lock/unlock for KVM SEA
    Change HAVE_ACPI_APEI_SEA to ACPI_APEI_SEA in KVM SEA case

V11:Change print_hex_dump calls to include ASCII output
    Change HAVE_ACPI_APEI_SEA to ACPI_APEI_SEA and make it 'default y'
    Add unknown print back when printing unknown CPER section
    Make sure to use "%s"" in CPER code
    Spacing fix when checking if SEA is enabled

V10:Fix spacing of trace event enabled if statement

V9: Move SEA_FnV_MASK to ESR_ELx_FnV
    Move HAVE_NMI into alphabetical order
    Remove duplicate hardirq.h include
    Only call ghes_notify_sea if HAVE_ACPI_APEI_SEA
    Make ACPI_APEI_SEA depend on ACPI_APEI_GHES
    Use phys_addr_t for physical address variable
    Make ghes_sea_add() return void
    Add include guard to ghes.h
    Verify HAVE_RAS before calling ras trace events
    Call __ghes_print_estatus() before __ghes_call_panic()
    Add trace_*_event_enabled() checks for both new trace events

V8: Remove SEA notifier
    Add FAR not valid bit check when populating the SEA error address
    Move nmi_enter/exit() to architecture specific code
    Add synchronize_rcu() usage to SEA handling
    Make GHES_IOREMAP_PAGES always 2
    Update ghes_ioremap_pfn_nmi() to work like ghes_ioremap_pfn_irq()
    Remove the SEA print from handle_guest_sea()

V7: Update a couple prints for ARM processor errors
    Add Print notifying if overflow occurred for ARM processor errors
    Check for ARM configuration to allow the compiler to ignore ARM code
     on non-ARM systems
    Use SEA acronym instead of spelling it out
    Update fault_info prints to be more clear
    Add NMI locking to SEA notification
    Remove error info structure from ARM trace event since there can be
     a variable amount of these structures

V6: Change HEST_TYPE_GENERIC_V2 to IS_HEST_TYPE_GENERIC_V2 for readability
    Move APEI helper defines from cper.h to ghes.h
    Add data_len decrement back into print loop
    Change references to ARMv8 to just ARM
    Rewrite ARM processor context info parsing
    Check valid bit of ARM error info field before printing it
    Add include of linux/uuid.h in ghes.c

V5: Fix GHES goto logic for error conditions
    Change ghes_do_read_ack to ghes_ack_error
    Make sure data version check is >= 3
    Use CPER helper functions in print functions
    Make handle_guest_sea() dummy function static for arm
    Add arm to subject line for KVM patch

V4: Add bit offset left shift to read_ack_write value
    Make HEST generic and generic_v2 structures a union in the ghes structure
    Move gdata v3 helper functions into ghes.h to avoid duplication
    Reorder the timestamp print and avoid memcpy
    Add helper functions for gdata size checking
    Rename the SEA functions
    Add helper function for GHES panics
    Set fru_id to NULL UUID at variable declaration
    Limit ARM trace event parameters to the needed structures
    Reorder the ARM trace event variables to save space
    Add comment for why we don't pass SEAs to the guest when it aborts
    Move ARM trace event call into GHES driver instead of CPER

V3: Fix unmapped address to the read_ack_register in ghes.c
    Add helper function to get the proper payload based on generic data entry
     version
    Move timestamp print to avoid changing function calls in cper.c
    Remove patch "arm64: exception: handle instruction abort at current EL"
     since the el1_ia handler is already added in 4.8
    Add EFI and ARM64 dependencies for HAVE_ACPI_APEI_SEA
    Add a new trace event for ARM type errors
    Add support to handle KVM guest SEAs

V2: Add PSCI state print for the ARMv8 error type.
    Separate timestamp year into year and century using BCD format.
    Rebase on top of ACPICA 20160318 release and remove header file changes
     in include/acpi/actbl1.h.
    Add panic OS with fatal error status block patch.
    Add processing of unrecognized CPER error section patches with updates
     from previous comments. Original patches: https://lkml.org/lkml/2015/9/8/646

V1: https://lkml.org/lkml/2016/2/5/544

Jonathan (Zhixiong) Zhang (1):
  acpi: apei: panic OS with fatal error status block

Tyler Baicar (9):
  acpi: apei: read ack upon ghes record consumption
  ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1
  efi: parse ARM processor error
  arm64: exception: handle Synchronous External Abort
  acpi: apei: handle SEA notification type for ARMv8
  efi: print unrecognized CPER section
  ras: acpi / apei: generate trace event for unrecognized CPER section
  trace, ras: add ARM processor error trace event
  arm/arm64: KVM: add guest SEA support

 arch/arm/include/asm/kvm_arm.h       |  10 ++
 arch/arm/include/asm/system_misc.h   |   5 +
 arch/arm/kvm/mmu.c                   |  41 +++++--
 arch/arm64/Kconfig                   |   2 +
 arch/arm64/include/asm/esr.h         |   1 +
 arch/arm64/include/asm/kvm_arm.h     |  10 ++
 arch/arm64/include/asm/system_misc.h |   2 +
 arch/arm64/mm/fault.c                |  71 ++++++++++--
 drivers/acpi/apei/Kconfig            |  15 +++
 drivers/acpi/apei/ghes.c             | 178 +++++++++++++++++++++++++----
 drivers/acpi/apei/hest.c             |   7 +-
 drivers/firmware/efi/cper.c          | 209 ++++++++++++++++++++++++++++++++---
 drivers/ras/ras.c                    |   2 +
 include/acpi/ghes.h                  |  34 +++++-
 include/linux/cper.h                 |  54 +++++++++
 include/ras/ras_event.h              |  90 +++++++++++++++
 16 files changed, 676 insertions(+), 55 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [next] | [standalone]

#1606048 — [PATCH V13 09/10] trace, ras: add ARM processor error trace event

From	Tyler Baicar <tbaicar@codeaurora.org>
Date	2017-03-21 23:50 +0100
Subject	[PATCH V13 09/10] trace, ras: add ARM processor error trace event
Message-ID	<tnALf-1nk-7@gated-at.bofh.it>
In reply to	#1606046

Currently there are trace events for the various RAS
errors with the exception of ARM processor type errors.
Add a new trace event for such errors so that the user
will know when they occur. These trace events are
consistent with the ARM processor error section type
defined in UEFI 2.6 spec section N.2.4.4.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
---
 drivers/acpi/apei/ghes.c    |  8 +++++++-
 drivers/firmware/efi/cper.c |  1 +
 drivers/ras/ras.c           |  1 +
 include/ras/ras_event.h     | 45 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 3ecbacc..230b095 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -515,7 +515,13 @@ static void ghes_do_proc(struct ghes *ghes,
 		}
 #endif
 #ifdef CONFIG_RAS
-		else if (trace_unknown_sec_event_enabled()) {
+		else if (!uuid_le_cmp(sec_type, CPER_SEC_PROC_ARM) &&
+			 trace_arm_event_enabled()) {
+			struct cper_sec_proc_arm *arm_err;
+
+			arm_err = acpi_hest_generic_data_payload(gdata);
+			trace_arm_event(arm_err);
+		} else if (trace_unknown_sec_event_enabled()) {
 			void *unknown_err = acpi_hest_generic_data_payload(gdata);
 			trace_unknown_sec_event(&sec_type,
 					fru_id, fru_text, sec_sev,
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index d263bc8..37a39af 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -35,6 +35,7 @@
 #include <linux/printk.h>
 #include <linux/bcd.h>
 #include <acpi/ghes.h>
+#include <ras/ras_event.h>
 
 #define INDENT_SP	" "
 
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index fb2500b..8ba5a94 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -28,3 +28,4 @@ static int __init ras_init(void)
 #endif
 EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
 EXPORT_TRACEPOINT_SYMBOL_GPL(unknown_sec_event);
+EXPORT_TRACEPOINT_SYMBOL_GPL(arm_event);
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 5861b6f..13befad 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -162,6 +162,51 @@
 );
 
 /*
+ * ARM Processor Events Report
+ *
+ * This event is generated when hardware detects an ARM processor error
+ * has occurred. UEFI 2.6 spec section N.2.4.4.
+ */
+TRACE_EVENT(arm_event,
+
+	TP_PROTO(const struct cper_sec_proc_arm *proc),
+
+	TP_ARGS(proc),
+
+	TP_STRUCT__entry(
+		__field(u64, mpidr)
+		__field(u64, midr)
+		__field(u32, running_state)
+		__field(u32, psci_state)
+		__field(u8, affinity)
+	),
+
+	TP_fast_assign(
+		if (proc->validation_bits & CPER_ARM_VALID_AFFINITY_LEVEL)
+			__entry->affinity = proc->affinity_level;
+		else
+			__entry->affinity = ~0;
+		if (proc->validation_bits & CPER_ARM_VALID_MPIDR)
+			__entry->mpidr = proc->mpidr;
+		else
+			__entry->mpidr = 0ULL;
+		__entry->midr = proc->midr;
+		if (proc->validation_bits & CPER_ARM_VALID_RUNNING_STATE) {
+			__entry->running_state = proc->running_state;
+			__entry->psci_state = proc->psci_state;
+		} else {
+			__entry->running_state = ~0;
+			__entry->psci_state = ~0;
+		}
+	),
+
+	TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
+		  "running state: %d; PSCI state: %d",
+		  __entry->affinity, __entry->mpidr, __entry->midr,
+		  __entry->running_state, __entry->psci_state)
+);
+
+/*
  * Unknown Section Report
  *
  * This event is generated when hardware detected a hardware
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1606172 — Re: [PATCH V13 09/10] trace, ras: add ARM processor error trace event

From	Xie XiuQi <xiexiuqi@huawei.com>
Date	2017-03-22 04:00 +0100
Subject	Re: [PATCH V13 09/10] trace, ras: add ARM processor error trace event
Message-ID	<tnEFb-40q-1@gated-at.bofh.it>
In reply to	#1606048

Reviewed-by: Xie XiuQi <xiexiuqi@huawei.com>

On 2017/3/22 6:47, Tyler Baicar wrote:
> Currently there are trace events for the various RAS
> errors with the exception of ARM processor type errors.
> Add a new trace event for such errors so that the user
> will know when they occur. These trace events are
> consistent with the ARM processor error section type
> defined in UEFI 2.6 spec section N.2.4.4.
> 
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> Acked-by: Steven Rostedt <rostedt@goodmis.org>
> ---
>  drivers/acpi/apei/ghes.c    |  8 +++++++-
>  drivers/firmware/efi/cper.c |  1 +
>  drivers/ras/ras.c           |  1 +
>  include/ras/ras_event.h     | 45 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 54 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 3ecbacc..230b095 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -515,7 +515,13 @@ static void ghes_do_proc(struct ghes *ghes,
>  		}
>  #endif
>  #ifdef CONFIG_RAS
> -		else if (trace_unknown_sec_event_enabled()) {
> +		else if (!uuid_le_cmp(sec_type, CPER_SEC_PROC_ARM) &&
> +			 trace_arm_event_enabled()) {
> +			struct cper_sec_proc_arm *arm_err;
> +
> +			arm_err = acpi_hest_generic_data_payload(gdata);
> +			trace_arm_event(arm_err);
> +		} else if (trace_unknown_sec_event_enabled()) {
>  			void *unknown_err = acpi_hest_generic_data_payload(gdata);
>  			trace_unknown_sec_event(&sec_type,
>  					fru_id, fru_text, sec_sev,
> diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
> index d263bc8..37a39af 100644
> --- a/drivers/firmware/efi/cper.c
> +++ b/drivers/firmware/efi/cper.c
> @@ -35,6 +35,7 @@
>  #include <linux/printk.h>
>  #include <linux/bcd.h>
>  #include <acpi/ghes.h>
> +#include <ras/ras_event.h>
>  
>  #define INDENT_SP	" "
>  
> diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
> index fb2500b..8ba5a94 100644
> --- a/drivers/ras/ras.c
> +++ b/drivers/ras/ras.c
> @@ -28,3 +28,4 @@ static int __init ras_init(void)
>  #endif
>  EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
>  EXPORT_TRACEPOINT_SYMBOL_GPL(unknown_sec_event);
> +EXPORT_TRACEPOINT_SYMBOL_GPL(arm_event);
> diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
> index 5861b6f..13befad 100644
> --- a/include/ras/ras_event.h
> +++ b/include/ras/ras_event.h
> @@ -162,6 +162,51 @@
>  );
>  
>  /*
> + * ARM Processor Events Report
> + *
> + * This event is generated when hardware detects an ARM processor error
> + * has occurred. UEFI 2.6 spec section N.2.4.4.
> + */
> +TRACE_EVENT(arm_event,
> +
> +	TP_PROTO(const struct cper_sec_proc_arm *proc),
> +
> +	TP_ARGS(proc),
> +
> +	TP_STRUCT__entry(
> +		__field(u64, mpidr)
> +		__field(u64, midr)
> +		__field(u32, running_state)
> +		__field(u32, psci_state)
> +		__field(u8, affinity)
> +	),
> +
> +	TP_fast_assign(
> +		if (proc->validation_bits & CPER_ARM_VALID_AFFINITY_LEVEL)
> +			__entry->affinity = proc->affinity_level;
> +		else
> +			__entry->affinity = ~0;
> +		if (proc->validation_bits & CPER_ARM_VALID_MPIDR)
> +			__entry->mpidr = proc->mpidr;
> +		else
> +			__entry->mpidr = 0ULL;
> +		__entry->midr = proc->midr;
> +		if (proc->validation_bits & CPER_ARM_VALID_RUNNING_STATE) {
> +			__entry->running_state = proc->running_state;
> +			__entry->psci_state = proc->psci_state;
> +		} else {
> +			__entry->running_state = ~0;
> +			__entry->psci_state = ~0;
> +		}
> +	),
> +
> +	TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
> +		  "running state: %d; PSCI state: %d",
> +		  __entry->affinity, __entry->mpidr, __entry->midr,
> +		  __entry->running_state, __entry->psci_state)
> +);
> +
> +/*
>   * Unknown Section Report
>   *
>   * This event is generated when hardware detected a hardware
> 

-- 
Thanks,
Xie XiuQi

[toc] | [prev] | [next] | [standalone]

#1606178 — Re: [PATCH V13 09/10] trace, ras: add ARM processor error trace event

From	"Baicar, Tyler" <tbaicar@codeaurora.org>
Date	2017-03-22 04:10 +0100
Subject	Re: [PATCH V13 09/10] trace, ras: add ARM processor error trace event
Message-ID	<tnEOS-4iL-21@gated-at.bofh.it>
In reply to	#1606172

Hello Xie XiuQi,

On 3/21/2017 8:48 PM, Xie XiuQi wrote:
> Reviewed-by: Xie XiuQi <xiexiuqi@huawei.com>
Thank you!
Tyler
>
> On 2017/3/22 6:47, Tyler Baicar wrote:
>> Currently there are trace events for the various RAS
>> errors with the exception of ARM processor type errors.
>> Add a new trace event for such errors so that the user
>> will know when they occur. These trace events are
>> consistent with the ARM processor error section type
>> defined in UEFI 2.6 spec section N.2.4.4.
>>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> Acked-by: Steven Rostedt <rostedt@goodmis.org>
>> ---
>>   drivers/acpi/apei/ghes.c    |  8 +++++++-
>>   drivers/firmware/efi/cper.c |  1 +
>>   drivers/ras/ras.c           |  1 +
>>   include/ras/ras_event.h     | 45 +++++++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 54 insertions(+), 1 deletion(-)
>>
>>

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1606049 — [PATCH V13 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

From	Tyler Baicar <tbaicar@codeaurora.org>
Date	2017-03-21 23:50 +0100
Subject	[PATCH V13 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1
Message-ID	<tnALg-1nk-9@gated-at.bofh.it>
In reply to	#1606046

Currently when a RAS error is reported it is not timestamped.
The ACPI 6.1 spec adds the timestamp field to the generic error
data entry v3 structure. The timestamp of when the firmware
generated the error is now being reported.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Reviewed-by: James Morse <james.morse@arm.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 drivers/acpi/apei/ghes.c    |  9 ++++---
 drivers/firmware/efi/cper.c | 63 +++++++++++++++++++++++++++++++++++----------
 include/acpi/ghes.h         | 22 ++++++++++++++++
 3 files changed, 77 insertions(+), 17 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 0241e36..9ddbb93 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -421,7 +421,8 @@ static void ghes_handle_memory_failure(struct acpi_hest_generic_data *gdata, int
 	int flags = -1;
 	int sec_sev = ghes_severity(gdata->error_severity);
 	struct cper_sec_mem_err *mem_err;
-	mem_err = (struct cper_sec_mem_err *)(gdata + 1);
+
+	mem_err = acpi_hest_generic_data_payload(gdata);
 
 	if (!(mem_err->validation_bits & CPER_MEM_VALID_PA))
 		return;
@@ -458,7 +459,8 @@ static void ghes_do_proc(struct ghes *ghes,
 		if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
 				 CPER_SEC_PLATFORM_MEM)) {
 			struct cper_sec_mem_err *mem_err;
-			mem_err = (struct cper_sec_mem_err *)(gdata+1);
+
+			mem_err = acpi_hest_generic_data_payload(gdata);
 			ghes_edac_report_mem_error(ghes, sev, mem_err);
 
 			arch_apei_report_mem_error(sev, mem_err);
@@ -468,7 +470,8 @@ static void ghes_do_proc(struct ghes *ghes,
 		else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
 				      CPER_SEC_PCIE)) {
 			struct cper_sec_pcie *pcie_err;
-			pcie_err = (struct cper_sec_pcie *)(gdata+1);
+
+			pcie_err = acpi_hest_generic_data_payload(gdata);
 			if (sev == GHES_SEV_RECOVERABLE &&
 			    sec_sev == GHES_SEV_RECOVERABLE &&
 			    pcie_err->validation_bits & CPER_PCIE_VALID_DEVICE_ID &&
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index d425374..8fa4e23 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -32,6 +32,9 @@
 #include <linux/acpi.h>
 #include <linux/pci.h>
 #include <linux/aer.h>
+#include <linux/printk.h>
+#include <linux/bcd.h>
+#include <acpi/ghes.h>
 
 #define INDENT_SP	" "
 
@@ -386,13 +389,37 @@ static void cper_print_pcie(const char *pfx, const struct cper_sec_pcie *pcie,
 	pfx, pcie->bridge.secondary_status, pcie->bridge.control);
 }
 
+static void cper_estatus_print_section_v300(const char *pfx,
+	const struct acpi_hest_generic_data_v300 *gdata)
+{
+	__u8 hour, min, sec, day, mon, year, century, *timestamp;
+
+	if (gdata->validation_bits & ACPI_HEST_GEN_VALID_TIMESTAMP) {
+		timestamp = (__u8 *)&(gdata->time_stamp);
+		sec = bcd2bin(timestamp[0]);
+		min = bcd2bin(timestamp[1]);
+		hour = bcd2bin(timestamp[2]);
+		day = bcd2bin(timestamp[4]);
+		mon = bcd2bin(timestamp[5]);
+		year = bcd2bin(timestamp[6]);
+		century = bcd2bin(timestamp[7]);
+		printk("%stime: %7s %02d%02d-%02d-%02d %02d:%02d:%02d\n", pfx,
+			0x01 & *(timestamp + 3) ? "precise" : "", century,
+			year, mon, day, hour, min, sec);
+	}
+}
+
 static void cper_estatus_print_section(
-	const char *pfx, const struct acpi_hest_generic_data *gdata, int sec_no)
+	const char *pfx, struct acpi_hest_generic_data *gdata, int sec_no)
 {
 	uuid_le *sec_type = (uuid_le *)gdata->section_type;
 	__u16 severity;
 	char newpfx[64];
 
+	if (acpi_hest_generic_data_version(gdata) >= 3)
+		cper_estatus_print_section_v300(pfx,
+			(const struct acpi_hest_generic_data_v300 *)gdata);
+
 	severity = gdata->error_severity;
 	printk("%s""Error %d, type: %s\n", pfx, sec_no,
 	       cper_severity_str(severity));
@@ -403,14 +430,18 @@ static void cper_estatus_print_section(
 
 	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
 	if (!uuid_le_cmp(*sec_type, CPER_SEC_PROC_GENERIC)) {
-		struct cper_sec_proc_generic *proc_err = (void *)(gdata + 1);
+		struct cper_sec_proc_generic *proc_err;
+
+		proc_err = acpi_hest_generic_data_payload(gdata);
 		printk("%s""section_type: general processor error\n", newpfx);
 		if (gdata->error_data_length >= sizeof(*proc_err))
 			cper_print_proc_generic(newpfx, proc_err);
 		else
 			goto err_section_too_small;
 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PLATFORM_MEM)) {
-		struct cper_sec_mem_err *mem_err = (void *)(gdata + 1);
+		struct cper_sec_mem_err *mem_err;
+
+		mem_err = acpi_hest_generic_data_payload(gdata);
 		printk("%s""section_type: memory error\n", newpfx);
 		if (gdata->error_data_length >=
 		    sizeof(struct cper_sec_mem_err_old))
@@ -419,7 +450,9 @@ static void cper_estatus_print_section(
 		else
 			goto err_section_too_small;
 	} else if (!uuid_le_cmp(*sec_type, CPER_SEC_PCIE)) {
-		struct cper_sec_pcie *pcie = (void *)(gdata + 1);
+		struct cper_sec_pcie *pcie;
+
+		pcie = acpi_hest_generic_data_payload(gdata);
 		printk("%s""section_type: PCIe error\n", newpfx);
 		if (gdata->error_data_length >= sizeof(*pcie))
 			cper_print_pcie(newpfx, pcie, gdata);
@@ -438,7 +471,7 @@ void cper_estatus_print(const char *pfx,
 			const struct acpi_hest_generic_status *estatus)
 {
 	struct acpi_hest_generic_data *gdata;
-	unsigned int data_len, gedata_len;
+	unsigned int data_len;
 	int sec_no = 0;
 	char newpfx[64];
 	__u16 severity;
@@ -451,12 +484,13 @@ void cper_estatus_print(const char *pfx,
 	printk("%s""event severity: %s\n", pfx, cper_severity_str(severity));
 	data_len = estatus->data_length;
 	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
+
 	snprintf(newpfx, sizeof(newpfx), "%s%s", pfx, INDENT_SP);
-	while (data_len >= sizeof(*gdata)) {
-		gedata_len = gdata->error_data_length;
+
+	while (data_len >= acpi_hest_generic_data_size(gdata)) {
 		cper_estatus_print_section(newpfx, gdata, sec_no);
-		data_len -= gedata_len + sizeof(*gdata);
-		gdata = (void *)(gdata + 1) + gedata_len;
+		data_len -= acpi_hest_generic_data_record_size(gdata);
+		gdata = acpi_hest_generic_data_next(gdata);
 		sec_no++;
 	}
 }
@@ -486,12 +520,13 @@ int cper_estatus_check(const struct acpi_hest_generic_status *estatus)
 		return rc;
 	data_len = estatus->data_length;
 	gdata = (struct acpi_hest_generic_data *)(estatus + 1);
-	while (data_len >= sizeof(*gdata)) {
-		gedata_len = gdata->error_data_length;
-		if (gedata_len > data_len - sizeof(*gdata))
+
+	while (data_len >= acpi_hest_generic_data_size(gdata)) {
+		gedata_len = acpi_hest_generic_data_error_length(gdata);
+		if (gedata_len > data_len - acpi_hest_generic_data_size(gdata))
 			return -EINVAL;
-		data_len -= gedata_len + sizeof(*gdata);
-		gdata = (void *)(gdata + 1) + gedata_len;
+		data_len -= gedata_len + acpi_hest_generic_data_size(gdata);
+		gdata = acpi_hest_generic_data_next(gdata);
 	}
 	if (data_len)
 		return -EINVAL;
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 68f088a..6ae318b 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -12,6 +12,18 @@
 #define GHES_TO_CLEAR		0x0001
 #define GHES_EXITING		0x0002
 
+#define acpi_hest_generic_data_error_length(gdata)	\
+	(((struct acpi_hest_generic_data *)(gdata))->error_data_length)
+#define acpi_hest_generic_data_size(gdata)		\
+	((acpi_hest_generic_data_version(gdata) >= 3) ?	\
+	sizeof(struct acpi_hest_generic_data_v300) :	\
+	sizeof(struct acpi_hest_generic_data))
+#define acpi_hest_generic_data_record_size(gdata)	\
+	(acpi_hest_generic_data_size(gdata) +		\
+	acpi_hest_generic_data_error_length(gdata))
+#define acpi_hest_generic_data_next(gdata)		\
+	((void *)(gdata) + acpi_hest_generic_data_record_size(gdata))
+
 struct ghes {
 	union {
 		struct acpi_hest_generic *generic;
@@ -73,3 +85,13 @@ static inline void ghes_edac_unregister(struct ghes *ghes)
 {
 }
 #endif
+
+#define acpi_hest_generic_data_version(gdata)			\
+	(gdata->revision >> 8)
+
+static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data *gdata)
+{
+	return acpi_hest_generic_data_version(gdata) >= 3 ?
+		(void *)(((struct acpi_hest_generic_data_v300 *)(gdata)) + 1) :
+		gdata + 1;
+}
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1606051 — [PATCH V13 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section

From	Tyler Baicar <tbaicar@codeaurora.org>
Date	2017-03-21 23:50 +0100
Subject	[PATCH V13 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section
Message-ID	<tnALg-1nk-11@gated-at.bofh.it>
In reply to	#1606046

UEFI spec allows for non-standard section in Common Platform Error
Record. This is defined in section N.2.3 of UEFI version 2.5.

Currently if the CPER section's type (UUID) does not match with
any section type that the kernel knows how to parse, trace event
is not generated for such section. And thus user is not able to know
happening of such hardware error, including error record of
non-standard section.

This commit generates a trace event which contains raw error data
for unrecognized CPER section.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
---
 drivers/acpi/apei/ghes.c | 24 ++++++++++++++++++++++--
 drivers/ras/ras.c        |  1 +
 include/ras/ras_event.h  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 7e3e5e0..3ecbacc 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -45,11 +45,13 @@
 #include <linux/aer.h>
 #include <linux/nmi.h>
 #include <linux/sched/clock.h>
+#include <linux/uuid.h>
 
 #include <acpi/actbl1.h>
 #include <acpi/ghes.h>
 #include <acpi/apei.h>
 #include <asm/tlbflush.h>
+#include <ras/ras_event.h>
 
 #include "apei-internal.h"
 
@@ -454,11 +456,21 @@ static void ghes_do_proc(struct ghes *ghes,
 {
 	int sev, sec_sev;
 	struct acpi_hest_generic_data *gdata;
+	uuid_le sec_type;
+	uuid_le *fru_id = &NULL_UUID_LE;
+	char *fru_text = "";
 
 	sev = ghes_severity(estatus->error_severity);
 	apei_estatus_for_each_section(estatus, gdata) {
 		sec_sev = ghes_severity(gdata->error_severity);
-		if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
+		sec_type = *(uuid_le *)gdata->section_type;
+
+		if (gdata->validation_bits & CPER_SEC_VALID_FRU_ID)
+			fru_id = (uuid_le *)gdata->fru_id;
+		if (gdata->validation_bits & CPER_SEC_VALID_FRU_TEXT)
+			fru_text = gdata->fru_text;
+
+		if (!uuid_le_cmp(sec_type,
 				 CPER_SEC_PLATFORM_MEM)) {
 			struct cper_sec_mem_err *mem_err;
 
@@ -469,7 +481,7 @@ static void ghes_do_proc(struct ghes *ghes,
 			ghes_handle_memory_failure(gdata, sev);
 		}
 #ifdef CONFIG_ACPI_APEI_PCIEAER
-		else if (!uuid_le_cmp(*(uuid_le *)gdata->section_type,
+		else if (!uuid_le_cmp(sec_type,
 				      CPER_SEC_PCIE)) {
 			struct cper_sec_pcie *pcie_err;
 
@@ -502,6 +514,14 @@ static void ghes_do_proc(struct ghes *ghes,
 
 		}
 #endif
+#ifdef CONFIG_RAS
+		else if (trace_unknown_sec_event_enabled()) {
+			void *unknown_err = acpi_hest_generic_data_payload(gdata);
+			trace_unknown_sec_event(&sec_type,
+					fru_id, fru_text, sec_sev,
+					unknown_err, gdata->error_data_length);
+		}
+#endif
 	}
 }
 
diff --git a/drivers/ras/ras.c b/drivers/ras/ras.c
index b67dd36..fb2500b 100644
--- a/drivers/ras/ras.c
+++ b/drivers/ras/ras.c
@@ -27,3 +27,4 @@ static int __init ras_init(void)
 EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
 #endif
 EXPORT_TRACEPOINT_SYMBOL_GPL(mc_event);
+EXPORT_TRACEPOINT_SYMBOL_GPL(unknown_sec_event);
diff --git a/include/ras/ras_event.h b/include/ras/ras_event.h
index 1791a12..5861b6f 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -162,6 +162,51 @@
 );
 
 /*
+ * Unknown Section Report
+ *
+ * This event is generated when hardware detected a hardware
+ * error event, which may be of non-standard section as defined
+ * in UEFI spec appendix "Common Platform Error Record", or may
+ * be of sections for which TRACE_EVENT is not defined.
+ *
+ */
+TRACE_EVENT(unknown_sec_event,
+
+	TP_PROTO(const uuid_le *sec_type,
+		 const uuid_le *fru_id,
+		 const char *fru_text,
+		 const u8 sev,
+		 const u8 *err,
+		 const u32 len),
+
+	TP_ARGS(sec_type, fru_id, fru_text, sev, err, len),
+
+	TP_STRUCT__entry(
+		__array(char, sec_type, 16)
+		__array(char, fru_id, 16)
+		__string(fru_text, fru_text)
+		__field(u8, sev)
+		__field(u32, len)
+		__dynamic_array(u8, buf, len)
+	),
+
+	TP_fast_assign(
+		memcpy(__entry->sec_type, sec_type, sizeof(uuid_le));
+		memcpy(__entry->fru_id, fru_id, sizeof(uuid_le));
+		__assign_str(fru_text, fru_text);
+		__entry->sev = sev;
+		__entry->len = len;
+		memcpy(__get_dynamic_array(buf), err, len);
+	),
+
+	TP_printk("severity: %d; sec type:%pU; FRU: %pU %s; data len:%d; raw data:%s",
+		  __entry->sev, __entry->sec_type,
+		  __entry->fru_id, __get_str(fru_text),
+		  __entry->len,
+		  __print_hex(__get_dynamic_array(buf), __entry->len))
+);
+
+/*
  * PCIe AER Trace event
  *
  * These events are generated when hardware detects a corrected or
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1606052 — [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	Tyler Baicar <tbaicar@codeaurora.org>
Date	2017-03-21 23:50 +0100
Subject	[PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tnALg-1nk-19@gated-at.bofh.it>
In reply to	#1606046

Currently external aborts are unsupported by the guest abort
handling. Add handling for SEAs so that the host kernel reports
SEAs which occur in the guest kernel.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
---
 arch/arm/include/asm/kvm_arm.h       | 10 +++++++++
 arch/arm/include/asm/system_misc.h   |  5 +++++
 arch/arm/kvm/mmu.c                   | 41 ++++++++++++++++++++++++++++++------
 arch/arm64/include/asm/kvm_arm.h     | 10 +++++++++
 arch/arm64/include/asm/system_misc.h |  2 ++
 arch/arm64/mm/fault.c                | 19 +++++++++++++++--
 drivers/acpi/apei/ghes.c             | 13 ++++++------
 include/acpi/ghes.h                  |  2 +-
 8 files changed, 86 insertions(+), 16 deletions(-)

diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
index a3f0b3d..ebf020b 100644
--- a/arch/arm/include/asm/kvm_arm.h
+++ b/arch/arm/include/asm/kvm_arm.h
@@ -187,6 +187,16 @@
 #define FSC_FAULT	(0x04)
 #define FSC_ACCESS	(0x08)
 #define FSC_PERM	(0x0c)
+#define FSC_SEA		(0x10)
+#define FSC_SEA_TTW0	(0x14)
+#define FSC_SEA_TTW1	(0x15)
+#define FSC_SEA_TTW2	(0x16)
+#define FSC_SEA_TTW3	(0x17)
+#define FSC_SECC	(0x18)
+#define FSC_SECC_TTW0	(0x1c)
+#define FSC_SECC_TTW1	(0x1d)
+#define FSC_SECC_TTW2	(0x1e)
+#define FSC_SECC_TTW3	(0x1f)
 
 /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
 #define HPFAR_MASK	(~0xf)
diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
index a3d61ad..ea45d94 100644
--- a/arch/arm/include/asm/system_misc.h
+++ b/arch/arm/include/asm/system_misc.h
@@ -24,4 +24,9 @@
 
 #endif /* !__ASSEMBLY__ */
 
+static inline int handle_guest_sea(unsigned long addr, unsigned int esr)
+{
+	return -1;
+}
+
 #endif /* __ASM_ARM_SYSTEM_MISC_H */
diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 962616f..105b6ab 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -29,6 +29,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_emulate.h>
 #include <asm/virt.h>
+#include <asm/system_misc.h>
 
 #include "trace.h"
 
@@ -1406,6 +1407,24 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
 		kvm_set_pfn_accessed(pfn);
 }
 
+static bool is_abort_synchronous(unsigned long fault_status) {
+	switch (fault_status) {
+	case FSC_SEA:
+	case FSC_SEA_TTW0:
+	case FSC_SEA_TTW1:
+	case FSC_SEA_TTW2:
+	case FSC_SEA_TTW3:
+	case FSC_SECC:
+	case FSC_SECC_TTW0:
+	case FSC_SECC_TTW1:
+	case FSC_SECC_TTW2:
+	case FSC_SECC_TTW3:
+		return true;
+	default:
+		return false;
+	}
+}
+
 /**
  * kvm_handle_guest_abort - handles all 2nd stage aborts
  * @vcpu:	the VCPU pointer
@@ -1426,23 +1445,31 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
 	unsigned long hva;
 	bool is_iabt, write_fault, writable;
 	gfn_t gfn;
-	int ret, idx;
+	int ret, idx, sea_status = 1;
+
+	/* Check the stage-2 fault is trans. fault or write fault */
+	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
+
+	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
+
+	/* The host kernel will handle the synchronous external abort. There
+	 * is no need to pass the error into the guest.
+	 */
+	if (is_abort_synchronous(fault_status))
+		sea_status = handle_guest_sea((unsigned long)fault_ipa,
+				    kvm_vcpu_get_hsr(vcpu));
 
 	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
-	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
+	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
 		kvm_inject_vabt(vcpu);
 		return 1;
 	}
 
-	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
-
 	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
 			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
 
-	/* Check the stage-2 fault is trans. fault or write fault */
-	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
 	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
-	    fault_status != FSC_ACCESS) {
+	    fault_status != FSC_ACCESS && sea_status) {
 		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
 			kvm_vcpu_trap_get_class(vcpu),
 			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 6e99978..61d694c 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -204,6 +204,16 @@
 #define FSC_FAULT	ESR_ELx_FSC_FAULT
 #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
 #define FSC_PERM	ESR_ELx_FSC_PERM
+#define FSC_SEA		ESR_ELx_FSC_EXTABT
+#define FSC_SEA_TTW0	(0x14)
+#define FSC_SEA_TTW1	(0x15)
+#define FSC_SEA_TTW2	(0x16)
+#define FSC_SEA_TTW3	(0x17)
+#define FSC_SECC	(0x18)
+#define FSC_SECC_TTW0	(0x1c)
+#define FSC_SECC_TTW1	(0x1d)
+#define FSC_SECC_TTW2	(0x1e)
+#define FSC_SECC_TTW3	(0x1f)
 
 /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
 #define HPFAR_MASK	(~UL(0xf))
diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
index bc81243..5b2cecd 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -58,4 +58,6 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
 
 #endif	/* __ASSEMBLY__ */
 
+int handle_guest_sea(unsigned long addr, unsigned int esr);
+
 #endif	/* __ASM_SYSTEM_MISC_H */
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index f7372ce..ee96307 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -497,6 +497,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 {
 	struct siginfo info;
+	int ret = 0;
 
 	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
 		 fault_name(esr), esr, addr);
@@ -508,7 +509,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 	 */
 	if (IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
 		nmi_enter();
-		ghes_notify_sea();
+		ret = ghes_notify_sea();
 		nmi_exit();
 	}
 
@@ -521,7 +522,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 		info.si_addr  = (void __user *)addr;
 	arm64_notify_die("", regs, &info, esr);
 
-	return 0;
+	return ret;
 }
 
 static const struct fault_info {
@@ -603,6 +604,20 @@ static const char *fault_name(unsigned int esr)
 }
 
 /*
+ * Handle Synchronous External Aborts that occur in a guest kernel.
+ */
+int handle_guest_sea(unsigned long addr, unsigned int esr)
+{
+	int ret = -ENOENT;
+
+	if(IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
+		ret = ghes_notify_sea();
+	}
+
+	return ret;
+}
+
+/*
  * Dispatch a data abort to the relevant handler.
  */
 asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 230b095..81eabc6 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -810,17 +810,18 @@ static int ghes_notify_sci(struct notifier_block *this,
 #ifdef CONFIG_ACPI_APEI_SEA
 static LIST_HEAD(ghes_sea);
 
-void ghes_notify_sea(void)
+int ghes_notify_sea(void)
 {
 	struct ghes *ghes;
+	int ret = -ENOENT;
 
-	/*
-	 * synchronize_rcu() will wait for nmi_exit(), so no need to
-	 * rcu_read_lock().
-	 */
+	rcu_read_lock();
 	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
-		ghes_proc(ghes);
+		if(!ghes_proc(ghes))
+			ret = 0;
 	}
+	rcu_read_unlock();
+	return ret;
 }
 
 static void ghes_sea_add(struct ghes *ghes)
diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
index 18bc935..2a727dc 100644
--- a/include/acpi/ghes.h
+++ b/include/acpi/ghes.h
@@ -99,6 +99,6 @@ static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data
 		gdata + 1;
 }
 
-void ghes_notify_sea(void);
+int ghes_notify_sea(void);
 
 #endif /* GHES_H */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1607757 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	Catalin Marinas <catalin.marinas@arm.com>
Date	2017-03-23 19:00 +0100
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tofbH-5C2-9@gated-at.bofh.it>
In reply to	#1606052

On Tue, Mar 21, 2017 at 04:47:05PM -0600, Tyler Baicar wrote:
> Currently external aborts are unsupported by the guest abort
> handling. Add handling for SEAs so that the host kernel reports
> SEAs which occur in the guest kernel.
> 
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> ---
>  arch/arm/include/asm/kvm_arm.h       | 10 +++++++++
>  arch/arm/include/asm/system_misc.h   |  5 +++++
>  arch/arm/kvm/mmu.c                   | 41 ++++++++++++++++++++++++++++++------
>  arch/arm64/include/asm/kvm_arm.h     | 10 +++++++++
>  arch/arm64/include/asm/system_misc.h |  2 ++
>  arch/arm64/mm/fault.c                | 19 +++++++++++++++--
>  drivers/acpi/apei/ghes.c             | 13 ++++++------
>  include/acpi/ghes.h                  |  2 +-
>  8 files changed, 86 insertions(+), 16 deletions(-)

For arm64:

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Marc, Christoffer, could you please check the kvm bits in here? Thanks.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1608414 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	Marc Zyngier <marc.zyngier@arm.com>
Date	2017-03-24 15:10 +0100
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<toy4F-2Fu-1@gated-at.bofh.it>
In reply to	#1606052

On 21/03/17 22:47, Tyler Baicar wrote:
> Currently external aborts are unsupported by the guest abort
> handling. Add handling for SEAs so that the host kernel reports
> SEAs which occur in the guest kernel.
> 
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> ---
>  arch/arm/include/asm/kvm_arm.h       | 10 +++++++++
>  arch/arm/include/asm/system_misc.h   |  5 +++++
>  arch/arm/kvm/mmu.c                   | 41 ++++++++++++++++++++++++++++++------
>  arch/arm64/include/asm/kvm_arm.h     | 10 +++++++++
>  arch/arm64/include/asm/system_misc.h |  2 ++
>  arch/arm64/mm/fault.c                | 19 +++++++++++++++--
>  drivers/acpi/apei/ghes.c             | 13 ++++++------
>  include/acpi/ghes.h                  |  2 +-
>  8 files changed, 86 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index a3f0b3d..ebf020b 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -187,6 +187,16 @@
>  #define FSC_FAULT	(0x04)
>  #define FSC_ACCESS	(0x08)
>  #define FSC_PERM	(0x0c)
> +#define FSC_SEA		(0x10)
> +#define FSC_SEA_TTW0	(0x14)
> +#define FSC_SEA_TTW1	(0x15)
> +#define FSC_SEA_TTW2	(0x16)
> +#define FSC_SEA_TTW3	(0x17)
> +#define FSC_SECC	(0x18)
> +#define FSC_SECC_TTW0	(0x1c)
> +#define FSC_SECC_TTW1	(0x1d)
> +#define FSC_SECC_TTW2	(0x1e)
> +#define FSC_SECC_TTW3	(0x1f)
>  
>  /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>  #define HPFAR_MASK	(~0xf)
> diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
> index a3d61ad..ea45d94 100644
> --- a/arch/arm/include/asm/system_misc.h
> +++ b/arch/arm/include/asm/system_misc.h
> @@ -24,4 +24,9 @@
>  
>  #endif /* !__ASSEMBLY__ */
>  
> +static inline int handle_guest_sea(unsigned long addr, unsigned int esr)
> +{
> +	return -1;
> +}
> +

Shouldn't this be in the #ifndef __ASSEMBLY__ block? The assembler is
definitely going to barf on that...

>  #endif /* __ASM_ARM_SYSTEM_MISC_H */
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 962616f..105b6ab 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -29,6 +29,7 @@
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/virt.h>
> +#include <asm/system_misc.h>
>  
>  #include "trace.h"
>  
> @@ -1406,6 +1407,24 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>  		kvm_set_pfn_accessed(pfn);
>  }
>  
> +static bool is_abort_synchronous(unsigned long fault_status) {
> +	switch (fault_status) {
> +	case FSC_SEA:
> +	case FSC_SEA_TTW0:
> +	case FSC_SEA_TTW1:
> +	case FSC_SEA_TTW2:
> +	case FSC_SEA_TTW3:
> +	case FSC_SECC:
> +	case FSC_SECC_TTW0:
> +	case FSC_SECC_TTW1:
> +	case FSC_SECC_TTW2:
> +	case FSC_SECC_TTW3:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /**
>   * kvm_handle_guest_abort - handles all 2nd stage aborts
>   * @vcpu:	the VCPU pointer
> @@ -1426,23 +1445,31 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	unsigned long hva;
>  	bool is_iabt, write_fault, writable;
>  	gfn_t gfn;
> -	int ret, idx;
> +	int ret, idx, sea_status = 1;
> +
> +	/* Check the stage-2 fault is trans. fault or write fault */
> +	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
> +
> +	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> +
> +	/* The host kernel will handle the synchronous external abort. There
> +	 * is no need to pass the error into the guest.
> +	 */
> +	if (is_abort_synchronous(fault_status))
> +		sea_status = handle_guest_sea((unsigned long)fault_ipa,
> +				    kvm_vcpu_get_hsr(vcpu));
>  
>  	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>  		kvm_inject_vabt(vcpu);
>  		return 1;
>  	}
>  
> -	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> -
>  	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
>  			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
>  
> -	/* Check the stage-2 fault is trans. fault or write fault */
> -	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>  	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
> -	    fault_status != FSC_ACCESS) {
> +	    fault_status != FSC_ACCESS && sea_status) {
>  		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>  			kvm_vcpu_trap_get_class(vcpu),
>  			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 6e99978..61d694c 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -204,6 +204,16 @@
>  #define FSC_FAULT	ESR_ELx_FSC_FAULT
>  #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
>  #define FSC_PERM	ESR_ELx_FSC_PERM
> +#define FSC_SEA		ESR_ELx_FSC_EXTABT
> +#define FSC_SEA_TTW0	(0x14)
> +#define FSC_SEA_TTW1	(0x15)
> +#define FSC_SEA_TTW2	(0x16)
> +#define FSC_SEA_TTW3	(0x17)
> +#define FSC_SECC	(0x18)
> +#define FSC_SECC_TTW0	(0x1c)
> +#define FSC_SECC_TTW1	(0x1d)
> +#define FSC_SECC_TTW2	(0x1e)
> +#define FSC_SECC_TTW3	(0x1f)
>  
>  /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>  #define HPFAR_MASK	(~UL(0xf))
> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index bc81243..5b2cecd 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -58,4 +58,6 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
>  
>  #endif	/* __ASSEMBLY__ */
>  
> +int handle_guest_sea(unsigned long addr, unsigned int esr);

Same remark here.

> +
>  #endif	/* __ASM_SYSTEM_MISC_H */
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index f7372ce..ee96307 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -497,6 +497,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  {
>  	struct siginfo info;
> +	int ret = 0;
>  
>  	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>  		 fault_name(esr), esr, addr);
> @@ -508,7 +509,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  	 */
>  	if (IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
>  		nmi_enter();
> -		ghes_notify_sea();
> +		ret = ghes_notify_sea();
>  		nmi_exit();
>  	}
>  
> @@ -521,7 +522,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  		info.si_addr  = (void __user *)addr;
>  	arm64_notify_die("", regs, &info, esr);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static const struct fault_info {
> @@ -603,6 +604,20 @@ static const char *fault_name(unsigned int esr)
>  }
>  
>  /*
> + * Handle Synchronous External Aborts that occur in a guest kernel.
> + */
> +int handle_guest_sea(unsigned long addr, unsigned int esr)
> +{
> +	int ret = -ENOENT;
> +
> +	if(IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
> +		ret = ghes_notify_sea();
> +	}
> +
> +	return ret;
> +}
> +
> +/*
>   * Dispatch a data abort to the relevant handler.
>   */
>  asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 230b095..81eabc6 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -810,17 +810,18 @@ static int ghes_notify_sci(struct notifier_block *this,
>  #ifdef CONFIG_ACPI_APEI_SEA
>  static LIST_HEAD(ghes_sea);
>  
> -void ghes_notify_sea(void)
> +int ghes_notify_sea(void)
>  {
>  	struct ghes *ghes;
> +	int ret = -ENOENT;
>  
> -	/*
> -	 * synchronize_rcu() will wait for nmi_exit(), so no need to
> -	 * rcu_read_lock().
> -	 */
> +	rcu_read_lock();
>  	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
> -		ghes_proc(ghes);
> +		if(!ghes_proc(ghes))
> +			ret = 0;
>  	}
> +	rcu_read_unlock();
> +	return ret;
>  }
>  
>  static void ghes_sea_add(struct ghes *ghes)
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 18bc935..2a727dc 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -99,6 +99,6 @@ static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data
>  		gdata + 1;
>  }
>  
> -void ghes_notify_sea(void);
> +int ghes_notify_sea(void);
>  
>  #endif /* GHES_H */
> 

Otherwise, the KVM changes look good to me. Assuming you fix the two
nits above:

Acked-by: Marc Zyngier <marc.zyngier@arm.com>

Thanks,
	
	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1611197 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	"Baicar, Tyler" <tbaicar@codeaurora.org>
Date	2017-03-28 18:40 +0200
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tq2k1-1UQ-11@gated-at.bofh.it>
In reply to	#1608414

Hello Marc,


On 3/24/2017 8:03 AM, Marc Zyngier wrote:
> On 21/03/17 22:47, Tyler Baicar wrote:
>> Currently external aborts are unsupported by the guest abort
>> handling. Add handling for SEAs so that the host kernel reports
>> SEAs which occur in the guest kernel.
>>
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> ---
>>   arch/arm/include/asm/kvm_arm.h       | 10 +++++++++
>>   arch/arm/include/asm/system_misc.h   |  5 +++++
>>   arch/arm/kvm/mmu.c                   | 41 ++++++++++++++++++++++++++++++------
>>   arch/arm64/include/asm/kvm_arm.h     | 10 +++++++++
>>   arch/arm64/include/asm/system_misc.h |  2 ++
>>   arch/arm64/mm/fault.c                | 19 +++++++++++++++--
>>   drivers/acpi/apei/ghes.c             | 13 ++++++------
>>   include/acpi/ghes.h                  |  2 +-
>>   8 files changed, 86 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
>> index a3f0b3d..ebf020b 100644
>> --- a/arch/arm/include/asm/kvm_arm.h
>> +++ b/arch/arm/include/asm/kvm_arm.h
>> @@ -187,6 +187,16 @@
>>   #define FSC_FAULT	(0x04)
>>   #define FSC_ACCESS	(0x08)
>>   #define FSC_PERM	(0x0c)
>> +#define FSC_SEA		(0x10)
>> +#define FSC_SEA_TTW0	(0x14)
>> +#define FSC_SEA_TTW1	(0x15)
>> +#define FSC_SEA_TTW2	(0x16)
>> +#define FSC_SEA_TTW3	(0x17)
>> +#define FSC_SECC	(0x18)
>> +#define FSC_SECC_TTW0	(0x1c)
>> +#define FSC_SECC_TTW1	(0x1d)
>> +#define FSC_SECC_TTW2	(0x1e)
>> +#define FSC_SECC_TTW3	(0x1f)
>>   
>>   /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>   #define HPFAR_MASK	(~0xf)
>> diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
>> index a3d61ad..ea45d94 100644
>> --- a/arch/arm/include/asm/system_misc.h
>> +++ b/arch/arm/include/asm/system_misc.h
>> @@ -24,4 +24,9 @@
>>   
>>   #endif /* !__ASSEMBLY__ */
>>   
>> +static inline int handle_guest_sea(unsigned long addr, unsigned int esr)
>> +{
>> +	return -1;
>> +}
>> +
> Shouldn't this be in the #ifndef __ASSEMBLY__ block? The assembler is
> definitely going to barf on that...
I will move this in the next patch set.
>>   #endif /* __ASM_ARM_SYSTEM_MISC_H */
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 962616f..105b6ab 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -29,6 +29,7 @@
>>   #include <asm/kvm_asm.h>
>>   #include <asm/kvm_emulate.h>
>>   #include <asm/virt.h>
>> +#include <asm/system_misc.h>
>>   
>>   #include "trace.h"
>>   
>> @@ -1406,6 +1407,24 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>>   		kvm_set_pfn_accessed(pfn);
>>   }
>>   
>> +static bool is_abort_synchronous(unsigned long fault_status) {
>> +	switch (fault_status) {
>> +	case FSC_SEA:
>> +	case FSC_SEA_TTW0:
>> +	case FSC_SEA_TTW1:
>> +	case FSC_SEA_TTW2:
>> +	case FSC_SEA_TTW3:
>> +	case FSC_SECC:
>> +	case FSC_SECC_TTW0:
>> +	case FSC_SECC_TTW1:
>> +	case FSC_SECC_TTW2:
>> +	case FSC_SECC_TTW3:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>   /**
>>    * kvm_handle_guest_abort - handles all 2nd stage aborts
>>    * @vcpu:	the VCPU pointer
>> @@ -1426,23 +1445,31 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>   	unsigned long hva;
>>   	bool is_iabt, write_fault, writable;
>>   	gfn_t gfn;
>> -	int ret, idx;
>> +	int ret, idx, sea_status = 1;
>> +
>> +	/* Check the stage-2 fault is trans. fault or write fault */
>> +	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>> +
>> +	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
>> +
>> +	/* The host kernel will handle the synchronous external abort. There
>> +	 * is no need to pass the error into the guest.
>> +	 */
>> +	if (is_abort_synchronous(fault_status))
>> +		sea_status = handle_guest_sea((unsigned long)fault_ipa,
>> +				    kvm_vcpu_get_hsr(vcpu));
>>   
>>   	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
>> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
>> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>>   		kvm_inject_vabt(vcpu);
>>   		return 1;
>>   	}
>>   
>> -	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
>> -
>>   	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
>>   			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
>>   
>> -	/* Check the stage-2 fault is trans. fault or write fault */
>> -	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>>   	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
>> -	    fault_status != FSC_ACCESS) {
>> +	    fault_status != FSC_ACCESS && sea_status) {
>>   		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>>   			kvm_vcpu_trap_get_class(vcpu),
>>   			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>> index 6e99978..61d694c 100644
>> --- a/arch/arm64/include/asm/kvm_arm.h
>> +++ b/arch/arm64/include/asm/kvm_arm.h
>> @@ -204,6 +204,16 @@
>>   #define FSC_FAULT	ESR_ELx_FSC_FAULT
>>   #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
>>   #define FSC_PERM	ESR_ELx_FSC_PERM
>> +#define FSC_SEA		ESR_ELx_FSC_EXTABT
>> +#define FSC_SEA_TTW0	(0x14)
>> +#define FSC_SEA_TTW1	(0x15)
>> +#define FSC_SEA_TTW2	(0x16)
>> +#define FSC_SEA_TTW3	(0x17)
>> +#define FSC_SECC	(0x18)
>> +#define FSC_SECC_TTW0	(0x1c)
>> +#define FSC_SECC_TTW1	(0x1d)
>> +#define FSC_SECC_TTW2	(0x1e)
>> +#define FSC_SECC_TTW3	(0x1f)
>>   
>>   /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>   #define HPFAR_MASK	(~UL(0xf))
>> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
>> index bc81243..5b2cecd 100644
>> --- a/arch/arm64/include/asm/system_misc.h
>> +++ b/arch/arm64/include/asm/system_misc.h
>> @@ -58,4 +58,6 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
>>   
>>   #endif	/* __ASSEMBLY__ */
>>   
>> +int handle_guest_sea(unsigned long addr, unsigned int esr);
> Same remark here.
Same here.
>
>> +
>>   #endif	/* __ASM_SYSTEM_MISC_H */
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index f7372ce..ee96307 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -497,6 +497,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   {
>>   	struct siginfo info;
>> +	int ret = 0;
>>   
>>   	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>>   		 fault_name(esr), esr, addr);
>> @@ -508,7 +509,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   	 */
>>   	if (IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
>>   		nmi_enter();
>> -		ghes_notify_sea();
>> +		ret = ghes_notify_sea();
>>   		nmi_exit();
>>   	}
>>   
>> @@ -521,7 +522,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   		info.si_addr  = (void __user *)addr;
>>   	arm64_notify_die("", regs, &info, esr);
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   static const struct fault_info {
>> @@ -603,6 +604,20 @@ static const char *fault_name(unsigned int esr)
>>   }
>>   
>>   /*
>> + * Handle Synchronous External Aborts that occur in a guest kernel.
>> + */
>> +int handle_guest_sea(unsigned long addr, unsigned int esr)
>> +{
>> +	int ret = -ENOENT;
>> +
>> +	if(IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
>> +		ret = ghes_notify_sea();
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>>    * Dispatch a data abort to the relevant handler.
>>    */
>>   asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 230b095..81eabc6 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -810,17 +810,18 @@ static int ghes_notify_sci(struct notifier_block *this,
>>   #ifdef CONFIG_ACPI_APEI_SEA
>>   static LIST_HEAD(ghes_sea);
>>   
>> -void ghes_notify_sea(void)
>> +int ghes_notify_sea(void)
>>   {
>>   	struct ghes *ghes;
>> +	int ret = -ENOENT;
>>   
>> -	/*
>> -	 * synchronize_rcu() will wait for nmi_exit(), so no need to
>> -	 * rcu_read_lock().
>> -	 */
>> +	rcu_read_lock();
>>   	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
>> -		ghes_proc(ghes);
>> +		if(!ghes_proc(ghes))
>> +			ret = 0;
>>   	}
>> +	rcu_read_unlock();
>> +	return ret;
>>   }
>>   
>>   static void ghes_sea_add(struct ghes *ghes)
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
>> index 18bc935..2a727dc 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -99,6 +99,6 @@ static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data
>>   		gdata + 1;
>>   }
>>   
>> -void ghes_notify_sea(void);
>> +int ghes_notify_sea(void);
>>   
>>   #endif /* GHES_H */
>>
> Otherwise, the KVM changes look good to me. Assuming you fix the two
> nits above:
>
> Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Thanks!
Tyler

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1608606 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	Christoffer Dall <cdall@linaro.org>
Date	2017-03-24 17:10 +0100
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tozWN-44H-15@gated-at.bofh.it>
In reply to	#1606052

On Tue, Mar 21, 2017 at 04:47:05PM -0600, Tyler Baicar wrote:
> Currently external aborts are unsupported by the guest abort
> handling. Add handling for SEAs so that the host kernel reports
> SEAs which occur in the guest kernel.

The logic in kvm_handle_guest_abort could deserve an explanation here.

Surely the whole logic of how KVM fits into the picture here and how
we're dealing with synchronous errors could be explained here, based on
the long conversations between you and James on many prior iterations on
this patch.

> 
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> ---
>  arch/arm/include/asm/kvm_arm.h       | 10 +++++++++
>  arch/arm/include/asm/system_misc.h   |  5 +++++
>  arch/arm/kvm/mmu.c                   | 41 ++++++++++++++++++++++++++++++------
>  arch/arm64/include/asm/kvm_arm.h     | 10 +++++++++
>  arch/arm64/include/asm/system_misc.h |  2 ++
>  arch/arm64/mm/fault.c                | 19 +++++++++++++++--
>  drivers/acpi/apei/ghes.c             | 13 ++++++------
>  include/acpi/ghes.h                  |  2 +-
>  8 files changed, 86 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index a3f0b3d..ebf020b 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -187,6 +187,16 @@
>  #define FSC_FAULT	(0x04)
>  #define FSC_ACCESS	(0x08)
>  #define FSC_PERM	(0x0c)
> +#define FSC_SEA		(0x10)
> +#define FSC_SEA_TTW0	(0x14)
> +#define FSC_SEA_TTW1	(0x15)
> +#define FSC_SEA_TTW2	(0x16)
> +#define FSC_SEA_TTW3	(0x17)
> +#define FSC_SECC	(0x18)
> +#define FSC_SECC_TTW0	(0x1c)
> +#define FSC_SECC_TTW1	(0x1d)
> +#define FSC_SECC_TTW2	(0x1e)
> +#define FSC_SECC_TTW3	(0x1f)
>  
>  /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>  #define HPFAR_MASK	(~0xf)
> diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
> index a3d61ad..ea45d94 100644
> --- a/arch/arm/include/asm/system_misc.h
> +++ b/arch/arm/include/asm/system_misc.h
> @@ -24,4 +24,9 @@
>  
>  #endif /* !__ASSEMBLY__ */
>  
> +static inline int handle_guest_sea(unsigned long addr, unsigned int esr)
> +{
> +	return -1;
> +}
> +
>  #endif /* __ASM_ARM_SYSTEM_MISC_H */
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 962616f..105b6ab 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -29,6 +29,7 @@
>  #include <asm/kvm_asm.h>
>  #include <asm/kvm_emulate.h>
>  #include <asm/virt.h>
> +#include <asm/system_misc.h>
>  
>  #include "trace.h"
>  
> @@ -1406,6 +1407,24 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>  		kvm_set_pfn_accessed(pfn);
>  }
>  
> +static bool is_abort_synchronous(unsigned long fault_status) {

this naming feels a bit weird to me.  For example, this will return
false for a normal translation fault, which is indeed synchronous.

Below you cann handle_guest_sea conditionally based on the result of
this function, so could it be called is_abort_sea ?

> +	switch (fault_status) {
> +	case FSC_SEA:
> +	case FSC_SEA_TTW0:
> +	case FSC_SEA_TTW1:
> +	case FSC_SEA_TTW2:
> +	case FSC_SEA_TTW3:
> +	case FSC_SECC:
> +	case FSC_SECC_TTW0:
> +	case FSC_SECC_TTW1:
> +	case FSC_SECC_TTW2:
> +	case FSC_SECC_TTW3:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  /**
>   * kvm_handle_guest_abort - handles all 2nd stage aborts
>   * @vcpu:	the VCPU pointer
> @@ -1426,23 +1445,31 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	unsigned long hva;
>  	bool is_iabt, write_fault, writable;
>  	gfn_t gfn;
> -	int ret, idx;
> +	int ret, idx, sea_status = 1;
> +
> +	/* Check the stage-2 fault is trans. fault or write fault */

I think this comment was supposed to belong with the check for the three
types below, not for calling kvm_vcpu_trap_get_fault_type, and I also
think the comment is outdated, because it doesn't mention the access
flag.

I suggest either removing the comment, or keep it where it was and
change to say "trans. fault, write fault, acces flag fault, or
sea_status", with some insight into what sea_status means here.

> +	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
> +
> +	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> +
> +	/* The host kernel will handle the synchronous external abort. There
> +	 * is no need to pass the error into the guest.
> +	 */

nit: commenting style.

> +	if (is_abort_synchronous(fault_status))
> +		sea_status = handle_guest_sea((unsigned long)fault_ipa,

will this ever be used on AArch32 in the future?

If so, casting a phys_addr_t for an IPA to an unsigned long is an
incredibly bad idea.  Why not just use phys_addr_t for handle_guest_sea?

> +				    kvm_vcpu_get_hsr(vcpu));
>  
>  	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>  		kvm_inject_vabt(vcpu);
>  		return 1;
>  	}
>  
> -	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> -
>  	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
>  			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
>  
> -	/* Check the stage-2 fault is trans. fault or write fault */
> -	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>  	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
> -	    fault_status != FSC_ACCESS) {
> +	    fault_status != FSC_ACCESS && sea_status) {

I don't understand the maning of sea_status here.  If sea_status is
non-zero then the fault_status matters, but if sea_status is zero, then
we just continue by handling this as an MMIO abort or a user fault?

What am I missing here?



>  		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>  			kvm_vcpu_trap_get_class(vcpu),
>  			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
> index 6e99978..61d694c 100644
> --- a/arch/arm64/include/asm/kvm_arm.h
> +++ b/arch/arm64/include/asm/kvm_arm.h
> @@ -204,6 +204,16 @@
>  #define FSC_FAULT	ESR_ELx_FSC_FAULT
>  #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
>  #define FSC_PERM	ESR_ELx_FSC_PERM
> +#define FSC_SEA		ESR_ELx_FSC_EXTABT
> +#define FSC_SEA_TTW0	(0x14)
> +#define FSC_SEA_TTW1	(0x15)
> +#define FSC_SEA_TTW2	(0x16)
> +#define FSC_SEA_TTW3	(0x17)
> +#define FSC_SECC	(0x18)
> +#define FSC_SECC_TTW0	(0x1c)
> +#define FSC_SECC_TTW1	(0x1d)
> +#define FSC_SECC_TTW2	(0x1e)
> +#define FSC_SECC_TTW3	(0x1f)
>  
>  /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>  #define HPFAR_MASK	(~UL(0xf))
> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
> index bc81243..5b2cecd 100644
> --- a/arch/arm64/include/asm/system_misc.h
> +++ b/arch/arm64/include/asm/system_misc.h
> @@ -58,4 +58,6 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
>  
>  #endif	/* __ASSEMBLY__ */
>  
> +int handle_guest_sea(unsigned long addr, unsigned int esr);
> +
>  #endif	/* __ASM_SYSTEM_MISC_H */
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index f7372ce..ee96307 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -497,6 +497,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  {
>  	struct siginfo info;
> +	int ret = 0;
>  
>  	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>  		 fault_name(esr), esr, addr);
> @@ -508,7 +509,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  	 */
>  	if (IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
>  		nmi_enter();
> -		ghes_notify_sea();
> +		ret = ghes_notify_sea();
>  		nmi_exit();
>  	}
>  
> @@ -521,7 +522,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>  		info.si_addr  = (void __user *)addr;
>  	arm64_notify_die("", regs, &info, esr);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static const struct fault_info {
> @@ -603,6 +604,20 @@ static const char *fault_name(unsigned int esr)
>  }
>  
>  /*
> + * Handle Synchronous External Aborts that occur in a guest kernel.
> + */

Can you document what the return value here means?

I have no idea what the semantics of ghes_notify_sea() are.

> +int handle_guest_sea(unsigned long addr, unsigned int esr)
> +{
> +	int ret = -ENOENT;
> +
> +	if(IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
> +		ret = ghes_notify_sea();
> +	}
> +
> +	return ret;
> +}
> +
> +/*
>   * Dispatch a data abort to the relevant handler.
>   */
>  asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,
> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
> index 230b095..81eabc6 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -810,17 +810,18 @@ static int ghes_notify_sci(struct notifier_block *this,
>  #ifdef CONFIG_ACPI_APEI_SEA
>  static LIST_HEAD(ghes_sea);
>  
> -void ghes_notify_sea(void)
> +int ghes_notify_sea(void)
>  {
>  	struct ghes *ghes;
> +	int ret = -ENOENT;
>  
> -	/*
> -	 * synchronize_rcu() will wait for nmi_exit(), so no need to
> -	 * rcu_read_lock().
> -	 */
> +	rcu_read_lock();
>  	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
> -		ghes_proc(ghes);
> +		if(!ghes_proc(ghes))
> +			ret = 0;
>  	}
> +	rcu_read_unlock();
> +	return ret;
>  }
>  
>  static void ghes_sea_add(struct ghes *ghes)
> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
> index 18bc935..2a727dc 100644
> --- a/include/acpi/ghes.h
> +++ b/include/acpi/ghes.h
> @@ -99,6 +99,6 @@ static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data
>  		gdata + 1;
>  }
>  
> -void ghes_notify_sea(void);
> +int ghes_notify_sea(void);
>  
>  #endif /* GHES_H */
> -- 
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
> 

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1610285 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	gengdongjiu <gengdongjiu@huawei.com>
Date	2017-03-28 06:30 +0200
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tpQVA-2fl-3@gated-at.bofh.it>
In reply to	#1608606

Hi Tyler,

I have a question for below code.

On 2017/3/25 0:01, Christoffer Dall wrote:
>  	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>  		kvm_inject_vabt(vcpu);
when it is SEA synchronized abort, why here inject a asynchronous abort through kvm_inject_vabt(vcpu) instead of synchronized abort? seem the original kvm source code is also do that, so I am confused about that.
thanks .

>  		return 1;
>  	}

[toc] | [prev] | [next] | [standalone]

#1611214 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	"Baicar, Tyler" <tbaicar@codeaurora.org>
Date	2017-03-28 18:50 +0200
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tq2tI-1Yg-29@gated-at.bofh.it>
In reply to	#1610285

Hello,


On 3/27/2017 10:19 PM, gengdongjiu wrote:
> Hi Tyler,
>
> I have a question for below code.
>
> On 2017/3/25 0:01, Christoffer Dall wrote:
>>   	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
>> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
>> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>>   		kvm_inject_vabt(vcpu);
> when it is SEA synchronized abort, why here inject a asynchronous abort through kvm_inject_vabt(vcpu) instead of synchronized abort? seem the original kvm source code is also do that, so I am confused about that.
> thanks .
If the SEA was properly handled, then sea_status will avoid injecting 
the vabt. I'm going to just return after the SEA handling in the next 
patch though.

Thanks,
Tyler
>
>>   		return 1;
>>   	}

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1611210 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	"Baicar, Tyler" <tbaicar@codeaurora.org>
Date	2017-03-28 18:50 +0200
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tq2tH-1Yg-11@gated-at.bofh.it>
In reply to	#1608606

Hello Christoffer,


On 3/24/2017 10:01 AM, Christoffer Dall wrote:
> On Tue, Mar 21, 2017 at 04:47:05PM -0600, Tyler Baicar wrote:
>> Currently external aborts are unsupported by the guest abort
>> handling. Add handling for SEAs so that the host kernel reports
>> SEAs which occur in the guest kernel.
> The logic in kvm_handle_guest_abort could deserve an explanation here.
>
> Surely the whole logic of how KVM fits into the picture here and how
> we're dealing with synchronous errors could be explained here, based on
> the long conversations between you and James on many prior iterations on
> this patch.
I will add additional info here.
>> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
>> ---
>>   arch/arm/include/asm/kvm_arm.h       | 10 +++++++++
>>   arch/arm/include/asm/system_misc.h   |  5 +++++
>>   arch/arm/kvm/mmu.c                   | 41 ++++++++++++++++++++++++++++++------
>>   arch/arm64/include/asm/kvm_arm.h     | 10 +++++++++
>>   arch/arm64/include/asm/system_misc.h |  2 ++
>>   arch/arm64/mm/fault.c                | 19 +++++++++++++++--
>>   drivers/acpi/apei/ghes.c             | 13 ++++++------
>>   include/acpi/ghes.h                  |  2 +-
>>   8 files changed, 86 insertions(+), 16 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
>> index a3f0b3d..ebf020b 100644
>> --- a/arch/arm/include/asm/kvm_arm.h
>> +++ b/arch/arm/include/asm/kvm_arm.h
>> @@ -187,6 +187,16 @@
>>   #define FSC_FAULT	(0x04)
>>   #define FSC_ACCESS	(0x08)
>>   #define FSC_PERM	(0x0c)
>> +#define FSC_SEA		(0x10)
>> +#define FSC_SEA_TTW0	(0x14)
>> +#define FSC_SEA_TTW1	(0x15)
>> +#define FSC_SEA_TTW2	(0x16)
>> +#define FSC_SEA_TTW3	(0x17)
>> +#define FSC_SECC	(0x18)
>> +#define FSC_SECC_TTW0	(0x1c)
>> +#define FSC_SECC_TTW1	(0x1d)
>> +#define FSC_SECC_TTW2	(0x1e)
>> +#define FSC_SECC_TTW3	(0x1f)
>>   
>>   /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>   #define HPFAR_MASK	(~0xf)
>> diff --git a/arch/arm/include/asm/system_misc.h b/arch/arm/include/asm/system_misc.h
>> index a3d61ad..ea45d94 100644
>> --- a/arch/arm/include/asm/system_misc.h
>> +++ b/arch/arm/include/asm/system_misc.h
>> @@ -24,4 +24,9 @@
>>   
>>   #endif /* !__ASSEMBLY__ */
>>   
>> +static inline int handle_guest_sea(unsigned long addr, unsigned int esr)
>> +{
>> +	return -1;
>> +}
>> +
>>   #endif /* __ASM_ARM_SYSTEM_MISC_H */
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 962616f..105b6ab 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -29,6 +29,7 @@
>>   #include <asm/kvm_asm.h>
>>   #include <asm/kvm_emulate.h>
>>   #include <asm/virt.h>
>> +#include <asm/system_misc.h>
>>   
>>   #include "trace.h"
>>   
>> @@ -1406,6 +1407,24 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>>   		kvm_set_pfn_accessed(pfn);
>>   }
>>   
>> +static bool is_abort_synchronous(unsigned long fault_status) {
> this naming feels a bit weird to me.  For example, this will return
> false for a normal translation fault, which is indeed synchronous.
>
> Below you cann handle_guest_sea conditionally based on the result of
> this function, so could it be called is_abort_sea ?
Yes, I will change the naming.
>> +	switch (fault_status) {
>> +	case FSC_SEA:
>> +	case FSC_SEA_TTW0:
>> +	case FSC_SEA_TTW1:
>> +	case FSC_SEA_TTW2:
>> +	case FSC_SEA_TTW3:
>> +	case FSC_SECC:
>> +	case FSC_SECC_TTW0:
>> +	case FSC_SECC_TTW1:
>> +	case FSC_SECC_TTW2:
>> +	case FSC_SECC_TTW3:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>   /**
>>    * kvm_handle_guest_abort - handles all 2nd stage aborts
>>    * @vcpu:	the VCPU pointer
>> @@ -1426,23 +1445,31 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>>   	unsigned long hva;
>>   	bool is_iabt, write_fault, writable;
>>   	gfn_t gfn;
>> -	int ret, idx;
>> +	int ret, idx, sea_status = 1;
>> +
>> +	/* Check the stage-2 fault is trans. fault or write fault */
> I think this comment was supposed to belong with the check for the three
> types below, not for calling kvm_vcpu_trap_get_fault_type, and I also
> think the comment is outdated, because it doesn't mention the access
> flag.
>
> I suggest either removing the comment, or keep it where it was and
> change to say "trans. fault, write fault, acces flag fault, or
> sea_status", with some insight into what sea_status means here.
Will remove.
>
>> +	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>> +
>> +	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
>> +
>> +	/* The host kernel will handle the synchronous external abort. There
>> +	 * is no need to pass the error into the guest.
>> +	 */
> nit: commenting style.
Will change.
>> +	if (is_abort_synchronous(fault_status))
>> +		sea_status = handle_guest_sea((unsigned long)fault_ipa,
> will this ever be used on AArch32 in the future?
>
> If so, casting a phys_addr_t for an IPA to an unsigned long is an
> incredibly bad idea.  Why not just use phys_addr_t for handle_guest_sea?
I'll change it to phys_addr_t.
>
>> +				    kvm_vcpu_get_hsr(vcpu));
>>   
>>   	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
>> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
>> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>>   		kvm_inject_vabt(vcpu);
>>   		return 1;
>>   	}
>>   
>> -	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
>> -
>>   	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
>>   			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
>>   
>> -	/* Check the stage-2 fault is trans. fault or write fault */
>> -	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>>   	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
>> -	    fault_status != FSC_ACCESS) {
>> +	    fault_status != FSC_ACCESS && sea_status) {
> I don't understand the maning of sea_status here.  If sea_status is
> non-zero then the fault_status matters, but if sea_status is zero, then
> we just continue by handling this as an MMIO abort or a user fault?
>
> What am I missing here?
Basically if the SEA was already handled we do not fall into this if. It 
wouldn't make sense to print unsupported FSC after properly handling an 
SEA. This will probably be removed as I will just return after the SEA 
handling.
>
>
>>   		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>>   			kvm_vcpu_trap_get_class(vcpu),
>>   			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),
>> diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
>> index 6e99978..61d694c 100644
>> --- a/arch/arm64/include/asm/kvm_arm.h
>> +++ b/arch/arm64/include/asm/kvm_arm.h
>> @@ -204,6 +204,16 @@
>>   #define FSC_FAULT	ESR_ELx_FSC_FAULT
>>   #define FSC_ACCESS	ESR_ELx_FSC_ACCESS
>>   #define FSC_PERM	ESR_ELx_FSC_PERM
>> +#define FSC_SEA		ESR_ELx_FSC_EXTABT
>> +#define FSC_SEA_TTW0	(0x14)
>> +#define FSC_SEA_TTW1	(0x15)
>> +#define FSC_SEA_TTW2	(0x16)
>> +#define FSC_SEA_TTW3	(0x17)
>> +#define FSC_SECC	(0x18)
>> +#define FSC_SECC_TTW0	(0x1c)
>> +#define FSC_SECC_TTW1	(0x1d)
>> +#define FSC_SECC_TTW2	(0x1e)
>> +#define FSC_SECC_TTW3	(0x1f)
>>   
>>   /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>>   #define HPFAR_MASK	(~UL(0xf))
>> diff --git a/arch/arm64/include/asm/system_misc.h b/arch/arm64/include/asm/system_misc.h
>> index bc81243..5b2cecd 100644
>> --- a/arch/arm64/include/asm/system_misc.h
>> +++ b/arch/arm64/include/asm/system_misc.h
>> @@ -58,4 +58,6 @@ void hook_debug_fault_code(int nr, int (*fn)(unsigned long, unsigned int,
>>   
>>   #endif	/* __ASSEMBLY__ */
>>   
>> +int handle_guest_sea(unsigned long addr, unsigned int esr);
>> +
>>   #endif	/* __ASM_SYSTEM_MISC_H */
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index f7372ce..ee96307 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -497,6 +497,7 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   {
>>   	struct siginfo info;
>> +	int ret = 0;
>>   
>>   	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>>   		 fault_name(esr), esr, addr);
>> @@ -508,7 +509,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   	 */
>>   	if (IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
>>   		nmi_enter();
>> -		ghes_notify_sea();
>> +		ret = ghes_notify_sea();
>>   		nmi_exit();
>>   	}
>>   
>> @@ -521,7 +522,7 @@ static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
>>   		info.si_addr  = (void __user *)addr;
>>   	arm64_notify_die("", regs, &info, esr);
>>   
>> -	return 0;
>> +	return ret;
>>   }
>>   
>>   static const struct fault_info {
>> @@ -603,6 +604,20 @@ static const char *fault_name(unsigned int esr)
>>   }
>>   
>>   /*
>> + * Handle Synchronous External Aborts that occur in a guest kernel.
>> + */
> Can you document what the return value here means?
>
> I have no idea what the semantics of ghes_notify_sea() are.
>
Yes, will do.

Thanks,
Tyler
>> +int handle_guest_sea(unsigned long addr, unsigned int esr)
>> +{
>> +	int ret = -ENOENT;
>> +
>> +	if(IS_ENABLED(CONFIG_ACPI_APEI_SEA)) {
>> +		ret = ghes_notify_sea();
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +/*
>>    * Dispatch a data abort to the relevant handler.
>>    */
>>   asmlinkage void __exception do_mem_abort(unsigned long addr, unsigned int esr,
>> diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
>> index 230b095..81eabc6 100644
>> --- a/drivers/acpi/apei/ghes.c
>> +++ b/drivers/acpi/apei/ghes.c
>> @@ -810,17 +810,18 @@ static int ghes_notify_sci(struct notifier_block *this,
>>   #ifdef CONFIG_ACPI_APEI_SEA
>>   static LIST_HEAD(ghes_sea);
>>   
>> -void ghes_notify_sea(void)
>> +int ghes_notify_sea(void)
>>   {
>>   	struct ghes *ghes;
>> +	int ret = -ENOENT;
>>   
>> -	/*
>> -	 * synchronize_rcu() will wait for nmi_exit(), so no need to
>> -	 * rcu_read_lock().
>> -	 */
>> +	rcu_read_lock();
>>   	list_for_each_entry_rcu(ghes, &ghes_sea, list) {
>> -		ghes_proc(ghes);
>> +		if(!ghes_proc(ghes))
>> +			ret = 0;
>>   	}
>> +	rcu_read_unlock();
>> +	return ret;
>>   }
>>   
>>   static void ghes_sea_add(struct ghes *ghes)
>> diff --git a/include/acpi/ghes.h b/include/acpi/ghes.h
>> index 18bc935..2a727dc 100644
>> --- a/include/acpi/ghes.h
>> +++ b/include/acpi/ghes.h
>> @@ -99,6 +99,6 @@ static inline void *acpi_hest_generic_data_payload(struct acpi_hest_generic_data
>>   		gdata + 1;
>>   }
>>   
>> -void ghes_notify_sea(void);
>> +int ghes_notify_sea(void);
>>   
>>   #endif /* GHES_H */
>> -- 
>> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
>> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
>> a Linux Foundation Collaborative Project.
>>
> Thanks,
> -Christoffer

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1610508 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	gengdongjiu <gengdongjiu@huawei.com>
Date	2017-03-28 12:00 +0200
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tpW4W-5IO-21@gated-at.bofh.it>
In reply to	#1606052

Hi,

On 2017/3/22 6:47, Tyler Baicar wrote:
> +	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> +
> +	/* The host kernel will handle the synchronous external abort. There
> +	 * is no need to pass the error into the guest.
> +	 */
> +	if (is_abort_synchronous(fault_status))
> +		sea_status = handle_guest_sea((unsigned long)fault_ipa,
> +				    kvm_vcpu_get_hsr(vcpu));
>  
>  	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>  		kvm_inject_vabt(vcpu);
>  		return 1;
>  	}
       After the host kernel correctly handle the synchronous external abort, the sea_status
       will return 0, so the code logical will be continue go-no, whether it is better directly return
       after correctly handle the SEA? such as below.

	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
  		kvm_inject_vabt(vcpu);
  		return 1;
  	} else
                return 1;

>  
> -	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> -
>  	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
>  			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
>  
> -	/* Check the stage-2 fault is trans. fault or write fault */
> -	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>  	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&

[toc] | [prev] | [next] | [standalone]

#1611211 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	"Baicar, Tyler" <tbaicar@codeaurora.org>
Date	2017-03-28 18:50 +0200
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tq2tH-1Yg-13@gated-at.bofh.it>
In reply to	#1610508

Hello,


On 3/28/2017 3:53 AM, gengdongjiu wrote:
> Hi,
>
> On 2017/3/22 6:47, Tyler Baicar wrote:
>> +	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
>> +
>> +	/* The host kernel will handle the synchronous external abort. There
>> +	 * is no need to pass the error into the guest.
>> +	 */
>> +	if (is_abort_synchronous(fault_status))
>> +		sea_status = handle_guest_sea((unsigned long)fault_ipa,
>> +				    kvm_vcpu_get_hsr(vcpu));
>>   
>>   	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
>> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
>> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>>   		kvm_inject_vabt(vcpu);
>>   		return 1;
>>   	}
>         After the host kernel correctly handle the synchronous external abort, the sea_status
>         will return 0, so the code logical will be continue go-no, whether it is better directly return
>         after correctly handle the SEA? such as below.
>
> 	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>    		kvm_inject_vabt(vcpu);
>    		return 1;
>    	} else
>                  return 1;
Yes, I will return after successful SEA handling in the next patch set.

Thanks,
Tyler
>
>>   
>> -	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
>> -
>>   	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
>>   			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
>>   
>> -	/* Check the stage-2 fault is trans. fault or write fault */
>> -	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>>   	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1610563 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

From	James Morse <james.morse@arm.com>
Date	2017-03-28 13:00 +0200
Subject	Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support
Message-ID	<tpX0Z-6nN-5@gated-at.bofh.it>
In reply to	#1606052

Hi Tyler,

On 21/03/17 22:47, Tyler Baicar wrote:
> Currently external aborts are unsupported by the guest abort
> handling. Add handling for SEAs so that the host kernel reports
> SEAs which occur in the guest kernel.

Looks good,

Can we squash the APEI changes into the patch that added them? This would be one
fewer patches that then need the ACPI maintainer to review.


> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 962616f..105b6ab 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -1406,6 +1407,24 @@ static void handle_access_fault(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>  		kvm_set_pfn_accessed(pfn);
>  }
>  
> +static bool is_abort_synchronous(unsigned long fault_status) {

I missed kvm_vcpu_dabt_isextabt() when I suggested we would need a helper (my
fault). Can we use that instead?

(my argument that the unused encodings are reserved doesn't hold if KVM is
already doing this... )


> @@ -1426,23 +1445,31 @@ int kvm_handle_guest_abort(struct kvm_vcpu *vcpu, struct kvm_run *run)
>  	unsigned long hva;
>  	bool is_iabt, write_fault, writable;
>  	gfn_t gfn;
> -	int ret, idx;
> +	int ret, idx, sea_status = 1;
> +
> +	/* Check the stage-2 fault is trans. fault or write fault */
> +	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
> +
> +	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> +
> +	/* The host kernel will handle the synchronous external abort. There
> +	 * is no need to pass the error into the guest.
> +	 */
> +	if (is_abort_synchronous(fault_status))
> +		sea_status = handle_guest_sea((unsigned long)fault_ipa,
> +				    kvm_vcpu_get_hsr(vcpu));


Why not return from here if the error has been handled?

You use sea_status to skip the next two things that KVM might do, but it goes on
to try and process this, possibly calling user_mem_abort(), surely all this is
unnecessary?


>  
>  	is_iabt = kvm_vcpu_trap_is_iabt(vcpu);
> -	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu))) {
> +	if (unlikely(!is_iabt && kvm_vcpu_dabt_isextabt(vcpu)) && sea_status) {
>  		kvm_inject_vabt(vcpu);
>  		return 1;
>  	}
>  
> -	fault_ipa = kvm_vcpu_get_fault_ipa(vcpu);
> -
>  	trace_kvm_guest_fault(*vcpu_pc(vcpu), kvm_vcpu_get_hsr(vcpu),
>  			      kvm_vcpu_get_hfar(vcpu), fault_ipa);
>  
> -	/* Check the stage-2 fault is trans. fault or write fault */
> -	fault_status = kvm_vcpu_trap_get_fault_type(vcpu);
>  	if (fault_status != FSC_FAULT && fault_status != FSC_PERM &&
> -	    fault_status != FSC_ACCESS) {
> +	    fault_status != FSC_ACCESS && sea_status) {
>  		kvm_err("Unsupported FSC: EC=%#x xFSC=%#lx ESR_EL2=%#lx\n",
>  			kvm_vcpu_trap_get_class(vcpu),
>  			(unsigned long)kvm_vcpu_trap_get_fault(vcpu),


Thanks,

James

[toc] | [prev] | [next] | [standalone]

#1606053 — [PATCH V13 04/10] arm64: exception: handle Synchronous External Abort

From	Tyler Baicar <tbaicar@codeaurora.org>
Date	2017-03-21 23:50 +0100
Subject	[PATCH V13 04/10] arm64: exception: handle Synchronous External Abort
Message-ID	<tnALg-1nk-21@gated-at.bofh.it>
In reply to	#1606046

SEA exceptions are often caused by an uncorrected hardware
error, and are handled when data abort and instruction abort
exception classes have specific values for their Fault Status
Code.
When SEA occurs, before killing the process, report the error
in the kernel logs.
Update fault_info[] with specific SEA faults so that the
new SEA handler is used.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Reviewed-by: James Morse <james.morse@arm.com>
---
 arch/arm64/include/asm/esr.h |  1 +
 arch/arm64/mm/fault.c        | 43 +++++++++++++++++++++++++++++++++----------
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index d14c478..f20c64a 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -83,6 +83,7 @@
 #define ESR_ELx_WNR		(UL(1) << 6)
 
 /* Shared ISS field definitions for Data/Instruction aborts */
+#define ESR_ELx_FnV		(UL(1) << 10)
 #define ESR_ELx_EA		(UL(1) << 9)
 #define ESR_ELx_S1PTW		(UL(1) << 7)
 
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 4bf899f..e17633d 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -488,6 +488,29 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 	return 1;
 }
 
+/*
+ * This abort handler deals with Synchronous External Abort.
+ * It calls notifiers, and then returns "fault".
+ */
+static int do_sea(unsigned long addr, unsigned int esr, struct pt_regs *regs)
+{
+	struct siginfo info;
+
+	pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
+		 fault_name(esr), esr, addr);
+
+	info.si_signo = SIGBUS;
+	info.si_errno = 0;
+	info.si_code  = 0;
+	if (esr & ESR_ELx_FnV)
+		info.si_addr = 0;
+	else
+		info.si_addr  = (void __user *)addr;
+	arm64_notify_die("", regs, &info, esr);
+
+	return 0;
+}
+
 static const struct fault_info {
 	int	(*fn)(unsigned long addr, unsigned int esr, struct pt_regs *regs);
 	int	sig;
@@ -510,22 +533,22 @@ static int do_bad(unsigned long addr, unsigned int esr, struct pt_regs *regs)
 	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 1 permission fault"	},
 	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 2 permission fault"	},
 	{ do_page_fault,	SIGSEGV, SEGV_ACCERR,	"level 3 permission fault"	},
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort"	},
+	{ do_sea,		SIGBUS,  0,		"synchronous external abort"	},
 	{ do_bad,		SIGBUS,  0,		"unknown 17"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 18"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 19"			},
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous external abort (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error"	},
+	{ do_sea,		SIGBUS,  0,		"level 0 (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 1 (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 2 (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 3 (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"synchronous parity or ECC error" },
 	{ do_bad,		SIGBUS,  0,		"unknown 25"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 26"			},
 	{ do_bad,		SIGBUS,  0,		"unknown 27"			},
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error (translation table walk)" },
-	{ do_bad,		SIGBUS,  0,		"synchronous parity error (translation table walk)" },
+	{ do_sea,		SIGBUS,  0,		"level 0 synchronous parity error (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 1 synchronous parity error (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 2 synchronous parity error (translation table walk)"	},
+	{ do_sea,		SIGBUS,  0,		"level 3 synchronous parity error (translation table walk)"	},
 	{ do_bad,		SIGBUS,  0,		"unknown 32"			},
 	{ do_alignment_fault,	SIGBUS,  BUS_ADRALN,	"alignment fault"		},
 	{ do_bad,		SIGBUS,  0,		"unknown 34"			},
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1607724 — Re: [PATCH V13 04/10] arm64: exception: handle Synchronous External Abort

From	Catalin Marinas <catalin.marinas@arm.com>
Date	2017-03-23 18:10 +0100
Subject	Re: [PATCH V13 04/10] arm64: exception: handle Synchronous External Abort
Message-ID	<toepk-5id-25@gated-at.bofh.it>
In reply to	#1606053

On Tue, Mar 21, 2017 at 04:46:59PM -0600, Tyler Baicar wrote:
> SEA exceptions are often caused by an uncorrected hardware
> error, and are handled when data abort and instruction abort
> exception classes have specific values for their Fault Status
> Code.
> When SEA occurs, before killing the process, report the error
> in the kernel logs.
> Update fault_info[] with specific SEA faults so that the
> new SEA handler is used.
> 
> Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
> CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
> Reviewed-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/include/asm/esr.h |  1 +
>  arch/arm64/mm/fault.c        | 43 +++++++++++++++++++++++++++++++++----------

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

[toc] | [prev] | [next] | [standalone]

#1606054 — [PATCH V13 07/10] efi: print unrecognized CPER section

From	Tyler Baicar <tbaicar@codeaurora.org>
Date	2017-03-21 23:50 +0100
Subject	[PATCH V13 07/10] efi: print unrecognized CPER section
Message-ID	<tnALg-1nk-23@gated-at.bofh.it>
In reply to	#1606046

UEFI spec allows for non-standard section in Common Platform Error
Record. This is defined in section N.2.3 of UEFI version 2.5.

Currently if the CPER section's type (UUID) does not match with
one of the section types that the kernel knows how to parse, the
section is skipped. Therefore, user is not able to see
such CPER data, for instance, error record of non-standard section.

For above mentioned case, this change prints out the raw data in
hex in dmesg buffer. Data length is taken from Error Data length
field of Generic Error Data Entry.

The following is a sample output from dmesg:
[  140.739180] {1}[Hardware Error]: Hardware error from APEI Generic Hardware Error Source: 2
[  140.739182] {1}[Hardware Error]: It has been corrected by h/w and requires no further action
[  140.739191] {1}[Hardware Error]: event severity: corrected
[  140.739196] {1}[Hardware Error]:  time: precise 2017-03-15 20:37:35
[  140.739197] {1}[Hardware Error]:  Error 0, type: corrected
[  140.739203] {1}[Hardware Error]:   section type: unknown, d2e2621c-f936-468d-0d84-15a4ed015c8b
[  140.739205] {1}[Hardware Error]:   section length: 568 (0x238)
[  140.739210] {1}[Hardware Error]:   00000000: 4d415201 4d492031 453a4d45 435f4343  .RAM1 IMEM:ECC_C
[  140.739214] {1}[Hardware Error]:   00000010: 53515f45 44525f42 00000000 00000000  E_QSB_RD........
[  140.739217] {1}[Hardware Error]:   00000020: 00000000 00000000 00000000 00000000  ................
[  140.739220] {1}[Hardware Error]:   00000030: 00000000 00000000 01010000 01010000  ................
[  140.739223] {1}[Hardware Error]:   00000040: 00000000 00000000 00000005 00000000  ................
[  140.739226] {1}[Hardware Error]:   00000050: 01010000 00000000 00000001 00dddd00  ................
...

The raw data from the error can then be decoded using vendor
specific tools.

Signed-off-by: Tyler Baicar <tbaicar@codeaurora.org>
CC: Jonathan (Zhixiong) Zhang <zjzhang@codeaurora.org>
Reviewed-by: James Morse <james.morse@arm.com>
---
 drivers/firmware/efi/cper.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index 56aa516..d263bc8 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -591,8 +591,16 @@ static void cper_estatus_print_section(
 			cper_print_proc_arm(newpfx, arm_err);
 		else
 			goto err_section_too_small;
-	} else
-		printk("%s""section type: unknown, %pUl\n", newpfx, sec_type);
+	} else {
+		const void *unknown_err;
+
+		unknown_err = acpi_hest_generic_data_payload(gdata);
+		printk("%ssection type: unknown, %pUl\n", newpfx, sec_type);
+		printk("%ssection length: %d (%#x)\n", newpfx,
+		       gdata->error_data_length, gdata->error_data_length);
+		print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4,
+			       unknown_err, gdata->error_data_length, true);
+	}
 
 	return;
 
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2 [1] 2 Next page →

csiph-web

[PATCH V13 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

Contents

#1606046 — [PATCH V13 00/10] Add UEFI 2.6 and ACPI 6.1 updates for RAS on ARM64

#1606048 — [PATCH V13 09/10] trace, ras: add ARM processor error trace event

#1606172 — Re: [PATCH V13 09/10] trace, ras: add ARM processor error trace event

#1606178 — Re: [PATCH V13 09/10] trace, ras: add ARM processor error trace event

#1606049 — [PATCH V13 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

#1606051 — [PATCH V13 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section

#1606052 — [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1607757 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1608414 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1611197 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1608606 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1610285 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1611214 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1611210 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1610508 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1611211 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1610563 — Re: [PATCH V13 10/10] arm/arm64: KVM: add guest SEA support

#1606053 — [PATCH V13 04/10] arm64: exception: handle Synchronous External Abort

#1607724 — Re: [PATCH V13 04/10] arm64: exception: handle Synchronous External Abort

#1606054 — [PATCH V13 07/10] efi: print unrecognized CPER section