On Tue, Nov 03, 2020 at 02:38:36PM +0100, Andrew Jones wrote:
> On Tue, Nov 03, 2020 at 11:23:54AM +0000, Dave Martin wrote:
> > On Mon, Nov 02, 2020 at 07:50:36PM +0100, Andrew Jones wrote:
> > > The instruction encodings of ID registers are preallocated. Until an
> > > encoding is assigned a purpose the register is RAZ. KVM's general ID
> > > register accessor functions already support both paths, RAZ or not.
> > > If for each ID register we can determine if it's RAZ or not, then all
> > > ID registers can build on the general functions. The register visibility
> > > function allows us to check whether a register should be completely
> > > hidden or not, extending it to also report when the register should
> > > be RAZ or not allows us to use it for ID registers as well.
> >
> > Nit: no statement of what the patch does.
>
> Hmm, I'm not sure what "...extending it to also report when the register
> should be RAZ or not allows us to use it for ID registers as well." is
> missing, other than spelling out that a new flag is being added for the
> extension. Please provide a suggestion.
Well, that's a subordinate clause, not a statement. The containing
sentence is a statement about the _implications_ of doing it, but
nothing says that it is actually done.
Often, a less condensed repeat of the subject line is enough, say,
something like the following, as a separate paragraph at the end:
Check for RAZ visibility in the ID register accessor functions.
(Or rather, there should be a concise statement in the commit message
saying what the patch does, and the subject line should be a suitably
condensed version of _that_.)
You might want to add a simple statement of what is achieved:
This allows the RAZ case to be handled in a generic way
for all system registers.
That makes the intention and value of the patch easy to spot, while the
wordy paragraph is available for anyone who wants to understand the
background and rationale in more detail.
(This is just my view, but I think it's generally helpful to reviewers
to follow this rough pattern -- it makes it easy to skip non-critical
parts of the description and come back to them later on. I might
propose edits in submitting-patches.rst to make this clearer -- and if
they are shot down in flames then I will shut up ;)
>
> >
> > You might want to point out that the introduced REG_RAZ functionality is
> > intentionally not used in this patch.
>
> OK
>
> >
> > > No functional change intended.
> > >
> > > Signed-off-by: Andrew Jones <drjones@xxxxxxxxxx>
> > > ---
> > > arch/arm64/kvm/sys_regs.c | 19 ++++++++++++++++---
> > > arch/arm64/kvm/sys_regs.h | 10 ++++++++++
> > > 2 files changed, 26 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> > > index 6ff0c15531ca..b8822a20b1ea 100644
> > > --- a/arch/arm64/kvm/sys_regs.c
> > > +++ b/arch/arm64/kvm/sys_regs.c
> > > @@ -1153,6 +1153,12 @@ static u64 read_id_reg(const struct kvm_vcpu *vcpu,
> > > return val;
> > > }
> > >
> > > +static unsigned int id_visibility(const struct kvm_vcpu *vcpu,
> > > + const struct sys_reg_desc *r)
> > > +{
> > > + return 0;
> > > +}
> > > +
> > > /* cpufeature ID register access trap handlers */
> > >
> > > static bool __access_id_reg(struct kvm_vcpu *vcpu,
> > > @@ -1171,7 +1177,9 @@ static bool access_id_reg(struct kvm_vcpu *vcpu,
> > > struct sys_reg_params *p,
> > > const struct sys_reg_desc *r)
> > > {
> > > - return __access_id_reg(vcpu, p, r, false);
> > > + bool raz = sysreg_visible_as_raz(vcpu, r);
> > > +
> > > + return __access_id_reg(vcpu, p, r, raz);
> > > }
> > >
> > > static bool access_raz_id_reg(struct kvm_vcpu *vcpu,
> > > @@ -1283,13 +1291,17 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
> > > static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > > const struct kvm_one_reg *reg, void __user *uaddr)
> > > {
> > > - return __get_id_reg(vcpu, rd, uaddr, false);
> > > + bool raz = sysreg_visible_as_raz(vcpu, rd);
> > > +
> > > + return __get_id_reg(vcpu, rd, uaddr, raz);
> > > }
> > >
> > > static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > > const struct kvm_one_reg *reg, void __user *uaddr)
> > > {
> > > - return __set_id_reg(vcpu, rd, uaddr, false);
> > > + bool raz = sysreg_visible_as_raz(vcpu, rd);
> > > +
> > > + return __set_id_reg(vcpu, rd, uaddr, raz);
> > > }
> > >
> > > static int get_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
> > > @@ -1381,6 +1393,7 @@ static bool access_mte_regs(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> > > .access = access_id_reg, \
> > > .get_user = get_id_reg, \
> > > .set_user = set_id_reg, \
> > > + .visibility = id_visibility, \
> >
> > This is just the default for ID_SANITISED, right?
>
> The default was not to have a visibility function. Now it has one. It
> still behaves the same way in this patch, though, because it just returns
> zero.
Ack.
Don't worry about this; again, I was halfway through remembering how the
code in this file worked...
[...]
Cheers
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
