On Tue, Nov 03, 2020 at 11:18:19AM +0000, Dave Martin wrote:
> On Mon, Nov 02, 2020 at 07:50:35PM +0100, Andrew Jones wrote:
> > ID registers are RAZ until they've been allocated a purpose, but
> > that doesn't mean they should be removed from the KVM_GET_REG_LIST
> > list. So far we only have one register, SYS_ID_AA64ZFR0_EL1, that
> > is hidden from userspace when its function is not present. Removing
> > the userspace visibility checks is enough to reexpose it, as it
> > already behaves as RAZ when the function is not present.
>
> Pleae state what the patch does. (The subject line serves as a summary
> of that, but the commit message should make sense without it.)
I don't like "This patch ..." type of sentences in the commit message,
but unless you have a better suggestion, then I'd just add a sentence
like
"This patch ensures we still expose sysreg '3, 0, 0, 4, 4'
(ID_AA64ZFR0_EL1) to userspace as RAZ when SVE is not implemented."
>
> Also, how exactly !vcpu_has_sve() causes ID_AA64ZFR0_EL1 to behave as
> RAZ with this change? (I'm not saying it doesn't, but the code is not
> trivial to follow...)
guest_id_aa64zfr0_el1() returns zero for the register when !vcpu_has_sve(),
and all the accessors (userspace and guest) build on it.
I'm not sure how helpful it would be to add that sentence to the commit
message though.
>
> >
> > Fixes: 73433762fcae ("KVM: arm64/sve: System register context switch and access support")
> > Cc: <stable@xxxxxxxxxxxxxxx> # v5.2+
> > Reported-by: 张东旭 <xu910121@xxxxxxxx>
> > Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> > ---
> > arch/arm64/kvm/sys_regs.c | 18 +-----------------
> > 1 file changed, 1 insertion(+), 17 deletions(-)
> >
> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > index fb12d3ef423a..6ff0c15531ca 100644
> > --- a/arch/arm64/kvm/sys_regs.c
> > +++ b/arch/arm64/kvm/sys_regs.c
> > @@ -1195,16 +1195,6 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,
> > return REG_HIDDEN_USER | REG_HIDDEN_GUEST;
> > }
> >
> > -/* Visibility overrides for SVE-specific ID registers */
> > -static unsigned int sve_id_visibility(const struct kvm_vcpu *vcpu,
> > - const struct sys_reg_desc *rd)
> > -{
> > - if (vcpu_has_sve(vcpu))
> > - return 0;
> > -
> > - return REG_HIDDEN_USER;
>
> In light of this change, I think that REG_HIDDEN_GUEST and
> REG_HIDDEN_USER are always either both set or both clear. Given the
> discussion on this issue, I'm thinking it probably doesn't even make
> sense for these to be independent (?)
>
> If REG_HIDDEN_USER is really redundant, I suggest stripping it out and
> simplifying the code appropriately.
>
> (In effect, I think your RAZ flag will do correctly what REG_HIDDEN_USER
> was trying to achieve.)
We could consolidate REG_HIDDEN_GUEST and REG_HIDDEN_USER into REG_HIDDEN,
which ZCR_EL1 and ptrauth registers will still use.
>
> > -}
> > -
> > /* Generate the emulated ID_AA64ZFR0_EL1 value exposed to the guest */
> > static u64 guest_id_aa64zfr0_el1(const struct kvm_vcpu *vcpu)
> > {
> > @@ -1231,9 +1221,6 @@ static int get_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > {
> > u64 val;
> >
> > - if (WARN_ON(!vcpu_has_sve(vcpu)))
> > - return -ENOENT;
> > -
> > val = guest_id_aa64zfr0_el1(vcpu);
> > return reg_to_user(uaddr, &val, reg->id);
> > }
> > @@ -1246,9 +1233,6 @@ static int set_id_aa64zfr0_el1(struct kvm_vcpu *vcpu,
> > int err;
> > u64 val;
> >
> > - if (WARN_ON(!vcpu_has_sve(vcpu)))
> > - return -ENOENT;
> > -
> > err = reg_from_user(&val, uaddr, id);
> > if (err)
> > return err;
> > @@ -1518,7 +1502,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
> > ID_SANITISED(ID_AA64PFR1_EL1),
> > ID_UNALLOCATED(4,2),
> > ID_UNALLOCATED(4,3),
> > - { SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, .visibility = sve_id_visibility },
> > + { SYS_DESC(SYS_ID_AA64ZFR0_EL1), access_id_aa64zfr0_el1, .get_user = get_id_aa64zfr0_el1, .set_user = set_id_aa64zfr0_el1, },
> > ID_UNALLOCATED(4,5),
> > ID_UNALLOCATED(4,6),
> > ID_UNALLOCATED(4,7),
>
> Otherwise looks reasonable.
>
Thanks,
drew
