On Thu, Aug 17, 2017 at 09:45:51AM +0100, Marc Zyngier wrote:

On 16/08/17 21:32, Dave Martin wrote:

quoted

On Wed, Aug 16, 2017 at 12:10:38PM +0100, Marc Zyngier wrote:

quoted

On 09/08/17 13:05, Dave Martin wrote:

quoted

Currently, a guest kernel sees the true CPU feature registers
(ID_*_EL1) when it reads them using MRS instructions.  This means
that the guest will observe features that are present in the
hardware but the host doesn't understand or doesn't provide support
for.  A guest may legimitately try to use such a feature as per the
architecture, but use of the feature may trap instead of working
normally, triggering undef injection into the guest.

This is not a problem for the host, but the guest may go wrong when
running on newer hardware than the host knows about.

This patch hides from guest VMs any AArch64-specific CPU features
that the host doesn't support, by exposing to the guest the
sanitised versions of the registers computed by the cpufeatures
framework, instead of the true hardware registers.  To achieve
this, HCR_EL2.TID3 is now set for AArch64 guests, and emulation
code is added to KVM to report the sanitised versions of the
affected registers in response to MRS and register reads from
userspace.

The affected registers are removed from invariant_sys_regs[] (since
the invariant_sys_regs handling is no longer quite correct for
them) and added to sys_reg_desgs[], with appropriate access(),
get_user() and set_user() methods.  No runtime vcpu storage is
allocated for the registers: instead, they are read on demand from
the cpufeatures framework.  This may need modification in the
future if there is a need for userspace to customise the features
visible to the guest.

Attempts by userspace to write the registers are handled similarly
to the current invariant_sys_regs handling: writes are permitted,
but only if they don't attempt to change the value.  This is
sufficient to support VM snapshot/restore from userspace.

Because of the additional registers, restoring a VM on an older
kernel may not work unless userspace knows how to handle the extra
VM registers exposed to the KVM user ABI by this patch.

Under the principle of least damage, this patch makes no attempt to
handle any of the other registers currently in
invariant_sys_regs[], or to emulate registers for AArch32: however,
these could be handled in a similar way in future, as necessary.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
---
 arch/arm64/kvm/hyp/switch.c |   6 ++
 arch/arm64/kvm/sys_regs.c   | 224 +++++++++++++++++++++++++++++++++++---------
 2 files changed, 185 insertions(+), 45 deletions(-)

[...]

quoted

quoted

quoted

+static bool __access_id_reg(struct kvm_vcpu *vcpu,
+			    struct sys_reg_params *p,
+			    const struct sys_reg_desc const *r,
+			    bool raz)
+{
+	if (p->is_write) {
+		kvm_inject_undefined(vcpu);
+		return false;
+	}

I don't think this is supposed to happen (should have UNDEF-ed at EL1).
You can call write_to_read_only() in that case, which will spit out a
warning and inject the exception.

I'll check this -- sounds about right.

If is should never happen, should I just delete that code or BUG()?  I
notice a BUG_ON() for a similar situation in access_vm_reg() for example.

Or do we not quite trust hardware not to get this wrong?
(It feels like the kind of thing that could slip through validation
and/or would be considered not worth a respin, but it seems wrong to
work around a theoretical hardware bug before it's confirmed to exist,
unless we think for some reason that it's really likely.)

That's the way we handle this for the rest of the accessors. We used to
have a BUG_ON(), but it is pretty silly to kill the whole system for
such a small deviation from the architecture. And maybe it is useless,
but it doesn't hurt either.

OK, that makes sense -- I'll follow the precedent here and call
write_to_read_only() if this happens.

quoted

quoted

quoted

+
+	p->regval = read_id_reg(r, raz);
+	return true;
+}

[...]

quoted

quoted

@@ -944,6 +1073,32 @@ static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_DBGVCR32_EL2), NULL, reset_val, DBGVCR32_EL2, 0 },
 
 	{ SYS_DESC(SYS_MPIDR_EL1), NULL, reset_mpidr, MPIDR_EL1 },
+
+	/*
+	 * All non-RAZ feature registers listed here must also be
+	 * present in arm64_ftr_regs[].
+	 */
+
+	/* AArch64 mappings of the AArch32 ID registers */
+	/* ID_AFR0_EL1 not exposed to guests for now */
+	ID(PFR0),	ID(PFR1),	ID(DFR0),	_ID_RAZ(1,3),
+	ID(MMFR0),	ID(MMFR1),	ID(MMFR2),	ID(MMFR3),
+	ID(ISAR0),	ID(ISAR1),	ID(ISAR2),	ID(ISAR3),
+	ID(ISAR4),	ID(ISAR5),	ID(MMFR4),	_ID_RAZ(2,7),
+	_ID(MVFR0),	_ID(MVFR1),	_ID(MVFR2),	_ID_RAZ(3,3),
+	_ID_RAZ(3,4),	_ID_RAZ(3,5),	_ID_RAZ(3,6),	_ID_RAZ(3,7),

#bikeshed:

OK, this is giving me a headache. Too many variants with similar names.
ID and _ID
I'm also slightly perplexed with the amalgamation of RAZ because the
register is not defined yet in the architecture, and RAZ because we
don't expose it (like ID_AFR0_EL1). Yes, there is a number of comments

This "raz" overloading already seems present in other places, such as the
cpufeatures code.  (Which is not necessarily a good reason for adding
more of it...)

quoted

to document that, but the code should aim to be be self-documenting. How
about IDRAZ() for those we want to "hide", and IDRSV for encodings that
are not allocated yet? It would look like this:

	IDREG(ID_PFR0),		IDREG(ID_PFR1),		IDREG(ID_DFR0),
	IDRAZ(ID_AFR0),		IDREG(ID_MMFR0),	IDREG(ID_MMFR1),
	IDREG(ID_MMFR2),	IDREG(ID_MMFR3),	IDREG(ID_ISAR0),
	IDREG(ID_ISAR1),	IDREG(ID_ISAR2),	IDREG(ID_ISAR3),
	IDREG(ID_ISAR4),	IDREG(ID_ISAR5),	IDREG(ID_MMFR4),
	IDRSV(2,7),		IDREG(MVFR0),		IDREG(MVFR1),
	IDREG(MVFR2),		IDRSV(3,3),		IDRSV(3,4),	
	IDRSV(3,5),		IDRSV(3,6),		IDRSV(3,7),

Yes, only 3 a line. Lines are cheap. And yes, they also have similar
names, but I said #bikeshed.

So, point taken, but the main reason for making this a table was to make
it easy to see by eye how the entries map to the encoding while hacking
this up, which helped me to make sure no entries were missed or in the
wrong place etc.

With 3 entries per line that visual map is lost, and with 2 entries per
line it's debatable whether it's worth having multiple entries per line
at all.

Let's be clear. I don't care at all about the number of entries per
line. I can widen my editor to 200 columns if I need to. If you think 4
is the way, keep it to 4.

My point is about the readability of both the macros and the
identifiers, and your initial proposal did seem to lack on both counts.

Agreed, I was just trying to explain why it ended up that way in the
first place, and I'm happy to change it.

quoted

So now that the table exists maybe we should just have one entry per
line like everything else -- it really depends on which option you think
is best for ongoing maintenance.


Having one per line allows much less cryptic names, allowing the
temptingly short but ambiguous "RAZ" to be avoided:

	ID_SANITISED(ID_ISAR5),
	ID_RAZ_FOR_GUEST(ID_AFR0),
	ID_UNALLOCATED(crm, op2)

With a whole line and different lengths, it's easier to pick out
the different cases by eye, so they don't all look like IDRXX (and are a
more tasteful colour perhaps).

Blank lines and/or comments can split the list into sensible blocks for
readability if needed.

If you're happy with naming along those broad lines then I'm happy to
see what it looks like.

Sure. If you're happy with that, so am I.

quoted

quoted

quoted

+
+	/* AArch64 ID registers */
+	ID(AA64PFR0),	ID(AA64PFR1),	_ID_RAZ(4,2),	_ID_RAZ(4,3),
+	_ID_RAZ(4,4),	_ID_RAZ(4,5),	_ID_RAZ(4,6),	_ID_RAZ(4,7),
+	ID(AA64DFR0),	ID(AA64DFR1),	_ID_RAZ(5,2),	_ID_RAZ(5,3),
+	/* ID_AA64AFR0_EL1 and ID_AA64AFR0_EL1 not exposed to guests for now */

There are no sysreg definitions for IA_AA64AFR{0,1}_EL1 yet.

If we want to macroise those rather than just commenting, I guess
they'll need adding in sysreg.h.  I'd prefer not to imply these are
"unallocated" or similar when the architecture does define them.

Can I take it there's no problem with zombie entries in sysreg.h so long
as they're at least referenced somewhere?  (Arguably they wouldn't be
zombies then, but hopefully you see what I mean.)

That'd be the right thing to do. The register exists, and KVM handles it
by returning 0 when a guest reads it. So I'd argue that it *must* be
defined in sysreg.h, and given its full visibility in that table.

OK, sounds good -- I'll reroll with that change.

Cheers
---Dave