Re: [kvm-unit-tests PATCH] arm/arm64: introduce is_aarch32

2016-11-16 Thread Wei Huang 


On 11/16/2016 04:02 PM, Christopher Covington wrote:
> On 11/16/2016 12:46 PM, Marc Zyngier wrote:
>> On 16/11/16 14:38, Andrew Jones wrote:
>>> ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
>>> function allows unit tests to make the distinction.
>>
>> Hi Drew,
>>
>> Overall, having to find out about the architecture is a bad idea most of
>> the time. We have feature registers for most things, and it definitely
>> makes more sense to check for those than trying to cast a wider net.
>>
>>>
>>> Signed-off-by: Andrew Jones 
>>>
>>> ---
>>> I'm actually unsure if there's a feature bit or not that I could
>>> probe instead. It'd be nice if somebody can confirm. Thanks, drew
> 
> I'd be happy to settle with the hard-coded CPU list.
> 
> But if you're curious about alternatives, I've taken a look through some
> documentation. ID_ISAR0.coproc describes whether mrrc is available but
> I think it is generally available on v7 and above. I think ID_ISAR5 will
> be zero on v7 and nonzero on v8-A32. But PMCR.LC seems like the best bit
> to check.
> 
>>> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
>>> index 84d5c7ce752b..b602e1fbbc2d 100644
>>> --- a/lib/arm64/asm/processor.h
>>> +++ b/lib/arm64/asm/processor.h
>>> @@ -81,5 +81,10 @@ DEFINE_GET_SYSREG32(mpidr)
>>>  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
>>> sp_usr);
>>>  extern bool is_user(void);
>>>  
>>> +static inline bool is_aarch32(void)
>>> +{
>>> +   return false;
>>> +}
>>> +
>>>  #endif /* !__ASSEMBLY__ */
>>>  #endif /* _ASMARM64_PROCESSOR_H_ */
>>>
>>
>> So the real question is: what are you trying to check for?
> 
> The question is "how many bits wide is pmccntr?" I think we
> can test whether writing PMCR.LC = 1 sticks. Based on the
> documentation, it seems to me like it wouldn't for v7 and
> would for v8-A32.
> 
> uint8_t size_pmccntr(void) {
>   uint32_t pmcr = get_pmcr();
>   if (pmcr & PMU_PMCR_LC_MASK)
> return 64;
>   set_pmcr(pmcr | (1 << PMU_PMCR_LC_SHIFT));
>   if (get_pmcr() & PMU_PMCR_LC_MASK)
> return 64;
>   return 32;
> }

This might actually be the solution if we can't find a more reliable
detection approach. I briefly tested it and it seemed to work.

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

Re: [kvm-unit-tests PATCH] arm/arm64: introduce is_aarch32

2016-11-16 Thread Christopher Covington 
On 11/16/2016 12:46 PM, Marc Zyngier wrote:
> On 16/11/16 14:38, Andrew Jones wrote:
>> ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
>> function allows unit tests to make the distinction.
> 
> Hi Drew,
> 
> Overall, having to find out about the architecture is a bad idea most of
> the time. We have feature registers for most things, and it definitely
> makes more sense to check for those than trying to cast a wider net.
> 
>>
>> Signed-off-by: Andrew Jones 
>>
>> ---
>> I'm actually unsure if there's a feature bit or not that I could
>> probe instead. It'd be nice if somebody can confirm. Thanks, drew

I'd be happy to settle with the hard-coded CPU list.

But if you're curious about alternatives, I've taken a look through some
documentation. ID_ISAR0.coproc describes whether mrrc is available but
I think it is generally available on v7 and above. I think ID_ISAR5 will
be zero on v7 and nonzero on v8-A32. But PMCR.LC seems like the best bit
to check.

>> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
>> index 84d5c7ce752b..b602e1fbbc2d 100644
>> --- a/lib/arm64/asm/processor.h
>> +++ b/lib/arm64/asm/processor.h
>> @@ -81,5 +81,10 @@ DEFINE_GET_SYSREG32(mpidr)
>>  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
>> sp_usr);
>>  extern bool is_user(void);
>>  
>> +static inline bool is_aarch32(void)
>> +{
>> +return false;
>> +}
>> +
>>  #endif /* !__ASSEMBLY__ */
>>  #endif /* _ASMARM64_PROCESSOR_H_ */
>>
> 
> So the real question is: what are you trying to check for?

The question is "how many bits wide is pmccntr?" I think we
can test whether writing PMCR.LC = 1 sticks. Based on the
documentation, it seems to me like it wouldn't for v7 and
would for v8-A32.

uint8_t size_pmccntr(void) {
  uint32_t pmcr = get_pmcr();
  if (pmcr & PMU_PMCR_LC_MASK)
return 64;
  set_pmcr(pmcr | (1 << PMU_PMCR_LC_SHIFT));
  if (get_pmcr() & PMU_PMCR_LC_MASK)
return 64;
  return 32;
}

Thanks,
Cov

-- 
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] arm/arm64: KVM: VGIC: limit ITARGETSR bits to number of VCPUs

2016-11-16 Thread Christoffer Dall 
On Wed, Nov 16, 2016 at 05:57:16PM +, Andre Przywara wrote:
> The GICv2 spec says in section 4.3.12 that a "CPU targets field bit that
> corresponds to an unimplemented CPU interface is RAZ/WI."
> Currently we allow the guest to write any value in there and it can
> read that back.
> Mask the written value with the proper CPU mask to be spec compliant.
> 
> Signed-off-by: Andre Przywara 

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

Re: [PATCH v8 1/7] arm/arm64: vgic: Implement support for userspace access

2016-11-16 Thread Christoffer Dall 
On Fri, Nov 04, 2016 at 04:43:27PM +0530, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K 
> 
> Read and write of some registers like ISPENDR and ICPENDR
> from userspace requires special handling when compared to
> guest access for these registers.
> 
> Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> for handling of ISPENDR, ICPENDR registers handling.
> 
> Add infrastructure to support guest and userspace read
> and write for the required registers
> Also moved vgic_uaccess from vgic-mmio-v2.c to vgic-mmio.c
> 
> Signed-off-by: Vijaya Kumar K 
> ---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c | 25 --
>  virt/kvm/arm/vgic/vgic-mmio-v3.c | 98 
> 
>  virt/kvm/arm/vgic/vgic-mmio.c| 78 
>  virt/kvm/arm/vgic/vgic-mmio.h| 19 
>  4 files changed, 169 insertions(+), 51 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> index b44b359..0b32f40 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> @@ -406,31 +406,6 @@ int vgic_v2_has_attr_regs(struct kvm_device *dev, struct 
> kvm_device_attr *attr)
>   return -ENXIO;
>  }
>  
> -/*
> - * When userland tries to access the VGIC register handlers, we need to
> - * create a usable struct vgic_io_device to be passed to the handlers and we
> - * have to set up a buffer similar to what would have happened if a guest 
> MMIO
> - * access occurred, including doing endian conversions on BE systems.
> - */
> -static int vgic_uaccess(struct kvm_vcpu *vcpu, struct vgic_io_device *dev,
> - bool is_write, int offset, u32 *val)
> -{
> - unsigned int len = 4;
> - u8 buf[4];
> - int ret;
> -
> - if (is_write) {
> - vgic_data_host_to_mmio_bus(buf, len, *val);
> - ret = kvm_io_gic_ops.write(vcpu, >dev, offset, len, buf);
> - } else {
> - ret = kvm_io_gic_ops.read(vcpu, >dev, offset, len, buf);
> - if (!ret)
> - *val = vgic_data_mmio_bus_to_host(buf, len);
> - }
> -
> - return ret;
> -}
> -
>  int vgic_v2_cpuif_uaccess(struct kvm_vcpu *vcpu, bool is_write,
> int offset, u32 *val)
>  {
> diff --git a/virt/kvm/arm/vgic/vgic-mmio-v3.c 
> b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> index 0d3c76a..ce2708d 100644
> --- a/virt/kvm/arm/vgic/vgic-mmio-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-mmio-v3.c
> @@ -209,6 +209,62 @@ static unsigned long vgic_mmio_read_v3_idregs(struct 
> kvm_vcpu *vcpu,
>   return 0;
>  }
>  
> +static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> +   gpa_t addr, unsigned int len)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + u32 value = 0;
> + int i;
> +
> + /*
> +  * A level triggerred interrupt pending state is latched in both
> +  * "soft_pending" and "line_level" variables. Userspace will save
> +  * and restore soft_pending and line_level separately.
> +  * Refer to Documentation/virtual/kvm/devices/arm-vgic-v3.txt
> +  * handling of ISPENDR and ICPENDR.
> +  */
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + if (irq->config == VGIC_CONFIG_LEVEL && irq->soft_pending)
> + value |= (1U << i);
> + if (irq->config == VGIC_CONFIG_EDGE && irq->pending)
> + value |= (1U << i);
> +
> + vgic_put_irq(vcpu->kvm, irq);
> + }
> +
> + return value;
> +}
> +
> +static void vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
> +   gpa_t addr, unsigned int len,
> +   unsigned long val)
> +{
> + u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> + int i;
> +
> + for (i = 0; i < len * 8; i++) {
> + struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> +
> + spin_lock(>irq_lock);
> + if (test_bit(i, )) {
> + irq->pending = true;
> + irq->soft_pending = true;

In the vgic_mmio_write_spending function we only set the soft_pending
state to true if the interrupt is a level-triggered interrupt.

Should we check if that's the case here as well before setting the
soft_pending state?

Otherwise, this patch looks good.

Thanks,
-Christoffer

> + vgic_queue_irq_unlock(vcpu->kvm, irq);
> + } else {
> + irq->soft_pending = false;
> + if (irq->config == VGIC_CONFIG_EDGE ||
> + (irq->config == VGIC_CONFIG_LEVEL &&
> + !irq->line_level))
> + irq->pending = false;
> + spin_unlock(>irq_lock);
> + }
> +
> +

Re: [PATCH v8 2/7] arm/arm64: vgic: Add distributor and redistributor access

2016-11-16 Thread Christoffer Dall 
On Fri, Nov 04, 2016 at 04:43:28PM +0530, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K 
> 
> VGICv3 Distributor and Redistributor registers are accessed using
> KVM_DEV_ARM_VGIC_GRP_DIST_REGS and KVM_DEV_ARM_VGIC_GRP_DIST_REGS

DIST_REGS and REDIST_REGS ?

> with KVM_SET_DEVICE_ATTR and KVM_GET_DEVICE_ATTR ioctls.
> These registers are accessed as 32-bit and cpu mpidr
> value passed along with register offset is used to identify the
> cpu for redistributor registers access.
> 
> The version of VGIC v3 specification is define here
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html

I think you should just point to the Documentation/... path in the
kernel now when it's merged.

> 
> Signed-off-by: Vijaya Kumar K 
> ---
>  arch/arm64/include/uapi/asm/kvm.h   |   4 +
>  virt/kvm/arm/vgic/vgic-kvm-device.c | 149 
> +---
>  virt/kvm/arm/vgic/vgic-mmio-v2.c|  16 +---
>  virt/kvm/arm/vgic/vgic-mmio-v3.c|  72 +
>  virt/kvm/arm/vgic/vgic-mmio.c   |  22 ++
>  virt/kvm/arm/vgic/vgic-mmio.h   |   4 +
>  virt/kvm/arm/vgic/vgic.h|  33 
>  7 files changed, 276 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/arm64/include/uapi/asm/kvm.h 
> b/arch/arm64/include/uapi/asm/kvm.h
> index 3051f86..56dc08d 100644
> --- a/arch/arm64/include/uapi/asm/kvm.h
> +++ b/arch/arm64/include/uapi/asm/kvm.h
> @@ -201,10 +201,14 @@ struct kvm_arch_memory_slot {
>  #define KVM_DEV_ARM_VGIC_GRP_CPU_REGS2
>  #define   KVM_DEV_ARM_VGIC_CPUID_SHIFT   32
>  #define   KVM_DEV_ARM_VGIC_CPUID_MASK(0xffULL << 
> KVM_DEV_ARM_VGIC_CPUID_SHIFT)
> +#define   KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT 32
> +#define   KVM_DEV_ARM_VGIC_V3_MPIDR_MASK \
> + (0xULL << KVM_DEV_ARM_VGIC_V3_MPIDR_SHIFT)
>  #define   KVM_DEV_ARM_VGIC_OFFSET_SHIFT  0
>  #define   KVM_DEV_ARM_VGIC_OFFSET_MASK   (0xULL << 
> KVM_DEV_ARM_VGIC_OFFSET_SHIFT)
>  #define KVM_DEV_ARM_VGIC_GRP_NR_IRQS 3
>  #define KVM_DEV_ARM_VGIC_GRP_CTRL4
> +#define KVM_DEV_ARM_VGIC_GRP_REDIST_REGS 5
>  #define   KVM_DEV_ARM_VGIC_CTRL_INIT 0
>  
>  /* Device Control API on vcpu fd */
> diff --git a/virt/kvm/arm/vgic/vgic-kvm-device.c 
> b/virt/kvm/arm/vgic/vgic-kvm-device.c
> index ce1f4ed..6c7d30c 100644
> --- a/virt/kvm/arm/vgic/vgic-kvm-device.c
> +++ b/virt/kvm/arm/vgic/vgic-kvm-device.c
> @@ -237,7 +237,7 @@ struct vgic_reg_attr {
>   gpa_t addr;
>  };
>  
> -static int parse_vgic_v2_attr(struct kvm_device *dev,
> +static int vgic_v2_parse_attr(struct kvm_device *dev,
> struct kvm_device_attr *attr,
> struct vgic_reg_attr *reg_attr)
>  {
> @@ -294,14 +294,14 @@ static bool lock_all_vcpus(struct kvm *kvm)
>  }
>  
>  /**
> - * vgic_attr_regs_access_v2 - allows user space to access VGIC v2 state
> + * vgic_v2_attr_regs_access - allows user space to access VGIC v2 state
>   *
>   * @dev:  kvm device handle
>   * @attr: kvm device attribute
>   * @reg:  address the value is read or written
>   * @is_write: true if userspace is writing a register
>   */
> -static int vgic_attr_regs_access_v2(struct kvm_device *dev,
> +static int vgic_v2_attr_regs_access(struct kvm_device *dev,
>   struct kvm_device_attr *attr,
>   u32 *reg, bool is_write)
>  {
> @@ -310,7 +310,7 @@ static int vgic_attr_regs_access_v2(struct kvm_device 
> *dev,
>   struct kvm_vcpu *vcpu;
>   int ret;
>  
> - ret = parse_vgic_v2_attr(dev, attr, _attr);
> + ret = vgic_v2_parse_attr(dev, attr, _attr);
>   if (ret)
>   return ret;
>  
> @@ -319,9 +319,10 @@ static int vgic_attr_regs_access_v2(struct kvm_device 
> *dev,
>  
>   mutex_lock(>kvm->lock);
>  
> - ret = vgic_init(dev->kvm);
> - if (ret)
> + if (unlikely(!vgic_initialized(dev->kvm))) {
> + ret = -EBUSY;
>   goto out;
> + }

eh, GICv2 should still support lazy init, shouldn't it?  Am I
misunderstanding this change?

>  
>   if (!lock_all_vcpus(dev->kvm)) {
>   ret = -EBUSY;
> @@ -364,7 +365,7 @@ static int vgic_v2_set_attr(struct kvm_device *dev,
>   if (get_user(reg, uaddr))
>   return -EFAULT;
>  
> - return vgic_attr_regs_access_v2(dev, attr, , true);
> + return vgic_v2_attr_regs_access(dev, attr, , true);
>   }
>   }
>  
> @@ -386,7 +387,7 @@ static int vgic_v2_get_attr(struct kvm_device *dev,
>   u32 __user *uaddr = (u32 __user *)(long)attr->addr;
>   u32 reg = 0;
>  
> - ret = vgic_attr_regs_access_v2(dev, attr, , false);
> + ret = vgic_v2_attr_regs_access(dev, attr, , false);
>   if (ret)
>   return ret;
>   return put_user(reg, uaddr);
> @@ -430,16 +431,141 @@ struct

Re: [PATCH v3 0/2] arm64: Support systems without FP/ASIMD

2016-11-16 Thread Catalin Marinas 
On Tue, Nov 08, 2016 at 01:56:19PM +, Suzuki K. Poulose wrote:
> This series adds supports to the kernel and KVM hyp to handle
> systems without FP/ASIMD properly. At the moment the kernel
> doesn't check if the FP unit is available before accessing
> the registers (e.g during context switch). Also for KVM,
> we trap the FP/ASIMD accesses and handle it by injecting an
> undefined instruction into the VM on systems without FP.
> 
> Tested on a FVP_Base-AEM-v8A model by disabling VFP on at
> least one CPU ( -C clusterX.cpuY.vfp-present=0 ).
> 
> Changes since V2:
>  - Dropped cleanup patch for arm64/crypto/aes-ce-ccm-glue.c
>  - Removed static_key check from cpus_have_cap. All users with
>constant caps should use the new API to make use of static_keys.
>  - Removed a dedicated static_key used in irqchip-gic-v3.c for
>Cavium errata with the new API.
> 
> Applies on v4.9-rc4 + [1] (which is pushed for rc5)
> 
> [1] http://marc.info/?l=linux-arm-kernel=147819889813214=2

I queued the patches for 4.10 with a slight modification as I haven't
cherry-picked the patch above. I'll push the for-next/core branch out
once I've done some testing.

Thanks.

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

[PATCH v2] arm/arm64: KVM: VGIC: limit ITARGETSR bits to number of VCPUs

2016-11-16 Thread Andre Przywara 
The GICv2 spec says in section 4.3.12 that a "CPU targets field bit that
corresponds to an unimplemented CPU interface is RAZ/WI."
Currently we allow the guest to write any value in there and it can
read that back.
Mask the written value with the proper CPU mask to be spec compliant.

Signed-off-by: Andre Przywara 
---
Changes v1 .. v2:
- use GENMASK() instead of open-coding mask
- drop explicit 0xff masking, since cpu_mask is stronger anyway

 virt/kvm/arm/vgic/vgic-mmio-v2.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
index b44b359..78e34bc 100644
--- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
+++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
@@ -129,6 +129,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
   unsigned long val)
 {
u32 intid = VGIC_ADDR_TO_INTID(addr, 8);
+   u8 cpu_mask = GENMASK(atomic_read(>kvm->online_vcpus) - 1, 0);
int i;
 
/* GICD_ITARGETSR[0-7] are read-only */
@@ -141,7 +142,7 @@ static void vgic_mmio_write_target(struct kvm_vcpu *vcpu,
 
spin_lock(>irq_lock);
 
-   irq->targets = (val >> (i * 8)) & 0xff;
+   irq->targets = (val >> (i * 8)) & cpu_mask;
target = irq->targets ? __ffs(irq->targets) : 0;
irq->target_vcpu = kvm_get_vcpu(vcpu->kvm, target);
 
-- 
2.9.0

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

Re: [kvm-unit-tests PATCH] arm/arm64: introduce is_aarch32

2016-11-16 Thread Marc Zyngier 
On 16/11/16 14:38, Andrew Jones wrote:
> ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
> function allows unit tests to make the distinction.

Hi Drew,

Overall, having to find out about the architecture is a bad idea most of
the time. We have feature registers for most things, and it definitely
makes more sense to check for those than trying to cast a wider net.

> 
> Signed-off-by: Andrew Jones 
> 
> ---
> I'm actually unsure if there's a feature bit or not that I could
> probe instead. It'd be nice if somebody can confirm. Thanks, drew
> 
>  lib/arm/asm/processor.h   | 20 
>  lib/arm64/asm/processor.h |  5 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
> index f25e7eee3666..223e54beb72a 100644
> --- a/lib/arm/asm/processor.h
> +++ b/lib/arm/asm/processor.h
> @@ -5,6 +5,7 @@
>   *
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
> +#include 
>  #include 
>  
>  enum vector {
> @@ -46,4 +47,23 @@ static inline unsigned int get_mpidr(void)
>  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
> sp_usr);
>  extern bool is_user(void);
>  
> +/*
> + * ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
> + * function allows unit tests to make the distinction.
> + */
> +static inline bool is_aarch32(void)

I'm really worried that you're not considering v7-A as AArch32.

> +{
> + /*
> +  * XXX: Unfortunately there's no feature bit we can probe for
> +  * this, so we do a hacky check for the processor type not being
> +  * a Cortex-A15, which is the only v7 type we currently use.
> +  */
> + unsigned long midr;
> +
> + asm volatile("MRC p15, 0, %0, c0, c0, 0" : "=r" (midr));
> + midr &= GENMASK(31, 24) | GENMASK(15, 4);
> +
> + return midr != ((0x41 << 24) | (0xc0f << 4));

And what about A7, A12, A17? They all are v7's.

> +}
> +
>  #endif /* _ASMARM_PROCESSOR_H_ */
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 84d5c7ce752b..b602e1fbbc2d 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -81,5 +81,10 @@ DEFINE_GET_SYSREG32(mpidr)
>  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
> sp_usr);
>  extern bool is_user(void);
>  
> +static inline bool is_aarch32(void)
> +{
> + return false;
> +}
> +
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASMARM64_PROCESSOR_H_ */
> 

So the real question is: what are you trying to check for?

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: [kvm-unit-tests PATCH] arm/arm64: introduce is_aarch32

2016-11-16 Thread Andre Przywara 
Hi Drew,

On 16/11/16 14:38, Andrew Jones wrote:
> ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
> function allows unit tests to make the distinction.

So the big question here is why you would like to know this?
If it is for a certain feature, you should check for that instead.

> Signed-off-by: Andrew Jones 
> 
> ---
> I'm actually unsure if there's a feature bit or not that I could
> probe instead. It'd be nice if somebody can confirm. Thanks, drew

Probing for a Cortex-A15 is definitely not the right thing. I think
under KVM you'd see Cortex-A7 and Cortex-A12/A17 if you run on one of those.

So there does not seem to be a dedicated feature bit, however you could
look for some ISA features that ARMv8 AA32 gained over ARMv7.
So one thing you could check for is ID_ISAR5[3:0] (SEVL).
The ARMv7 ARM says that this whole register is "Reserved, UNK", which
technically doesn't give you a lot to check for. But I guess it just
reads as zero on ARMv7 ;-)
The ARMv8 ARM on the other hands puts ARMv8 features in there, among the
new crypto instructions (which are optional), there is the SEVL
instruction, which the architecture mandates:
"In ARMv8-A the only permitted value is 0001."

So I guess the closest you could come to is to check for the lowest 4
bits of ID_ISAR5 to read as '0001'.

HTH,
Andre.

>  lib/arm/asm/processor.h   | 20 
>  lib/arm64/asm/processor.h |  5 +
>  2 files changed, 25 insertions(+)
> 
> diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
> index f25e7eee3666..223e54beb72a 100644
> --- a/lib/arm/asm/processor.h
> +++ b/lib/arm/asm/processor.h
> @@ -5,6 +5,7 @@
>   *
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
> +#include 
>  #include 
>  
>  enum vector {
> @@ -46,4 +47,23 @@ static inline unsigned int get_mpidr(void)
>  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
> sp_usr);
>  extern bool is_user(void);
>  
> +/*
> + * ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
> + * function allows unit tests to make the distinction.
> + */
> +static inline bool is_aarch32(void)
> +{
> + /*
> +  * XXX: Unfortunately there's no feature bit we can probe for
> +  * this, so we do a hacky check for the processor type not being
> +  * a Cortex-A15, which is the only v7 type we currently use.
> +  */
> + unsigned long midr;
> +
> + asm volatile("MRC p15, 0, %0, c0, c0, 0" : "=r" (midr));
> + midr &= GENMASK(31, 24) | GENMASK(15, 4);
> +
> + return midr != ((0x41 << 24) | (0xc0f << 4));
> +}
> +
>  #endif /* _ASMARM_PROCESSOR_H_ */
> diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
> index 84d5c7ce752b..b602e1fbbc2d 100644
> --- a/lib/arm64/asm/processor.h
> +++ b/lib/arm64/asm/processor.h
> @@ -81,5 +81,10 @@ DEFINE_GET_SYSREG32(mpidr)
>  extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
> sp_usr);
>  extern bool is_user(void);
>  
> +static inline bool is_aarch32(void)
> +{
> + return false;
> +}
> +
>  #endif /* !__ASSEMBLY__ */
>  #endif /* _ASMARM64_PROCESSOR_H_ */
> 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v3 1/1] KVM: ARM64: Fix the issues when guest PMCCFILTR is configured

2016-11-16 Thread Wei Huang 
KVM calls kvm_pmu_set_counter_event_type() when PMCCFILTR is configured.
But this function can't deals with PMCCFILTR correctly because the evtCount
bits of PMCCFILTR, which is reserved 0, conflits with the SW_INCR event
type of other PMXEVTYPER registers. To fix it, when eventsel == 0, this
function shouldn't return immediately; instead it needs to check further
if select_idx is ARMV8_PMU_CYCLE_IDX.

Another issue is that KVM shouldn't copy the eventsel bits of PMCCFILTER
blindly to attr.config. Instead it ought to convert the request to the
"cpu cycle" event type (i.e. 0x11).

To support this patch and to prevent duplicated definitions, a limited
set of ARMv8 perf event types were relocated from perf_event.c to
asm/perf_event.h.

Signed-off-by: Wei Huang 
---
 arch/arm64/include/asm/perf_event.h | 10 +-
 arch/arm64/kernel/perf_event.c  | 10 +-
 virt/kvm/arm/pmu.c  |  8 +---
 3 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/include/asm/perf_event.h 
b/arch/arm64/include/asm/perf_event.h
index 2065f46..38b6a2b 100644
--- a/arch/arm64/include/asm/perf_event.h
+++ b/arch/arm64/include/asm/perf_event.h
@@ -46,7 +46,15 @@
 #defineARMV8_PMU_EVTYPE_MASK   0xc800  /* Mask for writable 
bits */
 #defineARMV8_PMU_EVTYPE_EVENT  0x  /* Mask for EVENT bits 
*/
 
-#define ARMV8_PMU_EVTYPE_EVENT_SW_INCR 0   /* Software increment event */
+/*
+ * PMUv3 event types: required events
+ */
+#define ARMV8_PMUV3_PERFCTR_SW_INCR0x00
+#define ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL   0x03
+#define ARMV8_PMUV3_PERFCTR_L1D_CACHE  0x04
+#define ARMV8_PMUV3_PERFCTR_BR_MIS_PRED0x10
+#define ARMV8_PMUV3_PERFCTR_CPU_CYCLES 0x11
+#define ARMV8_PMUV3_PERFCTR_BR_PRED0x12
 
 /*
  * Event filters for PMUv3
diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
index a9310a6..57ae9d9 100644
--- a/arch/arm64/kernel/perf_event.c
+++ b/arch/arm64/kernel/perf_event.c
@@ -31,17 +31,9 @@
 
 /*
  * ARMv8 PMUv3 Performance Events handling code.
- * Common event types.
+ * Common event types (some are defined in asm/perf_event.h).
  */
 
-/* Required events. */
-#define ARMV8_PMUV3_PERFCTR_SW_INCR0x00
-#define ARMV8_PMUV3_PERFCTR_L1D_CACHE_REFILL   0x03
-#define ARMV8_PMUV3_PERFCTR_L1D_CACHE  0x04
-#define ARMV8_PMUV3_PERFCTR_BR_MIS_PRED0x10
-#define ARMV8_PMUV3_PERFCTR_CPU_CYCLES 0x11
-#define ARMV8_PMUV3_PERFCTR_BR_PRED0x12
-
 /* At least one of the following is required. */
 #define ARMV8_PMUV3_PERFCTR_INST_RETIRED   0x08
 #define ARMV8_PMUV3_PERFCTR_INST_SPEC  0x1B
diff --git a/virt/kvm/arm/pmu.c b/virt/kvm/arm/pmu.c
index 6e9c40e..69ccce3 100644
--- a/virt/kvm/arm/pmu.c
+++ b/virt/kvm/arm/pmu.c
@@ -305,7 +305,7 @@ void kvm_pmu_software_increment(struct kvm_vcpu *vcpu, u64 
val)
continue;
type = vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + i)
   & ARMV8_PMU_EVTYPE_EVENT;
-   if ((type == ARMV8_PMU_EVTYPE_EVENT_SW_INCR)
+   if ((type == ARMV8_PMUV3_PERFCTR_SW_INCR)
&& (enable & BIT(i))) {
reg = vcpu_sys_reg(vcpu, PMEVCNTR0_EL0 + i) + 1;
reg = lower_32_bits(reg);
@@ -379,7 +379,8 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, 
u64 data,
eventsel = data & ARMV8_PMU_EVTYPE_EVENT;
 
/* Software increment event does't need to be backed by a perf event */
-   if (eventsel == ARMV8_PMU_EVTYPE_EVENT_SW_INCR)
+   if (eventsel == ARMV8_PMUV3_PERFCTR_SW_INCR &&
+   select_idx != ARMV8_PMU_CYCLE_IDX)
return;
 
memset(, 0, sizeof(struct perf_event_attr));
@@ -391,7 +392,8 @@ void kvm_pmu_set_counter_event_type(struct kvm_vcpu *vcpu, 
u64 data,
attr.exclude_kernel = data & ARMV8_PMU_EXCLUDE_EL1 ? 1 : 0;
attr.exclude_hv = 1; /* Don't count EL2 events */
attr.exclude_host = 1; /* Don't count host events */
-   attr.config = eventsel;
+   attr.config = (select_idx == ARMV8_PMU_CYCLE_IDX) ?
+   ARMV8_PMUV3_PERFCTR_CPU_CYCLES : eventsel;
 
counter = kvm_pmu_get_counter_value(vcpu, select_idx);
/* The initial sample period (overflow count) of an event. */
-- 
2.7.4

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

Re: [Qemu-devel] [kvm-unit-tests PATCH v8 2/3] arm: pmu: Check cycle count increases

2016-11-16 Thread Christopher Covington 
On 11/16/2016 11:25 AM, Andrew Jones wrote:
> On Wed, Nov 16, 2016 at 11:08:42AM -0500, Christopher Covington wrote:
>> On 11/16/2016 08:01 AM, Andrew Jones wrote:
>>> On Tue, Nov 15, 2016 at 04:50:53PM -0600, Wei Huang wrote:


 On 11/14/2016 09:12 AM, Christopher Covington wrote:
> Hi Drew, Wei,
>
> On 11/14/2016 05:05 AM, Andrew Jones wrote:
>> On Fri, Nov 11, 2016 at 01:55:49PM -0600, Wei Huang wrote:
>>>
>>>
>>> On 11/11/2016 01:43 AM, Andrew Jones wrote:
 On Tue, Nov 08, 2016 at 12:17:14PM -0600, Wei Huang wrote:
> From: Christopher Covington 
>
> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
> even for the smallest delta of two subsequent reads.
>
> Signed-off-by: Christopher Covington 
> Signed-off-by: Wei Huang 
> ---
>  arm/pmu.c | 98 
> +++
>  1 file changed, 98 insertions(+)
>
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 0b29088..d5e3ac3 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -14,6 +14,7 @@
>   */
>  #include "libcflat.h"
>  
> +#define PMU_PMCR_E (1 << 0)
>  #define PMU_PMCR_N_SHIFT   11
>  #define PMU_PMCR_N_MASK0x1f
>  #define PMU_PMCR_ID_SHIFT  16
> @@ -21,6 +22,10 @@
>  #define PMU_PMCR_IMP_SHIFT 24
>  #define PMU_PMCR_IMP_MASK  0xff
>  
> +#define PMU_CYCLE_IDX  31
> +
> +#define NR_SAMPLES 10
> +
>  #if defined(__arm__)
>  static inline uint32_t pmcr_read(void)
>  {
> @@ -29,6 +34,47 @@ static inline uint32_t pmcr_read(void)
>   asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>   return ret;
>  }
> +
> +static inline void pmcr_write(uint32_t value)
> +{
> + asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (value));
> +}
> +
> +static inline void pmselr_write(uint32_t value)
> +{
> + asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));
> +}
> +
> +static inline void pmxevtyper_write(uint32_t value)
> +{
> + asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
> +}
> +
> +/*
> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, 
> returning 64
> + * bits doesn't seem worth the trouble when differential usage of 
> the result is
> + * expected (with differences that can easily fit in 32 bits). So 
> just return
> + * the lower 32 bits of the cycle count in AArch32.

 Like I said in the last review, I'd rather we not do this. We should
 return the full value and then the test case should confirm the upper
 32 bits are zero.
>>>
>>> Unless I miss something in ARM documentation, ARMv7 PMCCNTR is a 32-bit
>>> register. We can force it to a more coarse-grained cycle counter with
>>> PMCR.D bit=1 (see below). But it is still not a 64-bit register.
>
> AArch32 System Register Descriptions
> Performance Monitors registers
> PMCCNTR, Performance Monitors Cycle Count Register
>
> To access the PMCCNTR when accessing as a 32-bit register:
> MRC p15,0,,c9,c13,0 ; Read PMCCNTR[31:0] into Rt
> MCR p15,0,,c9,c13,0 ; Write Rt to PMCCNTR[31:0]. PMCCNTR[63:32] are 
> unchanged
>
> To access the PMCCNTR when accessing as a 64-bit register:
> MRRC p15,0,,,c9 ; Read PMCCNTR[31:0] into Rt and PMCCNTR[63:32] 
> into Rt2
> MCRR p15,0,,,c9 ; Write Rt to PMCCNTR[31:0] and Rt2 to 
> PMCCNTR[63:32]
>

 Thanks. I did some research based on your info and came back with the
 following proposals (Cov, correct me if I am wrong):

 By comparing A57 TRM (page 394 in [1]) with A15 TRM (page 273 in [2]), I
 think this 64-bit cycle register is only available when running under
 aarch32 compatibility mode on ARMv8 because it is not specified in A15
 TRM.
>>
>> That interpretation sounds really strange to me. My recollection is that the
>> cycle counter was available as a 64 bit register in ARMv7 as well. I would
>> expect the Cortex TRMs to omit such details. The ARMv7 Architecture Reference
>> Manual is the complete and authoritative source.
> 
> Yes, the v7 ARM ARM is the authoritative source, and it says 32-bit.
> Whereas the v8 ARM ARM wrt to AArch32 mode says it's both 32 and 64.

Just looked it up as well in the good old ARM DDI 0406C.c and you're absolutely
right. Sorry for the bad recollection.

Cov

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc.

Re: [Qemu-devel] [kvm-unit-tests PATCH v8 2/3] arm: pmu: Check cycle count increases

2016-11-16 Thread Andrew Jones 
On Wed, Nov 16, 2016 at 11:08:42AM -0500, Christopher Covington wrote:
> On 11/16/2016 08:01 AM, Andrew Jones wrote:
> > On Tue, Nov 15, 2016 at 04:50:53PM -0600, Wei Huang wrote:
> >>
> >>
> >> On 11/14/2016 09:12 AM, Christopher Covington wrote:
> >>> Hi Drew, Wei,
> >>>
> >>> On 11/14/2016 05:05 AM, Andrew Jones wrote:
>  On Fri, Nov 11, 2016 at 01:55:49PM -0600, Wei Huang wrote:
> >
> >
> > On 11/11/2016 01:43 AM, Andrew Jones wrote:
> >> On Tue, Nov 08, 2016 at 12:17:14PM -0600, Wei Huang wrote:
> >>> From: Christopher Covington 
> >>>
> >>> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
> >>> even for the smallest delta of two subsequent reads.
> >>>
> >>> Signed-off-by: Christopher Covington 
> >>> Signed-off-by: Wei Huang 
> >>> ---
> >>>  arm/pmu.c | 98 
> >>> +++
> >>>  1 file changed, 98 insertions(+)
> >>>
> >>> diff --git a/arm/pmu.c b/arm/pmu.c
> >>> index 0b29088..d5e3ac3 100644
> >>> --- a/arm/pmu.c
> >>> +++ b/arm/pmu.c
> >>> @@ -14,6 +14,7 @@
> >>>   */
> >>>  #include "libcflat.h"
> >>>  
> >>> +#define PMU_PMCR_E (1 << 0)
> >>>  #define PMU_PMCR_N_SHIFT   11
> >>>  #define PMU_PMCR_N_MASK0x1f
> >>>  #define PMU_PMCR_ID_SHIFT  16
> >>> @@ -21,6 +22,10 @@
> >>>  #define PMU_PMCR_IMP_SHIFT 24
> >>>  #define PMU_PMCR_IMP_MASK  0xff
> >>>  
> >>> +#define PMU_CYCLE_IDX  31
> >>> +
> >>> +#define NR_SAMPLES 10
> >>> +
> >>>  #if defined(__arm__)
> >>>  static inline uint32_t pmcr_read(void)
> >>>  {
> >>> @@ -29,6 +34,47 @@ static inline uint32_t pmcr_read(void)
> >>>   asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
> >>>   return ret;
> >>>  }
> >>> +
> >>> +static inline void pmcr_write(uint32_t value)
> >>> +{
> >>> + asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (value));
> >>> +}
> >>> +
> >>> +static inline void pmselr_write(uint32_t value)
> >>> +{
> >>> + asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));
> >>> +}
> >>> +
> >>> +static inline void pmxevtyper_write(uint32_t value)
> >>> +{
> >>> + asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
> >>> +}
> >>> +
> >>> +/*
> >>> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, 
> >>> returning 64
> >>> + * bits doesn't seem worth the trouble when differential usage of 
> >>> the result is
> >>> + * expected (with differences that can easily fit in 32 bits). So 
> >>> just return
> >>> + * the lower 32 bits of the cycle count in AArch32.
> >>
> >> Like I said in the last review, I'd rather we not do this. We should
> >> return the full value and then the test case should confirm the upper
> >> 32 bits are zero.
> >
> > Unless I miss something in ARM documentation, ARMv7 PMCCNTR is a 32-bit
> > register. We can force it to a more coarse-grained cycle counter with
> > PMCR.D bit=1 (see below). But it is still not a 64-bit register.
> >>>
> >>> AArch32 System Register Descriptions
> >>> Performance Monitors registers
> >>> PMCCNTR, Performance Monitors Cycle Count Register
> >>>
> >>> To access the PMCCNTR when accessing as a 32-bit register:
> >>> MRC p15,0,,c9,c13,0 ; Read PMCCNTR[31:0] into Rt
> >>> MCR p15,0,,c9,c13,0 ; Write Rt to PMCCNTR[31:0]. PMCCNTR[63:32] are 
> >>> unchanged
> >>>
> >>> To access the PMCCNTR when accessing as a 64-bit register:
> >>> MRRC p15,0,,,c9 ; Read PMCCNTR[31:0] into Rt and PMCCNTR[63:32] 
> >>> into Rt2
> >>> MCRR p15,0,,,c9 ; Write Rt to PMCCNTR[31:0] and Rt2 to 
> >>> PMCCNTR[63:32]
> >>>
> >>
> >> Thanks. I did some research based on your info and came back with the
> >> following proposals (Cov, correct me if I am wrong):
> >>
> >> By comparing A57 TRM (page 394 in [1]) with A15 TRM (page 273 in [2]), I
> >> think this 64-bit cycle register is only available when running under
> >> aarch32 compatibility mode on ARMv8 because it is not specified in A15
> >> TRM.
> 
> That interpretation sounds really strange to me. My recollection is that the
> cycle counter was available as a 64 bit register in ARMv7 as well. I would
> expect the Cortex TRMs to omit such details. The ARMv7 Architecture Reference
> Manual is the complete and authoritative source.

Yes, the v7 ARM ARM is the authoritative source, and it says 32-bit.
Whereas the v8 ARM ARM wrt to AArch32 mode says it's both 32 and 64.

> 
> >> To further verify it, I tested 32-bit pmu code on QEMU with TCG
> >> mode. The result is: accessing 64-bit PMCCNTR using the following
> >> assembly failed on A15:
> >>
> >>volatile("mrrc p15, 0, %0, %1, c9" : "=r" (lo), "=r" (hi));
> >> or
> >>volatile("mrrc p15, 0, %Q0, %R0, c9" :

Re: [Qemu-devel] [kvm-unit-tests PATCH v8 2/3] arm: pmu: Check cycle count increases

2016-11-16 Thread Christopher Covington 
On 11/16/2016 08:01 AM, Andrew Jones wrote:
> On Tue, Nov 15, 2016 at 04:50:53PM -0600, Wei Huang wrote:
>>
>>
>> On 11/14/2016 09:12 AM, Christopher Covington wrote:
>>> Hi Drew, Wei,
>>>
>>> On 11/14/2016 05:05 AM, Andrew Jones wrote:
 On Fri, Nov 11, 2016 at 01:55:49PM -0600, Wei Huang wrote:
>
>
> On 11/11/2016 01:43 AM, Andrew Jones wrote:
>> On Tue, Nov 08, 2016 at 12:17:14PM -0600, Wei Huang wrote:
>>> From: Christopher Covington 
>>>
>>> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
>>> even for the smallest delta of two subsequent reads.
>>>
>>> Signed-off-by: Christopher Covington 
>>> Signed-off-by: Wei Huang 
>>> ---
>>>  arm/pmu.c | 98 
>>> +++
>>>  1 file changed, 98 insertions(+)
>>>
>>> diff --git a/arm/pmu.c b/arm/pmu.c
>>> index 0b29088..d5e3ac3 100644
>>> --- a/arm/pmu.c
>>> +++ b/arm/pmu.c
>>> @@ -14,6 +14,7 @@
>>>   */
>>>  #include "libcflat.h"
>>>  
>>> +#define PMU_PMCR_E (1 << 0)
>>>  #define PMU_PMCR_N_SHIFT   11
>>>  #define PMU_PMCR_N_MASK0x1f
>>>  #define PMU_PMCR_ID_SHIFT  16
>>> @@ -21,6 +22,10 @@
>>>  #define PMU_PMCR_IMP_SHIFT 24
>>>  #define PMU_PMCR_IMP_MASK  0xff
>>>  
>>> +#define PMU_CYCLE_IDX  31
>>> +
>>> +#define NR_SAMPLES 10
>>> +
>>>  #if defined(__arm__)
>>>  static inline uint32_t pmcr_read(void)
>>>  {
>>> @@ -29,6 +34,47 @@ static inline uint32_t pmcr_read(void)
>>> asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>>> return ret;
>>>  }
>>> +
>>> +static inline void pmcr_write(uint32_t value)
>>> +{
>>> +   asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (value));
>>> +}
>>> +
>>> +static inline void pmselr_write(uint32_t value)
>>> +{
>>> +   asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));
>>> +}
>>> +
>>> +static inline void pmxevtyper_write(uint32_t value)
>>> +{
>>> +   asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
>>> +}
>>> +
>>> +/*
>>> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, 
>>> returning 64
>>> + * bits doesn't seem worth the trouble when differential usage of the 
>>> result is
>>> + * expected (with differences that can easily fit in 32 bits). So just 
>>> return
>>> + * the lower 32 bits of the cycle count in AArch32.
>>
>> Like I said in the last review, I'd rather we not do this. We should
>> return the full value and then the test case should confirm the upper
>> 32 bits are zero.
>
> Unless I miss something in ARM documentation, ARMv7 PMCCNTR is a 32-bit
> register. We can force it to a more coarse-grained cycle counter with
> PMCR.D bit=1 (see below). But it is still not a 64-bit register.
>>>
>>> AArch32 System Register Descriptions
>>> Performance Monitors registers
>>> PMCCNTR, Performance Monitors Cycle Count Register
>>>
>>> To access the PMCCNTR when accessing as a 32-bit register:
>>> MRC p15,0,,c9,c13,0 ; Read PMCCNTR[31:0] into Rt
>>> MCR p15,0,,c9,c13,0 ; Write Rt to PMCCNTR[31:0]. PMCCNTR[63:32] are 
>>> unchanged
>>>
>>> To access the PMCCNTR when accessing as a 64-bit register:
>>> MRRC p15,0,,,c9 ; Read PMCCNTR[31:0] into Rt and PMCCNTR[63:32] 
>>> into Rt2
>>> MCRR p15,0,,,c9 ; Write Rt to PMCCNTR[31:0] and Rt2 to 
>>> PMCCNTR[63:32]
>>>
>>
>> Thanks. I did some research based on your info and came back with the
>> following proposals (Cov, correct me if I am wrong):
>>
>> By comparing A57 TRM (page 394 in [1]) with A15 TRM (page 273 in [2]), I
>> think this 64-bit cycle register is only available when running under
>> aarch32 compatibility mode on ARMv8 because it is not specified in A15
>> TRM.

That interpretation sounds really strange to me. My recollection is that the
cycle counter was available as a 64 bit register in ARMv7 as well. I would
expect the Cortex TRMs to omit such details. The ARMv7 Architecture Reference
Manual is the complete and authoritative source.

>> To further verify it, I tested 32-bit pmu code on QEMU with TCG
>> mode. The result is: accessing 64-bit PMCCNTR using the following
>> assembly failed on A15:
>>
>>volatile("mrrc p15, 0, %0, %1, c9" : "=r" (lo), "=r" (hi));
>> or
>>volatile("mrrc p15, 0, %Q0, %R0, c9" : "=r" (val));

The PMU implementation on QEMU TCG mode is infantile. (I was trying to
write these tests to help guide fixes and enhancements in a
test-driven-development manner.) I would not trust QEMU TCG to behave
properly here. If you want to execute those instructions, is there anything
preventing you from doing it on hardware, or at least the Foundation Model?

>> Given this difference, I think there are two solutions for 64-bit
>>

Re: [PATCH] arm: Add simple arch timer test

2016-11-16 Thread Alexander Graf 



On 16/11/2016 16:54, Andrew Jones wrote:

On Mon, Sep 19, 2016 at 04:52:01PM +0200, Andrew Jones wrote:

On Mon, Sep 19, 2016 at 01:44:40PM +0200, Alexander Graf wrote:

All virtualization capable ARM cores support the ARM architected virtual timer.

This patch adds minimalistic checks whether we can fire a virtual timer event
using them. It does not actually trigger an interrupt yet, as infrastructure
for that is missing. However, it does check whether the timer pin is marked as
pending on the GIC.

Signed-off-by: Alexander Graf 
---
 arm/Makefile.arm64 |   2 +-
 arm/vtimer-test.c  | 122 +
 2 files changed, 123 insertions(+), 1 deletion(-)
 create mode 100644 arm/vtimer-test.c

diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
index 0b0761c..5b3fbe8 100644
--- a/arm/Makefile.arm64
+++ b/arm/Makefile.arm64
@@ -12,7 +12,7 @@ cflatobjs += lib/arm64/processor.o
 cflatobjs += lib/arm64/spinlock.o

 # arm64 specific tests
-tests =
+tests = $(TEST_DIR)/vtimer-test.flat

 include $(TEST_DIR)/Makefile.common

diff --git a/arm/vtimer-test.c b/arm/vtimer-test.c
new file mode 100644
index 000..a3e24ce
--- /dev/null
+++ b/arm/vtimer-test.c
@@ -0,0 +1,122 @@
+/*
+ * Unit tests for the virtual timer on the ARM virt machine
+ *
+ * Copyright (C) 2016, Alexander Graf 
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include 
+#include 
+
+#define VTIMER_CTL_ENABLE  (1 << 0)
+#define VTIMER_CTL_IMASK   (1 << 1)
+#define VTIMER_CTL_ISTATUS (1 << 2)
+
+#define VIRT_GIC_DIST_BASE 0x0800
+#define GIC_DIST_PENDING_SET0x200
+#define GIC_DIST_PENDING_CLEAR  0x280
+#define GIC_DIST_ACTIVE_SET 0x300
+#define GIC_DIST_ACTIVE_CLEAR   0x380
+
+#define VIRT_GIC_CPU_BASE  0x0801


Once I get my arm/gic series in we won't need to hard code
the gic base addresses and these gic register offsets will
also be provided.


+
+#define ARCH_TIMER_VIRT_IRQ   11
+#define ARCH_TIMER_S_EL1_IRQ  13
+#define ARCH_TIMER_NS_EL1_IRQ 14
+#define ARCH_TIMER_NS_EL2_IRQ 10


We can pull these out of the DT.


+
+static void write_vtimer_ctl(u64 val)
+{
+   asm volatile("msr cntv_ctl_el0, %0" : : "r" (val));
+}
+
+static void write_vtimer_cval(u64 val)
+{
+   asm volatile("msr cntv_cval_el0, %0" : : "r" (val));
+}
+
+static u64 read_vtimer_ctl(void)
+{
+   u64 val;
+   asm volatile("mrs %0, cntv_ctl_el0" : "=r" (val));
+   return val;
+}
+
+static u64 read_vtimer_cval(void)
+{
+   u64 val;
+   asm volatile("mrs %0, cntv_cval_el0" : "=r" (val));
+   return val;
+}
+
+static u64 read_vtimer_val(void)
+{
+   u64 val;
+   asm volatile("mrs %0, cntvct_el0" : "=r" (val));
+   return val;
+}
+
+static u64 read_vtimer_freq(void)
+{
+   u64 val;
+   asm volatile("mrs %0, cntfrq_el0" : "=r" (val));
+   return val;
+}
+
+static bool gic_vtimer_pending(void)
+{
+   u32 *pending = (u32*)(VIRT_GIC_DIST_BASE + GIC_DIST_PENDING_SET);
+   return (readl(pending) & (1 << (ARCH_TIMER_VIRT_IRQ + 16)));


nit: don't need the outer ()

I think I'd like to add the PPI(irq) macro we added to QEMU for
the +16's.


+}
+
+static bool test_cval_10msec(void)
+{
+   u64 time_10ms = read_vtimer_freq() / 100;
+   u64 time_1us = time_10ms / 1;
+   u64 before_timer = read_vtimer_val();
+   u64 after_timer, latency;
+
+   /* Program timer to fire in 10ms */
+   write_vtimer_cval(before_timer + time_10ms);
+
+   /* Wait for the timer to fire */
+   while (!(read_vtimer_ctl() & VTIMER_CTL_ISTATUS)) ;


If emulation fails to set the status bit then we'll spin forever.
That's OK if we set the test timeout (arm/unittests.cfg:timeout
for this test appropriately) Or, maybe can add a trial count here
that's sufficiently large?


+
+   /* It fired, check how long it took */
+   after_timer = read_vtimer_val();
+
+   latency = (after_timer - (before_timer + time_10ms));


nit: don't need the outer ()

Shouldn't latency be signed so we can see if the status bit was
set too soon? Or just compare time_10ms with after - before?


+   printf("After timer: 0x%lx\n", after_timer);
+   printf("Expected   : 0x%lx\n", before_timer + time_10ms);
+   printf("Latency: %ld us\n", latency / time_1us);
+
+   return latency < time_10ms;


Does this mean that the threshold for success is 10ms? If so,
then that's not too large?


+}
+
+static void test_vtimer(void)
+{
+   printf("\n=== vtimer with busy loop ===\n");


I guess this is a subtest header, i.e. this file will get
other subtests and headers like this one will divide up
the test output. That's fine, but new, at least for arm tests.
Currently we're dividing subtests with report prefixes. In
this case, instead of the above printf, we'd do

 report_prefix_push("busy-loop");

or whatever the prefix should be. At the end of the function
we'd pop the prefix. I

Re: [PATCH] arm: Add simple arch timer test

2016-11-16 Thread Andrew Jones 
On Mon, Sep 19, 2016 at 04:52:01PM +0200, Andrew Jones wrote:
> On Mon, Sep 19, 2016 at 01:44:40PM +0200, Alexander Graf wrote:
> > All virtualization capable ARM cores support the ARM architected virtual 
> > timer.
> > 
> > This patch adds minimalistic checks whether we can fire a virtual timer 
> > event
> > using them. It does not actually trigger an interrupt yet, as infrastructure
> > for that is missing. However, it does check whether the timer pin is marked 
> > as
> > pending on the GIC.
> > 
> > Signed-off-by: Alexander Graf 
> > ---
> >  arm/Makefile.arm64 |   2 +-
> >  arm/vtimer-test.c  | 122 
> > +
> >  2 files changed, 123 insertions(+), 1 deletion(-)
> >  create mode 100644 arm/vtimer-test.c
> > 
> > diff --git a/arm/Makefile.arm64 b/arm/Makefile.arm64
> > index 0b0761c..5b3fbe8 100644
> > --- a/arm/Makefile.arm64
> > +++ b/arm/Makefile.arm64
> > @@ -12,7 +12,7 @@ cflatobjs += lib/arm64/processor.o
> >  cflatobjs += lib/arm64/spinlock.o
> >  
> >  # arm64 specific tests
> > -tests =
> > +tests = $(TEST_DIR)/vtimer-test.flat
> >  
> >  include $(TEST_DIR)/Makefile.common
> >  
> > diff --git a/arm/vtimer-test.c b/arm/vtimer-test.c
> > new file mode 100644
> > index 000..a3e24ce
> > --- /dev/null
> > +++ b/arm/vtimer-test.c
> > @@ -0,0 +1,122 @@
> > +/*
> > + * Unit tests for the virtual timer on the ARM virt machine
> > + *
> > + * Copyright (C) 2016, Alexander Graf 
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version 2.
> > + */
> > +#include 
> > +#include 
> > +
> > +#define VTIMER_CTL_ENABLE  (1 << 0)
> > +#define VTIMER_CTL_IMASK   (1 << 1)
> > +#define VTIMER_CTL_ISTATUS (1 << 2)
> > +
> > +#define VIRT_GIC_DIST_BASE 0x0800
> > +#define GIC_DIST_PENDING_SET0x200
> > +#define GIC_DIST_PENDING_CLEAR  0x280
> > +#define GIC_DIST_ACTIVE_SET 0x300
> > +#define GIC_DIST_ACTIVE_CLEAR   0x380
> > +
> > +#define VIRT_GIC_CPU_BASE  0x0801
> 
> Once I get my arm/gic series in we won't need to hard code
> the gic base addresses and these gic register offsets will
> also be provided.
> 
> > +
> > +#define ARCH_TIMER_VIRT_IRQ   11
> > +#define ARCH_TIMER_S_EL1_IRQ  13
> > +#define ARCH_TIMER_NS_EL1_IRQ 14
> > +#define ARCH_TIMER_NS_EL2_IRQ 10
> 
> We can pull these out of the DT.
> 
> > +
> > +static void write_vtimer_ctl(u64 val)
> > +{
> > +   asm volatile("msr cntv_ctl_el0, %0" : : "r" (val));
> > +}
> > +
> > +static void write_vtimer_cval(u64 val)
> > +{
> > +   asm volatile("msr cntv_cval_el0, %0" : : "r" (val));
> > +}
> > +
> > +static u64 read_vtimer_ctl(void)
> > +{
> > +   u64 val;
> > +   asm volatile("mrs %0, cntv_ctl_el0" : "=r" (val));
> > +   return val;
> > +}
> > +
> > +static u64 read_vtimer_cval(void)
> > +{
> > +   u64 val;
> > +   asm volatile("mrs %0, cntv_cval_el0" : "=r" (val));
> > +   return val;
> > +}
> > +
> > +static u64 read_vtimer_val(void)
> > +{
> > +   u64 val;
> > +   asm volatile("mrs %0, cntvct_el0" : "=r" (val));
> > +   return val;
> > +}
> > +
> > +static u64 read_vtimer_freq(void)
> > +{
> > +   u64 val;
> > +   asm volatile("mrs %0, cntfrq_el0" : "=r" (val));
> > +   return val;
> > +}
> > +
> > +static bool gic_vtimer_pending(void)
> > +{
> > +   u32 *pending = (u32*)(VIRT_GIC_DIST_BASE + GIC_DIST_PENDING_SET);
> > +   return (readl(pending) & (1 << (ARCH_TIMER_VIRT_IRQ + 16)));
> 
> nit: don't need the outer ()
> 
> I think I'd like to add the PPI(irq) macro we added to QEMU for
> the +16's.
> 
> > +}
> > +
> > +static bool test_cval_10msec(void)
> > +{
> > +   u64 time_10ms = read_vtimer_freq() / 100;
> > +   u64 time_1us = time_10ms / 1;
> > +   u64 before_timer = read_vtimer_val();
> > +   u64 after_timer, latency;
> > +
> > +   /* Program timer to fire in 10ms */
> > +   write_vtimer_cval(before_timer + time_10ms);
> > +
> > +   /* Wait for the timer to fire */
> > +   while (!(read_vtimer_ctl() & VTIMER_CTL_ISTATUS)) ;
> 
> If emulation fails to set the status bit then we'll spin forever.
> That's OK if we set the test timeout (arm/unittests.cfg:timeout
> for this test appropriately) Or, maybe can add a trial count here
> that's sufficiently large?
> 
> > +
> > +   /* It fired, check how long it took */
> > +   after_timer = read_vtimer_val();
> > +
> > +   latency = (after_timer - (before_timer + time_10ms));
> 
> nit: don't need the outer ()
> 
> Shouldn't latency be signed so we can see if the status bit was
> set too soon? Or just compare time_10ms with after - before?
> 
> > +   printf("After timer: 0x%lx\n", after_timer);
> > +   printf("Expected   : 0x%lx\n", before_timer + time_10ms);
> > +   printf("Latency: %ld us\n", latency / time_1us);
> > +
> > +   return latency < time_10ms;
> 
> Does this mean that the threshold for success is 10ms? If so,
> then that's not too large?
> 
> > +}
> > +
> > +static void test_vtimer(void)
> > +{
> > +   printf("\n=== vtimer with busy

Re: [Qemu-devel] [kvm-unit-tests PATCH v8 2/3] arm: pmu: Check cycle count increases

2016-11-16 Thread Andrew Jones 

Just crossed my mind that we're missing isb's.

On Tue, Nov 08, 2016 at 12:17:14PM -0600, Wei Huang wrote:
> From: Christopher Covington 
> 
> Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
> even for the smallest delta of two subsequent reads.
> 
> Signed-off-by: Christopher Covington 
> Signed-off-by: Wei Huang 
> ---
>  arm/pmu.c | 98 
> +++
>  1 file changed, 98 insertions(+)
> 
> diff --git a/arm/pmu.c b/arm/pmu.c
> index 0b29088..d5e3ac3 100644
> --- a/arm/pmu.c
> +++ b/arm/pmu.c
> @@ -14,6 +14,7 @@
>   */
>  #include "libcflat.h"
>  
> +#define PMU_PMCR_E (1 << 0)
>  #define PMU_PMCR_N_SHIFT   11
>  #define PMU_PMCR_N_MASK0x1f
>  #define PMU_PMCR_ID_SHIFT  16
> @@ -21,6 +22,10 @@
>  #define PMU_PMCR_IMP_SHIFT 24
>  #define PMU_PMCR_IMP_MASK  0xff
>  
> +#define PMU_CYCLE_IDX  31
> +
> +#define NR_SAMPLES 10
> +
>  #if defined(__arm__)
>  static inline uint32_t pmcr_read(void)
>  {
> @@ -29,6 +34,47 @@ static inline uint32_t pmcr_read(void)
>   asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
>   return ret;
>  }
> +
> +static inline void pmcr_write(uint32_t value)
> +{
> + asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (value));
> +}
> +
> +static inline void pmselr_write(uint32_t value)
> +{
> + asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));

Probably want an isb here, users will call this and then immediately
another PMU reg write, like is done below

> +}
> +
> +static inline void pmxevtyper_write(uint32_t value)
> +{
> + asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
> +}
> +
> +/*
> + * While PMCCNTR can be accessed as a 64 bit coprocessor register, returning 
> 64
> + * bits doesn't seem worth the trouble when differential usage of the result 
> is
> + * expected (with differences that can easily fit in 32 bits). So just return
> + * the lower 32 bits of the cycle count in AArch32.

Also, while we're discussing confirming upper bits are as expected, I
guess we should confirm no overflow too. We should clear the overflow
bit PMOVSCLR_EL0.C before we use the counter, and then check it at some
point to confirm it's as expected. I guess that could be separate test
cases though.

> + */
> +static inline uint32_t pmccntr_read(void)
> +{
> + uint32_t cycles;
> +
> + asm volatile("mrc p15, 0, %0, c9, c13, 0" : "=r" (cycles));
> + return cycles;
> +}
> +
> +static inline void pmcntenset_write(uint32_t value)
> +{
> + asm volatile("mcr p15, 0, %0, c9, c12, 1" : : "r" (value));
> +}
> +
> +/* PMCCFILTR is an obsolete name for PMXEVTYPER31 in ARMv7 */
> +static inline void pmccfiltr_write(uint32_t value)
> +{
> + pmselr_write(PMU_CYCLE_IDX);
> + pmxevtyper_write(value);
> +}
>  #elif defined(__aarch64__)
>  static inline uint32_t pmcr_read(void)
>  {
> @@ -37,6 +83,29 @@ static inline uint32_t pmcr_read(void)
>   asm volatile("mrs %0, pmcr_el0" : "=r" (ret));
>   return ret;
>  }
> +
> +static inline void pmcr_write(uint32_t value)
> +{
> + asm volatile("msr pmcr_el0, %0" : : "r" (value));
> +}
> +
> +static inline uint32_t pmccntr_read(void)
> +{
> + uint32_t cycles;
> +
> + asm volatile("mrs %0, pmccntr_el0" : "=r" (cycles));
> + return cycles;
> +}
> +
> +static inline void pmcntenset_write(uint32_t value)
> +{
> + asm volatile("msr pmcntenset_el0, %0" : : "r" (value));
> +}
> +
> +static inline void pmccfiltr_write(uint32_t value)
> +{
> + asm volatile("msr pmccfiltr_el0, %0" : : "r" (value));
> +}
>  #endif
>  
>  /*
> @@ -63,11 +132,40 @@ static bool check_pmcr(void)
>   return ((pmcr >> PMU_PMCR_IMP_SHIFT) & PMU_PMCR_IMP_MASK) != 0;
>  }
>  
> +/*
> + * Ensure that the cycle counter progresses between back-to-back reads.
> + */
> +static bool check_cycles_increase(void)
> +{
> + pmcr_write(pmcr_read() | PMU_PMCR_E);

Need isb() here

> +
> + for (int i = 0; i < NR_SAMPLES; i++) {
> + unsigned long a, b;
> +
> + a = pmccntr_read();
> + b = pmccntr_read();
> +
> + if (a >= b) {
> + printf("Read %ld then %ld.\n", a, b);
> + return false;
> + }
> + }
> +
> + pmcr_write(pmcr_read() & ~PMU_PMCR_E);
> +

Need isb() here

> + return true;
> +}
> +
>  int main(void)
>  {
>   report_prefix_push("pmu");
>  
> + /* init for PMU event access, right now only care about cycle count */
> + pmcntenset_write(1 << PMU_CYCLE_IDX);
> + pmccfiltr_write(0); /* count cycles in EL0, EL1, but not EL2 */

Need isb() here

> +
>   report("Control register", check_pmcr());
> + report("Monotonically increasing cycle count", check_cycles_increase());
>  
>   return report_summary();
>  }
> -- 
> 1.8.3.1
> 
>

Thanks,
drew
___
kvmarm mailing list

Re: [PATCH v8 0/7] arm/arm64: vgic: Implement API for vGICv3 live migration

2016-11-16 Thread Christoffer Dall 
On Wed, Nov 16, 2016 at 08:24:16PM +0530, Vijay Kilari wrote:
> On Wed, Nov 16, 2016 at 5:17 PM, Christoffer Dall
>  wrote:
> > On Fri, Nov 04, 2016 at 04:43:26PM +0530, vijay.kil...@gmail.com wrote:
> >> From: Vijaya Kumar K 
> >>
> >> This patchset adds API for saving and restoring
> >> of VGICv3 registers to support live migration with new vgic feature.
> >> This API definition is as per version of VGICv3 specification
> >> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >>
> >> The patch 3 & 4 are picked from the Pavel's previous implementation.
> >> http://www.spinics.net/lists/kvm/msg122040.html
> >
> > Do we have QEMU/kvmtool patches somewhere at this point so that I can
> > test this?
> 
> I will send you next revision of QEMU patches tomorrow.
> 
Sounds good, thanks.

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

[kvm-unit-tests PATCH] arm/arm64: introduce is_aarch32

2016-11-16 Thread Andrew Jones 
ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
function allows unit tests to make the distinction.

Signed-off-by: Andrew Jones 

---
I'm actually unsure if there's a feature bit or not that I could
probe instead. It'd be nice if somebody can confirm. Thanks, drew

 lib/arm/asm/processor.h   | 20 
 lib/arm64/asm/processor.h |  5 +
 2 files changed, 25 insertions(+)

diff --git a/lib/arm/asm/processor.h b/lib/arm/asm/processor.h
index f25e7eee3666..223e54beb72a 100644
--- a/lib/arm/asm/processor.h
+++ b/lib/arm/asm/processor.h
@@ -5,6 +5,7 @@
  *
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
+#include 
 #include 
 
 enum vector {
@@ -46,4 +47,23 @@ static inline unsigned int get_mpidr(void)
 extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
sp_usr);
 extern bool is_user(void);
 
+/*
+ * ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
+ * function allows unit tests to make the distinction.
+ */
+static inline bool is_aarch32(void)
+{
+   /*
+* XXX: Unfortunately there's no feature bit we can probe for
+* this, so we do a hacky check for the processor type not being
+* a Cortex-A15, which is the only v7 type we currently use.
+*/
+   unsigned long midr;
+
+   asm volatile("MRC p15, 0, %0, c0, c0, 0" : "=r" (midr));
+   midr &= GENMASK(31, 24) | GENMASK(15, 4);
+
+   return midr != ((0x41 << 24) | (0xc0f << 4));
+}
+
 #endif /* _ASMARM_PROCESSOR_H_ */
diff --git a/lib/arm64/asm/processor.h b/lib/arm64/asm/processor.h
index 84d5c7ce752b..b602e1fbbc2d 100644
--- a/lib/arm64/asm/processor.h
+++ b/lib/arm64/asm/processor.h
@@ -81,5 +81,10 @@ DEFINE_GET_SYSREG32(mpidr)
 extern void start_usr(void (*func)(void *arg), void *arg, unsigned long 
sp_usr);
 extern bool is_user(void);
 
+static inline bool is_aarch32(void)
+{
+   return false;
+}
+
 #endif /* !__ASSEMBLY__ */
 #endif /* _ASMARM64_PROCESSOR_H_ */
-- 
2.7.4

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

Re: [Qemu-devel] [kvm-unit-tests PATCH v8 2/3] arm: pmu: Check cycle count increases

2016-11-16 Thread Andrew Jones 
On Tue, Nov 15, 2016 at 04:50:53PM -0600, Wei Huang wrote:
> 
> 
> On 11/14/2016 09:12 AM, Christopher Covington wrote:
> > Hi Drew, Wei,
> > 
> > On 11/14/2016 05:05 AM, Andrew Jones wrote:
> >> On Fri, Nov 11, 2016 at 01:55:49PM -0600, Wei Huang wrote:
> >>>
> >>>
> >>> On 11/11/2016 01:43 AM, Andrew Jones wrote:
>  On Tue, Nov 08, 2016 at 12:17:14PM -0600, Wei Huang wrote:
> > From: Christopher Covington 
> >
> > Ensure that reads of the PMCCNTR_EL0 are monotonically increasing,
> > even for the smallest delta of two subsequent reads.
> >
> > Signed-off-by: Christopher Covington 
> > Signed-off-by: Wei Huang 
> > ---
> >  arm/pmu.c | 98 
> > +++
> >  1 file changed, 98 insertions(+)
> >
> > diff --git a/arm/pmu.c b/arm/pmu.c
> > index 0b29088..d5e3ac3 100644
> > --- a/arm/pmu.c
> > +++ b/arm/pmu.c
> > @@ -14,6 +14,7 @@
> >   */
> >  #include "libcflat.h"
> >  
> > +#define PMU_PMCR_E (1 << 0)
> >  #define PMU_PMCR_N_SHIFT   11
> >  #define PMU_PMCR_N_MASK0x1f
> >  #define PMU_PMCR_ID_SHIFT  16
> > @@ -21,6 +22,10 @@
> >  #define PMU_PMCR_IMP_SHIFT 24
> >  #define PMU_PMCR_IMP_MASK  0xff
> >  
> > +#define PMU_CYCLE_IDX  31
> > +
> > +#define NR_SAMPLES 10
> > +
> >  #if defined(__arm__)
> >  static inline uint32_t pmcr_read(void)
> >  {
> > @@ -29,6 +34,47 @@ static inline uint32_t pmcr_read(void)
> > asm volatile("mrc p15, 0, %0, c9, c12, 0" : "=r" (ret));
> > return ret;
> >  }
> > +
> > +static inline void pmcr_write(uint32_t value)
> > +{
> > +   asm volatile("mcr p15, 0, %0, c9, c12, 0" : : "r" (value));
> > +}
> > +
> > +static inline void pmselr_write(uint32_t value)
> > +{
> > +   asm volatile("mcr p15, 0, %0, c9, c12, 5" : : "r" (value));
> > +}
> > +
> > +static inline void pmxevtyper_write(uint32_t value)
> > +{
> > +   asm volatile("mcr p15, 0, %0, c9, c13, 1" : : "r" (value));
> > +}
> > +
> > +/*
> > + * While PMCCNTR can be accessed as a 64 bit coprocessor register, 
> > returning 64
> > + * bits doesn't seem worth the trouble when differential usage of the 
> > result is
> > + * expected (with differences that can easily fit in 32 bits). So just 
> > return
> > + * the lower 32 bits of the cycle count in AArch32.
> 
>  Like I said in the last review, I'd rather we not do this. We should
>  return the full value and then the test case should confirm the upper
>  32 bits are zero.
> >>>
> >>> Unless I miss something in ARM documentation, ARMv7 PMCCNTR is a 32-bit
> >>> register. We can force it to a more coarse-grained cycle counter with
> >>> PMCR.D bit=1 (see below). But it is still not a 64-bit register.
> > 
> > AArch32 System Register Descriptions
> > Performance Monitors registers
> > PMCCNTR, Performance Monitors Cycle Count Register
> > 
> > To access the PMCCNTR when accessing as a 32-bit register:
> > MRC p15,0,,c9,c13,0 ; Read PMCCNTR[31:0] into Rt
> > MCR p15,0,,c9,c13,0 ; Write Rt to PMCCNTR[31:0]. PMCCNTR[63:32] are 
> > unchanged
> > 
> > To access the PMCCNTR when accessing as a 64-bit register:
> > MRRC p15,0,,,c9 ; Read PMCCNTR[31:0] into Rt and PMCCNTR[63:32] 
> > into Rt2
> > MCRR p15,0,,,c9 ; Write Rt to PMCCNTR[31:0] and Rt2 to 
> > PMCCNTR[63:32]
> > 
> 
> Thanks. I did some research based on your info and came back with the
> following proposals (Cov, correct me if I am wrong):
> 
> By comparing A57 TRM (page 394 in [1]) with A15 TRM (page 273 in [2]), I
> think this 64-bit cycle register is only available when running under
> aarch32 compatibility mode on ARMv8 because it is not specified in A15
> TRM.

OK, I hadn't realized that there would be differences between v7 and
AArch32. It looks like we need to add a function to the kvm-unit-tests
framework that enables unit tests to make that distinction, because we'll
want to explicitly test those differences in order to flush out emulation
bugs. I see now that Appendix K5 of the v8 ARM ARM lists some differences,
but this PMCCNTR difference isn't there...

As v8-A32 is an update/extension of v7-A, I'd expect there to be a RES0
bit in some v7 ID register that, on v8, is no longer reserved and a 1.
Unfortunately I just did some ARM doc skimming but can't find anything
like that. As we currently only use the cortex-a15 for our v7 processor,
then I guess we can just check MIDR, but yuck. Anyway, I'll send a
patch for that.

> To further verify it, I tested 32-bit pmu code on QEMU with TCG
> mode. The result is: accessing 64-bit PMCCNTR using the following
> assembly failed on A15:
> 
>volatile("mrrc p15, 0, %0, %1, c9" : "=r" (lo), "=r" (hi));
> or
>volatile("mrrc p15, 0, %Q0, %R0, c9" :

Re: [PATCH v8 0/7] arm/arm64: vgic: Implement API for vGICv3 live migration

2016-11-16 Thread Christoffer Dall 
On Fri, Nov 04, 2016 at 04:43:26PM +0530, vijay.kil...@gmail.com wrote:
> From: Vijaya Kumar K 
> 
> This patchset adds API for saving and restoring
> of VGICv3 registers to support live migration with new vgic feature.
> This API definition is as per version of VGICv3 specification
> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> 
> The patch 3 & 4 are picked from the Pavel's previous implementation.
> http://www.spinics.net/lists/kvm/msg122040.html

Do we have QEMU/kvmtool patches somewhere at this point so that I can
test this?

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