Re: [PATCH v4 07/10] ARM: Introduce MPIDR_LEVEL_SHIFT macro

2016-09-13 Thread Marc Zyngier 
On 13/09/16 11:32, Vladimir Murzin wrote:
> On 13/09/16 11:12, Marc Zyngier wrote:
>> On 13/09/16 10:04, Vladimir Murzin wrote:
>>> On 13/09/16 09:38, Christoffer Dall wrote:
 On Mon, Sep 12, 2016 at 03:49:21PM +0100, Vladimir Murzin wrote:
> vgic-v3 driver uses architecture specific MPIDR_LEVEL_SHIFT macro to
> encode the affinity in a form compatible with ICC_SGI* registers.
> Unfortunately, that macro is missing on ARM, so let's add it.
>
> Cc: Russell King 
> Signed-off-by: Vladimir Murzin 
> ---
>  arch/arm/include/asm/cputype.h |1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/include/asm/cputype.h 
> b/arch/arm/include/asm/cputype.h
> index 1ee94c7..e2d94c1 100644
> --- a/arch/arm/include/asm/cputype.h
> +++ b/arch/arm/include/asm/cputype.h
> @@ -55,6 +55,7 @@
>  
>  #define MPIDR_LEVEL_BITS 8
>  #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
> +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level)
>  

 I'm not sure I follow the correctness of this completely.

 This is called from vgic_v3_dispatch_sgi, which takes a u64 value, which
 may have something in the Aff3 field, which we now shift left 24 bits,
 but that is not the Aff3 field of AArch32's MPIDR.

 What is the rationale for this making sense again?
>>>
>>> IIUC, in such case we construct mpidr which won't match in match_mpidr()
>>> with the value we get from kvm_vcpu_get_mpidr_aff() and no SGI will be
>>> sent to the guest.
>>>
>>> Since we get that u64 value from the guest, I'd think it is something
>>> wrong is going on in the guest in case Aff3 is non-zero; however, we can
>>> hide it by zeroing out SGI Aff3 bits in access_gic_sgi().
>>
>> I don't think zeroing Aff3 is the right move, as the spec doesn't say
>> that Aff3 should be ignored in a write to ICC_SGI1R. On the other hand,
>> the spec says (in the context of the target list): "If a bit is 1 and
>> the bit does not correspond to a valid target PE, the bit must be
>> ignored by the Distributor".
>>
>> This makes me think that, unless ICC_SGI1R.IMR is set, we should simply
>> ignore that SGI because there is no way we can actually deliver it.
>>
>> Could you cook a small patch that would go on top of this series?
> 
> I assume you've meant ICC_SGI1R.IRM, aka broadcast. In this case,

Yes, sorry.

> vgic_v3_dispatch_sgi() seems already matches the logic you've described:
> 
> - if IRM == 1, send to everyone except self without check for mpidr
> - if IRM == 0, send to target iff matched to a valid mpidr
> 
> Am I missing something?

Not much. My only ask was that if Aff3 was set, we could take the
shortcut of not calling vgic_v3_dispatch_sgi() at all and return
immediately. But as you said, we already deal with the case of invalid
MPIDRs.

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 v4 07/10] ARM: Introduce MPIDR_LEVEL_SHIFT macro

2016-09-13 Thread Vladimir Murzin 
On 13/09/16 11:12, Marc Zyngier wrote:
> On 13/09/16 10:04, Vladimir Murzin wrote:
>> On 13/09/16 09:38, Christoffer Dall wrote:
>>> On Mon, Sep 12, 2016 at 03:49:21PM +0100, Vladimir Murzin wrote:
 vgic-v3 driver uses architecture specific MPIDR_LEVEL_SHIFT macro to
 encode the affinity in a form compatible with ICC_SGI* registers.
 Unfortunately, that macro is missing on ARM, so let's add it.

 Cc: Russell King 
 Signed-off-by: Vladimir Murzin 
 ---
  arch/arm/include/asm/cputype.h |1 +
  1 file changed, 1 insertion(+)

 diff --git a/arch/arm/include/asm/cputype.h 
 b/arch/arm/include/asm/cputype.h
 index 1ee94c7..e2d94c1 100644
 --- a/arch/arm/include/asm/cputype.h
 +++ b/arch/arm/include/asm/cputype.h
 @@ -55,6 +55,7 @@
  
  #define MPIDR_LEVEL_BITS 8
  #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
 +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level)
  
>>>
>>> I'm not sure I follow the correctness of this completely.
>>>
>>> This is called from vgic_v3_dispatch_sgi, which takes a u64 value, which
>>> may have something in the Aff3 field, which we now shift left 24 bits,
>>> but that is not the Aff3 field of AArch32's MPIDR.
>>>
>>> What is the rationale for this making sense again?
>>
>> IIUC, in such case we construct mpidr which won't match in match_mpidr()
>> with the value we get from kvm_vcpu_get_mpidr_aff() and no SGI will be
>> sent to the guest.
>>
>> Since we get that u64 value from the guest, I'd think it is something
>> wrong is going on in the guest in case Aff3 is non-zero; however, we can
>> hide it by zeroing out SGI Aff3 bits in access_gic_sgi().
> 
> I don't think zeroing Aff3 is the right move, as the spec doesn't say
> that Aff3 should be ignored in a write to ICC_SGI1R. On the other hand,
> the spec says (in the context of the target list): "If a bit is 1 and
> the bit does not correspond to a valid target PE, the bit must be
> ignored by the Distributor".
> 
> This makes me think that, unless ICC_SGI1R.IMR is set, we should simply
> ignore that SGI because there is no way we can actually deliver it.
> 
> Could you cook a small patch that would go on top of this series?

I assume you've meant ICC_SGI1R.IRM, aka broadcast. In this case,
vgic_v3_dispatch_sgi() seems already matches the logic you've described:

- if IRM == 1, send to everyone except self without check for mpidr
- if IRM == 0, send to target iff matched to a valid mpidr

Am I missing something?

Thanks
Vladimir

> 
> Thanks,
> 
>   M.
> 

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

[PATCH 3/7] KVM: arm/arm64: Register perf trace event notifier

2016-09-13 Thread Punit Agrawal 
Register a notifier to track state changes of perf trace events.

The notifier will enable taking appropriate action for trace events
targeting VM.

Signed-off-by: Punit Agrawal 
Cc: Christoffer Dall 
Cc: Marc Zyngier 
---
 arch/arm/include/asm/kvm_host.h   |   3 +
 arch/arm/kvm/arm.c|   2 +
 arch/arm64/include/asm/kvm_host.h |   8 +++
 arch/arm64/kvm/Kconfig|   4 ++
 arch/arm64/kvm/Makefile   |   1 +
 arch/arm64/kvm/perf_trace.c   | 122 ++
 6 files changed, 140 insertions(+)
 create mode 100644 arch/arm64/kvm/perf_trace.c

diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
index de338d9..609998e 100644
--- a/arch/arm/include/asm/kvm_host.h
+++ b/arch/arm/include/asm/kvm_host.h
@@ -280,6 +280,9 @@ static inline int kvm_arch_dev_ioctl_check_extension(struct 
kvm *kvm, long ext)
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+static inline int kvm_perf_trace_init(void) { return 0; }
+static inline int kvm_perf_trace_teardown(void) { return 0; }
+
 void kvm_mmu_wp_memory_region(struct kvm *kvm, int slot);
 
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
index 75f130e..e1b99c4 100644
--- a/arch/arm/kvm/arm.c
+++ b/arch/arm/kvm/arm.c
@@ -1220,6 +1220,7 @@ static int init_subsystems(void)
goto out;
 
kvm_perf_init();
+   kvm_perf_trace_init();
kvm_coproc_table_init();
 
 out:
@@ -1411,6 +1412,7 @@ out_err:
 void kvm_arch_exit(void)
 {
kvm_perf_teardown();
+   kvm_perf_trace_teardown();
 }
 
 static int arm_init(void)
diff --git a/arch/arm64/include/asm/kvm_host.h 
b/arch/arm64/include/asm/kvm_host.h
index 3eda975..f6ff8e5 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -345,6 +345,14 @@ int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
 int kvm_perf_init(void);
 int kvm_perf_teardown(void);
 
+#if !defined(CONFIG_KVM_PERF_TRACE)
+static inline int kvm_perf_trace_init(void) { return 0; }
+static inline int kvm_perf_trace_teardown(void) { return 0; }
+#else
+int kvm_perf_trace_init(void);
+int kvm_perf_trace_teardown(void);
+#endif
+
 struct kvm_vcpu *kvm_mpidr_to_vcpu(struct kvm *kvm, unsigned long mpidr);
 
 static inline void __cpu_init_hyp_mode(phys_addr_t pgd_ptr,
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 9c9edc9..56e9537 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -19,6 +19,9 @@ if VIRTUALIZATION
 config KVM_ARM_VGIC_V3
bool
 
+config KVM_PERF_TRACE
+bool
+
 config KVM
bool "Kernel-based Virtual Machine (KVM) support"
depends on OF
@@ -39,6 +42,7 @@ config KVM
select HAVE_KVM_MSI
select HAVE_KVM_IRQCHIP
select HAVE_KVM_IRQ_ROUTING
+   select KVM_PERF_TRACE if EVENT_TRACING && PERF_EVENTS
---help---
  Support hosting virtualized guest machines.
  We don't support KVM with 16K page tables yet, due to the multiple
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index 695eb3c..7d175e4 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -19,6 +19,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o
 kvm-$(CONFIG_KVM_ARM_HOST) += emulate.o inject_fault.o regmap.o
 kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o
 kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o 
sys_regs_generic_v8.o
+kvm-$(CONFIG_KVM_PERF_TRACE) += perf_trace.o
 
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic.o
 kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/vgic/vgic-init.o
diff --git a/arch/arm64/kvm/perf_trace.c b/arch/arm64/kvm/perf_trace.c
new file mode 100644
index 000..1cafbc9
--- /dev/null
+++ b/arch/arm64/kvm/perf_trace.c
@@ -0,0 +1,122 @@
+/*
+ * Copyright (C) 2016 ARM Ltd.
+ * Author: Punit Agrawal 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see .
+ */
+#include 
+#include 
+
+typedef int (*perf_trace_callback_fn)(struct kvm *kvm, bool enable);
+
+struct kvm_trace_hook {
+   char *key;  /* Name of the tracepoint to match */
+   perf_trace_callback_fn setup_fn;
+};
+
+static struct kvm_trace_hook trace_hook[] = {
+   { },
+};
+
+static perf_trace_callback_fn find_trace_callback(const

[PATCH 6/7] arm64: KVM: Handle trappable TLB instructions

2016-09-13 Thread Punit Agrawal 
The ARMv8 architecture allows trapping of TLB maintenane instructions
from EL0/EL1 to higher exception levels. On encountering a trappable TLB
instruction in a guest, an exception is taken to EL2.

Add functionality to handle emulating the TLB instructions.

Signed-off-by: Punit Agrawal 
Cc: Christoffer Dall 
Cc: Marc Zyngier 
---
 arch/arm64/include/asm/kvm_asm.h |  1 +
 arch/arm64/kvm/hyp/tlb.c | 75 +
 arch/arm64/kvm/sys_regs.c| 81 
 arch/arm64/kvm/trace.h   | 16 
 4 files changed, 173 insertions(+)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index 7561f63..1ac1cc3 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -49,6 +49,7 @@ extern char __kvm_hyp_vector[];
 extern void __kvm_flush_vm_context(void);
 extern void __kvm_tlb_flush_vmid_ipa(struct kvm *kvm, phys_addr_t ipa);
 extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
+extern void __kvm_emulate_tlb_invalidate(struct kvm *kvm, u32 sysreg, u64 
regval);
 
 extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
 
diff --git a/arch/arm64/kvm/hyp/tlb.c b/arch/arm64/kvm/hyp/tlb.c
index 4cda100..a9e5005 100644
--- a/arch/arm64/kvm/hyp/tlb.c
+++ b/arch/arm64/kvm/hyp/tlb.c
@@ -78,3 +78,78 @@ static void __hyp_text __tlb_flush_vm_context(void)
 }
 
 __alias(__tlb_flush_vm_context) void __kvm_flush_vm_context(void);
+
+/* Intentionally empty functions */
+static void __hyp_text __switch_to_hyp_role_nvhe(void) { }
+static void __hyp_text __switch_to_host_role_nvhe(void) { }
+
+static void __hyp_text __switch_to_hyp_role_vhe(void)
+{
+   u64 hcr = read_sysreg(hcr_el2);
+
+   /*
+* When VHE is enabled and HCR_EL2.TGE=1, EL1&0 TLB operations
+* apply to EL2&0 translation regime. As we prepare to emulate
+* guest TLB operation clear HCR_TGE to target TLB operations
+* to EL1&0 (guest).
+*/
+   hcr &= ~HCR_TGE;
+   write_sysreg(hcr, hcr_el2);
+}
+
+static void __hyp_text __switch_to_host_role_vhe(void)
+{
+   u64 hcr = read_sysreg(hcr_el2);
+
+   hcr |= HCR_TGE;
+   write_sysreg(hcr, hcr_el2);
+}
+
+static hyp_alternate_select(__switch_to_hyp_role,
+   __switch_to_hyp_role_nvhe,
+   __switch_to_hyp_role_vhe,
+   ARM64_HAS_VIRT_HOST_EXTN);
+
+static hyp_alternate_select(__switch_to_host_role,
+   __switch_to_host_role_nvhe,
+   __switch_to_host_role_vhe,
+   ARM64_HAS_VIRT_HOST_EXTN);
+
+static void __hyp_text __switch_to_guest_regime(struct kvm *kvm)
+{
+   write_sysreg(kvm->arch.vttbr, vttbr_el2);
+   __switch_to_hyp_role();
+   isb();
+}
+
+static void __hyp_text __switch_to_host_regime(void)
+{
+   __switch_to_host_role();
+   write_sysreg(0, vttbr_el2);
+}
+
+void __hyp_text
+__kvm_emulate_tlb_invalidate(struct kvm *kvm, u32 sys_op, u64 regval)
+{
+   kvm = kern_hyp_va(kvm);
+
+   /*
+* Switch to the guest before performing any TLB operations to
+* target the appropriate VMID
+*/
+   __switch_to_guest_regime(kvm);
+
+   /*
+*  TLB maintenance operations are broadcast to
+*  inner-shareable domain when HCR_FB is set (default for
+*  KVM).
+*
+*  Nuke all Stage 1 TLB entries for the VM. This will kill
+*  performance but it's always safe to do as we don't leave
+*  behind any strays in the TLB
+*/
+   __tlbi(vmalle1is);
+   isb();
+
+   __switch_to_host_regime();
+}
diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index e51367d..0e70da9 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -790,6 +790,18 @@ static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct 
sys_reg_params *p,
return true;
 }
 
+static bool emulate_tlb_invalidate(struct kvm_vcpu *vcpu, struct 
sys_reg_params *p,
+ const struct sys_reg_desc *r)
+{
+   u32 opcode = sys_reg(p->Op0, p->Op1, p->CRn, p->CRm, p->Op2);
+
+   kvm_call_hyp(__kvm_emulate_tlb_invalidate,
+vcpu->kvm, opcode, p->regval);
+   trace_kvm_tlb_invalidate(*vcpu_pc(vcpu), opcode);
+
+   return true;
+}
+
 /* Silly macro to expand the DBG{BCR,BVR,WVR,WCR}n_EL1 registers in one go */
 #define DBG_BCR_BVR_WCR_WVR_EL1(n) \
/* DBGBVRn_EL1 */   \
@@ -841,6 +853,35 @@ static const struct sys_reg_desc sys_reg_descs[] = {
{ Op0(0b01), Op1(0b000), CRn(0b0111), CRm(0b1110), Op2(0b010),
  access_dcsw },
 
+   /*
+* ARMv8 ARM: Table C5-4 TLB maintenance instructions
+* (Ref: ARMv8 ARM C5.1 version: ARM DDI

Re: [PATCH 1/3] arm64: alternative: add auto-nop infrastructure

2016-09-13 Thread Mark Rutland 
On Tue, Sep 13, 2016 at 09:36:14AM +0100, Ard Biesheuvel wrote:
> On 7 September 2016 at 11:07, Mark Rutland  wrote:
> > In some cases, one side of an alternative sequence is simply a number of
> > NOPs used to balance the other side. Keeping track of this manually is
> > tedious, and the presence of large chains of NOPs makes the code more
> > painful to read than necessary.
> >
> > To ameliorate matters, this patch adds a new alternative_else_nop_endif,
> > which automatically balances an alternative sequence with a trivial NOP
> > sled.
> >
> > In many cases, we would like a NOP-sled in the default case, and
> > instructions patched in in the presence of a feature. To enable the NOPs
> > to be generated automatically for this case, this patch also adds a new
> > alternative_if, and updates alternative_else and alternative_endif to
> > work with either alternative_if or alternative_endif.

[...]

> > +/*
> > + * Begin an alternative code sequence.
> >   */
> >  .macro alternative_if_not cap
> > +   .set .Lasm_alt_mode, 0
> 
> Given that only a single copy of this symbol will exist in an object
> file, is it still possible to use both variants in a single
> compilation/assembly unit?

Yes.

GAS allows the symbol to be set multiple times (so long as the
assignments are constant values). The last assignment "wins" when it
comes to output, but assembler macros are evaluated before this, and use
the most recent assignment.

In testing I hacked __kvm_call_hyp to use both:

ENTRY(__kvm_call_hyp)
alternative_if_not ARM64_HAS_VIRT_HOST_EXTN
str lr, [sp, #-16]!
hvc #0
ldr lr, [sp], #16
ret
alternative_else_nop_endif
alternative_if ARM64_HAS_VIRT_HOST_EXTN
hvc #0x539
alternative_else_nop_endif
b   __vhe_hyp_call
ENDPROC(__kvm_call_hyp)

Which, according to objdump gives me the expected result:

Disassembly of section .text:

 <__kvm_call_hyp>:
   0:   f81f0ffestr x30, [sp,#-16]!
   4:   d402hvc #0x0
   8:   f84107feldr x30, [sp],#16
   c:   d65f03c0ret
  10:   d503201fnop
  14:   1400b   0 <__vhe_hyp_call>

Disassembly of section .altinstr_replacement:

 <.altinstr_replacement>:
   0:   d503201fnop
   4:   d503201fnop
   8:   d503201fnop
   c:   d503201fnop
  10:   d400a722hvc #0x539

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

Re: [PATCH v4 08/10] ARM: Move system register accessors to asm/cp15.h

2016-09-13 Thread Christoffer Dall 
On Mon, Sep 12, 2016 at 03:49:22PM +0100, Vladimir Murzin wrote:
> Headers linux/irqchip/arm-gic.v3.h and arch/arm/include/asm/kvm_hyp.h
> are included in virt/kvm/arm/hyp/vgic-v3-sr.c and both define macros
> called __ACCESS_CP15 and __ACCESS_CP15_64 which obviously creates a
> conflict. These macros were introduced independently for GIC and KVM
> and, in fact, do the same thing.
> 
> As an option we could add prefixes to KVM and GIC version of macros so
> they won't clash, but it'd introduce code duplication.  Alternatively,
> we could keep macro in, say, GIC header and include it in KVM one (or
> vice versa), but such dependency would not look nicer.
> 
> So we follow arm64 way (it handles this via sysreg.h) and move only
> single set of macros to asm/cp15.h
> 
> Cc: Russell King 
> Signed-off-by: Vladimir Murzin 

Acked-by: Christoffer Dall 

> ---
>  arch/arm/include/asm/arch_gicv3.h |   27 +++
>  arch/arm/include/asm/cp15.h   |   15 +++
>  arch/arm/include/asm/kvm_hyp.h|   15 +--
>  3 files changed, 27 insertions(+), 30 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch_gicv3.h 
> b/arch/arm/include/asm/arch_gicv3.h
> index e08d151..af25c32 100644
> --- a/arch/arm/include/asm/arch_gicv3.h
> +++ b/arch/arm/include/asm/arch_gicv3.h
> @@ -22,9 +22,7 @@
>  
>  #include 
>  #include 
> -
> -#define __ACCESS_CP15(CRn, Op1, CRm, Op2)p15, Op1, %0, CRn, CRm, Op2
> -#define __ACCESS_CP15_64(Op1, CRm)   p15, Op1, %Q0, %R0, CRm
> +#include 
>  
>  #define ICC_EOIR1__ACCESS_CP15(c12, 0, c12, 1)
>  #define ICC_DIR  __ACCESS_CP15(c12, 0, c11, 1)
> @@ -102,58 +100,55 @@
>  
>  static inline void gic_write_eoir(u32 irq)
>  {
> - asm volatile("mcr " __stringify(ICC_EOIR1) : : "r" (irq));
> + write_sysreg(irq, ICC_EOIR1);
>   isb();
>  }
>  
>  static inline void gic_write_dir(u32 val)
>  {
> - asm volatile("mcr " __stringify(ICC_DIR) : : "r" (val));
> + write_sysreg(val, ICC_DIR);
>   isb();
>  }
>  
>  static inline u32 gic_read_iar(void)
>  {
> - u32 irqstat;
> + u32 irqstat = read_sysreg(ICC_IAR1);
>  
> - asm volatile("mrc " __stringify(ICC_IAR1) : "=r" (irqstat));
>   dsb(sy);
> +
>   return irqstat;
>  }
>  
>  static inline void gic_write_pmr(u32 val)
>  {
> - asm volatile("mcr " __stringify(ICC_PMR) : : "r" (val));
> + write_sysreg(val, ICC_PMR);
>  }
>  
>  static inline void gic_write_ctlr(u32 val)
>  {
> - asm volatile("mcr " __stringify(ICC_CTLR) : : "r" (val));
> + write_sysreg(val, ICC_CTLR);
>   isb();
>  }
>  
>  static inline void gic_write_grpen1(u32 val)
>  {
> - asm volatile("mcr " __stringify(ICC_IGRPEN1) : : "r" (val));
> + write_sysreg(val, ICC_IGRPEN1);
>   isb();
>  }
>  
>  static inline void gic_write_sgi1r(u64 val)
>  {
> - asm volatile("mcrr " __stringify(ICC_SGI1R) : : "r" (val));
> + write_sysreg(val, ICC_SGI1R);
>  }
>  
>  static inline u32 gic_read_sre(void)
>  {
> - u32 val;
> -
> - asm volatile("mrc " __stringify(ICC_SRE) : "=r" (val));
> - return val;
> + return read_sysreg(ICC_SRE);
>  }
>  
>  static inline void gic_write_sre(u32 val)
>  {
> - asm volatile("mcr " __stringify(ICC_SRE) : : "r" (val));
> + write_sysreg(val, ICC_SRE);
>   isb();
>  }
>  
> diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
> index c3f1152..dbdbce1 100644
> --- a/arch/arm/include/asm/cp15.h
> +++ b/arch/arm/include/asm/cp15.h
> @@ -49,6 +49,21 @@
>  
>  #ifdef CONFIG_CPU_CP15
>  
> +#define __ACCESS_CP15(CRn, Op1, CRm, Op2)\
> + "mrc", "mcr", __stringify(p15, Op1, %0, CRn, CRm, Op2), u32
> +#define __ACCESS_CP15_64(Op1, CRm)   \
> + "mrrc", "mcrr", __stringify(p15, Op1, %Q0, %R0, CRm), u64
> +
> +#define __read_sysreg(r, w, c, t) ({ \
> + t __val;\
> + asm volatile(r " " c : "=r" (__val));   \
> + __val;  \
> +})
> +#define read_sysreg(...) __read_sysreg(__VA_ARGS__)
> +
> +#define __write_sysreg(v, r, w, c, t)asm volatile(w " " c : : "r" 
> ((t)(v)))
> +#define write_sysreg(v, ...) __write_sysreg(v, __VA_ARGS__)
> +
>  extern unsigned long cr_alignment;   /* defined in entry-armv.S */
>  
>  static inline unsigned long get_cr(void)
> diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
> index 6eaff28..e604ad68 100644
> --- a/arch/arm/include/asm/kvm_hyp.h
> +++ b/arch/arm/include/asm/kvm_hyp.h
> @@ -20,28 +20,15 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
>  #define __hyp_text __section(.hyp.text) notrace
>  
> -#define __ACCESS_CP15(CRn, Op1, CRm, Op2)\
> - "mrc", "mcr", __stringify(p15, Op1, %0, CRn, CRm, Op2),

Re: [PATCH v4 09/10] ARM: gic-v3: Introduce 32-to-64-bit mappings for GICv3 cpu registers

2016-09-13 Thread Christoffer Dall 
On Mon, Sep 12, 2016 at 03:49:23PM +0100, Vladimir Murzin wrote:
> vgic-v3 save/restore routines are written in such way that they map
> arm64 system register naming nicely, but it does not fit to arm
> world. To keep virt/kvm/arm/hyp/vgic-v3-sr.c untouched we create a
> mapping with a function for each register mapping the 32-bit to the
> 64-bit accessors.
> 
> Please, note that 64-bit wide ICH_LR is split in two 32-bit halves
> (ICH_LR and ICH_LRC) accessed independently.
> 
> Signed-off-by: Vladimir Murzin 

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

Re: [PATCH v4 03/10] arm64: KVM: Move vgic-v3 save/restore to virt/kvm/arm/hyp

2016-09-13 Thread Christoffer Dall 
On Mon, Sep 12, 2016 at 03:49:17PM +0100, Vladimir Murzin wrote:
> So we can reuse the code under arch/arm
> 
> Signed-off-by: Vladimir Murzin 
> Acked-by: Marc Zyngier 

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

Re: [PATCH v4 04/10] KVM: arm64: vgic-its: Introduce config option to guard ITS specific code

2016-09-13 Thread Christoffer Dall 
On Mon, Sep 12, 2016 at 03:49:18PM +0100, Vladimir Murzin wrote:
> By now ITS code guarded with KVM_ARM_VGIC_V3 config option which was
> introduced to hide everything specific to vgic-v3 from 32-bit world.
> We are going to support vgic-v3 in 32-bit world and KVM_ARM_VGIC_V3
> will gone, but we don't have support for ITS there yet and we need to
> continue keeping ITS away.
> Introduce the new config option to prevent ITS code being build in
> 32-bit mode when support for vgic-v3 is done.
> 
> Signed-off-by: Vladimir Murzin 
> Acked-by: Marc Zyngier 

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

Re: [PATCH v4 05/10] KVM: arm: vgic: Fix compiler warnings when built for 32-bit

2016-09-13 Thread Christoffer Dall 
On Mon, Sep 12, 2016 at 03:49:19PM +0100, Vladimir Murzin wrote:
> Well, this patch is looking ahead of time, but we'll get following
> compiler warnings as soon as we introduce vgic-v3 to 32-bit world
> 
>   CC  arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.o
> arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c: In function 
> 'vgic_mmio_read_v3r_typer':
> arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c:184:35: warning: left 
> shift count >= width of type [-Wshift-count-overflow]
>   value = (mpidr & GENMASK(23, 0)) << 32;
>^
> In file included from ./include/linux/kernel.h:10:0,
>  from ./include/asm-generic/bug.h:13,
>  from ./arch/arm/include/asm/bug.h:59,
>  from ./include/linux/bug.h:4,
>  from ./include/linux/io.h:23,
>  from ./arch/arm/include/asm/arch_gicv3.h:23,
>  from ./include/linux/irqchip/arm-gic-v3.h:411,
>  from 
> arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c:14:
> arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c: In function 
> 'vgic_v3_dispatch_sgi':
> ./include/linux/bitops.h:6:24: warning: left shift count >= width of type 
> [-Wshift-count-overflow]
>  #define BIT(nr)   (1UL << (nr))
> ^
> arch/arm/kvm/../../../virt/kvm/arm/vgic/vgic-mmio-v3.c:614:20: note: in 
> expansion of macro 'BIT'
>   broadcast = reg & BIT(ICC_SGI1R_IRQ_ROUTING_MODE_BIT);
> ^
> Let's fix them now.
> 
> Signed-off-by: Vladimir Murzin 

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

Re: [PATCH v4 07/10] ARM: Introduce MPIDR_LEVEL_SHIFT macro

2016-09-13 Thread Christoffer Dall 
On Mon, Sep 12, 2016 at 03:49:21PM +0100, Vladimir Murzin wrote:
> vgic-v3 driver uses architecture specific MPIDR_LEVEL_SHIFT macro to
> encode the affinity in a form compatible with ICC_SGI* registers.
> Unfortunately, that macro is missing on ARM, so let's add it.
> 
> Cc: Russell King 
> Signed-off-by: Vladimir Murzin 
> ---
>  arch/arm/include/asm/cputype.h |1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
> index 1ee94c7..e2d94c1 100644
> --- a/arch/arm/include/asm/cputype.h
> +++ b/arch/arm/include/asm/cputype.h
> @@ -55,6 +55,7 @@
>  
>  #define MPIDR_LEVEL_BITS 8
>  #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
> +#define MPIDR_LEVEL_SHIFT(level) (MPIDR_LEVEL_BITS * level)
>  

I'm not sure I follow the correctness of this completely.

This is called from vgic_v3_dispatch_sgi, which takes a u64 value, which
may have something in the Aff3 field, which we now shift left 24 bits,
but that is not the Aff3 field of AArch32's MPIDR.

What is the rationale for this making sense again?

Thanks,
-Christoffer

>  #define MPIDR_AFFINITY_LEVEL(mpidr, level) \
>   ((mpidr >> (MPIDR_LEVEL_BITS * level)) & MPIDR_LEVEL_MASK)
> -- 
> 1.7.9.5
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v4 01/10] arm64: KVM: Use static keys for selecting the GIC backend

2016-09-13 Thread Christoffer Dall 
On Mon, Sep 12, 2016 at 03:49:15PM +0100, Vladimir Murzin wrote:
> Currently GIC backend is selected via alternative framework and this
> is fine. We are going to introduce vgic-v3 to 32-bit world and there
> we don't have patching framework in hand, so we can either check
> support for GICv3 every time we need to choose which backend to use or
> try to optimise it by using static keys. The later looks quite
> promising because we can share logic involved in selecting GIC backend
> between architectures if both uses static keys.
> 
> This patch moves arm64 from alternative to static keys framework for
> selecting GIC backend. For that we embed static key into vgic_global
> and enable the key during vgic initialisation based on what has
> already been exposed by the host GIC driver.
> 
> Signed-off-by: Vladimir Murzin 
> ---
>  arch/arm64/kvm/hyp/switch.c   |   21 +++--
>  include/kvm/arm_vgic.h|4 
>  virt/kvm/arm/vgic/vgic-init.c |4 
>  virt/kvm/arm/vgic/vgic.c  |2 +-
>  4 files changed, 20 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> index 5a84b45..d5c4cc5 100644
> --- a/arch/arm64/kvm/hyp/switch.c
> +++ b/arch/arm64/kvm/hyp/switch.c
> @@ -16,6 +16,8 @@
>   */
>  
>  #include 
> +#include 
> +
>  #include 
>  #include 
>  
> @@ -126,17 +128,13 @@ static void __hyp_text __deactivate_vm(struct kvm_vcpu 
> *vcpu)
>   write_sysreg(0, vttbr_el2);
>  }
>  
> -static hyp_alternate_select(__vgic_call_save_state,
> - __vgic_v2_save_state, __vgic_v3_save_state,
> - ARM64_HAS_SYSREG_GIC_CPUIF);
> -
> -static hyp_alternate_select(__vgic_call_restore_state,
> - __vgic_v2_restore_state, __vgic_v3_restore_state,
> - ARM64_HAS_SYSREG_GIC_CPUIF);
> -
>  static void __hyp_text __vgic_save_state(struct kvm_vcpu *vcpu)
>  {
> - __vgic_call_save_state()(vcpu);
> + if (static_branch_unlikely(_vgic_global_state.gicv3_cpuif))

It's a bit weird that we use _unlikely for GICv3 (at least if/when GICv3
hardware becomes mainstream), but as we don't have another primitive for
the 'default disabled' case, I suppose that's the best we can do.

> + __vgic_v3_save_state(vcpu);
> + else
> + __vgic_v2_save_state(vcpu);
> +
>   write_sysreg(read_sysreg(hcr_el2) & ~HCR_INT_OVERRIDE, hcr_el2);
>  }
>  
> @@ -149,7 +147,10 @@ static void __hyp_text __vgic_restore_state(struct 
> kvm_vcpu *vcpu)
>   val |= vcpu->arch.irq_lines;
>   write_sysreg(val, hcr_el2);
>  
> - __vgic_call_restore_state()(vcpu);
> + if (static_branch_unlikely(_vgic_global_state.gicv3_cpuif))
> + __vgic_v3_restore_state(vcpu);
> + else
> + __vgic_v2_restore_state(vcpu);
>  }
>  
>  static bool __hyp_text __true_value(void)
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index 19b698e..994665a 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -23,6 +23,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #define VGIC_V3_MAX_CPUS 255
>  #define VGIC_V2_MAX_CPUS 8
> @@ -63,6 +64,9 @@ struct vgic_global {
>  
>   /* Only needed for the legacy KVM_CREATE_IRQCHIP */
>   boolcan_emulate_gicv2;
> +
> + /* GIC system register CPU interface */
> + struct static_key_false gicv3_cpuif;

Documentation/static-keys.txt says that we are not supposed to use
struct static_key_false directly.  This will obviously work quite
nicely, but we could consider adding a pair of
DECLARE_STATIC_KEY_TRUE/FALSE macros that don't have the assignments,
but obviously this will need an ack from other maintainers.

Thoughts?


>  };
>  
>  extern struct vgic_global kvm_vgic_global_state;
> diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c
> index 83777c1..14d6718 100644
> --- a/virt/kvm/arm/vgic/vgic-init.c
> +++ b/virt/kvm/arm/vgic/vgic-init.c
> @@ -405,6 +405,10 @@ int kvm_vgic_hyp_init(void)
>   break;
>   case GIC_V3:
>   ret = vgic_v3_probe(gic_kvm_info);
> + if (!ret) {
> + 
> static_branch_enable(_vgic_global_state.gicv3_cpuif);
> + kvm_info("GIC system register CPU interface\n");

nit: add enabled to the info message?

> + }
>   break;
>   default:
>   ret = -ENODEV;
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index e83b7fe..8a529a7 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -29,7 +29,7 @@
>  #define DEBUG_SPINLOCK_BUG_ON(p)
>  #endif
>  
> -struct vgic_global __section(.hyp.text) kvm_vgic_global_state;
> +struct vgic_global __section(.hyp.text) kvm_vgic_global_state = 
> {.gicv3_cpuif = STATIC_KEY_FALSE_INIT,};
>  
>  /*
>   * Locking order is always:
> -- 
> 1.7.9.5
> 

Overall this looks