* [PATCH v2] KVM: arm/arm64: Fix external abort type matching
[not found] <0184EA26B2509940AA629AE1405DD7F2016E999C@DGGEMA503-MBS.china.huawei.com>
@ 2017-10-27 18:28 ` Marc Zyngier
2017-10-28 5:22 ` gengdongjiu
0 siblings, 1 reply; 4+ messages in thread
From: Marc Zyngier @ 2017-10-27 18:28 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Oct 27 2017 at 3:43:26 pm GMT, gengdongjiu <gengdongjiu@huawei.com> wrote:
>>
>> On Thu, Oct 26 2017 at 6:07:01 pm BST, Dongjiu Geng <gengdongjiu@huawei.com> 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@arm.com>
>> > CC: Tyler Baicar <tbaicar@codeaurora.org>
>> > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
[...]
>> > 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.
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v2] KVM: arm/arm64: Fix external abort type matching
2017-10-27 18:28 ` [PATCH v2] KVM: arm/arm64: Fix external abort type matching Marc Zyngier
@ 2017-10-28 5:22 ` gengdongjiu
0 siblings, 0 replies; 4+ messages in thread
From: gengdongjiu @ 2017-10-28 5:22 UTC (permalink / raw)
To: linux-arm-kernel
On 2017/10/28 2:28, Marc Zyngier wrote:
>> 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.
Ok, thanks.
>
>> 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.
Ok, thanks.
>
>> 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.
OK, got it, thanks
>
> Thanks,
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] KVM: arm/arm64: Fix external abort type matching
@ 2017-10-26 10:07 Dongjiu Geng
2017-10-27 14:55 ` Marc Zyngier
0 siblings, 1 reply; 4+ messages in thread
From: Dongjiu Geng @ 2017-10-26 10:07 UTC (permalink / raw)
To: linux-arm-kernel
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@arm.com>
CC: Tyler Baicar <tbaicar@codeaurora.org>
Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
---
As shown below code:
The kvm_vcpu_trap_get_fault_type() only return {I,D}FSC bit[5]:bit[2], not
the whole {I,D}FSC, but FSC_SEA_TTWx and FSC_SECC_TTWx are the whole {I,D}FSC
value.
static inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vcpu)
{
return kvm_vcpu_get_hsr(vcpu) & ESR_ELx_FSC_TYPE;
}
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_SECC:
case FSC_SECC_TTW0:
case FSC_SECC_TTW1:
case FSC_SECC_TTW2:
case FSC_SECC_TTW3:
return true;
default:
return false;
}
}
---
arch/arm/include/asm/kvm_arm.h | 10 ++--------
arch/arm/include/asm/kvm_emulate.h | 10 ++--------
arch/arm64/include/asm/kvm_arm.h | 10 ++--------
arch/arm64/include/asm/kvm_emulate.h | 10 ++--------
4 files changed, 8 insertions(+), 32 deletions(-)
diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
index c878145..b4b155c 100644
--- a/arch/arm/include/asm/kvm_arm.h
+++ b/arch/arm/include/asm/kvm_arm.h
@@ -188,15 +188,9 @@
#define FSC_ACCESS (0x08)
#define FSC_PERM (0x0c)
#define FSC_SEA (0x10)
-#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 (~0xf)
diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
index 98089ff..aed8279 100644
--- a/arch/arm/include/asm/kvm_emulate.h
+++ b/arch/arm/include/asm/kvm_emulate.h
@@ -205,15 +205,9 @@ static inline bool kvm_vcpu_dabt_isextabt(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;
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;
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v2] KVM: arm/arm64: Fix external abort type matching
2017-10-26 10:07 Dongjiu Geng
@ 2017-10-27 14:55 ` Marc Zyngier
0 siblings, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2017-10-27 14:55 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Oct 26 2017 at 6:07:01 pm BST, Dongjiu Geng <gengdongjiu@huawei.com> 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@arm.com>
> CC: Tyler Baicar <tbaicar@codeaurora.org>
> Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
>
> ---
> As shown below code:
>
> The kvm_vcpu_trap_get_fault_type() only return {I,D}FSC bit[5]:bit[2], not
> the whole {I,D}FSC, but FSC_SEA_TTWx and FSC_SECC_TTWx are the whole {I,D}FSC
> value.
>
> static inline u8 kvm_vcpu_trap_get_fault_type(const struct kvm_vcpu *vcpu)
> {
> return kvm_vcpu_get_hsr(vcpu) & ESR_ELx_FSC_TYPE;
> }
>
> 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_SECC:
> case FSC_SECC_TTW0:
> case FSC_SECC_TTW1:
> case FSC_SECC_TTW2:
> case FSC_SECC_TTW3:
> return true;
> default:
> return false;
> }
> }
> ---
> arch/arm/include/asm/kvm_arm.h | 10 ++--------
> arch/arm/include/asm/kvm_emulate.h | 10 ++--------
> arch/arm64/include/asm/kvm_arm.h | 10 ++--------
> arch/arm64/include/asm/kvm_emulate.h | 10 ++--------
> 4 files changed, 8 insertions(+), 32 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index c878145..b4b155c 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -188,15 +188,9 @@
> #define FSC_ACCESS (0x08)
> #define FSC_PERM (0x0c)
> #define FSC_SEA (0x10)
> -#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 (~0xf)
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 98089ff..aed8279 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -205,15 +205,9 @@ static inline bool kvm_vcpu_dabt_isextabt(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;
> 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.
If you were instead adding an accessor for the full fault syndrome, then
that would be a useful addition. But as it is, this patch looks
completely pointless.
Thanks,
M.
--
Jazz is not dead. It just smells funny.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-28 5:22 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <0184EA26B2509940AA629AE1405DD7F2016E999C@DGGEMA503-MBS.china.huawei.com>
2017-10-27 18:28 ` [PATCH v2] KVM: arm/arm64: Fix external abort type matching Marc Zyngier
2017-10-28 5:22 ` gengdongjiu
2017-10-26 10:07 Dongjiu Geng
2017-10-27 14:55 ` Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).