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).
> 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.
Can you take a look at my attempted explanation below and then we
can reconsider this?
[...]
>
> >
> > 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@xxxxxxx>
> > ---
> > 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
b) encodings that we unconditionally reflect back to the guest as an
Undef.
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.
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.
Conversely !sys_reg_check_present_or_raz() means that we can just
Undef the guest with no further logic required.
Does this rationale make things clearer? The naming is perhaps
unfortunate.
Cheers,
---Dave
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
