Re: [RFC PATCH 09/16] KVM: arm64: Allow ID registers to by dynamically read-as-zero

[Christoffer Dall <christoffer.dall@arm.com>]

On Wed, Aug 08, 2018 at 10:11:11AM +0100, Dave Martin wrote:
> On Tue, Aug 07, 2018 at 09:35:12PM +0200, Christoffer Dall wrote:
> > On Tue, Aug 07, 2018 at 12:09:58PM +0100, Dave Martin wrote:
> > > On Mon, Aug 06, 2018 at 03:03:24PM +0200, Christoffer Dall wrote:
> > > > Hi Dave,
> > > > 
> > > > I think there's a typo in the subject "to be" rather than "to by".
> > > > 
> > > > On Thu, Jun 21, 2018 at 03:57:33PM +0100, Dave Martin wrote:
> > > > > When a feature-dependent ID register is hidden from the guest, it
> > > > > needs to exhibit read-as-zero behaviour as defined by the Arm
> > > > > architecture, rather than appearing to be entirely absent.
> > > > > 
> > > > > This patch updates the ID register emulation logic to make use of
> > > > > the new check_present() method to determine whether the register
> > > > > should read as zero instead of yielding the host's sanitised
> > > > > value.  Because currently a false result from this method truncates
> > > > > the trap call chain before the sysreg's emulate method() is called,
> > > > > a flag is added to distinguish this special case, and helpers are
> > > > > refactored appropriately.
> > > > 
> > > > I don't understand this last sentence.
> > > > 
> > > > And I'm not really sure I understand the code either.
> > > > 
> > > > I can't seem to see any registers which are defined as !present && !raz,
> > > > which is what I thought this feature was all about.
> > > 
> > > !present and !raz is the default behaviour for everything that is not
> > > ID-register-like.  This patch is adding the !present && raz case (though
> > > that may not be a helpful way to descibe it ... see below).
> > 
> > Fair enough, but I don't really see why you need to classify a register
> > as !present && raz, because raz implies present AFAICT.
> > 
> > > 
> > > > In other words, what is the benefit of this more generic method as
> > > > opposed to having a wrapper around read_id_reg() for read_sve_id_reg()
> > > > which sets RAZ if there is no support for SVE in this context?
> > > 
> > > There may be other ways to factor this.  I can't now remember whay I
> > > went with this particular approach, except that I vaguely recall
> > > hitting some obstacles when doing things another way.
> > 
> > What I don't much care for is that we now seem to be mixing the concept
> > of whether something is present and the value it returns if it is
> > present in the overall system register handling logic.  And I don't
> > understand why this is a requirement.
> > 
> > > 
> > > Can you take a look at my attempted explanation below and then we
> > > can reconsider this?
> > 
> > Sure, see my comments below.
> > 
> > > 
> > > [...]
> > > 
> > > > 
> > > > > 
> > > > > This invloves some trivial updates to pass the vcpu pointer down
> > > > > into the ID register emulation/access functions.
> > > > > 
> > > > > A new ID_SANITISED_IF() macro is defined for declaring
> > > > > conditionally visible ID registers.
> > > > > 
> > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > > > > ---
> > > > >  arch/arm64/kvm/sys_regs.c | 51 ++++++++++++++++++++++++++++++-----------------
> > > > >  arch/arm64/kvm/sys_regs.h | 11 ++++++++++
> > > > >  2 files changed, 44 insertions(+), 18 deletions(-)
> > > > > 
> > > 
> > > [...]
> > > 
> > > > > @@ -1840,7 +1855,7 @@ static int emulate_cp(struct kvm_vcpu *vcpu,
> > > > >  
> > > > >  	r = find_reg(params, table, num);
> > > > >  
> > > > > -	if (likely(r) && sys_reg_present(vcpu, r)) {
> > > > > +	if (likely(r) && sys_reg_present_or_raz(vcpu, r)) {
> > > > >  		perform_access(vcpu, params, r);
> > > > >  		return 0;
> > > > >  	}
> > > > > @@ -2016,7 +2031,7 @@ static int emulate_sys_reg(struct kvm_vcpu *vcpu,
> > > > >  	if (!r)
> > > > >  		r = find_reg(params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
> > > > >  
> > > > > -	if (likely(r) && sys_reg_present(vcpu, r)) {
> > > > > +	if (likely(r) && sys_reg_present_or_raz(vcpu, r)) {
> > > > >  		perform_access(vcpu, params, r);
> > > > >  	} else {
> > > > >  		kvm_err("Unsupported guest sys_reg access at: %lx\n",
> > > > > @@ -2313,7 +2328,7 @@ int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
> > > > >  	if (!r)
> > > > >  		return get_invariant_sys_reg(reg->id, uaddr);
> > > > >  
> > > > > -	if (!sys_reg_present(vcpu, r))
> > > > > +	if (!sys_reg_present_or_raz(vcpu, r))
> > > > >  		return -ENOENT;
> > > > >  
> > > > >  	if (r->get_user)
> > > > > @@ -2337,7 +2352,7 @@ int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg
> > > > >  	if (!r)
> > > > >  		return set_invariant_sys_reg(reg->id, uaddr);
> > > > >  
> > > > > -	if (!sys_reg_present(vcpu, r))
> > > > > +	if (!sys_reg_present_or_raz(vcpu, r))
> > > > >  		return -ENOENT;
> > > 
> > > On Wed, Jul 25, 2018 at 04:46:55PM +0100, Alex Bennée wrote:
> > > > It's all very well being raz, but shouldn't you catch this further down
> > > > and not attempt to write the register that doesn't exist?
> > > 
> > > To be clear, is this a question about factoring, or do you think there's
> > > a bug here?
> > > 
> > > 
> > > In response to both sets of comments, I think the way the code is
> > > factored is causing some confusion.
> > > 
> > > The idea in my head was something like this:
> > > 
> > > System register encodings fall into two classes:
> > > 
> > >  a) encodings that we emulate in some way
> > 
> > this is present, then
> > 
> > >  b) encodings that we unconditionally reflect back to the guest as an
> > >     Undef.
> > 
> > this is !present, then
> > 
> > The previous change made this a configurable thing as opposed to a
> > static compile time thing, right?
> > > 
> > > Architecturally defined system registers fall into two classes:
> > > 
> > >  i) registers whose removal turns all accesses into an Undef
> > >  ii) registers whose removal exhibits some other behaviour.
> > 
> > I'm not sure what you mean by 'removal' here, and which architectural
> > concept that relates to, which makes it hard for me to parse the rest
> > here...
> > 
> > > 
> > > These two classifications overlap somwehat.
> > > 
> > > 
> > > From an emulation perspective, (b), and (i) in the "register not
> > > present" case, look the same: we trap the register and reflect an Undef
> > > directly back to the guest with no further action required.
> > > 
> > > From an emulation perspective, (a) and (ii) are also somewhat the
> > > same: we need to emulate something, although precisely what we need
> > > to do depends on which register it is and on whether the register is
> > > deemed present or not.
> > > 
> > > sys_reg_check_present_or_raz() thus means "falls under (a) or (ii)",
> > > i.e., some emulation is required and we need to call sysreg-specific
> > > methods to figure out precisely what we need to do.
> > 
> > yes, but we've always had that without the "or_raz" stuff at the lookup
> > level.  What has changed?
> > 
> > > 
> > > Conversely !sys_reg_check_present_or_raz() means that we can just
> > > Undef the guest with no further logic required.
> > 
> > Yes, but that's the same as !present, because raz then implies present,
> > see above.
> > 
> > > 
> > > Does this rationale make things clearer?  The naming is perhaps
> > > unfortunate.
> > > 
> > 
> > Unfortunately not so much.  I have a strong feeling you want to move
> > anything relating to something being emulated as RAZ/RAO/something else
> > into sysreg specific functions.
> 
> 
> The way I integrated this seemed natural at the time, but your
> reaction suggests that it may not be the right approach...
> 
> 
> At its heart, I'm trying to abstract out the special behaviour of
> all unallocated ID registers, so that we can decide at runtime which
> ones to hide fro the guest: within the ID register block, each
> unallocated register becomes RAZ, not UNDEFINED as would be the case
> for other system registers, so we need to capture both behaviours.
> 
> 
> If we want a generic handler for all the ID registers in sys_regs.c,
> then we need a flag to tell us whether to pass the ID register through
> from cpufeatures or to make it appear as zero.
> 
> For ZCR_EL1 on the other hand, we really want attempts to access that
> to reflect an Undef to the guest if we are pretending that SVE is not
> implemented.  Again if we want to filter out some sysregs in a runtime-
> controlled way, we need a flag to tell us whether to filter out a
> particular register.
> 
> So, we have two specific ways of rolling a feature that is really
> implemented in the hardware back to the ARMv8-A behaviour (RAZ for ID
> registers and Undef for anything else).
> 
> I tried to group these under a single concept of presence/absence,
> which is what check_present() is intended to check.  However, we
> don't really want ID registers to Undef when !check_present(): this
> is bodged around with the additional SR_RAZ_IF_ABSENT flag so that
> the decision about whether to make the register Undef or not can
> be made generic.
> 
> 
> It seems that this attempt at generalisation is creating more confusion
> than it solves, so I may abandon it and just handle ID_AA64PFR0_EL1 and
> ID_AA64ZFR0_EL1 specially.
> 
> When/if we've done that a few times for different features, it may
> become clearer what any generic framework for doing it should look
> like...
> 

I think there's just a reasonable amount of complexity in sys_regs.c
already, and the naming and concepts aren't clear from the reading the
code as it stands now.

I think it would probably help to flag things as ID registers as opposed
to 'RAZ' behavior, beccause it's clear we're then trying to support an
architectural concept.

However, I think of the generic infrastructre in sys_regs.c to be
worried about KVM-specifics, and the implementation of the
emulate/access functions to be concerned with architectural concepts.
That's just how I've always thought abuot this code.

Therefore, I would suggest wrapping all ID register accesses through
common ID register access functions (read/write) which handles the RAZ
case.

At least, I'd like to see if that becomes too horrible before taking
this route.

Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm