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

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

[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 <drjones@redhat.com>

---
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 <bitops.h>
 #include <asm/ptrace.h>
 
 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

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

[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 <drjones@redhat.com>
> 
> ---
> 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 <bitops.h>
>  #include <asm/ptrace.h>
>  
>  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_ */
> 

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

[Andrew Jones]

Hi Andre,

On Wed, Nov 16, 2016 at 05:42:08PM +0000, Andre Przywara wrote:
> 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.

The current motivation is determining the width of pmccntr, as Cov
points out in his reply to Marc. That was actually my first encounter
of a difference between v7-A and v8-A32, but now that I know there
are some, and that they may or may not have dedicated feature bits,
I'm convinced I should add this function, allowing unit tests to
specifically test those differences, particularly because they
likely untested by typical guest kernels.

> 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > 
> > ---
> > 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.

Ah yes, I forgot about '-cpu host' on those other processors.
Additionally, if we ever create a bare-metal build target for
kvm-unit-tests, then only checking for A15 is certainly a bad
idea.

> 
> 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'.

So this idea crossed my mind, and I did look at each ID_ register. I
wanted a RES0 on v7 that was guaranteed to be 1 (or vice versa RES1/0)
on v8, but didn't find anything.

Thanks,
drew

> 
> 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 <bitops.h>
> >  #include <asm/ptrace.h>
> >  
> >  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_ */
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

On Wed, Nov 16, 2016 at 03:38:36PM +0100, Andrew Jones wrote:
> 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 <drjones@redhat.com>
> 
> ---
> 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

> +/*
> + * ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
> + * function allows unit tests to make the distinction.
> + */

What do you want this for, specifically?

Since the CPUID registers were introduced, the architecture goes to
great lengths to not expose a version, and trying to guess from software
is a losing battle.

So I don't think we should try to distinguish ARMv8-A AArch32 from
ARMv7-A. We should test individual features, or if that's not possible,
group them in the same bucket.

Thanks,
Mark.

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

[Andrew Jones]

Hi Mark,

On Wed, Nov 16, 2016 at 05:45:44PM +0000, Mark Rutland wrote:
> On Wed, Nov 16, 2016 at 03:38:36PM +0100, Andrew Jones wrote:
> > 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 <drjones@redhat.com>
> > 
> > ---
> > 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
> 
> > +/*
> > + * ARMv7-A isn't exactly the same as ARMv8-A32 (AArch32). This
> > + * function allows unit tests to make the distinction.
> > + */
> 
> What do you want this for, specifically?
> 
> Since the CPUID registers were introduced, the architecture goes to
> great lengths to not expose a version, and trying to guess from software
> is a losing battle.
> 
> So I don't think we should try to distinguish ARMv8-A AArch32 from
> ARMv7-A. We should test individual features, or if that's not possible,
> group them in the same bucket.

Perhaps I was too quick to look for a general way to approach the first
place I saw the need, which is

 check_cntr(read_pmccntr());
 if (is_aarch64())
     check_cntr(read_pmccntr64());

Is PMCCNTR the only "weird" difference?

Thanks,
drew

> 
> Thanks,
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Rutland]

On Thu, Nov 17, 2016 at 05:45:41PM +0100, Andrew Jones wrote:
> Hi Mark,
> 
> On Wed, Nov 16, 2016 at 05:45:44PM +0000, Mark Rutland wrote:
> > So I don't think we should try to distinguish ARMv8-A AArch32 from
> > ARMv7-A. We should test individual features, or if that's not possible,
> > group them in the same bucket.
> 
> Perhaps I was too quick to look for a general way to approach the first
> place I saw the need, which is
> 
>  check_cntr(read_pmccntr());
>  if (is_aarch64())
>      check_cntr(read_pmccntr64());
> 
> Is PMCCNTR the only "weird" difference?

Define "weird". ;)

I believe the extension of PMCCNTR to 64 bits is part of PMUv3. So you
should be able to check ID_DFR0.PerfMon == 0b0011. For overflow you'll
also need to configure PMCR.LC.

Thanks,
Mark.

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

[Andrew Jones]

On Thu, Nov 17, 2016 at 05:47:51PM +0000, Mark Rutland wrote:
> On Thu, Nov 17, 2016 at 05:45:41PM +0100, Andrew Jones wrote:
> > Hi Mark,
> > 
> > On Wed, Nov 16, 2016 at 05:45:44PM +0000, Mark Rutland wrote:
> > > So I don't think we should try to distinguish ARMv8-A AArch32 from
> > > ARMv7-A. We should test individual features, or if that's not possible,
> > > group them in the same bucket.
> > 
> > Perhaps I was too quick to look for a general way to approach the first
> > place I saw the need, which is
> > 
> >  check_cntr(read_pmccntr());
> >  if (is_aarch64())
> >      check_cntr(read_pmccntr64());
> > 
> > Is PMCCNTR the only "weird" difference?
> 
> Define "weird". ;)
> 
> I believe the extension of PMCCNTR to 64 bits is part of PMUv3. So you
> should be able to check ID_DFR0.PerfMon == 0b0011. For overflow you'll
> also need to configure PMCR.LC.

Thanks Mark,

checking for ID_DFR0.PerfMon == 0b0011 for this case does indeed look
like the right thing to do.

Wei,

when you respin the PMU test can you add the additional 64-bit read,
when a PMUv3 is present?

Thanks,
drew


> 
> Thanks,
> Mark.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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 <drjones@redhat.com>
> 
> ---
> 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 <bitops.h>
>  #include <asm/ptrace.h>
>  
>  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...

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

[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 <drjones@redhat.com>
>>
>> ---
>> 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.

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

[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 <drjones@redhat.com>
>>>
>>> ---
>>> 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
> 

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

[Andrew Jones]

On Wed, Nov 16, 2016 at 05:02:59PM -0500, 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 <drjones@redhat.com>
> >>
> >> ---
> >> 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 for this Cov. I mostly agree with the proposal, except for two
reasons; the spec says the bit is UNK/SBZP for v7, which doesn't give
me 100% confidence that we can rely on it behaving a certain way. And,
I'd rather not rely on the emulation we're testing to work well enough
that we can probe it.

For our PMU testing purposes there is a somewhat ugly, but foolproof
way to manage it; we could just add a command line input that says
we're aarch32 (-cpu host,aarch64=off -append aarch32)

I'm still hoping we can come up with a general way to implement an
is_aarch32() though, assuming we'll want it elsewhere in tests.
Although that assumption may be wrong...

Thanks,
drew
 

> 
> 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.
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Christopher Covington]

On 11/17/2016 11:59 AM, Andrew Jones wrote:
> On Wed, Nov 16, 2016 at 05:02:59PM -0500, 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 <drjones@redhat.com>
>>>>
>>>> ---
>>>> 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 for this Cov. I mostly agree with the proposal, except for two
> reasons; the spec says the bit is UNK/SBZP for v7, which doesn't give
> me 100% confidence that we can rely on it behaving a certain way. And,

The v7 definition of UNK/SBZP (page Glossary-2736 of ARM DDI 0406C.c):

Hardware must ... Read-As-Zero, and must ignore writes

> I'd rather not rely on the emulation we're testing to work well enough
> that we can probe it.

I agree that it'd really be best to test that QEMU is behaving correctly
rather than assume it does. Real production hardware has passed the ARM
Architecture Validation Suite, but QEMU hasn't.

> For our PMU testing purposes there is a somewhat ugly, but foolproof
> way to manage it; we could just add a command line input that says
> we're aarch32 (-cpu host,aarch64=off -append aarch32)

I would group ARMv7 versus ARMv8-A32 as an "implementation defined"
choice. There are hundreds of these in the architecture, and many of
them require tests to behave differently. For example AES support:

if (DEVICE_SUPPORTS(AES)) {
  assert(ID_ISAR5.AES == 0001 || ID_ISAR5.AES == 0010);
  /* use some AES instructions and check the results */
} else {
  assrt(ID_ISAR5.AES == 0);
  /* maybe check that AES instructions throw undefined
     instruction exceptions */
}

I'm starting to most prefer the original suggestion, which I'll
reframe as built-in tables of implementation defined options, using
the MIDR (and auxiliary ID registers if necessary) to select which
table to use. That way the tests only have to rely on MIDR being
properly emulated, and can check all the other behavior step by step.

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.