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:
> > 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@xxxxxxx>
> >
> > ---
> >
> > 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...
>
> > + /* 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?
>
> [...]
>
> > +#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
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
