[PATCH v6 43/64] KVM: arm64: nv: arch_timer: Support hyp timer emulation

[Alexandru Elisei]

Hi,

On Mon, Mar 07, 2022 at 04:52:15PM +0000, Marc Zyngier wrote:
> On Mon, 07 Mar 2022 16:28:44 +0000,
> Alexandru Elisei <alexandru.elisei at arm.com> wrote:
> > 
> > Hi,
> > 
> > On Mon, Mar 07, 2022 at 03:48:19PM +0000, Marc Zyngier wrote:
> > > On 2022-03-07 14:52, Alexandru Elisei wrote:
> > > > Hi,
> > > > 
> > > > I was under the impression that KVM's nested virtualization doesn't
> > > > support
> > > > AArch32 in the guest, why is the subject about hyp mode aarch32 timers?
> > > 
> > > Where did you see *ANY* mention of AArch32?
> > 
> > I saw an implicit mention of aarch32 when the commit message used
> > the term "hyp", which is the name of an aarch32 execution mode.
> > 
> > > 
> > > Or is that a very roundabout way to object to the 'hyp' name?
> > 
> > Bingo.
> > 
> > > If that's the case, just apply a mental 's/hyp/el2/' substitution.
> > 
> > I'm a bit confused about that. Is that something that anyone reading
> > the patch should apply mentally when reading the patch, or is it
> > something that you plan to change in the commit subject?
> 
> Big picture:
> 
> maz at hot-poop:~/arm-platforms$ git grep -i hyp arch/arm64/|wc -l
> 1701
> maz at hot-poop:~/arm-platforms$ git grep -i el2 arch/arm64/|wc -l
> 1008
> 
> Are we going to also repaint all these 'hyp' references?

I didn't say, nor suggest that. I suggested that *this* commit could be changed
to use the architectural terminology, which I assumed it's the most natural
terminology to use in a patch series which has the goal to emulate the
architectural EL2. Obviously, opinions vary.

> 
> I really appreciate all the hard work you are putting in carefully
> reviewing the code. I *really* do. But bickering on this really
> doesn't help, and I know you understand exactly what this subject line
> means (you've been reviewing KVM code for long enough, and won't fool
> anyone).
> 
> The point you are trying to make really is moot. Everybody understands
> that HYP means EL2. I'd even argue that it is clearer than EL2,
> because it indicates that we're running at EL2 with the role of a
> hypervisor, which isn't that clear with running with VHE.

So HYP means EL2 or EL2 with the role of a hypervisor? Because when booting KVM
on a machine without FEAT_VHE, dmesg uses "hyp mode" to refer to EL2 without
FEAT_VHE. And is_hyp_ctxt(), which this series adds, refers to EL0 with
HCR_EL2.TGE set.

I've given these examples before (and others), in the end it's up to you how
precise you want the terminology to be and how easy to understand you want to
make the code.

Thanks,
Alex