On Fri, Oct 27 2017 at 3:43:26 pm GMT, gengdongjiu <gengdongjiu@xxxxxxxxxx> wrote:
>>
>> On Thu, Oct 26 2017 at 6:07:01 pm BST, Dongjiu Geng <gengdongjiu@xxxxxxxxxx> wrote:
>> > For this matching, current code using the {I,D}FSC range to match
>> > kvm_vcpu_trap_get_fault_type() return value, but
>> > kvm_vcpu_trap_get_fault_type() only return the part {I,D}FSC instead
>> > of whole, so fix this issue
>> >
>> > Value Type
>> > 0x10 FSC_SEA
>> > 0x14 FSC_SEA_TTW0 FSC_SEA_TTW1 FSC_SEA_TTW2 FSC_SEA_TTW3
>> > 0x18 FSC_SECC
>> > 0x1c FSC_SECC_TTW0 FSC_SECC_TTW1 FSC_SECC_TTW2 FSC_SECC_TTW3
>> >
>> > CC: James Morse <james.morse@xxxxxxx>
>> > CC: Tyler Baicar <tbaicar@xxxxxxxxxxxxxx>
>> > Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
[...]
>> > diff --git a/arch/arm64/include/asm/kvm_arm.h
>> > b/arch/arm64/include/asm/kvm_arm.h
>> > index 61d694c..c0f1798 100644
>> > --- a/arch/arm64/include/asm/kvm_arm.h
>> > +++ b/arch/arm64/include/asm/kvm_arm.h
>> > @@ -205,15 +205,9 @@
>> > #define FSC_ACCESS ESR_ELx_FSC_ACCESS
>> > #define FSC_PERM ESR_ELx_FSC_PERM
>> > #define FSC_SEA ESR_ELx_FSC_EXTABT
>> > -#define FSC_SEA_TTW0 (0x14)
>> > -#define FSC_SEA_TTW1 (0x15)
>> > -#define FSC_SEA_TTW2 (0x16)
>> > -#define FSC_SEA_TTW3 (0x17)
>> > +#define FSC_SEA_TTW (0x14)
>> > #define FSC_SECC (0x18)
>> > -#define FSC_SECC_TTW0 (0x1c)
>> > -#define FSC_SECC_TTW1 (0x1d)
>> > -#define FSC_SECC_TTW2 (0x1e)
>> > -#define FSC_SECC_TTW3 (0x1f)
>> > +#define FSC_SECC_TTW (0x1c)
>> >
>> > /* Hyp Prefetch Fault Address Register (HPFAR/HDFAR) */
>> > #define HPFAR_MASK (~UL(0xf))
>> > diff --git a/arch/arm64/include/asm/kvm_emulate.h
>> > b/arch/arm64/include/asm/kvm_emulate.h
>> > index e5df3fc..5cde311 100644
>> > --- a/arch/arm64/include/asm/kvm_emulate.h
>> > +++ b/arch/arm64/include/asm/kvm_emulate.h
>> > @@ -239,15 +239,9 @@ static inline bool kvm_vcpu_dabt_isextabt(const
>> > struct kvm_vcpu *vcpu) {
>> > switch (kvm_vcpu_trap_get_fault_type(vcpu)) {
>> > case FSC_SEA:
>> > - case FSC_SEA_TTW0:
>> > - case FSC_SEA_TTW1:
>> > - case FSC_SEA_TTW2:
>> > - case FSC_SEA_TTW3:
>> > + case FSC_SEA_TTW:
>> > case FSC_SECC:
>> > - case FSC_SECC_TTW0:
>> > - case FSC_SECC_TTW1:
>> > - case FSC_SECC_TTW2:
>> > - case FSC_SECC_TTW3:
>> > + case FSC_SECC_TTW:
>> > return true;
>> > default:
>> > return false;
>>
>> What is the bug you're fixing? From what I can tell, you're simply
>> removing valuable symbols from the kernel, and inventing another one
>> that doesn't exist in the architecture.
>
> Marc, I mean the "kvm_vcpu_trap_get_fault_type(vcpu))" return the
> value of fault type which is not the syndrome value {D,I}FSC.
> the case sentence matches the syndrome value instead of fault type.
> So the two are not match, I mean here has issue.
> So the FSC_SECC_TTW1/ FSC_SECC_TTW2/ FSC_SECC_TTW3 and FSC_SECC_TTW1/
> FSC_SECC_TTW2/ FSC_SECC_TTW3 are dead code.
>
> Below value are syndrome {D,I}FSC value, which is not fault type.
> FSC_SECC_TTW0:
> FSC_SECC_TTW1
> FSC_SECC_TTW2:
> FSC_SECC_TTW3:
> FSC_SEA_TTW1:
> FSC_SEA_TTW2
> FSC_SEA_TTW3:
>
> kvm_vcpu_trap_get_fault_type() will clear the low two bits to zero. So
> I use FSC_SEA_TTW represent "Synchronous external abort on translation
> table walk"
I understand that, and I certainly not keen on adding another "fault
type" for this.
> As we can see the Translation fault type "FSC_FAULT", which does not
> define the "FSC_FAULT0" "FSC_FAULT1" "FSC_FAULT2" "FSC_FAULT3".
> Because the fault type " FSC_FAULT " include the four cases.
Indeed, and I'm still not convinced this is the best thing we have in
the code.
> static inline u8 kvm_vcpu_trap_get_fault(const struct kvm_vcpu *vcpu)
> {
> return kvm_vcpu_get_hsr(vcpu) & 0x3f;
> }
Here you go. This should give you a pretty good idea of how to provide a
proper fix.
Thanks,
M.
--
Jazz is not dead. It just smells funny.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
