Re: [PATCH RESEND v2 5/6] target/arm/cpu: Enable 'el2' to work with host/max cpu

From: Andrew Jones <drjones@redhat.com>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: "Richard Henderson" <richard.henderson@linaro.org>,
	"QEMU Developers" <qemu-devel@nongnu.org>,
	"Andrea Bolognani" <abologna@redhat.com>,
	qemu-arm <qemu-arm@nongnu.org>, "Haibo Xu" <haibo.xu@linaro.org>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH RESEND v2 5/6] target/arm/cpu: Enable 'el2' to work with host/max cpu
Date: Tue, 27 Apr 2021 11:49:24 +0200	[thread overview]
Message-ID: <20210427094924.pgivurjrgtzovcwa@gator.home> (raw)
In-Reply-To: <CAFEAcA9+VpFp70rwZ5rWEQX9TxPH558jCiSU6Z=qPnUfieSukA@mail.gmail.com>

On Tue, Apr 27, 2021 at 10:38:00AM +0100, Peter Maydell wrote:
> On Tue, 27 Apr 2021 at 10:24, Andrew Jones <drjones@redhat.com> wrote:
> > I feel like there are way too many ways to track this feature now. If I
> > didn't lose count we have
> >
> >  1) cpu->has_el2
> >  2) cpu->env & ARM_FEATURE_EL2
> >  3) (for mach-virt) vms->virt
> >  4) possibly (and probably should) some ID register bits
> >
> > I realize the first three are already in use for TCG, but I'm guessing
> > we'll want to clean those up. What's the plan going forward? I presume
> > it'll be (4), but maybe something like (1) and/or (3) will stick around
> > for convenience. I'm pretty sure we want to avoid (2).
> 
> For new features added we want to use ID register bits. However,
> a lot of older pre-existing features are keyed off ARM_FEATURE_FOO
> flag bits. Trying to convert an ARM_FEATURE flag to use ID registers
> isn't necessarily 100% trivial (for instance, the ARM_FEATURE flag
> is always checkable regardless of AArch64 vs AArch32, but the ID
> register checks often need to be split up into separate ones checking
> the AArch32 or the AArch64 ID register value). So we aren't really
> doing conversion of existing flags. (I did a few which were easy
> because there were only a handful of checks of them.) As a side note,
> some features really can't be checked in ID registers, like
> ARM_FEATURE_V8_1M, so env->features is not going to go away completely.
> 
> The only use of cpu->has_foo should be for the QOM property -- the
> arm_cpu_realizefn() should look at it and then either clear the
> ARM_FEATURE flag or update the ID register bits depending on
> which one the feature is using.
> 
> vms->virt is for the board code (and controls more things than
> just whether the CPU itself has EL2).
>

Thanks for the summary, Peter. For this series, do you recommend
attempting to convert from ARM_FEATURE_EL2 to feature bits first? Or keep
the ARM_FEATURE flag? Also, while I agree we can't use vms->virt for the
same purposes as the CPU feature (however that's tracked), do you agree
vms->virt should be true when the CPU feature is enabled?

Thanks,
drew