Re: [Qemu-devel] [PATCH v3 15/22] target/arm: Add ARM_FEATURE_V7VE for v7 Virtualization Extensions

[Aaron Lindsay]

On Jun 01 09:57, Peter Maydell wrote:
> On 31 May 2018 at 21:39, Aaron Lindsay <address@hidden> wrote:
> > On May 31 15:18, Peter Maydell wrote:
> >>    if (arm_feature(env, ARM_FEATURE_V7VE) {
> >>        /* v7 Virtualization Extensions. In real hardware this implies
> >>         * EL2 and also the presence of the Security Extensions.
> >>         * For QEMU, for backwards-compatibility we implement some
> >>         * CPUs or CPU configs which have no actual EL2 or EL3 but do
> >>         * include the various other features that V7VE implies.
> >>         * Presence of EL2 itself is ARM_FEATURE_EL2, and of the
> >>         * Security Extensions is ARM_FEATURE_EL3.
> >>         */
> >>        set_feature(env, ARM_FEATURE_ARM_DIV);
> >
> > Is it safe to assume from your comment above regarding keeping ARM_DIV
> > separate from V7VE that the inclusion of it here is an oversight and
> > that only LPAE and V7 should be set if V7VE is? (and that V8 should
> > now directly imply both V7VE and ARM_DIV?)
> 
> No; V7VE always implies ARM_DIV. (ARM_DIV doesn't imply V7VE,
> though, which is why it is a separate feature bit.)

Okay, then I'm confused about some of the preexisting logic in
kvm_arm_get_host_cpu_features. The preexisting code in that function
sets ARM_DIV and THUMB_DIV based on the appropriate bits in ID_ISAR0. If
we already knew that
>    (by definition a host CPU which supports KVM has v7VE.)
and that all V7VE CPUs have ARM_DIV, why did the code there bother
checking ID_ISAR0 to begin with?

> switch (extract32(id_isar0, 24, 4)) {
> case 1:
>     set_feature(&features, ARM_FEATURE_THUMB_DIV);
>     break;
> case 2:
>     set_feature(&features, ARM_FEATURE_ARM_DIV);
>     set_feature(&features, ARM_FEATURE_THUMB_DIV);
>     break;
> default:
>     break;
> }

Should this switch/case be removed entirely?

-Aaron

-- 
Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.