Re: [kvm-unit-tests PATCH v10 3/3] arm: pmu: Add CPI checking

[Wei Huang]

On 11/21/2016 03:40 PM, Christopher Covington wrote:
> Hi Wei,
> 
> On 11/21/2016 03:24 PM, Wei Huang wrote:
>> From: Christopher Covington 
> 
> I really appreciate your work on these patches. If for any or all of these
> you have more lines added/modified than me (or using any other better
> metric), please make sure to change the author to be you with
> `git commit --amend --reset-author` or equivalent.

Sure, I will if needed. Regarding your comments below, I will fix the
patch series after Drew's comments, if any.

> 
>> Calculate the numbers of cycles per instruction (CPI) implied by ARM
>> PMU cycle counter values. The code includes a strict checking facility
>> intended for the -icount option in TCG mode in the configuration file.
>>
>> Signed-off-by: Christopher Covington 
>> Signed-off-by: Wei Huang 
>> ---
>>  arm/pmu.c | 119 
>> +-
>>  arm/unittests.cfg |  14 +++
>>  2 files changed, 132 insertions(+), 1 deletion(-)
>>
>> diff --git a/arm/pmu.c b/arm/pmu.c
>> index 176b070..129ef1e 100644
>> --- a/arm/pmu.c
>> +++ b/arm/pmu.c
>> @@ -104,6 +104,25 @@ static inline uint32_t id_dfr0_read(void)
>>  asm volatile("mrc p15, 0, %0, c0, c1, 2" : "=r" (val));
>>  return val;
>>  }
>> +
>> +/*
>> + * Extra instructions inserted by the compiler would be difficult to 
>> compensate
>> + * for, so hand assemble everything between, and including, the PMCR 
>> accesses
>> + * to start and stop counting. Total cycles = isb + mcr + 2*loop = 2 + 
>> 2*loop.
   
I will change the comment above to "Total instrs".

>> + */
>> +static inline void precise_cycles_loop(int loop, uint32_t pmcr)
> 
> Nit: I would call this precise_instrs_loop. How many cycles it takes is
> IMPLEMENTATION DEFINED.

You are right. The cycle indeed depends on the design. Will fix.

> 
>> +{
>> +asm volatile(
>> +"   mcr p15, 0, %[pmcr], c9, c12, 0\n"
>> +"   isb\n"
>> +"1: subs%[loop], %[loop], #1\n"
>> +"   bgt 1b\n"
> 
> Is there any chance we might need an isb here, to prevent the stop from 
> happening
> before or during the loop? Where ISBs are required, the Linux best practice 
> is to

In theory, I think this can happen when mcr is executed before all loop
instructions completed, causing pmccntr_read() to miss some cycles. But
QEMU TCG mode doesn't support out-order-execution. So the test
condition, "cpi > 0 && cycles != i * cpi", will never be TRUE. Because
cpi==0 in KVM, this same test condition won't be TRUE under KVM mode either.

> diligently comment why they are needed. Perhaps it would be a good habit to
> carry over into kvm-unit-tests.

Agreed. Most isb() instructions were added following CP15 writes (not
all CP15 writes, but at limited locations). We tried to follow what
Linux kernel does in perf_event.c. If you feel that any isb() place
needs special comment, I will be more than happy to add it.


___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH V5 07/10] efi: print unrecognized CPER section

[Tyler Baicar]

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.

Following is a sample output from dmesg:
[  115.771702] {1}[Hardware Error]: Hardware error from APEI Generic Hardware 
Error Source: 2
[  115.779042] {1}[Hardware Error]: It has been corrected by h/w and requires 
no further action
[  115.787456] {1}[Hardware Error]: event severity: corrected
[  115.792927] {1}[Hardware Error]:  Error 0, type: corrected
[  115.798415] {1}[Hardware Error]:  fru_id: 
----
[  115.805596] {1}[Hardware Error]:  fru_text:
[  115.816105] {1}[Hardware Error]:  section type: 
d2e2621c-f936-468d-0d84-15a4ed015c8b
[  115.823880] {1}[Hardware Error]:  section length: 88
[  115.828779] {1}[Hardware Error]:   : 0101 0002 5f434345 
525f4543
[  115.836153] {1}[Hardware Error]:   0010: 574d   

[  115.843531] {1}[Hardware Error]:   0020:    

[  115.850908] {1}[Hardware Error]:   0030:    

[  115.858288] {1}[Hardware Error]:   0040: fe80  0004 
5f434345
[  115.865665] {1}[Hardware Error]:   0050: 525f4543 574d

Signed-off-by: Jonathan (Zhixiong) Zhang 
Signed-off-by: Tyler Baicar 
---
 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 004aa1b..bbb576e 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -593,8 +593,16 @@ static void cper_estatus_print_section(
cper_print_proc_armv8(newpfx, armv8_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: %pUl\n", newpfx, sec_type);
+   printk("%ssection length: %d\n", newpfx,
+  gdata->error_data_length);
+   print_hex_dump(newpfx, "", DUMP_PREFIX_OFFSET, 16, 4,
+  unknown_err, gdata->error_data_length, 0);
+   }
 
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.

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH V5 09/10] trace, ras: add ARM processor error trace event

[Tyler Baicar]

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 
Acked-by: Steven Rostedt 
---
 drivers/acpi/apei/ghes.c|  9 +++-
 drivers/firmware/efi/cper.c |  1 +
 drivers/ras/ras.c   |  1 +
 include/ras/ras_event.h | 55 +
 4 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index c7fbbc1..1147e17 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -516,7 +516,14 @@ static void ghes_do_proc(struct ghes *ghes,
 
}
 #endif
-   else {
+   else if (!uuid_le_cmp(sec_type, CPER_SEC_PROC_ARMV8)) {
+   struct cper_sec_proc_armv8 *armv8_err;
+   struct cper_armv8_err_info *err_info;
+
+   armv8_err = acpi_hest_generic_data_payload(gdata);
+   err_info = (void *)(armv8_err +1);
+   trace_arm_event(armv8_err, err_info);
+   } else {
void *unknown_err = 
acpi_hest_generic_data_payload(gdata);
trace_unknown_sec_event(_type,
fru_id, fru_text, sec_sev,
diff --git a/drivers/firmware/efi/cper.c b/drivers/firmware/efi/cper.c
index bbb576e..0a0cd74 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #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 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(extlog_mem_event);
 #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..0060bba 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -162,6 +162,61 @@ TRACE_EVENT(mc_event,
 );
 
 /*
+ * 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_armv8 *proc,
+struct cper_armv8_err_info *err_info),
+
+   TP_ARGS(proc, err_info),
+
+   TP_STRUCT__entry(
+   __field(u64, mpidr)
+   __field(u64, midr)
+   __field(u64, info)
+   __field(u64, virt_fault_addr)
+   __field(u64, phys_fault_addr)
+   __field(u32, running_state)
+   __field(u32, psci_state)
+   __field(u16, err_count)
+   __field(u8, affinity)
+   __field(u8, version)
+   __field(u8, type)
+   __field(u8, flags)
+   ),
+
+   TP_fast_assign(
+   __entry->affinity = proc->affinity_level;
+   __entry->mpidr = proc->mpidr;
+   __entry->midr = proc->midr;
+   __entry->running_state = proc->running_state;
+   __entry->psci_state = proc->psci_state;
+   __entry->version = err_info->version;
+   __entry->type = err_info->type;
+   __entry->err_count = err_info->multiple_error;
+   __entry->flags = err_info->flags;
+   __entry->info = err_info->error_info;
+   __entry->virt_fault_addr = err_info->virt_fault_addr;
+   __entry->phys_fault_addr = err_info->physical_fault_addr;
+   ),
+
+   TP_printk("affinity level: %d; MPIDR: %016llx; MIDR: %016llx; "
+ "running state: %d; PSCI state: %d; version: %d; type: %d; "
+ "error count: 0x%04x; flags: 0x%02x; info: %016llx; "
+ "virtual fault address: %016llx; "
+ "physical fault address: %016llx",
+ __entry->affinity, __entry->mpidr, __entry->midr,
+ __entry->running_state, __entry->psci_state, __entry->version,
+ __entry->type, __entry->err_count, __entry->flags,
+ __entry->info, __entry->virt_fault_addr,
+ __entry->phys_fault_addr)
+);
+
+/*
  * 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.

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu

[PATCH V5 01/10] acpi: apei: read ack upon ghes record consumption

[Tyler Baicar]

A RAS (Reliability, Availability, Serviceability) controller
may be a separate processor running in parallel with OS
execution, and may generate error records for consumption by
the OS. If the RAS controller produces multiple error records,
then they may be overwritten before the OS has consumed them.

The Generic Hardware Error Source (GHES) v2 structure
introduces the capability for the OS to acknowledge the
consumption of the error record generated by the RAS
controller. A RAS controller supporting GHESv2 shall wait for
the acknowledgment before writing a new error record, thus
eliminating the race condition.

Signed-off-by: Jonathan (Zhixiong) Zhang 
Signed-off-by: Richard Ruigrok 
Signed-off-by: Tyler Baicar 
Signed-off-by: Naveen Kaje 
---
 drivers/acpi/apei/ghes.c | 49 +---
 drivers/acpi/apei/hest.c |  7 +--
 include/acpi/ghes.h  |  5 -
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 60746ef..b79abc5 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -45,6 +45,7 @@
 #include 
 #include 
 
+#include 
 #include 
 #include 
 #include 
@@ -79,6 +80,10 @@
((struct acpi_hest_generic_status *)\
 ((struct ghes_estatus_node *)(estatus_node) + 1))
 
+#define HEST_TYPE_GENERIC_V2(ghes) \
+   ((struct acpi_hest_header *)ghes->generic)->type == \
+ACPI_HEST_TYPE_GENERIC_ERROR_V2
+
 /*
  * This driver isn't really modular, however for the time being,
  * continuing to use module_param is the easiest way to remain
@@ -248,10 +253,18 @@ static struct ghes *ghes_new(struct acpi_hest_generic 
*generic)
ghes = kzalloc(sizeof(*ghes), GFP_KERNEL);
if (!ghes)
return ERR_PTR(-ENOMEM);
+
ghes->generic = generic;
+   if (HEST_TYPE_GENERIC_V2(ghes)) {
+   rc = apei_map_generic_address(
+   >generic_v2->read_ack_register);
+   if (rc)
+   goto err_free;
+   }
+
rc = apei_map_generic_address(>error_status_address);
if (rc)
-   goto err_free;
+   goto err_unmap_read_ack_addr;
error_block_length = generic->error_block_length;
if (error_block_length > GHES_ESTATUS_MAX_SIZE) {
pr_warning(FW_WARN GHES_PFX
@@ -263,13 +276,17 @@ static struct ghes *ghes_new(struct acpi_hest_generic 
*generic)
ghes->estatus = kmalloc(error_block_length, GFP_KERNEL);
if (!ghes->estatus) {
rc = -ENOMEM;
-   goto err_unmap;
+   goto err_unmap_status_addr;
}
 
return ghes;
 
-err_unmap:
+err_unmap_status_addr:
apei_unmap_generic_address(>error_status_address);
+err_unmap_read_ack_addr:
+   if (HEST_TYPE_GENERIC_V2(ghes))
+   apei_unmap_generic_address(
+   >generic_v2->read_ack_register);
 err_free:
kfree(ghes);
return ERR_PTR(rc);
@@ -279,6 +296,9 @@ static void ghes_fini(struct ghes *ghes)
 {
kfree(ghes->estatus);
apei_unmap_generic_address(>generic->error_status_address);
+   if (HEST_TYPE_GENERIC_V2(ghes))
+   apei_unmap_generic_address(
+   >generic_v2->read_ack_register);
 }
 
 static inline int ghes_severity(int severity)
@@ -648,6 +668,23 @@ static void ghes_estatus_cache_add(
rcu_read_unlock();
 }
 
+static int ghes_ack_error(struct acpi_hest_generic_v2 *generic_v2)
+{
+   int rc;
+   u64 val = 0;
+
+   rc = apei_read(, _v2->read_ack_register);
+   if (rc)
+   return rc;
+   val &= generic_v2->read_ack_preserve <<
+   generic_v2->read_ack_register.bit_offset;
+   val |= generic_v2->read_ack_write <<
+   generic_v2->read_ack_register.bit_offset;
+   rc = apei_write(val, _v2->read_ack_register);
+
+   return rc;
+}
+
 static int ghes_proc(struct ghes *ghes)
 {
int rc;
@@ -660,6 +697,12 @@ static int ghes_proc(struct ghes *ghes)
ghes_estatus_cache_add(ghes->generic, ghes->estatus);
}
ghes_do_proc(ghes, ghes->estatus);
+
+   if (HEST_TYPE_GENERIC_V2(ghes)) {
+   rc = ghes_ack_error(ghes->generic_v2);
+   if (rc)
+   return rc;
+   }
 out:
ghes_clear_estatus(ghes);
return 0;
diff --git a/drivers/acpi/apei/hest.c b/drivers/acpi/apei/hest.c
index 792a0d9..ef725a9 100644
--- a/drivers/acpi/apei/hest.c
+++ b/drivers/acpi/apei/hest.c
@@ -52,6 +52,7 @@ static const int hest_esrc_len_tab[ACPI_HEST_TYPE_RESERVED] = 
{
[ACPI_HEST_TYPE_AER_ENDPOINT] = sizeof(struct acpi_hest_aer),
[ACPI_HEST_TYPE_AER_BRIDGE] = sizeof(struct acpi_hest_aer_bridge),

[PATCH V5 08/10] ras: acpi / apei: generate trace event for unrecognized CPER section

[Tyler Baicar]

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: Jonathan (Zhixiong) Zhang 
Signed-off-by: Tyler Baicar 
---
 drivers/acpi/apei/ghes.c | 16 +++-
 drivers/ras/ras.c|  1 +
 include/ras/ras_event.h  | 45 +
 3 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 28f801c..c7fbbc1 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -49,6 +49,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #ifdef CONFIG_HAVE_ACPI_APEI_SEA
 #include 
@@ -458,12 +459,19 @@ 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 = _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);
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;
@@ -475,7 +483,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;
 
@@ -508,6 +516,12 @@ static void ghes_do_proc(struct ghes *ghes,
 
}
 #endif
+   else {
+   void *unknown_err = 
acpi_hest_generic_data_payload(gdata);
+   trace_unknown_sec_event(_type,
+   fru_id, fru_text, sec_sev,
+   unknown_err, gdata->error_data_length);
+   }
}
 }
 
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 @@ subsys_initcall(ras_init);
 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 @@ TRACE_EVENT(mc_event,
 );
 
 /*
+ * 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 

[PATCH V5 05/10] acpi: apei: handle SEA notification type for ARMv8

[Tyler Baicar]

ARM APEI extension proposal added SEA (Synchrounous External
Abort) notification type for ARMv8.
Add a new GHES error source handling function for SEA. If an error
source's notification type is SEA, then this function can be registered
into the SEA exception handler. That way GHES will parse and report
SEA exceptions when they occur.

Signed-off-by: Jonathan (Zhixiong) Zhang 
Signed-off-by: Tyler Baicar 
Signed-off-by: Naveen Kaje 
---
 arch/arm64/Kconfig|  1 +
 drivers/acpi/apei/Kconfig | 14 
 drivers/acpi/apei/ghes.c  | 83 +++
 3 files changed, 98 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index b380c87..ae34349 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -53,6 +53,7 @@ config ARM64
select HANDLE_DOMAIN_IRQ
select HARDIRQS_SW_RESEND
select HAVE_ACPI_APEI if (ACPI && EFI)
+   select HAVE_ACPI_APEI_SEA if (ACPI && EFI)
select HAVE_ALIGNED_STRUCT_PAGE if SLUB
select HAVE_ARCH_AUDITSYSCALL
select HAVE_ARCH_BITREVERSE
diff --git a/drivers/acpi/apei/Kconfig b/drivers/acpi/apei/Kconfig
index b0140c8..3786ff1 100644
--- a/drivers/acpi/apei/Kconfig
+++ b/drivers/acpi/apei/Kconfig
@@ -4,6 +4,20 @@ config HAVE_ACPI_APEI
 config HAVE_ACPI_APEI_NMI
bool
 
+config HAVE_ACPI_APEI_SEA
+   bool "APEI Synchronous External Abort logging/recovering support"
+   depends on ARM64
+   help
+ This option should be enabled if the system supports
+ firmware first handling of SEA (Synchronous External Abort).
+ SEA happens with certain faults of data abort or instruction
+ abort synchronous exceptions on ARMv8 systems. If a system
+ supports firmware first handling of SEA, the platform analyzes
+ and handles hardware error notifications with SEA, and it may then
+ form a HW error record for the OS to parse and handle. This
+ option allows the OS to look for such HW error record, and
+ take appropriate action.
+
 config ACPI_APEI
bool "ACPI Platform Error Interface (APEI)"
select MISC_FILESYSTEMS
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 9063d68..839a0e2 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -50,6 +50,10 @@
 #include 
 #include 
 
+#ifdef CONFIG_HAVE_ACPI_APEI_SEA
+#include 
+#endif
+
 #include "apei-internal.h"
 
 #define GHES_PFX   "GHES: "
@@ -770,6 +774,62 @@ static struct notifier_block ghes_notifier_sci = {
.notifier_call = ghes_notify_sci,
 };
 
+#ifdef CONFIG_HAVE_ACPI_APEI_SEA
+static LIST_HEAD(ghes_sea);
+
+static int ghes_notify_sea(struct notifier_block *this,
+ unsigned long event, void *data)
+{
+   struct ghes *ghes;
+   int ret = NOTIFY_DONE;
+
+   rcu_read_lock();
+   list_for_each_entry_rcu(ghes, _sea, list) {
+   if (!ghes_proc(ghes))
+   ret = NOTIFY_OK;
+   }
+   rcu_read_unlock();
+
+   return ret;
+}
+
+static struct notifier_block ghes_notifier_sea = {
+   .notifier_call = ghes_notify_sea,
+};
+
+static int ghes_sea_add(struct ghes *ghes)
+{
+   mutex_lock(_list_mutex);
+   if (list_empty(_sea))
+   register_synchronous_ext_abort_notifier(_notifier_sea);
+   list_add_rcu(>list, _sea);
+   mutex_unlock(_list_mutex);
+   return 0;
+}
+
+static void ghes_sea_remove(struct ghes *ghes)
+{
+   mutex_lock(_list_mutex);
+   list_del_rcu(>list);
+   if (list_empty(_sea))
+   unregister_synchronous_ext_abort_notifier(_notifier_sea);
+   mutex_unlock(_list_mutex);
+}
+#else /* CONFIG_HAVE_ACPI_APEI_SEA */
+static inline int ghes_sea_add(struct ghes *ghes)
+{
+   pr_err(GHES_PFX "ID: %d, trying to add SEA notification which is not 
supported\n",
+  ghes->generic->header.source_id);
+   return -ENOTSUPP;
+}
+
+static inline void ghes_sea_remove(struct ghes *ghes)
+{
+   pr_err(GHES_PFX "ID: %d, trying to remove SEA notification which is not 
supported\n",
+  ghes->generic->header.source_id);
+}
+#endif /* CONFIG_HAVE_ACPI_APEI_SEA */
+
 #ifdef CONFIG_HAVE_ACPI_APEI_NMI
 /*
  * printk is not safe in NMI context.  So in NMI handler, we allocate
@@ -1014,6 +1074,14 @@ static int ghes_probe(struct platform_device *ghes_dev)
case ACPI_HEST_NOTIFY_EXTERNAL:
case ACPI_HEST_NOTIFY_SCI:
break;
+   case ACPI_HEST_NOTIFY_SEA:
+   if (!IS_ENABLED(CONFIG_HAVE_ACPI_APEI_SEA)) {
+   pr_warn(GHES_PFX "Generic hardware error source: %d 
notified via SEA is not supported\n",
+   generic->header.source_id);
+   rc = -ENOTSUPP;
+   goto err;
+   }
+   break;
case ACPI_HEST_NOTIFY_NMI:

[PATCH V5 02/10] ras: acpi/apei: cper: generic error data entry v3 per ACPI 6.1

[Tyler Baicar]

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: Jonathan (Zhixiong) Zhang 
Signed-off-by: Richard Ruigrok 
Signed-off-by: Tyler Baicar 
Signed-off-by: Naveen Kaje 
---
 drivers/acpi/apei/ghes.c| 14 +++---
 drivers/firmware/efi/cper.c | 62 +++--
 include/acpi/ghes.h | 10 
 include/linux/cper.h| 12 +
 4 files changed, 80 insertions(+), 18 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index b79abc5..9063d68 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -420,7 +420,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;
@@ -450,14 +451,18 @@ static void ghes_do_proc(struct ghes *ghes,
 {
int sev, sec_sev;
struct acpi_hest_generic_data *gdata;
+   uuid_le sec_type;
 
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 (!uuid_le_cmp(sec_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);
@@ -467,7 +472,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..7e2439e 100644
--- a/drivers/firmware/efi/cper.c
+++ b/drivers/firmware/efi/cper.c
@@ -32,6 +32,9 @@
 #include 
 #include 
 #include 
+#include 
+#include 
+#include 
 
 #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)) {
-

[PATCH V5 06/10] acpi: apei: panic OS with fatal error status block

[Tyler Baicar]

From: "Jonathan (Zhixiong) Zhang" 

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.

Call panic() after CPER data in error status block is printed if
severity is fatal, before each error section is handled.

Signed-off-by: Jonathan (Zhixiong) Zhang 
Signed-off-by: Tyler Baicar 
---
 drivers/acpi/apei/ghes.c | 19 ++-
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index 839a0e2..28f801c 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -141,6 +141,8 @@ static unsigned long ghes_estatus_pool_size_request;
 static struct ghes_estatus_cache 
*ghes_estatus_caches[GHES_ESTATUS_CACHES_SIZE];
 static atomic_t ghes_estatus_cache_alloced;
 
+static int ghes_panic_timeout __read_mostly = 30;
+
 static int ghes_ioremap_init(void)
 {
ghes_ioremap_area = __get_vm_area(PAGE_SIZE * GHES_IOREMAP_PAGES,
@@ -695,6 +697,13 @@ static int ghes_ack_error(struct acpi_hest_generic_v2 
*generic_v2)
return rc;
 }
 
+static void __ghes_call_panic(void)
+{
+   if (panic_timeout == 0)
+   panic_timeout = ghes_panic_timeout;
+   panic("Fatal hardware error!");
+}
+
 static int ghes_proc(struct ghes *ghes)
 {
int rc;
@@ -706,6 +715,10 @@ static int ghes_proc(struct ghes *ghes)
if (ghes_print_estatus(NULL, ghes->generic, ghes->estatus))
ghes_estatus_cache_add(ghes->generic, ghes->estatus);
}
+   if (ghes_severity(ghes->estatus->error_severity) >= GHES_SEV_PANIC) {
+   __ghes_call_panic();
+   }
+
ghes_do_proc(ghes, ghes->estatus);
 
if (HEST_TYPE_GENERIC_V2(ghes)) {
@@ -850,8 +863,6 @@ static atomic_t ghes_in_nmi = ATOMIC_INIT(0);
 
 static LIST_HEAD(ghes_nmi);
 
-static int ghes_panic_timeout  __read_mostly = 30;
-
 static void ghes_proc_in_irq(struct irq_work *irq_work)
 {
struct llist_node *llnode, *next;
@@ -944,9 +955,7 @@ static void __ghes_panic(struct ghes *ghes)
__ghes_print_estatus(KERN_EMERG, ghes->generic, ghes->estatus);
 
/* reboot to log the error! */
-   if (panic_timeout == 0)
-   panic_timeout = ghes_panic_timeout;
-   panic("Fatal hardware error!");
+   __ghes_call_panic();
 }
 
 static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
-- 
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.

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Tyler Baicar]

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.

Depends on: [PATCH v14] acpi, apei, arm64: APEI initial support for aarch64.
https://lkml.org/lkml/2016/8/10/231

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 

[PATCH V5 04/10] arm64: exception: handle Synchronous External Abort

[Tyler Baicar]

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, go through
the handlers registered in the notification list.
Update fault_info[] with specific SEA faults so that the
new SEA handler is used.

Signed-off-by: Jonathan (Zhixiong) Zhang 
Signed-off-by: Tyler Baicar 
Signed-off-by: Naveen Kaje 
---
 arch/arm64/include/asm/system_misc.h | 13 
 arch/arm64/mm/fault.c| 58 +---
 2 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/arch/arm64/include/asm/system_misc.h 
b/arch/arm64/include/asm/system_misc.h
index 57f110b..9040e1d 100644
--- a/arch/arm64/include/asm/system_misc.h
+++ b/arch/arm64/include/asm/system_misc.h
@@ -64,4 +64,17 @@ extern void (*arm_pm_restart)(enum reboot_mode reboot_mode, 
const char *cmd);
 
 #endif /* __ASSEMBLY__ */
 
+/*
+ * The functions below are used to register and unregister callbacks
+ * that are to be invoked when a Synchronous External Abort (SEA)
+ * occurs. An SEA is raised by certain fault status codes that have
+ * either data or instruction abort as the exception class, and
+ * callbacks may be registered to parse or handle such hardware errors.
+ *
+ * Registered callbacks are run in an interrupt/atomic context. They
+ * are not allowed to block or sleep.
+ */
+int register_synchronous_ext_abort_notifier(struct notifier_block *nb);
+void unregister_synchronous_ext_abort_notifier(struct notifier_block *nb);
+
 #endif /* __ASM_SYSTEM_MISC_H */
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 05d2bd7..fcc49f1 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -39,6 +39,22 @@
 #include 
 #include 
 
+/*
+ * GHES SEA handler code may register a notifier call here to
+ * handle HW error record passed from platform.
+ */
+static ATOMIC_NOTIFIER_HEAD(sea_handler_chain);
+
+int register_synchronous_ext_abort_notifier(struct notifier_block *nb)
+{
+   return atomic_notifier_chain_register(_handler_chain, nb);
+}
+
+void unregister_synchronous_ext_abort_notifier(struct notifier_block *nb)
+{
+   atomic_notifier_chain_unregister(_handler_chain, nb);
+}
+
 static const char *fault_name(unsigned int esr);
 
 #ifdef CONFIG_KPROBES
@@ -480,6 +496,28 @@ 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_synch_ext_abort(unsigned long addr, unsigned int esr, struct 
pt_regs *regs)
+{
+   struct siginfo info;
+
+   atomic_notifier_call_chain(_handler_chain, 0, NULL);
+
+   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;
+   info.si_addr  = (void __user *)addr;
+   arm64_notify_die("", regs, , esr);
+
+   return 0;
+}
+
 static const struct fault_info {
int (*fn)(unsigned long addr, unsigned int esr, struct pt_regs 
*regs);
int sig;
@@ -502,22 +540,22 @@ static const struct fault_info {
{ 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_synch_ext_abort,   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 abort 
(translation table walk)" },
-   { do_bad,   SIGBUS,  0, "synchronous abort 
(translation table walk)" },
-   { do_bad,   SIGBUS,  0, "synchronous abort 
(translation table walk)" },
-   { do_bad,   SIGBUS,  0, "synchronous abort 
(translation table walk)" },
-   { do_bad,   SIGBUS,  0, "synchronous parity 
error"  },
+   { do_synch_ext_abort,   SIGBUS,  0, "level 0 SEA (trans tbl 
walk)"  },
+   { do_synch_ext_abort,   SIGBUS,  0, "level 1 SEA (trans tbl 
walk)"  },
+   { do_synch_ext_abort,   SIGBUS,  0, "level 2 SEA (trans tbl 
walk)"  },
+   { do_synch_ext_abort,   SIGBUS,  0, "level 3 SEA (trans tbl 

Re: [kvm-unit-tests PATCH v10 3/3] arm: pmu: Add CPI checking

[Christopher Covington]

Hi Wei,

On 11/21/2016 03:24 PM, Wei Huang wrote:
> From: Christopher Covington 

I really appreciate your work on these patches. If for any or all of these
you have more lines added/modified than me (or using any other better
metric), please make sure to change the author to be you with
`git commit --amend --reset-author` or equivalent.

> Calculate the numbers of cycles per instruction (CPI) implied by ARM
> PMU cycle counter values. The code includes a strict checking facility
> intended for the -icount option in TCG mode in the configuration file.
> 
> Signed-off-by: Christopher Covington 
> Signed-off-by: Wei Huang 
> ---
>  arm/pmu.c | 119 
> +-
>  arm/unittests.cfg |  14 +++
>  2 files changed, 132 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 176b070..129ef1e 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -104,6 +104,25 @@ static inline uint32_t id_dfr0_read(void)
>   asm volatile("mrc p15, 0, %0, c0, c1, 2" : "=r" (val));
>   return val;
>  }
> +
> +/*
> + * Extra instructions inserted by the compiler would be difficult to 
> compensate
> + * for, so hand assemble everything between, and including, the PMCR accesses
> + * to start and stop counting. Total cycles = isb + mcr + 2*loop = 2 + 
> 2*loop.
> + */
> +static inline void precise_cycles_loop(int loop, uint32_t pmcr)

Nit: I would call this precise_instrs_loop. How many cycles it takes is
IMPLEMENTATION DEFINED.

> +{
> + asm volatile(
> + "   mcr p15, 0, %[pmcr], c9, c12, 0\n"
> + "   isb\n"
> + "1: subs%[loop], %[loop], #1\n"
> + "   bgt 1b\n"

Is there any chance we might need an isb here, to prevent the stop from 
happening
before or during the loop? Where ISBs are required, the Linux best practice is 
to
diligently comment why they are needed. Perhaps it would be a good habit to
carry over into kvm-unit-tests.

> + "   mcr p15, 0, %[z], c9, c12, 0\n"
> + "   isb\n"
> + : [loop] "+r" (loop)
> + : [pmcr] "r" (pmcr), [z] "r" (0)
> + : "cc");
> +}
>  #elif defined(__aarch64__)
>  static inline uint32_t pmcr_read(void)
>  {
> @@ -150,6 +169,25 @@ static inline uint32_t id_dfr0_read(void)
>   asm volatile("mrs %0, id_dfr0_el1" : "=r" (id));
>   return id;
>  }
> +
> +/*
> + * Extra instructions inserted by the compiler would be difficult to 
> compensate
> + * for, so hand assemble everything between, and including, the PMCR accesses
> + * to start and stop counting. Total cycles = isb + msr + 2*loop = 2 + 
> 2*loop.
> + */
> +static inline void precise_cycles_loop(int loop, uint32_t pmcr)
> +{
> + asm volatile(
> + "   msr pmcr_el0, %[pmcr]\n"
> + "   isb\n"
> + "1: subs%[loop], %[loop], #1\n"
> + "   b.gt1b\n"
> + "   msr pmcr_el0, xzr\n"
> + "   isb\n"
> + : [loop] "+r" (loop)
> + : [pmcr] "r" (pmcr)
> + : "cc");
> +}
>  #endif
>  
>  /*
> @@ -208,6 +246,79 @@ static bool check_cycles_increase(void)
>   return success;
>  }
>  
> +/*
> + * Execute a known number of guest instructions. Only odd instruction counts
> + * greater than or equal to 3 are supported by the in-line assembly code. The

Nit: needs updating as well (or removal if you prefer)

> + * control register (PMCR_EL0) is initialized with the provided value 
> (allowing
> + * for example for the cycle counter or event counters to be reset). At the 
> end
> + * of the exact instruction loop, zero is written to PMCR_EL0 to disable
> + * counting, allowing the cycle counter or event counters to be read at the
> + * leisure of the calling code.
> + */
> +static void measure_instrs(int num, uint32_t pmcr)
> +{
> + int loop = (num - 2) / 2;
> +
> + assert(num >= 4 && ((num - 2) % 2 == 0));
> + precise_cycles_loop(loop, pmcr);
> +}
> +
> +/*
> + * Measure cycle counts for various known instruction counts. Ensure that the
> + * cycle counter progresses (similar to check_cycles_increase() but with more
> + * instructions and using reset and stop controls). If supplied a positive,
> + * nonzero CPI parameter, also strictly check that every measurement matches
> + * it. Strict CPI checking is used to test -icount mode.
> + */
> +static bool check_cpi(int cpi)
> +{
> + uint32_t pmcr = pmcr_read() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
> +
> + /* init before event access, this test only cares about cycle count */
> + pmcntenset_write(1 << PMU_CYCLE_IDX);
> + pmccfiltr_write(0); /* count cycles in EL0, EL1, but not EL2 */
> +
> + if (cpi > 0)
> + printf("Checking for CPI=%d.\n", cpi);
> + printf("instrs : cycles0 cycles1 ...\n");
> +
> + for (unsigned int i = 4; i < 300; i += 32) {
> + uint64_t avg, sum = 0;
> +
> + printf("%d :", i);
> + for (int j = 0; j < NR_SAMPLES; j++) 

[kvm-unit-tests PATCH v10 1/3] arm: Add PMU test

[Wei Huang]

From: Christopher Covington 

Beginning with a simple sanity check of the control register, add
a unit test for the ARM Performance Monitors Unit (PMU).

Signed-off-by: Christopher Covington 
Signed-off-by: Wei Huang 
Reviewed-by: Andrew Jones 
---
 arm/Makefile.common |  3 ++-
 arm/pmu.c   | 74 +
 arm/unittests.cfg   |  5 
 3 files changed, 81 insertions(+), 1 deletion(-)
 create mode 100644 arm/pmu.c

diff --git a/arm/Makefile.common b/arm/Makefile.common
index f37b5c2..5da2fdd 100644
--- a/arm/Makefile.common
+++ b/arm/Makefile.common
@@ -12,7 +12,8 @@ endif
 tests-common = \
$(TEST_DIR)/selftest.flat \
$(TEST_DIR)/spinlock-test.flat \
-   $(TEST_DIR)/pci-test.flat
+   $(TEST_DIR)/pci-test.flat \
+   $(TEST_DIR)/pmu.flat
 
 all: test_cases
 
diff --git a/arm/pmu.c b/arm/pmu.c
new file mode 100644
index 000..9d9c53b
--- /dev/null
+++ b/arm/pmu.c
@@ -0,0 +1,74 @@
+/*
+ * Test the ARM Performance Monitors Unit (PMU).
+ *
+ * Copyright (c) 2015-2016, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License version 2.1 and
+ * only version 2.1 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU Lesser General Public License
+ * for more details.
+ */
+#include "libcflat.h"
+#include "asm/barrier.h"
+
+#define PMU_PMCR_N_SHIFT   11
+#define PMU_PMCR_N_MASK0x1f
+#define PMU_PMCR_ID_SHIFT  16
+#define PMU_PMCR_ID_MASK   0xff
+#define PMU_PMCR_IMP_SHIFT 24
+#define PMU_PMCR_IMP_MASK  0xff
+
+#if defined(__arm__)
+static inline uint32_t pmcr_read(void)
+{
+   uint32_t ret;
+
+   asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
+   return ret;
+}
+#elif defined(__aarch64__)
+static inline uint32_t pmcr_read(void)
+{
+   uint32_t ret;
+
+   asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
+   return ret;
+}
+#endif
+
+/*
+ * As a simple sanity check on the PMCR_EL0, ensure the implementer field isn't
+ * null. Also print out a couple other interesting fields for diagnostic
+ * purposes. For example, as of fall 2016, QEMU TCG mode doesn't implement
+ * event counters and therefore reports zero event counters, but hopefully
+ * support for at least the instructions event will be added in the future and
+ * the reported number of event counters will become nonzero.
+ */
+static bool check_pmcr(void)
+{
+   uint32_t pmcr;
+
+   pmcr = pmcr_read();
+
+   printf("PMU implementer: %c\n",
+  (pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK);
+   printf("Identification code: 0x%x\n",
+  (pmcr >> PMU_PMCR_ID_SHIFT) & PMU_PMCR_ID_MASK);
+   printf("Event counters:  %d\n",
+  (pmcr >> PMU_PMCR_N_SHIFT) & PMU_PMCR_N_MASK);
+
+   return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0;
+}
+
+int main(void)
+{
+   report_prefix_push("pmu");
+
+   report("Control register", check_pmcr());
+
+   return report_summary();
+}
diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index ae32a42..816f494 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -58,3 +58,8 @@ groups = selftest
 [pci-test]
 file = pci-test.flat
 groups = pci
+
+# Test PMU support
+[pmu]
+file = pmu.flat
+groups = pmu
-- 
1.8.3.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v10 2/3] arm: pmu: Check cycle count increases

[Wei Huang]

From: Christopher Covington 

Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
even for the smallest delta of two subsequent reads.

Signed-off-by: Christopher Covington 
Signed-off-by: Wei Huang 
---
 arm/pmu.c | 156 ++
 1 file changed, 156 insertions(+)

diff --git a/arm/pmu.c b/arm/pmu.c
index 9d9c53b..176b070 100644
--- a/arm/pmu.c
+++ b/arm/pmu.c
@@ -15,6 +15,9 @@
 #include "libcflat.h"
 #include "asm/barrier.h"
 
+#define PMU_PMCR_E (1 << 0)
+#define PMU_PMCR_C (1 << 2)
+#define PMU_PMCR_LC(1 << 6)
 #define PMU_PMCR_N_SHIFT   11
 #define PMU_PMCR_N_MASK0x1f
 #define PMU_PMCR_ID_SHIFT  16
@@ -22,6 +25,14 @@
 #define PMU_PMCR_IMP_SHIFT 24
 #define PMU_PMCR_IMP_MASK  0xff
 
+#define ID_DFR0_PERFMON_SHIFT 24
+#define ID_DFR0_PERFMON_MASK  0xf
+
+#define PMU_CYCLE_IDX 31
+
+#define NR_SAMPLES 10
+
+static unsigned int pmu_version;
 #if defined(__arm__)
 static inline uint32_t pmcr_read(void)
 {
@@ -30,6 +41,69 @@ static inline uint32_t pmcr_read(void)
asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
return ret;
 }
+
+static inline void pmcr_write(uint32_t value)
+{
+   asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (value));
+   isb();
+}
+
+static inline void pmselr_write(uint32_t value)
+{
+   asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));
+   isb();
+}
+
+static inline void pmxevtyper_write(uint32_t value)
+{
+   asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
+}
+
+static inline uint64_t pmccntr_read(void)
+{
+   uint32_t lo, hi = 0;
+
+   if (pmu_version == 0x3)
+   asm volatile("mrrc p15, 0, %0, %1, c9" : "=r" (lo), "=r" (hi));
+   else
+   asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (lo));
+
+   return ((uint64_t)hi << 32) | lo;
+}
+
+static inline void pmccntr_write(uint64_t value)
+{
+   uint32_t lo, hi;
+
+   lo = value & 0x;
+   hi = (value >> 32) & 0x;
+
+   if (pmu_version == 0x3)
+   asm volatile("mcrr p15, 0, %0, %1, c9" : : "r" (lo), "r" (hi));
+   else
+   asm volatile("mcr p15, 0, %0, c9, c13, 0" : : "r" (lo));
+}
+
+static inline void pmcntenset_write(uint32_t value)
+{
+   asm volatile("mcr p15, 0, %0, c9, c12, 1" : : "r" (value));
+}
+
+/* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */
+static inline void pmccfiltr_write(uint32_t value)
+{
+   pmselr_write(PMU_CYCLE_IDX);
+   pmxevtyper_write(value);
+   isb();
+}
+
+static inline uint32_t id_dfr0_read(void)
+{
+   uint32_t val;
+
+   asm volatile("mrc p15, 0, %0, c0, c1, 2" : "=r" (val));
+   return val;
+}
 #elif defined(__aarch64__)
 static inline uint32_t pmcr_read(void)
 {
@@ -38,6 +112,44 @@ static inline uint32_t pmcr_read(void)
asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
return ret;
 }
+
+static inline void pmcr_write(uint32_t value)
+{
+   asm volatile("msr pmcr_el0, %0" : : "r" (value));
+   isb();
+}
+
+static inline uint64_t pmccntr_read(void)
+{
+   uint64_t cycles;
+
+   asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
+   return cycles;
+}
+
+static inline void pmccntr_write(uint64_t value)
+{
+   asm volatile("msr pmccntr_el0, %0" : : "r" (value));
+}
+
+static inline void pmcntenset_write(uint32_t value)
+{
+   asm volatile("msr pmcntenset_el0, %0" : : "r" (value));
+}
+
+static inline void pmccfiltr_write(uint32_t value)
+{
+   asm volatile("msr pmccfiltr_el0, %0" : : "r" (value));
+   isb();
+}
+
+static inline uint32_t id_dfr0_read(void)
+{
+   uint32_t id;
+
+   asm volatile("mrs %0, id_dfr0_el1" : "=r" (id));
+   return id;
+}
 #endif
 
 /*
@@ -64,11 +176,55 @@ static bool check_pmcr(void)
return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0;
 }
 
+/*
+ * Ensure that the cycle counter progresses between back-to-back reads.
+ */
+static bool check_cycles_increase(void)
+{
+   bool success = true;
+
+   /* init before event access, this test only cares about cycle count */
+   pmcntenset_write(1 << PMU_CYCLE_IDX);
+   pmccfiltr_write(0); /* count cycles in EL0, EL1, but not EL2 */
+   pmccntr_write(0);
+
+   pmcr_write(pmcr_read() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E);
+
+   for (int i = 0; i < NR_SAMPLES; i++) {
+   uint64_t a, b;
+
+   a = pmccntr_read();
+   b = pmccntr_read();
+
+   if (a >= b) {
+   printf("Read %"PRId64" then %"PRId64".\n", a, b);
+   success = false;
+   break;
+   }
+   }
+
+   pmcr_write(pmcr_read() & ~PMU_PMCR_E);
+
+   return success;
+}
+
+void pmu_init(void)
+{
+   uint32_t dfr0;
+
+   /* probe pmu version */
+   dfr0 = id_dfr0_read();
+   

Re: [kvm-unit-tests PATCH] configure: honour $ARCH and $CROSS_COMPILE

[Radim Krčmář]

2016-11-21 18:22+, Andre Przywara:
> Both environment variables seem to be standard in cross-compilation
> environments, especially with Linux.
> Let the configure script take those into account when setting the default
> values for --arch and --cross-prefix. Explicitly specifying the latter
> on the configure command line still works as expected.
> 
> Signed-off-by: Andre Przywara 
> ---
> Hi,
> 
> this maybe a personal itch to scratch here, since I set these two
> variables in my environment via a (sourced) script here and never have
> to care about the particular cross-compiler prefix, for instance.
> It looks rather generic, though, so I was wondering if this is useful
> upstream as well.

Definitely useful, thanks.

> diff --git a/configure b/configure
> @@ -8,8 +8,9 @@ objdump=objdump
>  ar=ar
>  addr2line=addr2line
>  arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
> +[ -n "$ARCH" ] && arch="$ARCH"
>  host=$arch

Host should be set before we override arch, though.

I can swap those two lines when applying, but would prefer something
like:

  host=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
  arch=${ARCH:-$host}

> -cross_prefix=
> +cross_prefix=${CROSS_COMPILE}
>  endian=""
>  pretty_print_stacks=yes

(And --help is not printing the default value for cross_prefix, which
 would be nice to change when we can have a non-empty one now.)
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH] configure: honour $ARCH and $CROSS_COMPILE

[Andre Przywara]

Both environment variables seem to be standard in cross-compilation
environments, especially with Linux.
Let the configure script take those into account when setting the default
values for --arch and --cross-prefix. Explicitly specifying the latter
on the configure command line still works as expected.

Signed-off-by: Andre Przywara 
---
Hi,

this maybe a personal itch to scratch here, since I set these two
variables in my environment via a (sourced) script here and never have
to care about the particular cross-compiler prefix, for instance.
It looks rather generic, though, so I was wondering if this is useful
upstream as well.

Cheers,
Andre.

 configure | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 995c8fa..9bb38ba 100755
--- a/configure
+++ b/configure
@@ -8,8 +8,9 @@ objdump=objdump
 ar=ar
 addr2line=addr2line
 arch=`uname -m | sed -e 's/i.86/i386/;s/arm.*/arm/;s/ppc64.*/ppc64/'`
+[ -n "$ARCH" ] && arch="$ARCH"
 host=$arch
-cross_prefix=
+cross_prefix=${CROSS_COMPILE}
 endian=""
 pretty_print_stacks=yes
 
-- 
2.9.0

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access

[Christoffer Dall]

On Mon, Nov 21, 2016 at 06:56:08PM +0530, Vijay Kilari wrote:
> On Mon, Nov 21, 2016 at 3:49 PM, Christoffer Dall
>  wrote:
> > On Fri, Nov 18, 2016 at 10:28:34PM +0530, Vijay Kilari wrote:
> >> On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
> >>  wrote:
> >> > On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
> >> >> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
> >> >>  wrote:
> >> >> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kil...@gmail.com 
> >> >> > wrote:
> >> >> >> From: Vijaya Kumar K 
> >> >> >>
> >> >> >> VGICv3 CPU interface registers are accessed using
> >> >> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> >> >> >> as 64-bit. The cpu MPIDR value is passed along with register id.
> >> >> >> is used to identify the cpu for registers access.
> >> >> >>
> >> >> >> The version of VGIC v3 specification is define here
> >> >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >> >> >>
> >> >> >> Signed-off-by: Pavel Fedin 
> >> >> >> Signed-off-by: Vijaya Kumar K 
> >> >> >> ---
> >> >> >>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
> >> >> >>  arch/arm64/kvm/Makefile |   1 +
> >> >> >>  include/kvm/arm_vgic.h  |   9 +
> >> >> >>  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 +++
> >> >> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  19 +++
> >> >> >>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 
> >> >> >> 
> >> >> >>  virt/kvm/arm/vgic/vgic-v3.c |   8 +
> >> >> >>  virt/kvm/arm/vgic/vgic.h|   4 +
> >> >> >>  8 files changed, 395 insertions(+)
> >> >> >>
> >> >> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> >> >> >> b/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> index 56dc08d..91c7137 100644
> >> >> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
> >> >> >>   (0xULL << 
> >> >> >> KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
> >> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
> >> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
> >> >> >> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> >> >> >> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0x)
> >> >> >>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> >> >> >>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
> >> >> >>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> >> >> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS6
> >> >> >> +
> >> >> >>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
> >> >> >>
> >> >> >>  /* Device Control API on vcpu fd */
> >> >> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> >> >> >> index d50a82a..1a14e29 100644
> >> >> >> --- a/arch/arm64/kvm/Makefile
> >> >> >> +++ b/arch/arm64/kvm/Makefile
> >> >> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += 
> >> >> >> $(KVM)/arm/vgic/vgic-mmio-v3.o
> >> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> >> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
> >> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> >> >> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
> >> >> >
> >> >> > Thi is making me wonder:  Are we properly handling GICv3 save/restore
> >> >> > for AArch32 now that we have GICv3 support for AArch32?  By properly I
> >> >> > mean that either it is clearly only supported on AArch64 systems or 
> >> >> > it's
> >> >> > supported on both AArch64 and AArch32, but it shouldn't break randomly
> >> >> > on AArch32.
> >> >>
> >> >> It supports both AArch64 and AArch64 in handling of system registers
> >> >> save/restore.
> >> >> All system registers that we save/restore are 32-bit for both aarch64
> >> >> and aarch32.
> >> >> Though opcode op0 should be zero for aarch32, the remaining Op and CRn 
> >> >> codes
> >> >> are same. However the codes sent by qemu is matched and register
> >> >> are handled properly irrespective of AArch32 or AArch64.
> >> >>
> >> >> I don't have platform which support AArch32 guests to verify.
> >> >
> >> > Actually this is not about the guest, it's about an ARMv8 AArch32 host
> >> > that has a GICv3.
> >> >
> >> > I just tried to do a v7 compile with your patches, and it results in an
> >> > epic failure, so there's something for you to look at.
> >> >
> >> >> >
> >> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
> >> >> >>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> >> >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> >> >> index 002f092..730a18a 100644
> >> >> >> --- a/include/kvm/arm_vgic.h
> >> >> >> +++ b/include/kvm/arm_vgic.h
> >> >> >> @@ -71,6 +71,9 @@ struct vgic_global {
> >> >> >>
> >> >> >>   /* GIC system register CPU interface */
> >> >> >>   struct static_key_false 

Re: [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access

[Christoffer Dall]

On Mon, Nov 21, 2016 at 07:02:36PM +0530, Vijay Kilari wrote:
> On Sun, Nov 20, 2016 at 6:50 PM, Christoffer Dall
>  wrote:
> > On Sat, Nov 19, 2016 at 12:18:53AM +0530, Vijay Kilari wrote:
> >> On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
> >>  wrote:
> >> > On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
> >> >> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
> >> >>  wrote:
> >> >> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kil...@gmail.com 
> >> >> > wrote:
> >> >> >> From: Vijaya Kumar K 
> >> >> >>
> >> >> >> VGICv3 CPU interface registers are accessed using
> >> >> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> >> >> >> as 64-bit. The cpu MPIDR value is passed along with register id.
> >> >> >> is used to identify the cpu for registers access.
> >> >> >>
> >> >> >> The version of VGIC v3 specification is define here
> >> >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >> >> >>
> >> >> >> Signed-off-by: Pavel Fedin 
> >> >> >> Signed-off-by: Vijaya Kumar K 
> >> >> >> ---
> >> >> >>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
> >> >> >>  arch/arm64/kvm/Makefile |   1 +
> >> >> >>  include/kvm/arm_vgic.h  |   9 +
> >> >> >>  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 +++
> >> >> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  19 +++
> >> >> >>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 
> >> >> >> 
> >> >> >>  virt/kvm/arm/vgic/vgic-v3.c |   8 +
> >> >> >>  virt/kvm/arm/vgic/vgic.h|   4 +
> >> >> >>  8 files changed, 395 insertions(+)
> >> >> >>
> >> >> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> >> >> >> b/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> index 56dc08d..91c7137 100644
> >> >> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> >> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
> >> >> >>   (0xULL << 
> >> >> >> KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
> >> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
> >> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
> >> >> >> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> >> >> >> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0x)
> >> >> >>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> >> >> >>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
> >> >> >>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> >> >> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS6
> >> >> >> +
> >> >> >>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
> >> >> >>
> >> >> >>  /* Device Control API on vcpu fd */
> >> >> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> >> >> >> index d50a82a..1a14e29 100644
> >> >> >> --- a/arch/arm64/kvm/Makefile
> >> >> >> +++ b/arch/arm64/kvm/Makefile
> >> >> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += 
> >> >> >> $(KVM)/arm/vgic/vgic-mmio-v3.o
> >> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> >> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
> >> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> >> >> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
> >> >> >
> >> >> > Thi is making me wonder:  Are we properly handling GICv3 save/restore
> >> >> > for AArch32 now that we have GICv3 support for AArch32?  By properly I
> >> >> > mean that either it is clearly only supported on AArch64 systems or 
> >> >> > it's
> >> >> > supported on both AArch64 and AArch32, but it shouldn't break randomly
> >> >> > on AArch32.
> >> >>
> >> >> It supports both AArch64 and AArch64 in handling of system registers
> >> >> save/restore.
> >> >> All system registers that we save/restore are 32-bit for both aarch64
> >> >> and aarch32.
> >> >> Though opcode op0 should be zero for aarch32, the remaining Op and CRn 
> >> >> codes
> >> >> are same. However the codes sent by qemu is matched and register
> >> >> are handled properly irrespective of AArch32 or AArch64.
> >> >>
> >> >> I don't have platform which support AArch32 guests to verify.
> >> >
> >> > Actually this is not about the guest, it's about an ARMv8 AArch32 host
> >> > that has a GICv3.
> >> >
> >> > I just tried to do a v7 compile with your patches, and it results in an
> >> > epic failure, so there's something for you to look at.
> >> >
> >>
> >> Could you please share you config file?. I tried with multi_v7 defconfig 
> >> with
> >> CONFIG KVM and CONFIG_KVM_ARM_HOST enabled. it compiled for me.
> >
> > I think this has to do with which branch you apply your patches to.
> > When applied to kvmarm/next, it fails.
> >
> > Here's the integration I did:
> > https://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git 
> > tmp-gicv3-migrate-v8
> >
> > Here's the config:
> > https://transfer.sh/xkAxp/.config
> >
> 
> 

Re: [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access

[Vijay Kilari]

On Sun, Nov 20, 2016 at 6:50 PM, Christoffer Dall
 wrote:
> On Sat, Nov 19, 2016 at 12:18:53AM +0530, Vijay Kilari wrote:
>> On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
>>  wrote:
>> > On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
>> >> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
>> >>  wrote:
>> >> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kil...@gmail.com wrote:
>> >> >> From: Vijaya Kumar K 
>> >> >>
>> >> >> VGICv3 CPU interface registers are accessed using
>> >> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>> >> >> as 64-bit. The cpu MPIDR value is passed along with register id.
>> >> >> is used to identify the cpu for registers access.
>> >> >>
>> >> >> The version of VGIC v3 specification is define here
>> >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>> >> >>
>> >> >> Signed-off-by: Pavel Fedin 
>> >> >> Signed-off-by: Vijaya Kumar K 
>> >> >> ---
>> >> >>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
>> >> >>  arch/arm64/kvm/Makefile |   1 +
>> >> >>  include/kvm/arm_vgic.h  |   9 +
>> >> >>  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 +++
>> >> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  19 +++
>> >> >>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 
>> >> >> 
>> >> >>  virt/kvm/arm/vgic/vgic-v3.c |   8 +
>> >> >>  virt/kvm/arm/vgic/vgic.h|   4 +
>> >> >>  8 files changed, 395 insertions(+)
>> >> >>
>> >> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> >> >> b/arch/arm64/include/uapi/asm/kvm.h
>> >> >> index 56dc08d..91c7137 100644
>> >> >> --- a/arch/arm64/include/uapi/asm/kvm.h
>> >> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> >> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
>> >> >>   (0xULL << 
>> >> >> KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
>> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
>> >> >> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>> >> >> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0x)
>> >> >>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
>> >> >>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
>> >> >>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>> >> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS6
>> >> >> +
>> >> >>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
>> >> >>
>> >> >>  /* Device Control API on vcpu fd */
>> >> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> >> >> index d50a82a..1a14e29 100644
>> >> >> --- a/arch/arm64/kvm/Makefile
>> >> >> +++ b/arch/arm64/kvm/Makefile
>> >> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += 
>> >> >> $(KVM)/arm/vgic/vgic-mmio-v3.o
>> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>> >> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
>> >> >
>> >> > Thi is making me wonder:  Are we properly handling GICv3 save/restore
>> >> > for AArch32 now that we have GICv3 support for AArch32?  By properly I
>> >> > mean that either it is clearly only supported on AArch64 systems or it's
>> >> > supported on both AArch64 and AArch32, but it shouldn't break randomly
>> >> > on AArch32.
>> >>
>> >> It supports both AArch64 and AArch64 in handling of system registers
>> >> save/restore.
>> >> All system registers that we save/restore are 32-bit for both aarch64
>> >> and aarch32.
>> >> Though opcode op0 should be zero for aarch32, the remaining Op and CRn 
>> >> codes
>> >> are same. However the codes sent by qemu is matched and register
>> >> are handled properly irrespective of AArch32 or AArch64.
>> >>
>> >> I don't have platform which support AArch32 guests to verify.
>> >
>> > Actually this is not about the guest, it's about an ARMv8 AArch32 host
>> > that has a GICv3.
>> >
>> > I just tried to do a v7 compile with your patches, and it results in an
>> > epic failure, so there's something for you to look at.
>> >
>>
>> Could you please share you config file?. I tried with multi_v7 defconfig with
>> CONFIG KVM and CONFIG_KVM_ARM_HOST enabled. it compiled for me.
>
> I think this has to do with which branch you apply your patches to.
> When applied to kvmarm/next, it fails.
>
> Here's the integration I did:
> https://git.linaro.org/people/christoffer.dall/linux-kvm-arm.git 
> tmp-gicv3-migrate-v8
>
> Here's the config:
> https://transfer.sh/xkAxp/.config
>

Thanks for shareing the details, I could reproduce them.
However virt/kvm/arm/vgic/vgic-sys-reg-v3.c is written with
sys_regs_desc for AArch64.
For AArch32/v7, it has be to coproc_reg. I propose to add separate file for arm
which handles ICC* reg save/restore using coproc_reg.

> Here's the 

Re: [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access

[Vijay Kilari]

On Mon, Nov 21, 2016 at 3:49 PM, Christoffer Dall
 wrote:
> On Fri, Nov 18, 2016 at 10:28:34PM +0530, Vijay Kilari wrote:
>> On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
>>  wrote:
>> > On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
>> >> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
>> >>  wrote:
>> >> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kil...@gmail.com wrote:
>> >> >> From: Vijaya Kumar K 
>> >> >>
>> >> >> VGICv3 CPU interface registers are accessed using
>> >> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
>> >> >> as 64-bit. The cpu MPIDR value is passed along with register id.
>> >> >> is used to identify the cpu for registers access.
>> >> >>
>> >> >> The version of VGIC v3 specification is define here
>> >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
>> >> >>
>> >> >> Signed-off-by: Pavel Fedin 
>> >> >> Signed-off-by: Vijaya Kumar K 
>> >> >> ---
>> >> >>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
>> >> >>  arch/arm64/kvm/Makefile |   1 +
>> >> >>  include/kvm/arm_vgic.h  |   9 +
>> >> >>  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 +++
>> >> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  19 +++
>> >> >>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 
>> >> >> 
>> >> >>  virt/kvm/arm/vgic/vgic-v3.c |   8 +
>> >> >>  virt/kvm/arm/vgic/vgic.h|   4 +
>> >> >>  8 files changed, 395 insertions(+)
>> >> >>
>> >> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
>> >> >> b/arch/arm64/include/uapi/asm/kvm.h
>> >> >> index 56dc08d..91c7137 100644
>> >> >> --- a/arch/arm64/include/uapi/asm/kvm.h
>> >> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
>> >> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
>> >> >>   (0xULL << 
>> >> >> KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
>> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
>> >> >> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>> >> >> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0x)
>> >> >>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
>> >> >>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
>> >> >>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>> >> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS6
>> >> >> +
>> >> >>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
>> >> >>
>> >> >>  /* Device Control API on vcpu fd */
>> >> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
>> >> >> index d50a82a..1a14e29 100644
>> >> >> --- a/arch/arm64/kvm/Makefile
>> >> >> +++ b/arch/arm64/kvm/Makefile
>> >> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += 
>> >> >> $(KVM)/arm/vgic/vgic-mmio-v3.o
>> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
>> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
>> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
>> >> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
>> >> >
>> >> > Thi is making me wonder:  Are we properly handling GICv3 save/restore
>> >> > for AArch32 now that we have GICv3 support for AArch32?  By properly I
>> >> > mean that either it is clearly only supported on AArch64 systems or it's
>> >> > supported on both AArch64 and AArch32, but it shouldn't break randomly
>> >> > on AArch32.
>> >>
>> >> It supports both AArch64 and AArch64 in handling of system registers
>> >> save/restore.
>> >> All system registers that we save/restore are 32-bit for both aarch64
>> >> and aarch32.
>> >> Though opcode op0 should be zero for aarch32, the remaining Op and CRn 
>> >> codes
>> >> are same. However the codes sent by qemu is matched and register
>> >> are handled properly irrespective of AArch32 or AArch64.
>> >>
>> >> I don't have platform which support AArch32 guests to verify.
>> >
>> > Actually this is not about the guest, it's about an ARMv8 AArch32 host
>> > that has a GICv3.
>> >
>> > I just tried to do a v7 compile with your patches, and it results in an
>> > epic failure, so there's something for you to look at.
>> >
>> >> >
>> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
>> >> >>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
>> >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> >> >> index 002f092..730a18a 100644
>> >> >> --- a/include/kvm/arm_vgic.h
>> >> >> +++ b/include/kvm/arm_vgic.h
>> >> >> @@ -71,6 +71,9 @@ struct vgic_global {
>> >> >>
>> >> >>   /* GIC system register CPU interface */
>> >> >>   struct static_key_false gicv3_cpuif;
>> >> >> +
>> >> >> + /* Cache ICH_VTR_EL2 reg value */
>> >> >> + u32 ich_vtr_el2;
>> >> >>  };
>> >> >>
>> >> >>  extern struct vgic_global kvm_vgic_global_state;
>> >> >> @@ -269,6 +272,12 @@ struct vgic_cpu {
>> >> >>   u64 pendbaser;

Re: [PATCH v8 6/7] arm/arm64: vgic: Implement VGICv3 CPU interface access

[Christoffer Dall]

On Fri, Nov 18, 2016 at 10:28:34PM +0530, Vijay Kilari wrote:
> On Thu, Nov 17, 2016 at 9:39 PM, Christoffer Dall
>  wrote:
> > On Thu, Nov 17, 2016 at 09:25:59PM +0530, Vijay Kilari wrote:
> >> On Thu, Nov 17, 2016 at 12:22 AM, Christoffer Dall
> >>  wrote:
> >> > On Fri, Nov 04, 2016 at 04:43:32PM +0530, vijay.kil...@gmail.com wrote:
> >> >> From: Vijaya Kumar K 
> >> >>
> >> >> VGICv3 CPU interface registers are accessed using
> >> >> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> >> >> as 64-bit. The cpu MPIDR value is passed along with register id.
> >> >> is used to identify the cpu for registers access.
> >> >>
> >> >> The version of VGIC v3 specification is define here
> >> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >> >>
> >> >> Signed-off-by: Pavel Fedin 
> >> >> Signed-off-by: Vijaya Kumar K 
> >> >> ---
> >> >>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
> >> >>  arch/arm64/kvm/Makefile |   1 +
> >> >>  include/kvm/arm_vgic.h  |   9 +
> >> >>  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 +++
> >> >>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  19 +++
> >> >>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 324 
> >> >> 
> >> >>  virt/kvm/arm/vgic/vgic-v3.c |   8 +
> >> >>  virt/kvm/arm/vgic/vgic.h|   4 +
> >> >>  8 files changed, 395 insertions(+)
> >> >>
> >> >> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> >> >> b/arch/arm64/include/uapi/asm/kvm.h
> >> >> index 56dc08d..91c7137 100644
> >> >> --- a/arch/arm64/include/uapi/asm/kvm.h
> >> >> +++ b/arch/arm64/include/uapi/asm/kvm.h
> >> >> @@ -206,9 +206,12 @@ struct kvm_arch_memory_slot {
> >> >>   (0xULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
> >> >>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
> >> >> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
> >> >> +#define   KVM_DEV_ARM_VGIC_SYSREG_INSTR_MASK (0x)
> >> >>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
> >> >>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
> >> >>  #define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
> >> >> +#define KVM_DEV_ARM_VGIC_CPU_SYSREGS6
> >> >> +
> >> >>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
> >> >>
> >> >>  /* Device Control API on vcpu fd */
> >> >> diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
> >> >> index d50a82a..1a14e29 100644
> >> >> --- a/arch/arm64/kvm/Makefile
> >> >> +++ b/arch/arm64/kvm/Makefile
> >> >> @@ -32,5 +32,6 @@ kvm-$(CONFIG_KVM_ARM_HOST) += 
> >> >> $(KVM)/arm/vgic/vgic-mmio-v3.o
> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-kvm-device.o
> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-its.o
> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/irqchip.o
> >> >> +kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-sys-reg-v3.o
> >> >
> >> > Thi is making me wonder:  Are we properly handling GICv3 save/restore
> >> > for AArch32 now that we have GICv3 support for AArch32?  By properly I
> >> > mean that either it is clearly only supported on AArch64 systems or it's
> >> > supported on both AArch64 and AArch32, but it shouldn't break randomly
> >> > on AArch32.
> >>
> >> It supports both AArch64 and AArch64 in handling of system registers
> >> save/restore.
> >> All system registers that we save/restore are 32-bit for both aarch64
> >> and aarch32.
> >> Though opcode op0 should be zero for aarch32, the remaining Op and CRn 
> >> codes
> >> are same. However the codes sent by qemu is matched and register
> >> are handled properly irrespective of AArch32 or AArch64.
> >>
> >> I don't have platform which support AArch32 guests to verify.
> >
> > Actually this is not about the guest, it's about an ARMv8 AArch32 host
> > that has a GICv3.
> >
> > I just tried to do a v7 compile with your patches, and it results in an
> > epic failure, so there's something for you to look at.
> >
> >> >
> >> >>  kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/arch_timer.o
> >> >>  kvm-$(CONFIG_KVM_ARM_PMU) += $(KVM)/arm/pmu.o
> >> >> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> >> >> index 002f092..730a18a 100644
> >> >> --- a/include/kvm/arm_vgic.h
> >> >> +++ b/include/kvm/arm_vgic.h
> >> >> @@ -71,6 +71,9 @@ struct vgic_global {
> >> >>
> >> >>   /* GIC system register CPU interface */
> >> >>   struct static_key_false gicv3_cpuif;
> >> >> +
> >> >> + /* Cache ICH_VTR_EL2 reg value */
> >> >> + u32 ich_vtr_el2;
> >> >>  };
> >> >>
> >> >>  extern struct vgic_global kvm_vgic_global_state;
> >> >> @@ -269,6 +272,12 @@ struct vgic_cpu {
> >> >>   u64 pendbaser;
> >> >>
> >> >>   bool lpis_enabled;
> >> >> +
> >> >> + /* Cache guest priority bits */
> >> >> + u32 num_pri_bits;
> >> >> +
> >> >> + /* Cache guest interrupt ID bits */
> >> >> +

Re: [kvm-unit-tests PATCH v9 3/3] arm: pmu: Add CPI checking

[Andrew Jones]

On Fri, Nov 18, 2016 at 10:15:42PM -0600, Wei Huang wrote:
> From: Christopher Covington 
> 
> Calculate the numbers of cycles per instruction (CPI) implied by ARM
> PMU cycle counter values. The code includes a strict checking facility
> intended for the -icount option in TCG mode in the configuration file.
> 
> Signed-off-by: Christopher Covington 
> Signed-off-by: Wei Huang 
> ---
>  arm/pmu.c | 111 
> +-
>  arm/unittests.cfg |  14 +++
>  2 files changed, 124 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index fa87de4..b36c4fb 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -104,6 +104,25 @@ static inline uint32_t id_dfr0_read(void)
>   asm volatile("mrc p15, 0, %0, c0, c1, 2" : "=r" (val));
>   return val;
>  }
> +
> +/*
> + * Extra instructions inserted by the compiler would be difficult to 
> compensate
> + * for, so hand assemble everything between, and including, the PMCR accesses
> + * to start and stop counting.
> + */
> +static inline void loop(int i, uint32_t pmcr)

Thought you were going to rename this function.

> +{
> + asm volatile(
> + "   mcr p15, 0, %[pmcr], c9, c12, 0\n"
> + "   isb\n"
> + "1: subs%[i], %[i], #1\n"
> + "   bgt 1b\n"
> + "   mcr p15, 0, %[z], c9, c12, 0\n"
> + "   isb\n"
> + : [i] "+r" (i)
> + : [pmcr] "r" (pmcr), [z] "r" (0)
> + : "cc");
> +}
>  #elif defined(__aarch64__)
>  static inline uint32_t pmcr_read(void)
>  {
> @@ -150,6 +169,25 @@ static inline uint32_t id_dfr0_read(void)
>   asm volatile("mrs %0, id_dfr0_el1" : "=r" (id));
>   return id;
>  }
> +
> +/*
> + * Extra instructions inserted by the compiler would be difficult to 
> compensate
> + * for, so hand assemble everything between, and including, the PMCR accesses
> + * to start and stop counting.
> + */
> +static inline void loop(int i, uint32_t pmcr)
> +{
> + asm volatile(
> + "   msr pmcr_el0, %[pmcr]\n"
> + "   isb\n"
> + "1: subs%[i], %[i], #1\n"
> + "   b.gt1b\n"
> + "   msr pmcr_el0, xzr\n"
> + "   isb\n"
> + : [i] "+r" (i)
> + : [pmcr] "r" (pmcr)
> + : "cc");
> +}
>  #endif
>  
>  /*
> @@ -204,6 +242,71 @@ static bool check_cycles_increase(void)
>   return success;
>  }
>  
> +/*
> + * Execute a known number of guest instructions. Only odd instruction counts
> + * greater than or equal to 3 are supported by the in-line assembly code. The
> + * control register (PMCR_EL0) is initialized with the provided value 
> (allowing
> + * for example for the cycle counter or event counters to be reset). At the 
> end
> + * of the exact instruction loop, zero is written to PMCR_EL0 to disable
> + * counting, allowing the cycle counter or event counters to be read at the
> + * leisure of the calling code.
> + */
> +static void measure_instrs(int num, uint32_t pmcr)
> +{
> + int i = (num - 1) / 2;
> +
> + assert(num >= 3 && ((num - 1) % 2 == 0));
> + loop(i, pmcr);
> +}
> +
> +/*
> + * Measure cycle counts for various known instruction counts. Ensure that the
> + * cycle counter progresses (similar to check_cycles_increase() but with more
> + * instructions and using reset and stop controls). If supplied a positive,
> + * nonzero CPI parameter, also strictly check that every measurement matches
> + * it. Strict CPI checking is used to test -icount mode.
> + */
> +static bool check_cpi(int cpi)
> +{
> + uint32_t pmcr = pmcr_read() | PMU_PMCR_LC | PMU_PMCR_C | PMU_PMCR_E;
> + 
> + if (cpi > 0)
> + printf("Checking for CPI=%d.\n", cpi);
> + printf("instrs : cycles0 cycles1 ...\n");
> +
> + for (unsigned int i = 3; i < 300; i += 32) {
> + uint64_t avg, sum = 0;
> +
> + printf("%d :", i);
> + for (int j = 0; j < NR_SAMPLES; j++) {
> + uint64_t cycles;
> +
> + pmccntr_write(0);
> + measure_instrs(i, pmcr);
> + cycles = pmccntr_read();
> + printf(" %"PRId64"", cycles);
> +
> + /*
> +  * The cycles taken by the loop above should fit in
> +  * 32 bits easily. We check the upper 32 bits of the
> +  * cycle counter to make sure there is no supprise.
> +  */
> + if (!cycles || (cpi > 0 && cycles != i * cpi) ||
> + (cycles & 0x)) {

 (cycles >> 32) != 0 would look better.

> + printf("\n");

We have 3 cases where we return false here. How about doing the tests
separately and adding descriptive print statements for each?

 if (!cycles) {
 printf("\ncycles not incrementing!\n");
 return false;
 } else if (cpi > 0 && cycles != i * cpi) {