* [PATCH 0/3] KVM: arm64: Failback on unsupported huge pages
@ 2020-10-25 0:27 Gavin Shan
2020-10-25 0:27 ` [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled Gavin Shan
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Gavin Shan @ 2020-10-25 0:27 UTC (permalink / raw)
To: kvmarm; +Cc: will, linux-kernel, maz
Guest fails to boot when the memory is backed up by hugetlbfs regions,
which correspond to contiguous PMDs or PTEs. For example, the guest
fails to boot when its memory is backed up by 64KB hugetlbfs pages.
The first two patches are sorts of cleanup, not introducing any logical
changes. The last patch resolves the issue by fail the unsupported huge
page sizes back to nearby one. Ideally, we teach the stage-2 page table
to use contiguous mapping in this case, but the page-table walker doesn't
it well and needs some sort of reworks and I will do that in the future.
Gavin Shan (3):
KVM: arm64: Check if 52-bits PA is enabled
KVM: arm64: Don't map PUD huge page if it's not available
KVM: arm64: Failback on unsupported huge page sizes
arch/arm64/kvm/hyp/pgtable.c | 10 ++++++----
arch/arm64/kvm/mmu.c | 12 +++++++++++-
2 files changed, 17 insertions(+), 5 deletions(-)
--
2.23.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled
2020-10-25 0:27 [PATCH 0/3] KVM: arm64: Failback on unsupported huge pages Gavin Shan
@ 2020-10-25 0:27 ` Gavin Shan
2020-10-25 9:52 ` Marc Zyngier
2020-10-25 0:27 ` [PATCH 2/3] KVM: arm64: Don't map PUD huge page if it's not available Gavin Shan
2020-10-25 0:27 ` [PATCH 3/3] KVM: arm64: Failback on unsupported huge page sizes Gavin Shan
2 siblings, 1 reply; 14+ messages in thread
From: Gavin Shan @ 2020-10-25 0:27 UTC (permalink / raw)
To: kvmarm; +Cc: will, linux-kernel, maz
The 52-bits physical address is disabled until CONFIG_ARM64_PA_BITS_52
is chosen. This uses option for that check, to avoid the unconditional
check on PAGE_SHIFT in the hot path and thus save some CPU cycles.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
arch/arm64/kvm/hyp/pgtable.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
index 0cdf6e461cbd..fd850353ee89 100644
--- a/arch/arm64/kvm/hyp/pgtable.c
+++ b/arch/arm64/kvm/hyp/pgtable.c
@@ -132,8 +132,9 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
{
u64 pa = pte & KVM_PTE_ADDR_MASK;
- if (PAGE_SHIFT == 16)
- pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
+#ifdef CONFIG_ARM64_PA_BITS_52
+ pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
+#endif
return pa;
}
@@ -142,8 +143,9 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
{
kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
- if (PAGE_SHIFT == 16)
- pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
+#ifdef CONFIG_ARM64_PA_BITS_52
+ pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
+#endif
return pte;
}
--
2.23.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/3] KVM: arm64: Don't map PUD huge page if it's not available
2020-10-25 0:27 [PATCH 0/3] KVM: arm64: Failback on unsupported huge pages Gavin Shan
2020-10-25 0:27 ` [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled Gavin Shan
@ 2020-10-25 0:27 ` Gavin Shan
2020-10-25 10:05 ` Marc Zyngier
2020-10-25 0:27 ` [PATCH 3/3] KVM: arm64: Failback on unsupported huge page sizes Gavin Shan
2 siblings, 1 reply; 14+ messages in thread
From: Gavin Shan @ 2020-10-25 0:27 UTC (permalink / raw)
To: kvmarm; +Cc: will, linux-kernel, maz
PUD huge page isn't available when CONFIG_ARM64_4K_PAGES is disabled.
In this case, we needn't try to map the memory through PUD huge pages
to save some CPU cycles in the hot path.
This also corrects the code style issue, which was introduced by
commit <523b3999e5f6> ("KVM: arm64: Try PMD block mappings if PUD mappings
are not supported").
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
arch/arm64/kvm/mmu.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index a816cb8e619b..0f51585adc04 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -787,9 +787,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
vma_shift = PAGE_SHIFT;
}
+#ifdef CONFIG_ARM64_4K_PAGES
if (vma_shift == PUD_SHIFT &&
!fault_supports_stage2_huge_mapping(memslot, hva, PUD_SIZE))
- vma_shift = PMD_SHIFT;
+ vma_shift = PMD_SHIFT;
+#endif
if (vma_shift == PMD_SHIFT &&
!fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
--
2.23.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 3/3] KVM: arm64: Failback on unsupported huge page sizes
2020-10-25 0:27 [PATCH 0/3] KVM: arm64: Failback on unsupported huge pages Gavin Shan
2020-10-25 0:27 ` [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled Gavin Shan
2020-10-25 0:27 ` [PATCH 2/3] KVM: arm64: Don't map PUD huge page if it's not available Gavin Shan
@ 2020-10-25 0:27 ` Gavin Shan
2020-10-25 10:48 ` Marc Zyngier
2 siblings, 1 reply; 14+ messages in thread
From: Gavin Shan @ 2020-10-25 0:27 UTC (permalink / raw)
To: kvmarm; +Cc: will, linux-kernel, maz
The huge page could be mapped through multiple contiguous PMDs or PTEs.
The corresponding huge page sizes aren't supported by the page table
walker currently.
This fails the unsupported huge page sizes to the near one. Otherwise,
the guest can't boot successfully: CONT_PMD_SHIFT and CONT_PTE_SHIFT
fail back to PMD_SHIFT and PAGE_SHIFT separately.
Signed-off-by: Gavin Shan <gshan@redhat.com>
---
arch/arm64/kvm/mmu.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index 0f51585adc04..81cbdc368246 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -793,12 +793,20 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
vma_shift = PMD_SHIFT;
#endif
+ if (vma_shift == CONT_PMD_SHIFT)
+ vma_shift = PMD_SHIFT;
+
if (vma_shift == PMD_SHIFT &&
!fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
force_pte = true;
vma_shift = PAGE_SHIFT;
}
+ if (vma_shift == CONT_PTE_SHIFT) {
+ force_pte = true;
+ vma_shift = PAGE_SHIFT;
+ }
+
vma_pagesize = 1UL << vma_shift;
if (vma_pagesize == PMD_SIZE || vma_pagesize == PUD_SIZE)
fault_ipa &= ~(vma_pagesize - 1);
--
2.23.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled
2020-10-25 0:27 ` [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled Gavin Shan
@ 2020-10-25 9:52 ` Marc Zyngier
2020-10-25 22:23 ` Gavin Shan
0 siblings, 1 reply; 14+ messages in thread
From: Marc Zyngier @ 2020-10-25 9:52 UTC (permalink / raw)
To: Gavin Shan; +Cc: will, kvmarm, linux-kernel
On Sun, 25 Oct 2020 01:27:37 +0100,
Gavin Shan <gshan@redhat.com> wrote:
>
> The 52-bits physical address is disabled until CONFIG_ARM64_PA_BITS_52
> is chosen. This uses option for that check, to avoid the unconditional
> check on PAGE_SHIFT in the hot path and thus save some CPU cycles.
PAGE_SHIFT is known at compile time, and this code is dropped by the
compiler if the selected page size is not 64K. This patch really only
makes the code slightly less readable and the "CPU cycles" argument
doesn't hold at all.
So what are you trying to solve exactly?
M.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> arch/arm64/kvm/hyp/pgtable.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
> index 0cdf6e461cbd..fd850353ee89 100644
> --- a/arch/arm64/kvm/hyp/pgtable.c
> +++ b/arch/arm64/kvm/hyp/pgtable.c
> @@ -132,8 +132,9 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
> {
> u64 pa = pte & KVM_PTE_ADDR_MASK;
>
> - if (PAGE_SHIFT == 16)
> - pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
> +#ifdef CONFIG_ARM64_PA_BITS_52
> + pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
> +#endif
>
> return pa;
> }
> @@ -142,8 +143,9 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
> {
> kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
>
> - if (PAGE_SHIFT == 16)
> - pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
> +#ifdef CONFIG_ARM64_PA_BITS_52
> + pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
> +#endif
>
> return pte;
> }
> --
> 2.23.0
>
>
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Don't map PUD huge page if it's not available
2020-10-25 0:27 ` [PATCH 2/3] KVM: arm64: Don't map PUD huge page if it's not available Gavin Shan
@ 2020-10-25 10:05 ` Marc Zyngier
2020-10-25 22:27 ` Gavin Shan
0 siblings, 1 reply; 14+ messages in thread
From: Marc Zyngier @ 2020-10-25 10:05 UTC (permalink / raw)
To: Gavin Shan; +Cc: will, kvmarm, linux-kernel
On Sun, 25 Oct 2020 01:27:38 +0100,
Gavin Shan <gshan@redhat.com> wrote:
>
> PUD huge page isn't available when CONFIG_ARM64_4K_PAGES is disabled.
> In this case, we needn't try to map the memory through PUD huge pages
> to save some CPU cycles in the hot path.
>
> This also corrects the code style issue, which was introduced by
> commit <523b3999e5f6> ("KVM: arm64: Try PMD block mappings if PUD mappings
> are not supported").
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> arch/arm64/kvm/mmu.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index a816cb8e619b..0f51585adc04 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -787,9 +787,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> vma_shift = PAGE_SHIFT;
> }
>
> +#ifdef CONFIG_ARM64_4K_PAGES
> if (vma_shift == PUD_SHIFT &&
> !fault_supports_stage2_huge_mapping(memslot, hva, PUD_SIZE))
> - vma_shift = PMD_SHIFT;
> + vma_shift = PMD_SHIFT;
> +#endif
>
> if (vma_shift == PMD_SHIFT &&
> !fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
I really don't buy the "CPU cycles" argument here either. Can you
actually measure any difference here?
You have taken a fault, gone through a full guest exit, triaged it,
and are about to into a page mapping operation which may result in a
TLBI, and reenter the guest. It only happen a handful of times per
page over the lifetime of the guest unless you start swapping. Hot
path? I don't think so.
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: arm64: Failback on unsupported huge page sizes
2020-10-25 0:27 ` [PATCH 3/3] KVM: arm64: Failback on unsupported huge page sizes Gavin Shan
@ 2020-10-25 10:48 ` Marc Zyngier
2020-10-25 23:04 ` Gavin Shan
0 siblings, 1 reply; 14+ messages in thread
From: Marc Zyngier @ 2020-10-25 10:48 UTC (permalink / raw)
To: Gavin Shan; +Cc: will, kvmarm, linux-kernel
On Sun, 25 Oct 2020 01:27:39 +0100,
Gavin Shan <gshan@redhat.com> wrote:
>
> The huge page could be mapped through multiple contiguous PMDs or PTEs.
> The corresponding huge page sizes aren't supported by the page table
> walker currently.
>
> This fails the unsupported huge page sizes to the near one. Otherwise,
> the guest can't boot successfully: CONT_PMD_SHIFT and CONT_PTE_SHIFT
> fail back to PMD_SHIFT and PAGE_SHIFT separately.
>
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
> arch/arm64/kvm/mmu.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index 0f51585adc04..81cbdc368246 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -793,12 +793,20 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> vma_shift = PMD_SHIFT;
> #endif
>
> + if (vma_shift == CONT_PMD_SHIFT)
> + vma_shift = PMD_SHIFT;
> +
> if (vma_shift == PMD_SHIFT &&
> !fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
> force_pte = true;
> vma_shift = PAGE_SHIFT;
> }
>
> + if (vma_shift == CONT_PTE_SHIFT) {
> + force_pte = true;
> + vma_shift = PAGE_SHIFT;
> + }
> +
> vma_pagesize = 1UL << vma_shift;
> if (vma_pagesize == PMD_SIZE || vma_pagesize == PUD_SIZE)
> fault_ipa &= ~(vma_pagesize - 1);
Yup, nice catch. However, I think we should take this opportunity to
rationalise the logic here, and catch future discrepancies (should
someone add contiguous PUD or something similarly silly). How about
something like this (untested):
diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
index cc323d96c9d4..d9a13a8a82e0 100644
--- a/arch/arm64/kvm/mmu.c
+++ b/arch/arm64/kvm/mmu.c
@@ -787,14 +787,31 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
vma_shift = PAGE_SHIFT;
}
- if (vma_shift == PUD_SHIFT &&
- !fault_supports_stage2_huge_mapping(memslot, hva, PUD_SIZE))
- vma_shift = PMD_SHIFT;
+ switch (vma_shift) {
+ case PUD_SHIFT:
+ if (fault_supports_stage2_huge_mapping(memslot, hva, PUD_SIZE))
+ break;
+ fallthrough;
- if (vma_shift == PMD_SHIFT &&
- !fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
- force_pte = true;
+ case CONT_PMD_SHIFT:
+ vma_shift = PMD_SHIFT;
+ fallthrough;
+
+ case PMD_SHIFT:
+ if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE))
+ break;
+ fallthrough;
+
+ case CONT_PTE_SHIFT:
vma_shift = PAGE_SHIFT;
+ force_pte = true;
+ fallthrough;
+
+ case PAGE_SHIFT:
+ break;
+
+ default:
+ WARN_ONCE(1, "Unknown vma_shift %d", vma_shift);
}
vma_pagesize = 1UL << vma_shift;
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled
2020-10-25 9:52 ` Marc Zyngier
@ 2020-10-25 22:23 ` Gavin Shan
2020-10-26 8:40 ` Will Deacon
2020-10-26 8:53 ` Marc Zyngier
0 siblings, 2 replies; 14+ messages in thread
From: Gavin Shan @ 2020-10-25 22:23 UTC (permalink / raw)
To: Marc Zyngier; +Cc: will, kvmarm, linux-kernel
Hi Marc,
On 10/25/20 8:52 PM, Marc Zyngier wrote:
> On Sun, 25 Oct 2020 01:27:37 +0100,
> Gavin Shan <gshan@redhat.com> wrote:
>>
>> The 52-bits physical address is disabled until CONFIG_ARM64_PA_BITS_52
>> is chosen. This uses option for that check, to avoid the unconditional
>> check on PAGE_SHIFT in the hot path and thus save some CPU cycles.
>
> PAGE_SHIFT is known at compile time, and this code is dropped by the
> compiler if the selected page size is not 64K. This patch really only
> makes the code slightly less readable and the "CPU cycles" argument
> doesn't hold at all.
>
> So what are you trying to solve exactly?
>
There are two points covered by the patch: (1) The 52-bits physical address
is visible only when CONFIG_ARM64_PA_BITS_52 is enabled in arch/arm64 code.
The code looks consistent with this option used here. (2) I had the assumption
that gcc doesn't optimize the code and PAGE_SHIFT is always checked in order
to get higher 4 physical address bits, but you said gcc should optimize the
code accordingly. However, it would be still nice to make the code explicit.
Thanks,
Gavin
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> arch/arm64/kvm/hyp/pgtable.c | 10 ++++++----
>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/hyp/pgtable.c b/arch/arm64/kvm/hyp/pgtable.c
>> index 0cdf6e461cbd..fd850353ee89 100644
>> --- a/arch/arm64/kvm/hyp/pgtable.c
>> +++ b/arch/arm64/kvm/hyp/pgtable.c
>> @@ -132,8 +132,9 @@ static u64 kvm_pte_to_phys(kvm_pte_t pte)
>> {
>> u64 pa = pte & KVM_PTE_ADDR_MASK;
>>
>> - if (PAGE_SHIFT == 16)
>> - pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
>> +#ifdef CONFIG_ARM64_PA_BITS_52
>> + pa |= FIELD_GET(KVM_PTE_ADDR_51_48, pte) << 48;
>> +#endif
>>
>> return pa;
>> }
>> @@ -142,8 +143,9 @@ static kvm_pte_t kvm_phys_to_pte(u64 pa)
>> {
>> kvm_pte_t pte = pa & KVM_PTE_ADDR_MASK;
>>
>> - if (PAGE_SHIFT == 16)
>> - pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
>> +#ifdef CONFIG_ARM64_PA_BITS_52
>> + pte |= FIELD_PREP(KVM_PTE_ADDR_51_48, pa >> 48);
>> +#endif
>>
>> return pte;
>> }
>> --
>> 2.23.0
>>
>>
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/3] KVM: arm64: Don't map PUD huge page if it's not available
2020-10-25 10:05 ` Marc Zyngier
@ 2020-10-25 22:27 ` Gavin Shan
0 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2020-10-25 22:27 UTC (permalink / raw)
To: Marc Zyngier; +Cc: will, kvmarm, linux-kernel
Hi Marc,
On 10/25/20 9:05 PM, Marc Zyngier wrote:
> On Sun, 25 Oct 2020 01:27:38 +0100,
> Gavin Shan <gshan@redhat.com> wrote:
>>
>> PUD huge page isn't available when CONFIG_ARM64_4K_PAGES is disabled.
>> In this case, we needn't try to map the memory through PUD huge pages
>> to save some CPU cycles in the hot path.
>>
>> This also corrects the code style issue, which was introduced by
>> commit <523b3999e5f6> ("KVM: arm64: Try PMD block mappings if PUD mappings
>> are not supported").
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> arch/arm64/kvm/mmu.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index a816cb8e619b..0f51585adc04 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -787,9 +787,11 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>> vma_shift = PAGE_SHIFT;
>> }
>>
>> +#ifdef CONFIG_ARM64_4K_PAGES
>> if (vma_shift == PUD_SHIFT &&
>> !fault_supports_stage2_huge_mapping(memslot, hva, PUD_SIZE))
>> - vma_shift = PMD_SHIFT;
>> + vma_shift = PMD_SHIFT;
>> +#endif
>>
>> if (vma_shift == PMD_SHIFT &&
>> !fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
>
>
> I really don't buy the "CPU cycles" argument here either. Can you
> actually measure any difference here?
>
> You have taken a fault, gone through a full guest exit, triaged it,
> and are about to into a page mapping operation which may result in a
> TLBI, and reenter the guest. It only happen a handful of times per
> page over the lifetime of the guest unless you start swapping. Hot
> path? I don't think so.
>
Thanks for the explanation. Agreed and I will drop this in v2.
Thanks,
Gavin
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: arm64: Failback on unsupported huge page sizes
2020-10-25 10:48 ` Marc Zyngier
@ 2020-10-25 23:04 ` Gavin Shan
2020-10-26 8:55 ` Marc Zyngier
0 siblings, 1 reply; 14+ messages in thread
From: Gavin Shan @ 2020-10-25 23:04 UTC (permalink / raw)
To: Marc Zyngier; +Cc: will, kvmarm, linux-kernel
Hi Marc,
On 10/25/20 9:48 PM, Marc Zyngier wrote:
> On Sun, 25 Oct 2020 01:27:39 +0100,
> Gavin Shan <gshan@redhat.com> wrote:
>>
>> The huge page could be mapped through multiple contiguous PMDs or PTEs.
>> The corresponding huge page sizes aren't supported by the page table
>> walker currently.
>>
>> This fails the unsupported huge page sizes to the near one. Otherwise,
>> the guest can't boot successfully: CONT_PMD_SHIFT and CONT_PTE_SHIFT
>> fail back to PMD_SHIFT and PAGE_SHIFT separately.
>>
>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>> ---
>> arch/arm64/kvm/mmu.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>> index 0f51585adc04..81cbdc368246 100644
>> --- a/arch/arm64/kvm/mmu.c
>> +++ b/arch/arm64/kvm/mmu.c
>> @@ -793,12 +793,20 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>> vma_shift = PMD_SHIFT;
>> #endif
>>
>> + if (vma_shift == CONT_PMD_SHIFT)
>> + vma_shift = PMD_SHIFT;
>> +
>> if (vma_shift == PMD_SHIFT &&
>> !fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
>> force_pte = true;
>> vma_shift = PAGE_SHIFT;
>> }
>>
>> + if (vma_shift == CONT_PTE_SHIFT) {
>> + force_pte = true;
>> + vma_shift = PAGE_SHIFT;
>> + }
>> +
>> vma_pagesize = 1UL << vma_shift;
>> if (vma_pagesize == PMD_SIZE || vma_pagesize == PUD_SIZE)
>> fault_ipa &= ~(vma_pagesize - 1);
>
> Yup, nice catch. However, I think we should take this opportunity to
> rationalise the logic here, and catch future discrepancies (should
> someone add contiguous PUD or something similarly silly). How about
> something like this (untested):
>
Yeah, I started the work to support contiguous PMDs/PTEs, but I'm not
sure when I can post the patches for review as my time becomes a bit
fragmented recently. At least, I need focus on "async page fault" in
the coming weeks :)
Thanks for the suggested code and it worked for me. I'll post v2 to
integrate them. However, I would like to drop PATCH[1] and PATCH[2]
as I really don't have strong reasons to have them.
Thanks,
Gavin
> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
> index cc323d96c9d4..d9a13a8a82e0 100644
> --- a/arch/arm64/kvm/mmu.c
> +++ b/arch/arm64/kvm/mmu.c
> @@ -787,14 +787,31 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> vma_shift = PAGE_SHIFT;
> }
>
> - if (vma_shift == PUD_SHIFT &&
> - !fault_supports_stage2_huge_mapping(memslot, hva, PUD_SIZE))
> - vma_shift = PMD_SHIFT;
> + switch (vma_shift) {
> + case PUD_SHIFT:
> + if (fault_supports_stage2_huge_mapping(memslot, hva, PUD_SIZE))
> + break;
> + fallthrough;
>
> - if (vma_shift == PMD_SHIFT &&
> - !fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
> - force_pte = true;
> + case CONT_PMD_SHIFT:
> + vma_shift = PMD_SHIFT;
> + fallthrough;
> +
> + case PMD_SHIFT:
> + if (fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE))
> + break;
> + fallthrough;
> +
> + case CONT_PTE_SHIFT:
> vma_shift = PAGE_SHIFT;
> + force_pte = true;
> + fallthrough;
> +
> + case PAGE_SHIFT:
> + break;
> +
> + default:
> + WARN_ONCE(1, "Unknown vma_shift %d", vma_shift);
> }
>
> vma_pagesize = 1UL << vma_shift;
>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled
2020-10-25 22:23 ` Gavin Shan
@ 2020-10-26 8:40 ` Will Deacon
2020-10-26 8:53 ` Marc Zyngier
1 sibling, 0 replies; 14+ messages in thread
From: Will Deacon @ 2020-10-26 8:40 UTC (permalink / raw)
To: Gavin Shan; +Cc: Marc Zyngier, kvmarm, linux-kernel
On Mon, Oct 26, 2020 at 09:23:31AM +1100, Gavin Shan wrote:
> On 10/25/20 8:52 PM, Marc Zyngier wrote:
> > On Sun, 25 Oct 2020 01:27:37 +0100,
> > Gavin Shan <gshan@redhat.com> wrote:
> > >
> > > The 52-bits physical address is disabled until CONFIG_ARM64_PA_BITS_52
> > > is chosen. This uses option for that check, to avoid the unconditional
> > > check on PAGE_SHIFT in the hot path and thus save some CPU cycles.
> >
> > PAGE_SHIFT is known at compile time, and this code is dropped by the
> > compiler if the selected page size is not 64K. This patch really only
> > makes the code slightly less readable and the "CPU cycles" argument
> > doesn't hold at all.
> >
> > So what are you trying to solve exactly?
> >
>
> There are two points covered by the patch: (1) The 52-bits physical address
> is visible only when CONFIG_ARM64_PA_BITS_52 is enabled in arch/arm64 code.
> The code looks consistent with this option used here. (2) I had the assumption
> that gcc doesn't optimize the code and PAGE_SHIFT is always checked in order
> to get higher 4 physical address bits, but you said gcc should optimize the
> code accordingly. However, it would be still nice to make the code explicit.
I don't know: adding #ifdef CONFIG_ lines just reduces the coverage we
get from CI, so unless the code is actually causing a problem then I'd be
inclined to leave it as-is.
Will
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled
2020-10-25 22:23 ` Gavin Shan
2020-10-26 8:40 ` Will Deacon
@ 2020-10-26 8:53 ` Marc Zyngier
2020-10-26 22:48 ` Gavin Shan
1 sibling, 1 reply; 14+ messages in thread
From: Marc Zyngier @ 2020-10-26 8:53 UTC (permalink / raw)
To: Gavin Shan; +Cc: will, kvmarm, linux-kernel
On 2020-10-25 22:23, Gavin Shan wrote:
> Hi Marc,
>
> On 10/25/20 8:52 PM, Marc Zyngier wrote:
>> On Sun, 25 Oct 2020 01:27:37 +0100,
>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>> The 52-bits physical address is disabled until
>>> CONFIG_ARM64_PA_BITS_52
>>> is chosen. This uses option for that check, to avoid the
>>> unconditional
>>> check on PAGE_SHIFT in the hot path and thus save some CPU cycles.
>>
>> PAGE_SHIFT is known at compile time, and this code is dropped by the
>> compiler if the selected page size is not 64K. This patch really only
>> makes the code slightly less readable and the "CPU cycles" argument
>> doesn't hold at all.
>>
>> So what are you trying to solve exactly?
>>
>
> There are two points covered by the patch: (1) The 52-bits physical
> address
> is visible only when CONFIG_ARM64_PA_BITS_52 is enabled in arch/arm64
> code.
> The code looks consistent with this option used here. (2) I had the
> assumption
> that gcc doesn't optimize the code and PAGE_SHIFT is always checked in
> order
> to get higher 4 physical address bits, but you said gcc should optimize
> the
> code accordingly. However, it would be still nice to make the code
> explicit.
Conditional compilation only results in more breakages, specially for
configs
that hardly anyone uses (big-endian and 64K pages are the two that
bitrot very
quickly).
So if anything can build without #ifdef, I'll take that anytime. If the
compiler
doesn't optimize it away, let's fix the compiler.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/3] KVM: arm64: Failback on unsupported huge page sizes
2020-10-25 23:04 ` Gavin Shan
@ 2020-10-26 8:55 ` Marc Zyngier
0 siblings, 0 replies; 14+ messages in thread
From: Marc Zyngier @ 2020-10-26 8:55 UTC (permalink / raw)
To: Gavin Shan; +Cc: will, kvmarm, linux-kernel
On 2020-10-25 23:04, Gavin Shan wrote:
> Hi Marc,
>
> On 10/25/20 9:48 PM, Marc Zyngier wrote:
>> On Sun, 25 Oct 2020 01:27:39 +0100,
>> Gavin Shan <gshan@redhat.com> wrote:
>>>
>>> The huge page could be mapped through multiple contiguous PMDs or
>>> PTEs.
>>> The corresponding huge page sizes aren't supported by the page table
>>> walker currently.
>>>
>>> This fails the unsupported huge page sizes to the near one.
>>> Otherwise,
>>> the guest can't boot successfully: CONT_PMD_SHIFT and CONT_PTE_SHIFT
>>> fail back to PMD_SHIFT and PAGE_SHIFT separately.
>>>
>>> Signed-off-by: Gavin Shan <gshan@redhat.com>
>>> ---
>>> arch/arm64/kvm/mmu.c | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c
>>> index 0f51585adc04..81cbdc368246 100644
>>> --- a/arch/arm64/kvm/mmu.c
>>> +++ b/arch/arm64/kvm/mmu.c
>>> @@ -793,12 +793,20 @@ static int user_mem_abort(struct kvm_vcpu
>>> *vcpu, phys_addr_t fault_ipa,
>>> vma_shift = PMD_SHIFT;
>>> #endif
>>> + if (vma_shift == CONT_PMD_SHIFT)
>>> + vma_shift = PMD_SHIFT;
>>> +
>>> if (vma_shift == PMD_SHIFT &&
>>> !fault_supports_stage2_huge_mapping(memslot, hva, PMD_SIZE)) {
>>> force_pte = true;
>>> vma_shift = PAGE_SHIFT;
>>> }
>>> + if (vma_shift == CONT_PTE_SHIFT) {
>>> + force_pte = true;
>>> + vma_shift = PAGE_SHIFT;
>>> + }
>>> +
>>> vma_pagesize = 1UL << vma_shift;
>>> if (vma_pagesize == PMD_SIZE || vma_pagesize == PUD_SIZE)
>>> fault_ipa &= ~(vma_pagesize - 1);
>>
>> Yup, nice catch. However, I think we should take this opportunity to
>> rationalise the logic here, and catch future discrepancies (should
>> someone add contiguous PUD or something similarly silly). How about
>> something like this (untested):
>>
>
> Yeah, I started the work to support contiguous PMDs/PTEs, but I'm not
> sure when I can post the patches for review as my time becomes a bit
> fragmented recently. At least, I need focus on "async page fault" in
> the coming weeks :)
>
> Thanks for the suggested code and it worked for me. I'll post v2 to
> integrate them. However, I would like to drop PATCH[1] and PATCH[2]
> as I really don't have strong reasons to have them.
Yes, please drop these patches, and focus on the actual bug fix.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled
2020-10-26 8:53 ` Marc Zyngier
@ 2020-10-26 22:48 ` Gavin Shan
0 siblings, 0 replies; 14+ messages in thread
From: Gavin Shan @ 2020-10-26 22:48 UTC (permalink / raw)
To: Marc Zyngier; +Cc: will, kvmarm, linux-kernel
On 10/26/20 7:53 PM, Marc Zyngier wrote:
> On 2020-10-25 22:23, Gavin Shan wrote:
>> Hi Marc,
>>
>> On 10/25/20 8:52 PM, Marc Zyngier wrote:
>>> On Sun, 25 Oct 2020 01:27:37 +0100,
>>> Gavin Shan <gshan@redhat.com> wrote:
>>>>
>>>> The 52-bits physical address is disabled until CONFIG_ARM64_PA_BITS_52
>>>> is chosen. This uses option for that check, to avoid the unconditional
>>>> check on PAGE_SHIFT in the hot path and thus save some CPU cycles.
>>>
>>> PAGE_SHIFT is known at compile time, and this code is dropped by the
>>> compiler if the selected page size is not 64K. This patch really only
>>> makes the code slightly less readable and the "CPU cycles" argument
>>> doesn't hold at all.
>>>
>>> So what are you trying to solve exactly?
>>>
>>
>> There are two points covered by the patch: (1) The 52-bits physical address
>> is visible only when CONFIG_ARM64_PA_BITS_52 is enabled in arch/arm64 code.
>> The code looks consistent with this option used here. (2) I had the assumption
>> that gcc doesn't optimize the code and PAGE_SHIFT is always checked in order
>> to get higher 4 physical address bits, but you said gcc should optimize the
>> code accordingly. However, it would be still nice to make the code explicit.
>
> Conditional compilation only results in more breakages, specially for configs
> that hardly anyone uses (big-endian and 64K pages are the two that bitrot very
> quickly).
>
> So if anything can build without #ifdef, I'll take that anytime. If the compiler
> doesn't optimize it away, let's fix the compiler.
>
Ok. PATCH[1] and PATCH[2] have been dropped in v2.
Cheers,
Gavin
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2020-10-26 22:48 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-25 0:27 [PATCH 0/3] KVM: arm64: Failback on unsupported huge pages Gavin Shan
2020-10-25 0:27 ` [PATCH 1/3] KVM: arm64: Check if 52-bits PA is enabled Gavin Shan
2020-10-25 9:52 ` Marc Zyngier
2020-10-25 22:23 ` Gavin Shan
2020-10-26 8:40 ` Will Deacon
2020-10-26 8:53 ` Marc Zyngier
2020-10-26 22:48 ` Gavin Shan
2020-10-25 0:27 ` [PATCH 2/3] KVM: arm64: Don't map PUD huge page if it's not available Gavin Shan
2020-10-25 10:05 ` Marc Zyngier
2020-10-25 22:27 ` Gavin Shan
2020-10-25 0:27 ` [PATCH 3/3] KVM: arm64: Failback on unsupported huge page sizes Gavin Shan
2020-10-25 10:48 ` Marc Zyngier
2020-10-25 23:04 ` Gavin Shan
2020-10-26 8: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).