Hi Mark,
On 6/29/20 9:00 PM, Mark Rutland wrote:
On Mon, Jun 29, 2020 at 07:18:41PM +1000, Gavin Shan wrote:
There are a set of inline functions defined in kvm_emulate.h. Those
functions reads ESR from vCPU fault information struct and then operate
on it. So it's tied with vCPU fault information and vCPU struct. It
limits their usage scope.
This detaches these functions from the vCPU struct by introducing an
other set of inline functions in esr.h to manupulate the specified
ESR value. With it, the inline functions defined in kvm_emulate.h
can call these inline functions (in esr.h) instead. This shouldn't
cause any functional changes.
Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
TBH, I'm not sure that this patch makes much sense on its own.
We already use vcpu_get_esr(), which is the bit that'd have to change if
we didn't pass the vcpu around, and the new helpers are just consuming
the value in a sifferent way rather than a necessarily simpler way.
Further comments on that front below.
---
arch/arm64/include/asm/esr.h | 32 +++++++++++++++++++++
arch/arm64/include/asm/kvm_emulate.h | 43 ++++++++++++----------------
2 files changed, 51 insertions(+), 24 deletions(-)
diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 035003acfa87..950204c5fbe1 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -326,6 +326,38 @@ static inline bool esr_is_data_abort(u32 esr)
return ec == ESR_ELx_EC_DABT_LOW || ec == ESR_ELx_EC_DABT_CUR;
}
+#define ESR_DECLARE_CHECK_FUNC(name, field) \
+static inline bool esr_is_##name(u32 esr) \
+{ \
+ return !!(esr & (field)); \
+}
+#define ESR_DECLARE_GET_FUNC(name, mask, shift) \
+static inline u32 esr_get_##name(u32 esr) \
+{ \
+ return ((esr & (mask)) >> (shift)); \
+}
+
+ESR_DECLARE_CHECK_FUNC(il_32bit, ESR_ELx_IL);
+ESR_DECLARE_CHECK_FUNC(condition, ESR_ELx_CV);
+ESR_DECLARE_CHECK_FUNC(dabt_valid, ESR_ELx_ISV);
+ESR_DECLARE_CHECK_FUNC(dabt_sse, ESR_ELx_SSE);
+ESR_DECLARE_CHECK_FUNC(dabt_sf, ESR_ELx_SF);
+ESR_DECLARE_CHECK_FUNC(dabt_s1ptw, ESR_ELx_S1PTW);
+ESR_DECLARE_CHECK_FUNC(dabt_write, ESR_ELx_WNR);
+ESR_DECLARE_CHECK_FUNC(dabt_cm, ESR_ELx_CM);
+
+ESR_DECLARE_GET_FUNC(class, ESR_ELx_EC_MASK, ESR_ELx_EC_SHIFT);
+ESR_DECLARE_GET_FUNC(fault, ESR_ELx_FSC, 0);
+ESR_DECLARE_GET_FUNC(fault_type, ESR_ELx_FSC_TYPE, 0);
+ESR_DECLARE_GET_FUNC(condition, ESR_ELx_COND_MASK, ESR_ELx_COND_SHIFT);
+ESR_DECLARE_GET_FUNC(hvc_imm, ESR_ELx_xVC_IMM_MASK, 0);
+ESR_DECLARE_GET_FUNC(dabt_iss_nisv_sanitized,
+ (ESR_ELx_CM | ESR_ELx_WNR | ESR_ELx_FSC), 0);
+ESR_DECLARE_GET_FUNC(dabt_rd, ESR_ELx_SRT_MASK, ESR_ELx_SRT_SHIFT);
+ESR_DECLARE_GET_FUNC(dabt_as, ESR_ELx_SAS, ESR_ELx_SAS_SHIFT);
+ESR_DECLARE_GET_FUNC(sys_rt, ESR_ELx_SYS64_ISS_RT_MASK,
+ ESR_ELx_SYS64_ISS_RT_SHIFT);
I'm really not keen on this, as I think it's abstracting the problem at
the wrong level, hiding information and making things harder to reason
about rather than abstracting that.
I strongly suspect the right thing to do is use FIELD_GET() in-place in
the functions below, e.g.
!!FIELD_GET(esr, ESR_ELx_IL);
... rather than:
esr_get_il_32bit(esr);
... as that avoids the wrapper entirely, minimizing indirection and
making the codebase simpler to navigate.
For the cases where we *really* want a helper, i'd rather write those
out explicitly, e.g.
It will be no difference except to use FIELD_GET() to make the code
more explicit. Maybe I didn't fully understand your comments here.
Please let me know if something like below is what you expect?
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index c9ba0df47f7d..e8294edcd8f4 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -343,7 +343,7 @@ static __always_inline unsigned int kvm_vcpu_dabt_get_as(const struct kvm_vcpu *
/* This one is not specific to Data Abort */
static __always_inline bool kvm_vcpu_trap_il_is32bit(const struct kvm_vcpu *vcpu)
{
- return !!(kvm_vcpu_get_esr(vcpu) & ESR_ELx_IL);
+ return !!FIELD_GET(kvm_vcpu_get_esr(vcpu), ESR_ELx_IL);
}
If my understanding is correct, I think we needn't change the code
and this patch can be dropped.
#define esr_get_hvc_imm(esr) FIELD_GET(esr, ESR_ELx_xVC_IMM_MASK)
... but I'm not sure if we really need those given these are mostly used
*once* below.
We don't need these for now, but will be needed when the next revision
of async page fault is posted. Lets ignore this requirement for now
because I can revisit it when the async page fault patchset is posted.
That time, we can have accessors defined in esr.h and helpers in
kvm_emulate.h use those accessors. It's similar to what you're suggesting.
#define esr_get_hvc_imm(esr) FIELD_GET(esr, ESR_ELx_xVC_IMM_MASK)
static inline u32 kvm_vcpu_hvc_get_imm(const struct kvm_vcpu *vcpu)
{
return esr_get_hvc_imm(kvm_vcpu_get_esr(vcpu));
}
By the way, it's long way to reach that point because I'm still in the
middle of working on virtualizing SDEI currently.
+
const char *esr_get_class_string(u32 esr);
#endif /* __ASSEMBLY */
diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
index c9ba0df47f7d..9337d90c517f 100644
--- a/arch/arm64/include/asm/kvm_emulate.h
+++ b/arch/arm64/include/asm/kvm_emulate.h
@@ -266,12 +266,8 @@ static __always_inline u32 kvm_vcpu_get_esr(const struct kvm_vcpu *vcpu)
static __always_inline int kvm_vcpu_get_condition(const struct kvm_vcpu *vcpu)
{
- u32 esr = kvm_vcpu_get_esr(vcpu);
-
- if (esr & ESR_ELx_CV)
- return (esr & ESR_ELx_COND_MASK) >> ESR_ELx_COND_SHIFT;
-
- return -1;
+ return esr_is_condition(kvm_vcpu_get_esr(vcpu)) ?
+ esr_get_condition(kvm_vcpu_get_esr(vcpu)) : -1;
}
Do we really need to change the structure of this code? I thought this
was purely about decooupling helpers from the vcpu struct. This could
have stayed as:
static __always_inline int kvm_vcpu_get_condition(const struct kvm_vcpu *vcpu)
{
u32 esr = kvm_vcpu_get_esr(vcpu);
if (esr_is_condition(esr))
return esr_get_condition(esr);
return -1;
}
... or:
static __always_inline int kvm_vcpu_get_condition(const struct kvm_vcpu *vcpu)
{
u32 esr = kvm_vcpu_get_esr(vcpu);
if (FEILD_GET(esr, ESR_ELx_CV))
return FIELD_GET(esr, ESR_ELx_COND_MASK);
return -1;
}
It's not needed to change the structure of the code, but it does
reduce the lines of codes. It's kind of my personal taste :)
[...]
Thanks,
Gavin
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
