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

[Baicar, Tyler]

Hello James,


On 2/3/2017 9:00 AM, James Morse wrote:

On 01/02/17 17:16, Tyler Baicar wrote:

ARM APEI extension proposal added SEA (Synchronous 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.

It's worth adding details of the other things this patch changes, just to alert
busy reviewers, something like:
An SEA can interrupt code that had interrupts masked and is treated as an NMI.
To aid this the page of address space for mapping APEI buffers while in_nmi() is
always reserved, and ghes_ioremap_pfn_nmi() is changed to use the helper methods
to find the prot_t to map with in the same way as ghes_ioremap_pfn_irq().

I'll add this in in the next version.

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..f92778d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -53,6 +53,8 @@ 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_NMI if HAVE_ACPI_APEI_SEA

Nit: This section of the file is largely in alphabetical order, can we try to
keep it that way?!

Yes, will do!

select HAVE_ALIGNED_STRUCT_PAGE if SLUB
select HAVE_ARCH_AUDITSYSCALL
select HAVE_ARCH_BITREVERSE
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 9ae7e65..5a5a096 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -30,6 +30,7 @@
  #include 
  #include 
  #include 
+#include 

This header is already included by this file further up.

Oops, guess I missed that :)
  
  #include 

  #include 
@@ -41,6 +42,8 @@
  #include 
  #include 
  
+#include 

+
  static const char *fault_name(unsigned int esr);
  
  #ifdef CONFIG_KPROBES

@@ -500,6 +503,14 @@ static int do_sea(unsigned long addr, unsigned int esr, 
struct pt_regs *regs)
pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
 fault_name(esr), esr, addr);
  
+	/*

+* synchronize_rcu() will wait for nmi_exit(), so no need to
+* rcu_read_lock().
+*/

This comment should go against the use of RCU in ghes_notify_sea(), but there
should be something here to explain the surprise use of nmi. Something like:
Synchronous aborts may interrupt code which had interrupts masked. Before
calling out into the wider kernel tell the interested subsystems.

Sounds good. I'll update the comments.

This should be wrapped in:
if (IS_ENABLED(HAVE_ACPI_APEI_SEA)) {


+   nmi_enter();
+   ghes_notify_sea();
+   nmi_exit();

}

To avoid breaking systems that don't have SEA configured.

Yes, I'll add that check in the next version.

+
info.si_signo = SIGBUS;
info.si_errno = 0;
info.si_code  = 0;
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

depends on CONFIG_ACPI_APEI_GHES
?

(I think this is what the kbuild robot has managed to miss out!)

Will do.

+   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

Nit: notifications from SEA,

Will do.

+ 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

Nit: 'HW'->hardware. This is spelled out for the other seven uses in the file.

Will do.

+ 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 b25e7cf..8756172 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -114,11 +114,7 @@
   * Two virtual pages are used, one for IRQ/PROCESS context, the other for
   * NMI context (optionally).
   */
-#ifdef CONFIG_HAVE_ACPI_APEI_NMI
  #define GHES_IOREMAP_PAGES   2
-#else
-#define GHES_IOREMAP_PAGES   1
-#endif
  #define GHES_IOREMAP_IRQ_PAGE(base)   (base)
  #define GHES_IOREMAP_NMI_PAGE(base)   ((base) + PAGE_SIZE)
  
@@ -156,11 +152,14 @@ static void ghes_ioremap_exit(void)
  
  static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)

  {
-   unsigned long vaddr;
+ 

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

[Baicar, Tyler]

Hello James,


On 2/3/2017 8:59 AM, James Morse wrote:

On 01/02/17 17:16, 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.
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 156169c..9ae7e65 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -487,6 +487,31 @@ static int do_bad(unsigned long addr, unsigned int esr, 
struct pt_regs *regs)
return 1;
  }
  
+#define SEA_FnV_MASK	0x0400

There are a glut of ESR_ELx_ macros in arch/arm64/include/asm/esr.h, could this
be fitted in there in a similar format?


--- 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)

I'll make this change in the next version.

+
+/*
+ * 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 & SEA_FnV_MASK)
+   info.si_addr = 0;
+   else
+   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;
@@ -509,22 +534,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"  },

This will print:

Synchronous External Abort: synchronous external abort

It looks odd, but I can't think of anything better to put there.



{ 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)"   },

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

[Baicar, Tyler]

Hello Steve,


On 2/1/2017 8:15 PM, Steven Rostedt wrote:

On Wed,  1 Feb 2017 10:16:52 -0700
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 
Acked-by: Steven Rostedt 
---
  drivers/acpi/apei/ghes.c|  7 ++-
  drivers/firmware/efi/cper.c |  1 +
  drivers/ras/ras.c   |  1 +
  include/ras/ras_event.h | 34 ++
  4 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c
index a989345..013faf0 100644
--- a/drivers/acpi/apei/ghes.c
+++ b/drivers/acpi/apei/ghes.c
@@ -512,7 +512,12 @@ static void ghes_do_proc(struct ghes *ghes,
  
  		}

  #endif
-   else {
+   else if (!uuid_le_cmp(sec_type, CPER_SEC_PROC_ARM)) {
+   struct cper_sec_proc_arm *arm_err;
+
+   arm_err = acpi_hest_generic_data_payload(gdata);
+   trace_arm_event(arm_err);

According to the kbuild failure, I'm guessing this file requires a:

  #include 
I add that include in patch 8/10 of this series, so ghes.c has the 
include before this patch. The kbuild
complained about the same thing for both trace events added in this 
series. Upon further debug, it
looks like I'll need to verify that CONFIG_RAS is enabled to make these 
trace event calls. It's strange
that kbuild didn't complain in earlier versions of this series because 
these have been this way the

whole time :) I'll add the config check in the next series.

Thanks,
Tyler

-- Steve


+   } 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 48cb8ee..0ec678e 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 @@ 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..b36db48 100644
--- a/include/ras/ras_event.h
+++ b/include/ras/ras_event.h
@@ -162,6 +162,40 @@
  );
  
  /*

+ * 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(
+   __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;
+   ),
+
+   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.

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

Re: [PATCH v2 2/5] KVM: arm/arm64: Add ARM user space interrupt signaling ABI

[Peter Maydell]

On 3 February 2017 at 14:56, Christoffer Dall  wrote:
> From: Christoffer Dall 
>
> We have 2 modes for dealing with interrupts in the ARM world. We can
> either handle them all using hardware acceleration through the vgic or
> we can emulate a gic in user space and only drive CPU IRQ pins from
> there.
>
> Unfortunately, when driving IRQs from user space, we never tell user
> space about events from devices emulated inside the kernel, which may
> result in interrupt line state changes, so we lose out on for example
> timer and PMU events if we run with user space gic emulation.
>
> Define an ABI to publish such device output levels to userspace.
>
> Signed-off-by: Alexander Graf 
> Signed-off-by: Christoffer Dall 


Acked-by: Peter Maydell 

for the userspace ABI/docs part.

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

Re: [RFC v4 00/10] Provide the EL1 physical timer to the VM

[Marc Zyngier]

On 03/02/17 16:51, Jintack Lim wrote:
> On Fri, Feb 3, 2017 at 11:14 AM, Marc Zyngier  wrote:
>> On 03/02/17 15:19, Jintack Lim wrote:
>>> The ARM architecture defines the EL1 physical timer and the virtual timer,
>>> and it is reasonable for an OS to expect to be able to access both.
>>> However, the current KVM implementation does not provide the EL1 physical
>>> timer to VMs but terminates VMs on access to the timer.
>>>
>>> This patch series enables VMs to use the EL1 physical timer through
>>> trap-and-emulate only on arm64. The KVM host emulates each EL1 physical
>>> timer register access and sets up the background timer accordingly.  When
>>> the background timer expires, the KVM host injects EL1 physical timer
>>> interrupts to the VM.  Alternatively, it's also possible to allow VMs to
>>> access the EL1 physical timer without trapping.  However, this requires
>>> somehow using the EL2 physical timer for the Linux host while running the
>>> VM instead of the EL1 physical timer.  Right now I just implemented
>>> trap-and-emulate because this was straightforward to do, and I leave it to
>>> future work to determine if transferring the EL1 physical timer state to
>>> the EL2 timer provides any performance benefit.
>>>
>>> This feature will be useful for any OS that wishes to access the EL1
>>> physical timer. Nested virtualization is one of those use cases. A nested
>>> hypervisor running inside a VM would think it has full access to the
>>> hardware and naturally tries to use the EL1 physical timer as Linux would
>>> do. Other nested hypervisors may try to use the EL2 physical timer as Xen
>>> would do, but supporting the EL2 physical timer to the VM is out of scope
>>> of this patch series. This patch series will make it easy to add the EL2
>>> timer support in the future, though.
>>>
>>> Note that Linux VMs booting in EL1 will be unaffected by this patch series
>>> and will continue to use only the virtual timer and this patch series will
>>> therefore not introduce any performance degredation as a result of
>>> trap-and-emulate.
>>>
>>> v3 => v4:
>>>  - Fix a bug that prevents a VM from booting on 32-bit architecture
>>>  - Clarify that the emulated physical timer is only supported on arm64
>>>in the cover letter
>>
>> Hi Jintack,
>>
>> I've now applied this to queue, and will push it out later today.
> 
> Thanks, Marc.
> 
>>
>> Out of curiosity, is there any reason why this is arm64 only?
> 
> It was simply because I didn't have a convenient 32bit architecture
> develop environment at hand and didn't spend time to set it up myself
> :(
> (As specified in the nesting RFC patch series cover letter, the
> nesting patches are compiled, but not tested on 32-bit architecture
> yet.)
> I guess it's time to set it up.
> 
>> As far as
>> I can tell, we're only missing the cp15 handling (both for arm and in
>> the 32bit handling in arm64).
> 
> I think so, too. I can't promise when, but I'll try to add those once
> I set the develop environment.

That's fine, we can add these later (and maybe I'll just do it, since it
is pretty trivial).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC v4 00/10] Provide the EL1 physical timer to the VM

[Jintack Lim]

On Fri, Feb 3, 2017 at 11:14 AM, Marc Zyngier  wrote:
> On 03/02/17 15:19, Jintack Lim wrote:
>> The ARM architecture defines the EL1 physical timer and the virtual timer,
>> and it is reasonable for an OS to expect to be able to access both.
>> However, the current KVM implementation does not provide the EL1 physical
>> timer to VMs but terminates VMs on access to the timer.
>>
>> This patch series enables VMs to use the EL1 physical timer through
>> trap-and-emulate only on arm64. The KVM host emulates each EL1 physical
>> timer register access and sets up the background timer accordingly.  When
>> the background timer expires, the KVM host injects EL1 physical timer
>> interrupts to the VM.  Alternatively, it's also possible to allow VMs to
>> access the EL1 physical timer without trapping.  However, this requires
>> somehow using the EL2 physical timer for the Linux host while running the
>> VM instead of the EL1 physical timer.  Right now I just implemented
>> trap-and-emulate because this was straightforward to do, and I leave it to
>> future work to determine if transferring the EL1 physical timer state to
>> the EL2 timer provides any performance benefit.
>>
>> This feature will be useful for any OS that wishes to access the EL1
>> physical timer. Nested virtualization is one of those use cases. A nested
>> hypervisor running inside a VM would think it has full access to the
>> hardware and naturally tries to use the EL1 physical timer as Linux would
>> do. Other nested hypervisors may try to use the EL2 physical timer as Xen
>> would do, but supporting the EL2 physical timer to the VM is out of scope
>> of this patch series. This patch series will make it easy to add the EL2
>> timer support in the future, though.
>>
>> Note that Linux VMs booting in EL1 will be unaffected by this patch series
>> and will continue to use only the virtual timer and this patch series will
>> therefore not introduce any performance degredation as a result of
>> trap-and-emulate.
>>
>> v3 => v4:
>>  - Fix a bug that prevents a VM from booting on 32-bit architecture
>>  - Clarify that the emulated physical timer is only supported on arm64
>>in the cover letter
>
> Hi Jintack,
>
> I've now applied this to queue, and will push it out later today.

Thanks, Marc.

>
> Out of curiosity, is there any reason why this is arm64 only?

It was simply because I didn't have a convenient 32bit architecture
develop environment at hand and didn't spend time to set it up myself
:(
(As specified in the nesting RFC patch series cover letter, the
nesting patches are compiled, but not tested on 32-bit architecture
yet.)
I guess it's time to set it up.

> As far as
> I can tell, we're only missing the cp15 handling (both for arm and in
> the 32bit handling in arm64).

I think so, too. I can't promise when, but I'll try to add those once
I set the develop environment.

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

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

Re: [RFC v4 00/10] Provide the EL1 physical timer to the VM

[Marc Zyngier]

On 03/02/17 15:19, Jintack Lim wrote:
> The ARM architecture defines the EL1 physical timer and the virtual timer,
> and it is reasonable for an OS to expect to be able to access both.
> However, the current KVM implementation does not provide the EL1 physical
> timer to VMs but terminates VMs on access to the timer.
> 
> This patch series enables VMs to use the EL1 physical timer through
> trap-and-emulate only on arm64. The KVM host emulates each EL1 physical
> timer register access and sets up the background timer accordingly.  When
> the background timer expires, the KVM host injects EL1 physical timer
> interrupts to the VM.  Alternatively, it's also possible to allow VMs to
> access the EL1 physical timer without trapping.  However, this requires
> somehow using the EL2 physical timer for the Linux host while running the
> VM instead of the EL1 physical timer.  Right now I just implemented
> trap-and-emulate because this was straightforward to do, and I leave it to
> future work to determine if transferring the EL1 physical timer state to
> the EL2 timer provides any performance benefit.
> 
> This feature will be useful for any OS that wishes to access the EL1
> physical timer. Nested virtualization is one of those use cases. A nested
> hypervisor running inside a VM would think it has full access to the
> hardware and naturally tries to use the EL1 physical timer as Linux would
> do. Other nested hypervisors may try to use the EL2 physical timer as Xen
> would do, but supporting the EL2 physical timer to the VM is out of scope
> of this patch series. This patch series will make it easy to add the EL2
> timer support in the future, though.
> 
> Note that Linux VMs booting in EL1 will be unaffected by this patch series
> and will continue to use only the virtual timer and this patch series will
> therefore not introduce any performance degredation as a result of
> trap-and-emulate.
> 
> v3 => v4:
>  - Fix a bug that prevents a VM from booting on 32-bit architecture
>  - Clarify that the emulated physical timer is only supported on arm64
>in the cover letter

Hi Jintack,

I've now applied this to queue, and will push it out later today.

Out of curiosity, is there any reason why this is arm64 only? As far as
I can tell, we're only missing the cp15 handling (both for arm and in
the 32bit handling in arm64).

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[James Morse]

Hi Tyler,

On 01/02/17 17:16, Tyler Baicar wrote:
> ARM APEI extension proposal added SEA (Synchronous 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.

It's worth adding details of the other things this patch changes, just to alert
busy reviewers, something like:
An SEA can interrupt code that had interrupts masked and is treated as an NMI.
To aid this the page of address space for mapping APEI buffers while in_nmi() is
always reserved, and ghes_ioremap_pfn_nmi() is changed to use the helper methods
to find the prot_t to map with in the same way as ghes_ioremap_pfn_irq().


> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1117421..f92778d 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -53,6 +53,8 @@ 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_NMI if HAVE_ACPI_APEI_SEA

Nit: This section of the file is largely in alphabetical order, can we try to
keep it that way?!


>   select HAVE_ALIGNED_STRUCT_PAGE if SLUB
>   select HAVE_ARCH_AUDITSYSCALL
>   select HAVE_ARCH_BITREVERSE
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 9ae7e65..5a5a096 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -30,6 +30,7 @@
>  #include 
>  #include 
>  #include 

> +#include 

This header is already included by this file further up.


>  
>  #include 
>  #include 
> @@ -41,6 +42,8 @@
>  #include 
>  #include 
>  
> +#include 
> +
>  static const char *fault_name(unsigned int esr);
>  
>  #ifdef CONFIG_KPROBES
> @@ -500,6 +503,14 @@ static int do_sea(unsigned long addr, unsigned int esr, 
> struct pt_regs *regs)
>   pr_err("Synchronous External Abort: %s (0x%08x) at 0x%016lx\n",
>fault_name(esr), esr, addr);
>  

> + /*
> +  * synchronize_rcu() will wait for nmi_exit(), so no need to
> +  * rcu_read_lock().
> +  */

This comment should go against the use of RCU in ghes_notify_sea(), but there
should be something here to explain the surprise use of nmi. Something like:
Synchronous aborts may interrupt code which had interrupts masked. Before
calling out into the wider kernel tell the interested subsystems.


This should be wrapped in:
if (IS_ENABLED(HAVE_ACPI_APEI_SEA)) {

> + nmi_enter();
> + ghes_notify_sea();
> + nmi_exit();

}

To avoid breaking systems that don't have SEA configured.


> +
>   info.si_signo = SIGBUS;
>   info.si_errno = 0;
>   info.si_code  = 0;
> 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

depends on CONFIG_ACPI_APEI_GHES
?

(I think this is what the kbuild robot has managed to miss out!)


> + 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

Nit: notifications from SEA,


> +   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

Nit: 'HW'->hardware. This is spelled out for the other seven uses in the file.


> +   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 b25e7cf..8756172 100644
> --- a/drivers/acpi/apei/ghes.c
> +++ b/drivers/acpi/apei/ghes.c
> @@ -114,11 +114,7 @@
>   * Two virtual pages are used, one for IRQ/PROCESS context, the other for
>   * NMI context (optionally).
>   */
> -#ifdef CONFIG_HAVE_ACPI_APEI_NMI
>  #define GHES_IOREMAP_PAGES   2
> -#else
> -#define GHES_IOREMAP_PAGES   1
> -#endif
>  #define GHES_IOREMAP_IRQ_PAGE(base)  (base)
>  #define GHES_IOREMAP_NMI_PAGE(base)  ((base) + PAGE_SIZE)
>  
> @@ -156,11 +152,14 @@ static void ghes_ioremap_exit(void)
>  
>  static void __iomem *ghes_ioremap_pfn_nmi(u64 pfn)
>  {
> - unsigned long vaddr;
> + unsigned long vaddr, paddr;
> + pgprot_t prot;
>  
>   vaddr = (unsigned 

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

[James Morse]

Hi Tyler,

On 01/02/17 17:16, 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.

> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 156169c..9ae7e65 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -487,6 +487,31 @@ static int do_bad(unsigned long addr, unsigned int esr, 
> struct pt_regs *regs)
>   return 1;
>  }
>  
> +#define SEA_FnV_MASK 0x0400

There are a glut of ESR_ELx_ macros in arch/arm64/include/asm/esr.h, could this
be fitted in there in a similar format?


--- 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)


> +
> +/*
> + * 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 & SEA_FnV_MASK)
> + info.si_addr = 0;
> + else
> + 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;
> @@ -509,22 +534,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"},

This will print:
> Synchronous External Abort: synchronous external abort

It looks odd, but I can't think of anything better to put there.


>   { 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 

[RFC v4 09/10] KVM: arm64: Add the EL1 physical timer access handler

[Jintack Lim]

KVM traps on the EL1 phys timer accesses from VMs, but it doesn't handle
those traps. This results in terminating VMs. Instead, set a handler for
the EL1 phys timer access, and inject an undefined exception as an
intermediate step.

Signed-off-by: Jintack Lim 
Reviewed-by: Christoffer Dall 
---
 arch/arm64/kvm/sys_regs.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index caa47ce..1cd3464 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -820,6 +820,30 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct 
sys_reg_params *p,
  CRm((0b1100 | (((n) >> 3) & 0x3))), Op2(((n) & 0x7)), \
  access_pmu_evtyper, reset_unknown, (PMEVTYPER0_EL0 + n), }
 
+static bool access_cntp_tval(struct kvm_vcpu *vcpu,
+   struct sys_reg_params *p,
+   const struct sys_reg_desc *r)
+{
+   kvm_inject_undefined(vcpu);
+   return true;
+}
+
+static bool access_cntp_ctl(struct kvm_vcpu *vcpu,
+   struct sys_reg_params *p,
+   const struct sys_reg_desc *r)
+{
+   kvm_inject_undefined(vcpu);
+   return true;
+}
+
+static bool access_cntp_cval(struct kvm_vcpu *vcpu,
+   struct sys_reg_params *p,
+   const struct sys_reg_desc *r)
+{
+   kvm_inject_undefined(vcpu);
+   return true;
+}
+
 /*
  * Architected system registers.
  * Important: Must be sorted ascending by Op0, Op1, CRn, CRm, Op2
@@ -1029,6 +1053,16 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, 
struct sys_reg_params *p,
{ Op0(0b11), Op1(0b011), CRn(0b1101), CRm(0b), Op2(0b011),
  NULL, reset_unknown, TPIDRRO_EL0 },
 
+   /* CNTP_TVAL_EL0 */
+   { Op0(0b11), Op1(0b011), CRn(0b1110), CRm(0b0010), Op2(0b000),
+ access_cntp_tval },
+   /* CNTP_CTL_EL0 */
+   { Op0(0b11), Op1(0b011), CRn(0b1110), CRm(0b0010), Op2(0b001),
+ access_cntp_ctl },
+   /* CNTP_CVAL_EL0 */
+   { Op0(0b11), Op1(0b011), CRn(0b1110), CRm(0b0010), Op2(0b010),
+ access_cntp_cval },
+
/* PMEVCNTRn_EL0 */
PMU_PMEVCNTR_EL0(0),
PMU_PMEVCNTR_EL0(1),
-- 
1.9.1


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

[RFC v4 06/10] KVM: arm/arm64: Update the physical timer interrupt level

[Jintack Lim]

Now that we maintain the EL1 physical timer register states of VMs,
update the physical timer interrupt level along with the virtual one.

Signed-off-by: Jintack Lim 
Acked-by: Christoffer Dall 
---
 virt/kvm/arm/arch_timer.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index dbd0af1..7f9a664 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -186,6 +186,7 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
 {
struct arch_timer_cpu *timer = >arch.timer_cpu;
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+   struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
/*
 * If userspace modified the timer registers via SET_ONE_REG before
@@ -199,6 +200,9 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
kvm_timer_update_irq(vcpu, !vtimer->irq.level, vtimer);
 
+   if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
+   kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
+
return 0;
 }
 
-- 
1.9.1


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

[RFC v4 03/10] KVM: arm/arm64: Decouple kvm timer functions from virtual timer

[Jintack Lim]

Now that we have a separate structure for timer context, make functions
generic so that they can work with any timer context, not just the
virtual timer context.  This does not change the virtual timer
functionality.

Signed-off-by: Jintack Lim 
Acked-by: Marc Zyngier 
Acked-by: Christoffer Dall 
---
 arch/arm/kvm/arm.c   |  2 +-
 include/kvm/arm_arch_timer.h |  2 +-
 virt/kvm/arm/arch_timer.c| 54 
 3 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index f93f2171..0ecd6cf 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -300,7 +300,7 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 
 int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
 {
-   return kvm_timer_should_fire(vcpu);
+   return kvm_timer_should_fire(vcpu_vtimer(vcpu));
 }
 
 void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu)
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 2c8560b..f46fa3b 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -66,7 +66,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
 int kvm_arm_timer_set_reg(struct kvm_vcpu *, u64 regid, u64 value);
 
-bool kvm_timer_should_fire(struct kvm_vcpu *vcpu);
+bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx);
 void kvm_timer_schedule(struct kvm_vcpu *vcpu);
 void kvm_timer_unschedule(struct kvm_vcpu *vcpu);
 
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 5004a67..5261f98 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -98,13 +98,12 @@ static void kvm_timer_inject_irq_work(struct work_struct 
*work)
kvm_vcpu_kick(vcpu);
 }
 
-static u64 kvm_timer_compute_delta(struct kvm_vcpu *vcpu)
+static u64 kvm_timer_compute_delta(struct arch_timer_context *timer_ctx)
 {
u64 cval, now;
-   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
-   cval = vtimer->cnt_cval;
-   now = kvm_phys_timer_read() - vtimer->cntvoff;
+   cval = timer_ctx->cnt_cval;
+   now = kvm_phys_timer_read() - timer_ctx->cntvoff;
 
if (now < cval) {
u64 ns;
@@ -133,7 +132,7 @@ static enum hrtimer_restart kvm_timer_expire(struct hrtimer 
*hrt)
 * PoV (NTP on the host may have forced it to expire
 * early). If we should have slept longer, restart it.
 */
-   ns = kvm_timer_compute_delta(vcpu);
+   ns = kvm_timer_compute_delta(vcpu_vtimer(vcpu));
if (unlikely(ns)) {
hrtimer_forward_now(hrt, ns_to_ktime(ns));
return HRTIMER_RESTART;
@@ -143,43 +142,39 @@ static enum hrtimer_restart kvm_timer_expire(struct 
hrtimer *hrt)
return HRTIMER_NORESTART;
 }
 
-static bool kvm_timer_irq_can_fire(struct kvm_vcpu *vcpu)
+static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx)
 {
-   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
-
-   return !(vtimer->cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
-   (vtimer->cnt_ctl & ARCH_TIMER_CTRL_ENABLE);
+   return !(timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
+   (timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_ENABLE);
 }
 
-bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
+bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx)
 {
-   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
u64 cval, now;
 
-   if (!kvm_timer_irq_can_fire(vcpu))
+   if (!kvm_timer_irq_can_fire(timer_ctx))
return false;
 
-   cval = vtimer->cnt_cval;
-   now = kvm_phys_timer_read() - vtimer->cntvoff;
+   cval = timer_ctx->cnt_cval;
+   now = kvm_phys_timer_read() - timer_ctx->cntvoff;
 
return cval <= now;
 }
 
-static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
+static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level,
+struct arch_timer_context *timer_ctx)
 {
int ret;
-   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
BUG_ON(!vgic_initialized(vcpu->kvm));
 
-   vtimer->active_cleared_last = false;
-   vtimer->irq.level = new_level;
-   trace_kvm_timer_update_irq(vcpu->vcpu_id, vtimer->irq.irq,
-  vtimer->irq.level);
+   timer_ctx->active_cleared_last = false;
+   timer_ctx->irq.level = new_level;
+   trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
+  timer_ctx->irq.level);
 
-   ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
-vtimer->irq.irq,
-vtimer->irq.level);
+   ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id, timer_ctx->irq.irq,
+ 

[RFC v4 08/10] KVM: arm/arm64: Set up a background timer for the physical timer emulation

[Jintack Lim]

Set a background timer for the EL1 physical timer emulation while VMs
are running, so that VMs get the physical timer interrupts in a timely
manner.

Schedule the background timer on entry to the VM and cancel it on exit.
This would not have any performance impact to the guest OSes that
currently use the virtual timer since the physical timer is always not
enabled.

Signed-off-by: Jintack Lim 
Reviewed-by: Christoffer Dall 
---
 virt/kvm/arm/arch_timer.c | 25 -
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 0ea7452..33257b5 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -229,6 +229,22 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
return 0;
 }
 
+/* Schedule the background timer for the emulated timer. */
+static void kvm_timer_emulate(struct kvm_vcpu *vcpu,
+ struct arch_timer_context *timer_ctx)
+{
+   struct arch_timer_cpu *timer = >arch.timer_cpu;
+
+   if (kvm_timer_should_fire(timer_ctx))
+   return;
+
+   if (!kvm_timer_irq_can_fire(timer_ctx))
+   return;
+
+   /*  The timer has not yet expired, schedule a background timer */
+   timer_arm(timer, kvm_timer_compute_delta(timer_ctx));
+}
+
 /*
  * Schedule the background timer before calling kvm_vcpu_block, so that this
  * thread is removed from its waitqueue and made runnable when there's a timer
@@ -286,6 +302,9 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
if (kvm_timer_update_state(vcpu))
return;
 
+   /* Set the background timer for the physical timer emulation. */
+   kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
+
/*
* If we enter the guest with the virtual input level to the VGIC
* asserted, then we have already told the VGIC what we need to, and
@@ -348,7 +367,11 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
 {
struct arch_timer_cpu *timer = >arch.timer_cpu;
 
-   BUG_ON(timer_is_armed(timer));
+   /*
+* This is to cancel the background timer for the physical timer
+* emulation if it is set.
+*/
+   timer_disarm(timer);
 
/*
 * The guest could have modified the timer registers or the timer
-- 
1.9.1


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

[RFC v4 05/10] KVM: arm/arm64: Initialize the emulated EL1 physical timer

[Jintack Lim]

Initialize the emulated EL1 physical timer with the default irq number.

Signed-off-by: Jintack Lim 
Reviewed-by: Christoffer Dall 
---
 arch/arm/kvm/reset.c | 9 -
 arch/arm64/kvm/reset.c   | 9 -
 include/kvm/arm_arch_timer.h | 3 ++-
 virt/kvm/arm/arch_timer.c| 9 +++--
 4 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kvm/reset.c b/arch/arm/kvm/reset.c
index 4b5e802..1da8b2d 100644
--- a/arch/arm/kvm/reset.c
+++ b/arch/arm/kvm/reset.c
@@ -37,6 +37,11 @@
.usr_regs.ARM_cpsr = SVC_MODE | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT,
 };
 
+static const struct kvm_irq_level cortexa_ptimer_irq = {
+   { .irq = 30 },
+   .level = 1,
+};
+
 static const struct kvm_irq_level cortexa_vtimer_irq = {
{ .irq = 27 },
.level = 1,
@@ -58,6 +63,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
struct kvm_regs *reset_regs;
const struct kvm_irq_level *cpu_vtimer_irq;
+   const struct kvm_irq_level *cpu_ptimer_irq;
 
switch (vcpu->arch.target) {
case KVM_ARM_TARGET_CORTEX_A7:
@@ -65,6 +71,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
reset_regs = _regs_reset;
vcpu->arch.midr = read_cpuid_id();
cpu_vtimer_irq = _vtimer_irq;
+   cpu_ptimer_irq = _ptimer_irq;
break;
default:
return -ENODEV;
@@ -77,5 +84,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
kvm_reset_coprocs(vcpu);
 
/* Reset arch_timer context */
-   return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
+   return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
 }
diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
index e95d4f6..d9e9697 100644
--- a/arch/arm64/kvm/reset.c
+++ b/arch/arm64/kvm/reset.c
@@ -46,6 +46,11 @@
COMPAT_PSR_I_BIT | COMPAT_PSR_F_BIT),
 };
 
+static const struct kvm_irq_level default_ptimer_irq = {
+   .irq= 30,
+   .level  = 1,
+};
+
 static const struct kvm_irq_level default_vtimer_irq = {
.irq= 27,
.level  = 1,
@@ -104,6 +109,7 @@ int kvm_arch_dev_ioctl_check_extension(struct kvm *kvm, 
long ext)
 int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
 {
const struct kvm_irq_level *cpu_vtimer_irq;
+   const struct kvm_irq_level *cpu_ptimer_irq;
const struct kvm_regs *cpu_reset;
 
switch (vcpu->arch.target) {
@@ -117,6 +123,7 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
}
 
cpu_vtimer_irq = _vtimer_irq;
+   cpu_ptimer_irq = _ptimer_irq;
break;
}
 
@@ -130,5 +137,5 @@ int kvm_reset_vcpu(struct kvm_vcpu *vcpu)
kvm_pmu_vcpu_reset(vcpu);
 
/* Reset timer */
-   return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq);
+   return kvm_timer_vcpu_reset(vcpu, cpu_vtimer_irq, cpu_ptimer_irq);
 }
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 6445a3d..f1d2fba0 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -58,7 +58,8 @@ struct arch_timer_cpu {
 int kvm_timer_hyp_init(void);
 int kvm_timer_enable(struct kvm_vcpu *vcpu);
 int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
-const struct kvm_irq_level *irq);
+const struct kvm_irq_level *virt_irq,
+const struct kvm_irq_level *phys_irq);
 void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
 void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 5261f98..dbd0af1 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -327,9 +327,11 @@ void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu)
 }
 
 int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
-const struct kvm_irq_level *irq)
+const struct kvm_irq_level *virt_irq,
+const struct kvm_irq_level *phys_irq)
 {
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+   struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
/*
 * The vcpu timer irq number cannot be determined in
@@ -337,7 +339,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 * kvm_vcpu_set_target(). To handle this, we determine
 * vcpu timer irq number when the vcpu is reset.
 */
-   vtimer->irq.irq = irq->irq;
+   vtimer->irq.irq = virt_irq->irq;
+   ptimer->irq.irq = phys_irq->irq;
 
/*
 * The bits in CNTV_CTL are architecturally reset to UNKNOWN for ARMv8
@@ -346,6 +349,7 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 * the ARMv7 architecture.
 */
vtimer->cnt_ctl = 0;
+   ptimer->cnt_ctl = 0;
kvm_timer_update_state(vcpu);
 
return 0;
@@ -376,6 +380,7 @@ void 

[RFC v4 07/10] KVM: arm/arm64: Set a background timer to the earliest timer expiration

[Jintack Lim]

When scheduling a background timer, consider both of the virtual and
physical timer and pick the earliest expiration time.

Signed-off-by: Jintack Lim 
Reviewed-by: Christoffer Dall 
---
 arch/arm/kvm/arm.c|  3 ++-
 virt/kvm/arm/arch_timer.c | 53 +++
 2 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 0ecd6cf..21c493a 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -300,7 +300,8 @@ void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
 
 int kvm_cpu_has_pending_timer(struct kvm_vcpu *vcpu)
 {
-   return kvm_timer_should_fire(vcpu_vtimer(vcpu));
+   return kvm_timer_should_fire(vcpu_vtimer(vcpu)) ||
+  kvm_timer_should_fire(vcpu_ptimer(vcpu));
 }
 
 void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu)
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 7f9a664..0ea7452 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -118,6 +118,35 @@ static u64 kvm_timer_compute_delta(struct 
arch_timer_context *timer_ctx)
return 0;
 }
 
+static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx)
+{
+   return !(timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
+   (timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_ENABLE);
+}
+
+/*
+ * Returns the earliest expiration time in ns among guest timers.
+ * Note that it will return 0 if none of timers can fire.
+ */
+static u64 kvm_timer_earliest_exp(struct kvm_vcpu *vcpu)
+{
+   u64 min_virt = ULLONG_MAX, min_phys = ULLONG_MAX;
+   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+   struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+
+   if (kvm_timer_irq_can_fire(vtimer))
+   min_virt = kvm_timer_compute_delta(vtimer);
+
+   if (kvm_timer_irq_can_fire(ptimer))
+   min_phys = kvm_timer_compute_delta(ptimer);
+
+   /* If none of timers can fire, then return 0 */
+   if ((min_virt == ULLONG_MAX) && (min_phys == ULLONG_MAX))
+   return 0;
+
+   return min(min_virt, min_phys);
+}
+
 static enum hrtimer_restart kvm_timer_expire(struct hrtimer *hrt)
 {
struct arch_timer_cpu *timer;
@@ -132,7 +161,7 @@ static enum hrtimer_restart kvm_timer_expire(struct hrtimer 
*hrt)
 * PoV (NTP on the host may have forced it to expire
 * early). If we should have slept longer, restart it.
 */
-   ns = kvm_timer_compute_delta(vcpu_vtimer(vcpu));
+   ns = kvm_timer_earliest_exp(vcpu);
if (unlikely(ns)) {
hrtimer_forward_now(hrt, ns_to_ktime(ns));
return HRTIMER_RESTART;
@@ -142,12 +171,6 @@ static enum hrtimer_restart kvm_timer_expire(struct 
hrtimer *hrt)
return HRTIMER_NORESTART;
 }
 
-static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx)
-{
-   return !(timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
-   (timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_ENABLE);
-}
-
 bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx)
 {
u64 cval, now;
@@ -215,26 +238,30 @@ void kvm_timer_schedule(struct kvm_vcpu *vcpu)
 {
struct arch_timer_cpu *timer = >arch.timer_cpu;
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+   struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
 
BUG_ON(timer_is_armed(timer));
 
/*
-* No need to schedule a background timer if the guest timer has
+* No need to schedule a background timer if any guest timer has
 * already expired, because kvm_vcpu_block will return before putting
 * the thread to sleep.
 */
-   if (kvm_timer_should_fire(vtimer))
+   if (kvm_timer_should_fire(vtimer) || kvm_timer_should_fire(ptimer))
return;
 
/*
-* If the timer is not capable of raising interrupts (disabled or
+* If both timers are not capable of raising interrupts (disabled or
 * masked), then there's no more work for us to do.
 */
-   if (!kvm_timer_irq_can_fire(vtimer))
+   if (!kvm_timer_irq_can_fire(vtimer) && !kvm_timer_irq_can_fire(ptimer))
return;
 
-   /*  The timer has not yet expired, schedule a background timer */
-   timer_arm(timer, kvm_timer_compute_delta(vtimer));
+   /*
+* The guest timers have not yet expired, schedule a background timer.
+* Set the earliest expiration time among the guest timers.
+*/
+   timer_arm(timer, kvm_timer_earliest_exp(vcpu));
 }
 
 void kvm_timer_unschedule(struct kvm_vcpu *vcpu)
-- 
1.9.1


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

[RFC v4 01/10] KVM: arm/arm64: Abstract virtual timer context into separate structure

[Jintack Lim]

Abstract virtual timer context into a separate structure and change all
callers referring to timer registers, irq state and so on. No change in
functionality.

This is about to become very handy when adding the EL1 physical timer.

Signed-off-by: Jintack Lim 
Acked-by: Christoffer Dall 
Acked-by: Marc Zyngier 
---
 include/kvm/arm_arch_timer.h | 27 -
 virt/kvm/arm/arch_timer.c| 69 +++-
 virt/kvm/arm/hyp/timer-sr.c  | 10 ---
 3 files changed, 56 insertions(+), 50 deletions(-)

diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index 5c970ce..daad3c1 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -28,15 +28,20 @@ struct arch_timer_kvm {
u64 cntvoff;
 };
 
-struct arch_timer_cpu {
+struct arch_timer_context {
/* Registers: control register, timer value */
-   u32 cntv_ctl;   /* Saved/restored */
-   u64 cntv_cval;  /* Saved/restored */
+   u32 cnt_ctl;
+   u64 cnt_cval;
+
+   /* Timer IRQ */
+   struct kvm_irq_levelirq;
+
+   /* Active IRQ state caching */
+   boolactive_cleared_last;
+};
 
-   /*
-* Anything that is not used directly from assembly code goes
-* here.
-*/
+struct arch_timer_cpu {
+   struct arch_timer_context   vtimer;
 
/* Background timer used when the guest is not running */
struct hrtimer  timer;
@@ -47,12 +52,6 @@ struct arch_timer_cpu {
/* Background timer active */
boolarmed;
 
-   /* Timer IRQ */
-   struct kvm_irq_levelirq;
-
-   /* Active IRQ state caching */
-   boolactive_cleared_last;
-
/* Is the timer enabled */
boolenabled;
 };
@@ -77,4 +76,6 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu);
 
 void kvm_timer_init_vhe(void);
+
+#define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
 #endif
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 91ecf48..d3556b3 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -37,7 +37,7 @@
 
 void kvm_timer_vcpu_put(struct kvm_vcpu *vcpu)
 {
-   vcpu->arch.timer_cpu.active_cleared_last = false;
+   vcpu_vtimer(vcpu)->active_cleared_last = false;
 }
 
 static u64 kvm_phys_timer_read(void)
@@ -102,7 +102,7 @@ static u64 kvm_timer_compute_delta(struct kvm_vcpu *vcpu)
 {
u64 cval, now;
 
-   cval = vcpu->arch.timer_cpu.cntv_cval;
+   cval = vcpu_vtimer(vcpu)->cnt_cval;
now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
 
if (now < cval) {
@@ -144,21 +144,21 @@ static enum hrtimer_restart kvm_timer_expire(struct 
hrtimer *hrt)
 
 static bool kvm_timer_irq_can_fire(struct kvm_vcpu *vcpu)
 {
-   struct arch_timer_cpu *timer = >arch.timer_cpu;
+   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
-   return !(timer->cntv_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
-   (timer->cntv_ctl & ARCH_TIMER_CTRL_ENABLE);
+   return !(vtimer->cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) &&
+   (vtimer->cnt_ctl & ARCH_TIMER_CTRL_ENABLE);
 }
 
 bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
 {
-   struct arch_timer_cpu *timer = >arch.timer_cpu;
+   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
u64 cval, now;
 
if (!kvm_timer_irq_can_fire(vcpu))
return false;
 
-   cval = timer->cntv_cval;
+   cval = vtimer->cnt_cval;
now = kvm_phys_timer_read() - vcpu->kvm->arch.timer.cntvoff;
 
return cval <= now;
@@ -167,18 +167,18 @@ bool kvm_timer_should_fire(struct kvm_vcpu *vcpu)
 static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level)
 {
int ret;
-   struct arch_timer_cpu *timer = >arch.timer_cpu;
+   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
 
BUG_ON(!vgic_initialized(vcpu->kvm));
 
-   timer->active_cleared_last = false;
-   timer->irq.level = new_level;
-   trace_kvm_timer_update_irq(vcpu->vcpu_id, timer->irq.irq,
-  timer->irq.level);
+   vtimer->active_cleared_last = false;
+   vtimer->irq.level = new_level;
+   trace_kvm_timer_update_irq(vcpu->vcpu_id, vtimer->irq.irq,
+  vtimer->irq.level);
 
ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
-timer->irq.irq,
-timer->irq.level);
+vtimer->irq.irq,
+

[RFC v4 04/10] KVM: arm/arm64: Add the EL1 physical timer context

[Jintack Lim]

Add the EL1 physical timer context.

Signed-off-by: Jintack Lim 
Acked-by: Christoffer Dall 
---
 include/kvm/arm_arch_timer.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index f46fa3b..6445a3d 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -40,6 +40,7 @@ struct arch_timer_context {
 
 struct arch_timer_cpu {
struct arch_timer_context   vtimer;
+   struct arch_timer_context   ptimer;
 
/* Background timer used when the guest is not running */
struct hrtimer  timer;
@@ -75,4 +76,5 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 void kvm_timer_init_vhe(void);
 
 #define vcpu_vtimer(v) (&(v)->arch.timer_cpu.vtimer)
+#define vcpu_ptimer(v) (&(v)->arch.timer_cpu.ptimer)
 #endif
-- 
1.9.1


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

[RFC v4 00/10] Provide the EL1 physical timer to the VM

[Jintack Lim]

The ARM architecture defines the EL1 physical timer and the virtual timer,
and it is reasonable for an OS to expect to be able to access both.
However, the current KVM implementation does not provide the EL1 physical
timer to VMs but terminates VMs on access to the timer.

This patch series enables VMs to use the EL1 physical timer through
trap-and-emulate only on arm64. The KVM host emulates each EL1 physical
timer register access and sets up the background timer accordingly.  When
the background timer expires, the KVM host injects EL1 physical timer
interrupts to the VM.  Alternatively, it's also possible to allow VMs to
access the EL1 physical timer without trapping.  However, this requires
somehow using the EL2 physical timer for the Linux host while running the
VM instead of the EL1 physical timer.  Right now I just implemented
trap-and-emulate because this was straightforward to do, and I leave it to
future work to determine if transferring the EL1 physical timer state to
the EL2 timer provides any performance benefit.

This feature will be useful for any OS that wishes to access the EL1
physical timer. Nested virtualization is one of those use cases. A nested
hypervisor running inside a VM would think it has full access to the
hardware and naturally tries to use the EL1 physical timer as Linux would
do. Other nested hypervisors may try to use the EL2 physical timer as Xen
would do, but supporting the EL2 physical timer to the VM is out of scope
of this patch series. This patch series will make it easy to add the EL2
timer support in the future, though.

Note that Linux VMs booting in EL1 will be unaffected by this patch series
and will continue to use only the virtual timer and this patch series will
therefore not introduce any performance degredation as a result of
trap-and-emulate.

v3 => v4:
 - Fix a bug that prevents a VM from booting on 32-bit architecture
 - Clarify that the emulated physical timer is only supported on arm64
   in the cover letter

v2 => v3:
 - Rebase on kvmarm/queue
 - Take kvm->lock to synchronize cntvoff across all vtimers
 - Remove unnecessary function parameters
 - Add comments

v1 => v2:
 - Rebase on kvm-arm-for-4.10-rc4
 - To make it simple, schedule the background timer for the EL1 physical timer
   emulation on every entry to the VM and cancel it on exit.
 - Change timer_context structure to have cntvoff and restore enable field back
   to arch_timer_cpu structure
Jintack Lim (10):
  KVM: arm/arm64: Abstract virtual timer context into separate structure
  KVM: arm/arm64: Move cntvoff to each timer context
  KVM: arm/arm64: Decouple kvm timer functions from virtual timer
  KVM: arm/arm64: Add the EL1 physical timer context
  KVM: arm/arm64: Initialize the emulated EL1 physical timer
  KVM: arm/arm64: Update the physical timer interrupt level
  KVM: arm/arm64: Set a background timer to the earliest timer
expiration
  KVM: arm/arm64: Set up a background timer for the physical timer
emulation
  KVM: arm64: Add the EL1 physical timer access handler
  KVM: arm/arm64: Emulate the EL1 phys timer registers

 arch/arm/include/asm/kvm_host.h   |   3 -
 arch/arm/kvm/arm.c|   4 +-
 arch/arm/kvm/reset.c  |   9 +-
 arch/arm64/include/asm/kvm_host.h |   3 -
 arch/arm64/kvm/reset.c|   9 +-
 arch/arm64/kvm/sys_regs.c |  65 +
 include/kvm/arm_arch_timer.h  |  39 
 virt/kvm/arm/arch_timer.c | 200 ++
 virt/kvm/arm/hyp/timer-sr.c   |  13 +--
 9 files changed, 249 insertions(+), 96 deletions(-)

-- 
1.9.1


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

[PATCH v2 1/5] KVM: arm/arm64: Cleanup the arch timer code's irqchip checking

[Christoffer Dall]

From: Christoffer Dall 

Currently we check if we have an in-kernel irqchip and if the vgic was
properly implemented several places in the arch timer code.  But, we
already predicate our enablement of the arm timers on having a valid
and initialized gic, so we can simply check if the timers are enabled or
not.

This also gets rid of the ugly "error that's not an error but used to
signal that the timer shouldn't poke the gic" construct we have.

Signed-off-by: Christoffer Dall 
---
 virt/kvm/arm/arch_timer.c | 14 ++
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 35d7100..5099b30 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -189,8 +189,6 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, 
bool new_level,
 {
int ret;
 
-   BUG_ON(!vgic_initialized(vcpu->kvm));
-
timer_ctx->active_cleared_last = false;
timer_ctx->irq.level = new_level;
trace_kvm_timer_update_irq(vcpu->vcpu_id, timer_ctx->irq.irq,
@@ -205,7 +203,7 @@ static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, 
bool new_level,
  * Check if there was a change in the timer state (should we raise or lower
  * the line level to the GIC).
  */
-static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
+static void kvm_timer_update_state(struct kvm_vcpu *vcpu)
 {
struct arch_timer_cpu *timer = >arch.timer_cpu;
struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
@@ -217,16 +215,14 @@ static int kvm_timer_update_state(struct kvm_vcpu *vcpu)
 * because the guest would never see the interrupt.  Instead wait
 * until we call this function from kvm_timer_flush_hwstate.
 */
-   if (!vgic_initialized(vcpu->kvm) || !timer->enabled)
-   return -ENODEV;
+   if (!timer->enabled)
+   return;
 
if (kvm_timer_should_fire(vtimer) != vtimer->irq.level)
kvm_timer_update_irq(vcpu, !vtimer->irq.level, vtimer);
 
if (kvm_timer_should_fire(ptimer) != ptimer->irq.level)
kvm_timer_update_irq(vcpu, !ptimer->irq.level, ptimer);
-
-   return 0;
 }
 
 /* Schedule the background timer for the emulated timer. */
@@ -299,9 +295,11 @@ void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu)
bool phys_active;
int ret;
 
-   if (kvm_timer_update_state(vcpu))
+   if (unlikely(!timer->enabled))
return;
 
+   kvm_timer_update_state(vcpu);
+
/* Set the background timer for the physical timer emulation. */
kvm_timer_emulate(vcpu, vcpu_ptimer(vcpu));
 
-- 
2.9.0

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

[PATCH v2 0/5] Support userspace irqchip with arch timers

[Christoffer Dall]

This series is the second version of the rework of the patches to support
architected timers with a userspace irqchip sent by Alexander Graf [1].

We first cleanup some of the timer code to make it easier to understand
what is being done in the later patches, and then define the ABI,
implement timers support, implement PMU support, and finally advertise
the features.

These patches are based on the recent work from Jintack to support the
physical timer in addition to the virtual timer.  This series including
its dependencies can be found here:

git://git.kernel.org/pub/scm/linux/kernel/git/cdall/linux.git irqs-to-user-v2

I tested this using Alex's QEMU patch with his suggestion to fix the SMP
breakage on top.  While this works for UP guests, SMP guests seem to
still fail.  It would be good if someone could help me have a look at
that since most likely it's a QEMU issue as we established last time.  I
pushed a temporary branch for this stuff:

https://git.linaro.org/people/christoffer.dall/qemu-arm.git no-kvm-irqchip

Changes since v1:
 - Rework the ABI to support devices in general as opposed to just
   timers
 - Support the PMU in addition to timers
 - Also support the physical timer (rebased on Jintack's work)
 - Updated some comments where I noticed things were out of date.

Several changes have been made compared to v7 of the original single
patch, including:
 - Rewording ABI documentation to be more in line with the ARM
   architecture
 - Add an explicit check for needing to notify userspace of a level
   change instead of propagating the value
 - Changes to commenting throughout to more accurately describe the
   architecture concepts we try to maintain
 - Reword of functions, for example from sync to update when the date
   only flows one direction


[1]: https://lists.cs.columbia.edu/pipermail/kvmarm/2016-September/021867.html
[2]: git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next

Christoffer Dall (5):
  KVM: arm/arm64: Cleanup the arch timer code's irqchip checking
  KVM: arm/arm64: Add ARM user space interrupt signaling ABI
  KVM: arm/arm64: Support arch timers with a userspace gic
  KVM: arm/arm64: Report PMU overflow interrupts to userspace irqchip
  KVM: arm/arm64: Advertise support for KVM_CAP_ARM_USER_IRQ

 Documentation/virtual/kvm/api.txt |  41 
 arch/arm/include/uapi/asm/kvm.h   |   2 +
 arch/arm/kvm/arm.c|  28 ++---
 arch/arm64/include/uapi/asm/kvm.h |   2 +
 include/kvm/arm_arch_timer.h  |   2 +
 include/kvm/arm_pmu.h |   7 +++
 include/uapi/linux/kvm.h  |   8 +++
 virt/kvm/arm/arch_timer.c | 127 ++
 virt/kvm/arm/pmu.c|  42 +++--
 9 files changed, 220 insertions(+), 39 deletions(-)

-- 
2.9.0

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

[PATCH v2 3/5] KVM: arm/arm64: Support arch timers with a userspace gic

[Christoffer Dall]

If you're running with a userspace gic or other interrupt constroller
(that is no vgic in the kernel), then you have so far not been able to
use the architected timers, because the output of the architected
timers, which are driven inside the kernel, was a kernel-only construct
between the arch timer code and the vgic.

This patch implements the new KVM_CAP_ARM_USER_IRQ feature, where we use a
side channel on the kvm_run structure, run->s.regs.device_irq_level, to
always notify userspace of the timer output levels when using a userspace
irqchip.

This works by ensureing that before we enter the guest, if the timer
output level has changed compared to what we last told userspace, we
don't enter the guest, but instead return to userspace to notify it of
the new level.  If we are exiting, because of an MMIO for example, and
the level changed at the same time, the value is also updated and
userspace can sample the line as it needs.  This is nicely achieved
simply always updating the timer_irq_level field after the main run
loop.

Note that the kvm_timer_update_irq trace event is changed to show the
host IRQ number for the timer instead of the guest IRQ number, because
the kernel no longer know which IRQ userspace wires up the timer signal
to.

Also note that this patch implements all required functionality but does
not yet advertise the capability.

Signed-off-by: Alexander Graf 
Signed-off-by: Christoffer Dall 
---
 arch/arm/kvm/arm.c   |  18 +++
 include/kvm/arm_arch_timer.h |   2 +
 virt/kvm/arm/arch_timer.c| 121 +++
 3 files changed, 110 insertions(+), 31 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 21c493a..505f928 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -513,13 +513,7 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu)
return ret;
}
 
-   /*
-* Enable the arch timers only if we have an in-kernel VGIC
-* and it has been properly initialized, since we cannot handle
-* interrupts from the virtual timer with a userspace gic.
-*/
-   if (irqchip_in_kernel(kvm) && vgic_initialized(kvm))
-   ret = kvm_timer_enable(vcpu);
+   ret = kvm_timer_enable(vcpu);
 
return ret;
 }
@@ -633,9 +627,12 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
kvm_run *run)
local_irq_disable();
 
/*
-* Re-check atomic conditions
+* If we have a singal pending, or need to notify a userspace
+* irqchip about timer level changes, then we exit (and update
+* the timer level state in kvm_timer_update_run below).
 */
-   if (signal_pending(current)) {
+   if (signal_pending(current) ||
+   kvm_timer_should_notify_user(vcpu)) {
ret = -EINTR;
run->exit_reason = KVM_EXIT_INTR;
}
@@ -707,6 +704,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
kvm_run *run)
ret = handle_exit(vcpu, run, ret);
}
 
+   /* Tell userspace about in-kernel device output levels */
+   kvm_timer_update_run(vcpu);
+
if (vcpu->sigset_active)
sigprocmask(SIG_SETMASK, , NULL);
return ret;
diff --git a/include/kvm/arm_arch_timer.h b/include/kvm/arm_arch_timer.h
index fe797d6..295584f 100644
--- a/include/kvm/arm_arch_timer.h
+++ b/include/kvm/arm_arch_timer.h
@@ -63,6 +63,8 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu);
 void kvm_timer_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_timer_sync_hwstate(struct kvm_vcpu *vcpu);
+bool kvm_timer_should_notify_user(struct kvm_vcpu *vcpu);
+void kvm_timer_update_run(struct kvm_vcpu *vcpu);
 void kvm_timer_vcpu_terminate(struct kvm_vcpu *vcpu);
 
 u64 kvm_arm_timer_get_reg(struct kvm_vcpu *, u64 regid);
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 5099b30..5dc2167 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -184,6 +184,27 @@ bool kvm_timer_should_fire(struct arch_timer_context 
*timer_ctx)
return cval <= now;
 }
 
+/*
+ * Reflect the timer output level into the kvm_run structure
+ */
+void kvm_timer_update_run(struct kvm_vcpu *vcpu)
+{
+   struct arch_timer_context *vtimer = vcpu_vtimer(vcpu);
+   struct arch_timer_context *ptimer = vcpu_ptimer(vcpu);
+   struct kvm_sync_regs *regs = >run->s.regs;
+
+   if (likely(irqchip_in_kernel(vcpu->kvm)))
+   return;
+
+   /* Populate the device bitmap with the timer states */
+   regs->device_irq_level &= ~(KVM_ARM_DEV_EL1_VTIMER |
+   KVM_ARM_DEV_EL1_PTIMER);
+   if (vtimer->irq.level)
+   regs->device_irq_level |= KVM_ARM_DEV_EL1_VTIMER;
+  

[PATCH v2 4/5] KVM: arm/arm64: Report PMU overflow interrupts to userspace irqchip

[Christoffer Dall]

From: Christoffer Dall 

When not using an in-kernel VGIC, but instead emulating an interrupt
controller in userspace, we should report the PMU overflow status to
that userspace interrupt controller using the KVM_CAP_ARM_USER_IRQ
feature.

Signed-off-by: Christoffer Dall 
---
 arch/arm/kvm/arm.c|  9 ++---
 include/kvm/arm_pmu.h |  7 +++
 virt/kvm/arm/pmu.c| 42 ++
 3 files changed, 51 insertions(+), 7 deletions(-)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 505f928..92f38f6 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -628,11 +628,13 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
kvm_run *run)
 
/*
 * If we have a singal pending, or need to notify a userspace
-* irqchip about timer level changes, then we exit (and update
-* the timer level state in kvm_timer_update_run below).
+* irqchip about timer or PMU level changes, then we exit (and
+* update the timer level state in kvm_timer_update_run
+* below).
 */
if (signal_pending(current) ||
-   kvm_timer_should_notify_user(vcpu)) {
+   kvm_timer_should_notify_user(vcpu) ||
+   kvm_pmu_should_notify_user(vcpu)) {
ret = -EINTR;
run->exit_reason = KVM_EXIT_INTR;
}
@@ -706,6 +708,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct 
kvm_run *run)
 
/* Tell userspace about in-kernel device output levels */
kvm_timer_update_run(vcpu);
+   kvm_pmu_update_run(vcpu);
 
if (vcpu->sigset_active)
sigprocmask(SIG_SETMASK, , NULL);
diff --git a/include/kvm/arm_pmu.h b/include/kvm/arm_pmu.h
index 92e7e97..1ab4633 100644
--- a/include/kvm/arm_pmu.h
+++ b/include/kvm/arm_pmu.h
@@ -50,6 +50,8 @@ void kvm_pmu_enable_counter(struct kvm_vcpu *vcpu, u64 val);
 void kvm_pmu_overflow_set(struct kvm_vcpu *vcpu, u64 val);
 void kvm_pmu_flush_hwstate(struct kvm_vcpu *vcpu);
 void kvm_pmu_sync_hwstate(struct kvm_vcpu *vcpu);
+bool kvm_pmu_should_notify_user(struct kvm_vcpu *vcpu);
+void kvm_pmu_update_run(struct kvm_vcpu *vcpu);
 void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val);
 void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val);
 void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, u64 data,
@@ -85,6 +87,11 @@ static inline void kvm_pmu_enable_counter(struct kvm_vcpu 
*vcpu, u64 val) {}
 static inline void kvm_pmu_overflow_set(struct kvm_vcpu *vcpu, u64 val) {}
 static inline void kvm_pmu_flush_hwstate(struct kvm_vcpu *vcpu) {}
 static inline void kvm_pmu_sync_hwstate(struct kvm_vcpu *vcpu) {}
+static inline bool kvm_pmu_should_notify_user(struct kvm_vcpu *vcpu)
+{
+   return false;
+}
+static inline void kvm_pmu_update_run(struct kvm_vcpu *vcpu) {}
 static inline void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 val) 
{}
 static inline void kvm_pmu_handle_pmcr(struct kvm_vcpu *vcpu, u64 val) {}
 static inline void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu,
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 69ccce3..51218be 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -230,13 +230,47 @@ static void kvm_pmu_update_state(struct kvm_vcpu *vcpu)
return;
 
overflow = !!kvm_pmu_overflow_status(vcpu);
-   if (pmu->irq_level != overflow) {
-   pmu->irq_level = overflow;
-   kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
-   pmu->irq_num, overflow);
+   if (pmu->irq_level == overflow)
+   return;
+
+   pmu->irq_level = overflow;
+
+   if (likely(irqchip_in_kernel(vcpu->kvm))) {
+   int ret;
+   ret = kvm_vgic_inject_irq(vcpu->kvm, vcpu->vcpu_id,
+ pmu->irq_num, overflow);
+   WARN_ON(ret);
}
 }
 
+bool kvm_pmu_should_notify_user(struct kvm_vcpu *vcpu)
+{
+   struct kvm_pmu *pmu = >arch.pmu;
+   struct kvm_sync_regs *sregs = >run->s.regs;
+   bool run_level = sregs->device_irq_level & KVM_ARM_DEV_PMU;
+
+   if (likely(irqchip_in_kernel(vcpu->kvm)))
+   return false;
+
+   return pmu->irq_level != run_level;
+}
+
+/*
+ * Reflect the PMU overflow interrupt output level into the kvm_run structure
+ */
+void kvm_pmu_update_run(struct kvm_vcpu *vcpu)
+{
+   struct kvm_sync_regs *regs = >run->s.regs;
+
+   if (likely(irqchip_in_kernel(vcpu->kvm)))
+   return;
+
+   /* Populate the timer bitmap for user space */
+   regs->device_irq_level &= ~KVM_ARM_DEV_PMU;
+   if (vcpu->arch.pmu.irq_level)
+   regs->device_irq_level |= KVM_ARM_DEV_PMU;
+}
+
 /**
  * kvm_pmu_flush_hwstate - flush pmu state to cpu
  * 

[PATCH v2 5/5] KVM: arm/arm64: Advertise support for KVM_CAP_ARM_USER_IRQ

[Christoffer Dall]

From: Christoffer Dall 

Now when we support both the virtual timer and PMU reporting interrupts
to userspace, we can advertise this support.

Signed-off-by: Christoffer Dall 
---
 arch/arm/kvm/arm.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 92f38f6..71cf78e 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -226,6 +226,13 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
else
r = kvm->arch.vgic.msis_require_devid;
break;
+   case KVM_CAP_ARM_USER_IRQ:
+   /*
+* 1: EL1_VTIMER, EL1_PTIMER, and PMU.
+* (bump this number if adding more devices)
+*/
+   r = 1;
+   break;
default:
r = kvm_arch_dev_ioctl_check_extension(kvm, ext);
break;
-- 
2.9.0

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

Re: [RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Marc Zyngier]

On 03/02/17 14:00, Peter Maydell wrote:
> On 12 January 2017 at 15:56, Eric Auger  wrote:
>> Add description for how to access vITS registers and how to flush/restore
>> vITS tables into/from memory
>>
>> Signed-off-by: Eric Auger 
>> ---
>>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 
>> ++
>>  1 file changed, 70 insertions(+)
>>
>> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt 
>> b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> index 6081a5b..bd74613 100644
>> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
>> @@ -36,3 +36,73 @@ Groups:
>>  -ENXIO:  ITS not properly configured as required prior to setting
>>   this attribute
>>  -ENOMEM: Memory shortage when allocating ITS internal data
>> +
>> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
>> +  Attributes:
>> +  The attr field of kvm_device_attr encodes the offset of the
>> +  ITS register, relative to the ITS control frame base address
>> +  (ITS_base).
>> +
>> +  kvm_device_attr.addr points to a __u64 value whatever the width
>> +  of the addressed register (32/64 bits).
>> +
>> +  Writes to read-only registers are ignored by the kernel except
>> +  for a single register, GITS_READR. Normally this register is RO
>> +  but it needs to be restored otherwise commands in the queue will
>> +  be re-executed after CWRITER setting.
> 
> Dumb question -- is it possible/sensible to process the command
> queue rather than migrating a list of outstanding commands?

It is not always possible, specially if the ITS implements the "stall"
mechanism, where it waits for SW acknowledgement in case of a command error.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[RESEND PATCH] KVM: arm/arm64: vgic: Stop injecting the MSI occurrence twice

[Shanker Donthineni]

The IRQFD framework calls the architecture dependent function
twice if the corresponding GSI type is edge triggered. For ARM,
the function kvm_set_msi() is getting called twice whenever the
IRQFD receives the event signal. The rest of the code path is
trying to inject the MSI without any validation checks. No need
to call the function vgic_its_inject_msi() second time to avoid
an unnecessary overhead in IRQ queue logic. It also avoids the
possibility of VM seeing the MSI twice.

Simple fix, return -1 if the argument 'level' value is zero.

Signed-off-by: Shanker Donthineni 
Reviewed-by: Eric Auger 
Reviewed-by: Christoffer Dall 
---
Forgot to CC the kvmarm list earlier, including now.

 virt/kvm/arm/vgic/vgic-irqfd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-irqfd.c b/virt/kvm/arm/vgic/vgic-irqfd.c
index d918dcf..f138ed2 100644
--- a/virt/kvm/arm/vgic/vgic-irqfd.c
+++ b/virt/kvm/arm/vgic/vgic-irqfd.c
@@ -99,6 +99,9 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
if (!vgic_has_its(kvm))
return -ENODEV;
 
+   if (!level)
+   return -1;
+
return vgic_its_inject_msi(kvm, );
 }
 
-- 
Qualcomm Datacenter Technologies, Inc. on behalf of the 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

Re: [RFC 01/13] KVM: arm/arm64: Add vITS save/restore API documentation

[Peter Maydell]

On 12 January 2017 at 15:56, Eric Auger  wrote:
> Add description for how to access vITS registers and how to flush/restore
> vITS tables into/from memory
>
> Signed-off-by: Eric Auger 
> ---
>  Documentation/virtual/kvm/devices/arm-vgic-its.txt | 70 
> ++
>  1 file changed, 70 insertions(+)
>
> diff --git a/Documentation/virtual/kvm/devices/arm-vgic-its.txt 
> b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> index 6081a5b..bd74613 100644
> --- a/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> +++ b/Documentation/virtual/kvm/devices/arm-vgic-its.txt
> @@ -36,3 +36,73 @@ Groups:
>  -ENXIO:  ITS not properly configured as required prior to setting
>   this attribute
>  -ENOMEM: Memory shortage when allocating ITS internal data
> +
> +  KVM_DEV_ARM_VGIC_GRP_ITS_REGS
> +  Attributes:
> +  The attr field of kvm_device_attr encodes the offset of the
> +  ITS register, relative to the ITS control frame base address
> +  (ITS_base).
> +
> +  kvm_device_attr.addr points to a __u64 value whatever the width
> +  of the addressed register (32/64 bits).
> +
> +  Writes to read-only registers are ignored by the kernel except
> +  for a single register, GITS_READR. Normally this register is RO
> +  but it needs to be restored otherwise commands in the queue will
> +  be re-executed after CWRITER setting.

Dumb question -- is it possible/sensible to process the command
queue rather than migrating a list of outstanding commands?

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

Re: [PATCH] KVM: arm/arm64: vgic: Stop injecting the MSI occurrence twice

[Christoffer Dall]

Hi Shanker,

[please cc the kvmarm list for kvm/arm patches, thanks]

On Thu, Feb 02, 2017 at 08:30:03PM -0600, Shanker Donthineni wrote:
> The IRQFD framework calls the architecture dependent function
> twice if the corresponding GSI type is edge triggered. For ARM,
> the function kvm_set_msi() is getting called twice whenever the
> IRQFD receives the event signal. The rest of the code path is
> trying to inject the MSI without any validation checks. No need
> to call the function vgic_its_inject_msi() second time to avoid
> an unnecessary overhead in IRQ queue logic. It also avoids the
> possibility of VM seeing the MSI twice.
> 
> Simple fix, return -1 if the argument 'level' value is zero.
> 
> Signed-off-by: Shanker Donthineni 
> ---
>  virt/kvm/arm/vgic/vgic-irqfd.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-irqfd.c b/virt/kvm/arm/vgic/vgic-irqfd.c
> index d918dcf..f138ed2 100644
> --- a/virt/kvm/arm/vgic/vgic-irqfd.c
> +++ b/virt/kvm/arm/vgic/vgic-irqfd.c
> @@ -99,6 +99,9 @@ int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
>   if (!vgic_has_its(kvm))
>   return -ENODEV;
>  
> + if (!level)
> + return -1;
> +
>   return vgic_its_inject_msi(kvm, );
>  }
>  
> -- 

Reviewed-by: Christoffer Dall 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC v3 00/10] Provide the EL1 physical timer to the VM

[Christoffer Dall]

On Fri, Feb 3, 2017 at 2:14 PM, Jintack Lim  wrote:
> On Fri, Feb 3, 2017 at 7:33 AM, Christoffer Dall  wrote:
>> On Thu, Feb 02, 2017 at 09:51:13AM -0500, Jintack Lim wrote:
>>> On Thu, Feb 2, 2017 at 7:31 AM, Christoffer Dall  wrote:
>>> > Hi Jintack,
>>> >
>>> > On Wed, Feb 01, 2017 at 12:43:00PM -0500, Jintack Lim wrote:
>>> >> The ARM architecture defines the EL1 physical timer and the virtual 
>>> >> timer,
>>> >> and it is reasonable for an OS to expect to be able to access both.
>>> >> However, the current KVM implementation does not provide the EL1 physical
>>> >> timer to VMs but terminates VMs on access to the timer.
>>> >>
>>> >> This patch series enables VMs to use the EL1 physical timer through
>>> >> trap-and-emulate.  The KVM host emulates each EL1 physical timer register
>>> >> access and sets up the background timer accordingly.  When the background
>>> >> timer expires, the KVM host injects EL1 physical timer interrupts to the
>>> >> VM.  Alternatively, it's also possible to allow VMs to access the EL1
>>> >> physical timer without trapping.  However, this requires somehow using 
>>> >> the
>>> >> EL2 physical timer for the Linux host while running the VM instead of the
>>> >> EL1 physical timer.  Right now I just implemented trap-and-emulate 
>>> >> because
>>> >> this was straightforward to do, and I leave it to future work to 
>>> >> determine
>>> >> if transferring the EL1 physical timer state to the EL2 timer provides 
>>> >> any
>>> >> performance benefit.
>>> >>
>>> >> This feature will be useful for any OS that wishes to access the EL1
>>> >> physical timer. Nested virtualization is one of those use cases. A nested
>>> >> hypervisor running inside a VM would think it has full access to the
>>> >> hardware and naturally tries to use the EL1 physical timer as Linux would
>>> >> do. Other nested hypervisors may try to use the EL2 physical timer as Xen
>>> >> would do, but supporting the EL2 physical timer to the VM is out of scope
>>> >> of this patch series. This patch series will make it easy to add the EL2
>>> >> timer support in the future, though.
>>> >>
>>> >> Note that Linux VMs booting in EL1 will be unaffected by this patch 
>>> >> series
>>> >> and will continue to use only the virtual timer and this patch series 
>>> >> will
>>> >> therefore not introduce any performance degredation as a result of
>>> >> trap-and-emulate.
>>> >>
>>> >> v2 => v3:
>>> >>  - Rebase on kvmarm/queue
>>> >>  - Take kvm->lock to synchronize cntvoff across all vtimers
>>> >>  - Remove unnecessary function parameters
>>> >>  - Add comments
>>> >
>>> > I just gave v3 a test run on my TC2 (32-bit platform) and my guest
>>> > quickly locks up trying to run cyclictest or when booting the machine it
>>> > stalls with RCU timeouts.
>>>
>>> Ok. It's my fault not to specify that the emulated physical timer is
>>> supported/tested on arm64.
>>> On 32-bit platform, it is supposed to show the same behavior as
>>> before, but I haven't tested.
>>> Were you using the physical timer or the virtual timer for the guest?
>>>
>>> >
>>> > Could you have a look?
>>>
>>> Sure, I'll have a look. I don't have access to my Cubietruck today,
>>> but I can work on that tomorrow.
>>>
>>
>> Don't bother, I've figured this out for you.
>
> Thanks a lot.
>
>>
>> You need the following fixup to your patch:
>
> Ok. I'll post v4 soon.
> You've already do "acked-by" for this commit. Do I need to change it
> to "signed-off-by"?
>

I guess so, technically.  I don't care deeply though.

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

Re: [RFC v3 00/10] Provide the EL1 physical timer to the VM

[Jintack Lim]

On Fri, Feb 3, 2017 at 7:33 AM, Christoffer Dall  wrote:
> On Thu, Feb 02, 2017 at 09:51:13AM -0500, Jintack Lim wrote:
>> On Thu, Feb 2, 2017 at 7:31 AM, Christoffer Dall  wrote:
>> > Hi Jintack,
>> >
>> > On Wed, Feb 01, 2017 at 12:43:00PM -0500, Jintack Lim wrote:
>> >> The ARM architecture defines the EL1 physical timer and the virtual timer,
>> >> and it is reasonable for an OS to expect to be able to access both.
>> >> However, the current KVM implementation does not provide the EL1 physical
>> >> timer to VMs but terminates VMs on access to the timer.
>> >>
>> >> This patch series enables VMs to use the EL1 physical timer through
>> >> trap-and-emulate.  The KVM host emulates each EL1 physical timer register
>> >> access and sets up the background timer accordingly.  When the background
>> >> timer expires, the KVM host injects EL1 physical timer interrupts to the
>> >> VM.  Alternatively, it's also possible to allow VMs to access the EL1
>> >> physical timer without trapping.  However, this requires somehow using the
>> >> EL2 physical timer for the Linux host while running the VM instead of the
>> >> EL1 physical timer.  Right now I just implemented trap-and-emulate because
>> >> this was straightforward to do, and I leave it to future work to determine
>> >> if transferring the EL1 physical timer state to the EL2 timer provides any
>> >> performance benefit.
>> >>
>> >> This feature will be useful for any OS that wishes to access the EL1
>> >> physical timer. Nested virtualization is one of those use cases. A nested
>> >> hypervisor running inside a VM would think it has full access to the
>> >> hardware and naturally tries to use the EL1 physical timer as Linux would
>> >> do. Other nested hypervisors may try to use the EL2 physical timer as Xen
>> >> would do, but supporting the EL2 physical timer to the VM is out of scope
>> >> of this patch series. This patch series will make it easy to add the EL2
>> >> timer support in the future, though.
>> >>
>> >> Note that Linux VMs booting in EL1 will be unaffected by this patch series
>> >> and will continue to use only the virtual timer and this patch series will
>> >> therefore not introduce any performance degredation as a result of
>> >> trap-and-emulate.
>> >>
>> >> v2 => v3:
>> >>  - Rebase on kvmarm/queue
>> >>  - Take kvm->lock to synchronize cntvoff across all vtimers
>> >>  - Remove unnecessary function parameters
>> >>  - Add comments
>> >
>> > I just gave v3 a test run on my TC2 (32-bit platform) and my guest
>> > quickly locks up trying to run cyclictest or when booting the machine it
>> > stalls with RCU timeouts.
>>
>> Ok. It's my fault not to specify that the emulated physical timer is
>> supported/tested on arm64.
>> On 32-bit platform, it is supposed to show the same behavior as
>> before, but I haven't tested.
>> Were you using the physical timer or the virtual timer for the guest?
>>
>> >
>> > Could you have a look?
>>
>> Sure, I'll have a look. I don't have access to my Cubietruck today,
>> but I can work on that tomorrow.
>>
>
> Don't bother, I've figured this out for you.

Thanks a lot.

>
> You need the following fixup to your patch:

Ok. I'll post v4 soon.
You've already do "acked-by" for this commit. Do I need to change it
to "signed-off-by"?

>
> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
> index 93c811c..35d7100 100644
> --- a/virt/kvm/arm/arch_timer.c
> +++ b/virt/kvm/arm/arch_timer.c
> @@ -410,14 +410,21 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
>  }
>
>  /* Make the updates of cntvoff for all vtimer contexts atomic */
> -static void update_vtimer_cntvoff(struct kvm *kvm, u64 cntvoff)
> +static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff)
>  {
> int i;
> -   struct kvm_vcpu *vcpu;
> +   struct kvm *kvm = vcpu->kvm;
> +   struct kvm_vcpu *tmp;
>
> mutex_lock(>lock);
> -   kvm_for_each_vcpu(i, vcpu, kvm)
> -   vcpu_vtimer(vcpu)->cntvoff = cntvoff;
> +   kvm_for_each_vcpu(i, tmp, kvm)
> +   vcpu_vtimer(tmp)->cntvoff = cntvoff;
> +
> +   /*
> +* When called from the vcpu create path, the CPU being created is not
> +* included in the loop above, so we just set it here as well.
> +*/
> +   vcpu_vtimer(vcpu)->cntvoff = cntvoff;
> mutex_unlock(>lock);
>  }
>
> @@ -426,7 +433,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
> struct arch_timer_cpu *timer = >arch.timer_cpu;
>
> /* Synchronize cntvoff across all vtimers of a VM. */
> -   update_vtimer_cntvoff(vcpu->kvm, kvm_phys_timer_read());
> +   update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
> vcpu_ptimer(vcpu)->cntvoff = 0;
>
> INIT_WORK(>expired, kvm_timer_inject_irq_work);
> @@ -448,7 +455,7 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 
> regid, u64 value)
> vtimer->cnt_ctl = value;
> break;
> 

Re: [RFC v3 00/10] Provide the EL1 physical timer to the VM

[Christoffer Dall]

On Thu, Feb 02, 2017 at 09:51:13AM -0500, Jintack Lim wrote:
> On Thu, Feb 2, 2017 at 7:31 AM, Christoffer Dall  wrote:
> > Hi Jintack,
> >
> > On Wed, Feb 01, 2017 at 12:43:00PM -0500, Jintack Lim wrote:
> >> The ARM architecture defines the EL1 physical timer and the virtual timer,
> >> and it is reasonable for an OS to expect to be able to access both.
> >> However, the current KVM implementation does not provide the EL1 physical
> >> timer to VMs but terminates VMs on access to the timer.
> >>
> >> This patch series enables VMs to use the EL1 physical timer through
> >> trap-and-emulate.  The KVM host emulates each EL1 physical timer register
> >> access and sets up the background timer accordingly.  When the background
> >> timer expires, the KVM host injects EL1 physical timer interrupts to the
> >> VM.  Alternatively, it's also possible to allow VMs to access the EL1
> >> physical timer without trapping.  However, this requires somehow using the
> >> EL2 physical timer for the Linux host while running the VM instead of the
> >> EL1 physical timer.  Right now I just implemented trap-and-emulate because
> >> this was straightforward to do, and I leave it to future work to determine
> >> if transferring the EL1 physical timer state to the EL2 timer provides any
> >> performance benefit.
> >>
> >> This feature will be useful for any OS that wishes to access the EL1
> >> physical timer. Nested virtualization is one of those use cases. A nested
> >> hypervisor running inside a VM would think it has full access to the
> >> hardware and naturally tries to use the EL1 physical timer as Linux would
> >> do. Other nested hypervisors may try to use the EL2 physical timer as Xen
> >> would do, but supporting the EL2 physical timer to the VM is out of scope
> >> of this patch series. This patch series will make it easy to add the EL2
> >> timer support in the future, though.
> >>
> >> Note that Linux VMs booting in EL1 will be unaffected by this patch series
> >> and will continue to use only the virtual timer and this patch series will
> >> therefore not introduce any performance degredation as a result of
> >> trap-and-emulate.
> >>
> >> v2 => v3:
> >>  - Rebase on kvmarm/queue
> >>  - Take kvm->lock to synchronize cntvoff across all vtimers
> >>  - Remove unnecessary function parameters
> >>  - Add comments
> >
> > I just gave v3 a test run on my TC2 (32-bit platform) and my guest
> > quickly locks up trying to run cyclictest or when booting the machine it
> > stalls with RCU timeouts.
> 
> Ok. It's my fault not to specify that the emulated physical timer is
> supported/tested on arm64.
> On 32-bit platform, it is supposed to show the same behavior as
> before, but I haven't tested.
> Were you using the physical timer or the virtual timer for the guest?
> 
> >
> > Could you have a look?
> 
> Sure, I'll have a look. I don't have access to my Cubietruck today,
> but I can work on that tomorrow.
> 

Don't bother, I've figured this out for you.

You need the following fixup to your patch:

diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c
index 93c811c..35d7100 100644
--- a/virt/kvm/arm/arch_timer.c
+++ b/virt/kvm/arm/arch_timer.c
@@ -410,14 +410,21 @@ int kvm_timer_vcpu_reset(struct kvm_vcpu *vcpu,
 }
 
 /* Make the updates of cntvoff for all vtimer contexts atomic */
-static void update_vtimer_cntvoff(struct kvm *kvm, u64 cntvoff)
+static void update_vtimer_cntvoff(struct kvm_vcpu *vcpu, u64 cntvoff)
 {
int i;
-   struct kvm_vcpu *vcpu;
+   struct kvm *kvm = vcpu->kvm;
+   struct kvm_vcpu *tmp;
 
mutex_lock(>lock);
-   kvm_for_each_vcpu(i, vcpu, kvm)
-   vcpu_vtimer(vcpu)->cntvoff = cntvoff;
+   kvm_for_each_vcpu(i, tmp, kvm)
+   vcpu_vtimer(tmp)->cntvoff = cntvoff;
+
+   /*
+* When called from the vcpu create path, the CPU being created is not
+* included in the loop above, so we just set it here as well.
+*/
+   vcpu_vtimer(vcpu)->cntvoff = cntvoff;
mutex_unlock(>lock);
 }
 
@@ -426,7 +433,7 @@ void kvm_timer_vcpu_init(struct kvm_vcpu *vcpu)
struct arch_timer_cpu *timer = >arch.timer_cpu;
 
/* Synchronize cntvoff across all vtimers of a VM. */
-   update_vtimer_cntvoff(vcpu->kvm, kvm_phys_timer_read());
+   update_vtimer_cntvoff(vcpu, kvm_phys_timer_read());
vcpu_ptimer(vcpu)->cntvoff = 0;
 
INIT_WORK(>expired, kvm_timer_inject_irq_work);
@@ -448,7 +455,7 @@ int kvm_arm_timer_set_reg(struct kvm_vcpu *vcpu, u64 regid, 
u64 value)
vtimer->cnt_ctl = value;
break;
case KVM_REG_ARM_TIMER_CNT:
-   update_vtimer_cntvoff(vcpu->kvm, kvm_phys_timer_read() - value);
+   update_vtimer_cntvoff(vcpu, kvm_phys_timer_read() - value);
break;
case KVM_REG_ARM_TIMER_CVAL:
vtimer->cnt_cval = value;

This is an amuzing one.

Thanks,
-Christoffer

Re: [PATCH v2 3/3] arm64: kvm: add support for the extended 64bit ccsidr

[Sudeep Holla]

Hi Marc,

On 01/02/17 16:02, Marc Zyngier wrote:
[...]

> 
> I'm a bit worried about this patch. If we snapshot a VM on a 32bit
> CCSIDR system, and restore it on a 64bit CSSIDR system (or the reverse),
> what happens? My hunch is that we cannot restore the VM properly.
>

I agree. I had a look at QEMU as you suggested offline. Looks like QEMU
emulate these registers with predefined values for each core type. Also
it looks like it's not using the existing DEMUX_ID_CCSIDR

> Now, I'm questioning the need for having those altogether, as we do a
> lot of work to prevent the guest from actually using that geometry (and
> on a big-little system, this hardly works).
> 

If we can conclude there are no users for DEMUX_ID_CCSIDR, we can remove
it all together instead of introducing new one for 64bit.

Are there any other users of these interface provided by KVM after from
kvmtool and qemu ?

-- 
Regards,
Sudeep
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm