On Wed, Feb 20, 2019 at 03:37:26PM +0000, Mark Rutland wrote:

On Mon, Feb 18, 2019 at 07:52:25PM +0000, Dave Martin wrote:

quoted

Some optional features of the Arm architecture add new system
registers that are not present in the base architecture.

Where these features are optional for the guest, the visibility of
these registers may need to depend on some runtime configuration,
such as a flag passed to KVM_ARM_VCPU_INIT.

For example, ZCR_EL1 and ID_AA64ZFR0_EL1 need to be hidden if SVE
is not enabled for the guest, even though these registers may be
present in the hardware and visible to the host at EL2.

Adding special-case checks all over the place for individual
registers is going to get messy as the number of conditionally-
visible registers grows.

In order to help solve this problem, this patch adds a new sysreg
method restrictions() that can be used to hook in any needed
runtime visibility checks.  This method can currently return
REG_NO_USER to inhibit enumeration and ioctl access to the register
for userspace, and REG_NO_GUEST to inhibit runtime access by the
guest using MSR/MRS.

This allows a conditionally modified view of individual system
registers such as the CPU ID registers, in addition to completely
hiding register where appropriate.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>

---

Changes since v4:

 * Move from a boolean sysreg property that just suppresses register
   enumeration via KVM_GET_REG_LIST, to a multi-flag property that
   allows independent runtime control of MRS/MSR and user ioctl access.

   This allows registers to be either hidden completely, or to have
   hybrid behaviours (such as the not-enumerated, RAZ, WAZ behaviour of
   "non-present" CPU ID regs).

Sorry for bikeshedding...

quoted

+	/* Check for regs disabled by runtime config */
+	if (restrictions(vcpu, r) & REG_NO_GUEST) {

Maybe it's worth wrapping this as something like

	reg_runtime_hidden_from_guest(vcpu, r)

... and avoid exposing the raw flags to all the places we have to check?

[...]

quoted

+#define REG_NO_USER	(1 << 0) /* hidden from userspace ioctl interface */
+#define REG_NO_GUEST	(1 << 1) /* hidden from guest */

Perhaps REG_USER_HIDDEN and REG_GUEST_HIDDEN?

I'm not attached to any particular naming, so I'm not opposed to making
changes similar to those you suggest.

There are some anomalies right now:

1) Currently, we can express REG_NO_GUEST by itself, which is a of an
odd thing to have.  I'm not sure whether that's a problem or not.
Keeping the flags as-is at least keeps the code simple.

2) These flags do not quite have the obvious semantics: these are
overrides rather than determining precisely when a reg is/isn't
accessible.

So, REG_NO_USER means "don't even call this reg's get/set_user(): forbid
user access unconditionally", whereas lack of this flag means "call the
appropriate get/set_user() function to find out what to do, which may
or may not result in forbidding the access".

Maybe this subtlety is just a question of clear commenting.  I can't
think of obviously-correct names that won't be stupidly verbose...

Thoughts?

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel