This is based on a suggestion from Will [0] to try out the asid
based kvm vmid solution as a separate VMID allocator instead of
the shared lib approach attempted in v4[1].
The idea is to compare both the approaches and see whether the
shared lib solution with callbacks make sense or not.
Though we are not yet using the pinned vmids yet, patch #2 has
code for pinned vmid support. This is just to help the comparison.
Test Setup/Results
----------------
The measurement was made with maxcpus set to 8 and with the
number of VMID limited to 4-bit. The test involves running
concurrently 40 guests with 2 vCPUs. Each guest will then
execute hackbench 5 times before exiting.
The performance difference between the current algo and the
new one are(avg. of 10 runs):
- 1.9% less entry/exit from the guest
- 0.5% faster
This is more or less comparable to v4 numbers.
For the complete series, please see,
https://github.com/hisilicon/kernel-dev/tree/private-v5.12-rc7-vmid-2nd-rfc
and for the shared asid lib v4 solution,
https://github.com/hisilicon/kernel-dev/tree/private-v5.12-rc7-asid-v4
As you can see there are of course code duplication with this
approach but may be this one is more easy to maintain considering
the complexity involved.
Please take a look and let me know your feedback.
Thanks,
Shameer
[0] https://lore.kernel.org/lkml/20210422160846.GB2214@willie-the-truck/
[1] https://lore.kernel.org/lkml/[email protected]/
Julien Grall (2):
arch/arm64: Introduce a capability to tell whether 16-bit VMID is
available
kvm/arm: Align the VMID allocation with the arm64 ASID one
Shameer Kolothum (1):
kvm/arm: Introduce a new vmid allocator for KVM
arch/arm64/include/asm/cpucaps.h | 3 +-
arch/arm64/include/asm/kvm_asm.h | 4 +-
arch/arm64/include/asm/kvm_host.h | 11 +-
arch/arm64/include/asm/kvm_mmu.h | 7 +-
arch/arm64/kernel/cpufeature.c | 9 +
arch/arm64/kvm/Makefile | 2 +-
arch/arm64/kvm/arm.c | 115 ++++--------
arch/arm64/kvm/hyp/nvhe/hyp-main.c | 6 +-
arch/arm64/kvm/hyp/nvhe/tlb.c | 10 +-
arch/arm64/kvm/hyp/vhe/tlb.c | 10 +-
arch/arm64/kvm/mmu.c | 1 -
arch/arm64/kvm/vmid.c | 285 +++++++++++++++++++++++++++++
12 files changed, 354 insertions(+), 109 deletions(-)
create mode 100644 arch/arm64/kvm/vmid.c
--
2.17.1
From: Julien Grall <[email protected]>
At the moment, the function kvm_get_vmid_bits() is looking up for the
sanitized value of ID_AA64MMFR1_EL1 and extract the information
regarding the number of VMID bits supported.
This is fine as the function is mainly used during VMID roll-over. New
use in a follow-up patch will require the function to be called a every
context switch so we want the function to be more efficient.
A new capability is introduced to tell whether 16-bit VMID is
available.
Signed-off-by: Julien Grall <[email protected]>
Signed-off-by: Shameer Kolothum <[email protected]>
---
arch/arm64/include/asm/cpucaps.h | 3 ++-
arch/arm64/include/asm/kvm_mmu.h | 4 +---
arch/arm64/kernel/cpufeature.c | 9 +++++++++
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
index c40f2490cd7b..acb92da5c254 100644
--- a/arch/arm64/include/asm/cpucaps.h
+++ b/arch/arm64/include/asm/cpucaps.h
@@ -67,7 +67,8 @@
#define ARM64_HAS_LDAPR 59
#define ARM64_KVM_PROTECTED_MODE 60
#define ARM64_WORKAROUND_NVIDIA_CARMEL_CNP 61
+#define ARM64_HAS_16BIT_VMID 62
-#define ARM64_NCAPS 62
+#define ARM64_NCAPS 63
#endif /* __ASM_CPUCAPS_H */
diff --git a/arch/arm64/include/asm/kvm_mmu.h b/arch/arm64/include/asm/kvm_mmu.h
index 90873851f677..c3080966ef83 100644
--- a/arch/arm64/include/asm/kvm_mmu.h
+++ b/arch/arm64/include/asm/kvm_mmu.h
@@ -213,9 +213,7 @@ void kvm_toggle_cache(struct kvm_vcpu *vcpu, bool was_enabled);
static inline unsigned int kvm_get_vmid_bits(void)
{
- int reg = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
-
- return get_vmid_bits(reg);
+ return cpus_have_const_cap(ARM64_HAS_16BIT_VMID) ? 16 : 8;
}
/*
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index e5281e1c8f1d..ff956fb2f712 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -2203,6 +2203,15 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
.matches = has_cpuid_feature,
.min_field_value = 1,
},
+ {
+ .capability = ARM64_HAS_16BIT_VMID,
+ .type = ARM64_CPUCAP_SYSTEM_FEATURE,
+ .sys_reg = SYS_ID_AA64MMFR1_EL1,
+ .field_pos = ID_AA64MMFR1_VMIDBITS_SHIFT,
+ .sign = FTR_UNSIGNED,
+ .min_field_value = ID_AA64MMFR1_VMIDBITS_16,
+ .matches = has_cpuid_feature,
+ },
{},
};
--
2.17.1
Hi,
> -----Original Message-----
> From: Shameerali Kolothum Thodi
> Sent: 06 May 2021 17:52
> To: [email protected]; [email protected];
> [email protected]
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Linuxarm
> <[email protected]>
> Subject: [RFC PATCH 0/3] kvm/arm: New VMID allocator based on asid(2nd
> approach)
>
> This is based on a suggestion from Will [0] to try out the asid
> based kvm vmid solution as a separate VMID allocator instead of
> the shared lib approach attempted in v4[1].
>
> The idea is to compare both the approaches and see whether the
> shared lib solution with callbacks make sense or not.
A gentle ping on this. Please take a look and let me know.
Thanks,
Shameer
> Though we are not yet using the pinned vmids yet, patch #2 has
> code for pinned vmid support. This is just to help the comparison.
>
> Test Setup/Results
> ----------------
> The measurement was made with maxcpus set to 8 and with the
> number of VMID limited to 4-bit. The test involves running
> concurrently 40 guests with 2 vCPUs. Each guest will then
> execute hackbench 5 times before exiting.
>
> The performance difference between the current algo and the
> new one are(avg. of 10 runs):
> - 1.9% less entry/exit from the guest
> - 0.5% faster
> This is more or less comparable to v4 numbers.
>
> For the complete series, please see,
> https://github.com/hisilicon/kernel-dev/tree/private-v5.12-rc7-vmid-2nd-rfc
>
> and for the shared asid lib v4 solution,
> https://github.com/hisilicon/kernel-dev/tree/private-v5.12-rc7-asid-v4
>
> As you can see there are of course code duplication with this
> approach but may be this one is more easy to maintain considering
> the complexity involved.
>
> Please take a look and let me know your feedback.
>
> Thanks,
> Shameer
>
>
> [0] https://lore.kernel.org/lkml/20210422160846.GB2214@willie-the-truck/
> [1]
> https://lore.kernel.org/lkml/20210414112312.13704-1-shameerali.kolothum.t
> [email protected]/
>
> Julien Grall (2):
> arch/arm64: Introduce a capability to tell whether 16-bit VMID is
> available
> kvm/arm: Align the VMID allocation with the arm64 ASID one
>
> Shameer Kolothum (1):
> kvm/arm: Introduce a new vmid allocator for KVM
>
> arch/arm64/include/asm/cpucaps.h | 3 +-
> arch/arm64/include/asm/kvm_asm.h | 4 +-
> arch/arm64/include/asm/kvm_host.h | 11 +-
> arch/arm64/include/asm/kvm_mmu.h | 7 +-
> arch/arm64/kernel/cpufeature.c | 9 +
> arch/arm64/kvm/Makefile | 2 +-
> arch/arm64/kvm/arm.c | 115 ++++--------
> arch/arm64/kvm/hyp/nvhe/hyp-main.c | 6 +-
> arch/arm64/kvm/hyp/nvhe/tlb.c | 10 +-
> arch/arm64/kvm/hyp/vhe/tlb.c | 10 +-
> arch/arm64/kvm/mmu.c | 1 -
> arch/arm64/kvm/vmid.c | 285
> +++++++++++++++++++++++++++++
> 12 files changed, 354 insertions(+), 109 deletions(-)
> create mode 100644 arch/arm64/kvm/vmid.c
>
> --
> 2.17.1
On Fri, 04 Jun 2021 09:13:10 +0100,
Shameerali Kolothum Thodi <[email protected]> wrote:
>
> Hi,
>
> > -----Original Message-----
> > From: Shameerali Kolothum Thodi
> > Sent: 06 May 2021 17:52
> > To: [email protected]; [email protected];
> > [email protected]
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Linuxarm
> > <[email protected]>
> > Subject: [RFC PATCH 0/3] kvm/arm: New VMID allocator based on asid(2nd
> > approach)
> >
> > This is based on a suggestion from Will [0] to try out the asid
> > based kvm vmid solution as a separate VMID allocator instead of
> > the shared lib approach attempted in v4[1].
> >
> > The idea is to compare both the approaches and see whether the
> > shared lib solution with callbacks make sense or not.
>
> A gentle ping on this. Please take a look and let me know.
I had a look and I don't overly dislike it. I'd like to see the code
without the pinned stuff though, at least to ease the reviewing. I
haven't tested it in anger, but I have pushed the rebased code at [1]
as it really didn't apply to 5.13-rc4.
One thing I'm a bit worried about is that we so far relied on VMID 0
never being allocated to a guest, which is now crucial for protected
KVM. I can't really convince myself that this can never happen with
this. Plus, I've found this nugget:
<quote
max_pinned_vmids = NUM_USER_VMIDS - num_possible_cpus() - 2;
</quote>
What is this "- 2"? My hunch is that it should really be "- 1" as VMID
0 is reserved, and we have no equivalent of KPTI for S2.
M.
[1] https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h=kvm-arm64/mmu/vmid
--
Without deviation from the norm, progress is not possible.
Hi Marc,
> -----Original Message-----
> From: Marc Zyngier [mailto:[email protected]]
> Sent: 04 June 2021 14:55
> To: Shameerali Kolothum Thodi <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; Alexandru Elisei
> <[email protected]>; Linuxarm <[email protected]>
> Subject: Re: [RFC PATCH 0/3] kvm/arm: New VMID allocator based on asid(2nd
> approach)
>
> On Fri, 04 Jun 2021 09:13:10 +0100,
> Shameerali Kolothum Thodi <[email protected]>
> wrote:
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Shameerali Kolothum Thodi
> > > Sent: 06 May 2021 17:52
> > > To: [email protected]; [email protected];
> > > [email protected]
> > > Cc: [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected];
> > > [email protected]; [email protected]; Linuxarm
> > > <[email protected]>
> > > Subject: [RFC PATCH 0/3] kvm/arm: New VMID allocator based on asid(2nd
> > > approach)
> > >
> > > This is based on a suggestion from Will [0] to try out the asid
> > > based kvm vmid solution as a separate VMID allocator instead of
> > > the shared lib approach attempted in v4[1].
> > >
> > > The idea is to compare both the approaches and see whether the
> > > shared lib solution with callbacks make sense or not.
> >
> > A gentle ping on this. Please take a look and let me know.
>
> I had a look and I don't overly dislike it. I'd like to see the code
> without the pinned stuff though, at least to ease the reviewing. I
> haven't tested it in anger, but I have pushed the rebased code at [1]
> as it really didn't apply to 5.13-rc4.
Thanks for taking a look and the rebase. I will remove the pinned stuff
in the next revision as that was added just to compare against the previous
version.
>
> One thing I'm a bit worried about is that we so far relied on VMID 0
> never being allocated to a guest, which is now crucial for protected
> KVM. I can't really convince myself that this can never happen with
> this.
Hmm..not sure I quite follow that. As per the current logic vmid 0 is
reserved and is not allocated to Guest.
> Plus, I've found this nugget:
>
> <quote
> max_pinned_vmids = NUM_USER_VMIDS - num_possible_cpus() - 2;
> </quote>
>
> What is this "- 2"? My hunch is that it should really be "- 1" as VMID
> 0 is reserved, and we have no equivalent of KPTI for S2.
I think this is more related to the "pinned vmid" stuff and was borrowed from
the asid_update_limit() fn in arch/arm64/mm/context.c. But I missed the
comment that explains the reason behind it. It says,
---x---
/*
* There must always be an ASID available after rollover. Ensure that,
* even if all CPUs have a reserved ASID and the maximum number of ASIDs
* are pinned, there still is at least one empty slot in the ASID map.
*/
max_pinned_asids = num_available_asids - num_possible_cpus() - 2;
---x---
So this is to make sure we will have at least one VMID available after rollover
in case we have pinned the max number of VMIDs. I will include that comment
to make it clear.
Thanks,
Shameer
>
> M.
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/log/?h
> =kvm-arm64/mmu/vmid
>
> --
> Without deviation from the norm, progress is not possible.
On Fri, 04 Jun 2021 15:51:29 +0100,
Shameerali Kolothum Thodi <[email protected]> wrote:
>
> Hi Marc,
>
> > -----Original Message-----
> > From: Marc Zyngier [mailto:[email protected]]
> > Sent: 04 June 2021 14:55
> > To: Shameerali Kolothum Thodi <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; Alexandru Elisei
> > <[email protected]>; Linuxarm <[email protected]>
> > Subject: Re: [RFC PATCH 0/3] kvm/arm: New VMID allocator based on asid(2nd
> > approach)
> >
> > On Fri, 04 Jun 2021 09:13:10 +0100,
> > Shameerali Kolothum Thodi <[email protected]>
> > wrote:
> > >
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: Shameerali Kolothum Thodi
> > > > Sent: 06 May 2021 17:52
> > > > To: [email protected]; [email protected];
> > > > [email protected]
> > > > Cc: [email protected]; [email protected]; [email protected];
> > > > [email protected]; [email protected];
> > > > [email protected]; [email protected]; Linuxarm
> > > > <[email protected]>
> > > > Subject: [RFC PATCH 0/3] kvm/arm: New VMID allocator based on asid(2nd
> > > > approach)
> > > >
> > > > This is based on a suggestion from Will [0] to try out the asid
> > > > based kvm vmid solution as a separate VMID allocator instead of
> > > > the shared lib approach attempted in v4[1].
> > > >
> > > > The idea is to compare both the approaches and see whether the
> > > > shared lib solution with callbacks make sense or not.
> > >
> > > A gentle ping on this. Please take a look and let me know.
> >
> > I had a look and I don't overly dislike it. I'd like to see the code
> > without the pinned stuff though, at least to ease the reviewing. I
> > haven't tested it in anger, but I have pushed the rebased code at [1]
> > as it really didn't apply to 5.13-rc4.
>
> Thanks for taking a look and the rebase. I will remove the pinned stuff
> in the next revision as that was added just to compare against the previous
> version.
>
> >
> > One thing I'm a bit worried about is that we so far relied on VMID 0
> > never being allocated to a guest, which is now crucial for protected
> > KVM. I can't really convince myself that this can never happen with
> > this.
>
> Hmm..not sure I quite follow that. As per the current logic vmid 0 is
> reserved and is not allocated to Guest.
And that's the bit I'm struggling to validate here. I guess it works
because cur_idx is set to 1 in new_vmid().
>
> > Plus, I've found this nugget:
> >
> > <quote
> > max_pinned_vmids = NUM_USER_VMIDS - num_possible_cpus() - 2;
> > </quote>
> >
> > What is this "- 2"? My hunch is that it should really be "- 1" as VMID
> > 0 is reserved, and we have no equivalent of KPTI for S2.
>
> I think this is more related to the "pinned vmid" stuff and was borrowed from
> the asid_update_limit() fn in arch/arm64/mm/context.c. But I missed the
> comment that explains the reason behind it. It says,
>
> ---x---
> /*
> * There must always be an ASID available after rollover. Ensure that,
> * even if all CPUs have a reserved ASID and the maximum number of ASIDs
> * are pinned, there still is at least one empty slot in the ASID map.
> */
> max_pinned_asids = num_available_asids - num_possible_cpus() - 2;
> ---x---
>
> So this is to make sure we will have at least one VMID available
> after rollover in case we have pinned the max number of VMIDs. I
> will include that comment to make it clear.
That doesn't really explain the -2. Or is that that we have one for
the extra empty slot, and one for the reserved?
Jean-Philippe?
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
On Fri, Jun 04, 2021 at 04:27:39PM +0100, Marc Zyngier wrote:
> > > Plus, I've found this nugget:
> > >
> > > <quote
> > > max_pinned_vmids = NUM_USER_VMIDS - num_possible_cpus() - 2;
> > > </quote>
> > >
> > > What is this "- 2"? My hunch is that it should really be "- 1" as VMID
> > > 0 is reserved, and we have no equivalent of KPTI for S2.
> >
> > I think this is more related to the "pinned vmid" stuff and was borrowed from
> > the asid_update_limit() fn in arch/arm64/mm/context.c. But I missed the
> > comment that explains the reason behind it. It says,
> >
> > ---x---
> > /*
> > * There must always be an ASID available after rollover. Ensure that,
> > * even if all CPUs have a reserved ASID and the maximum number of ASIDs
> > * are pinned, there still is at least one empty slot in the ASID map.
> > */
> > max_pinned_asids = num_available_asids - num_possible_cpus() - 2;
> > ---x---
> >
> > So this is to make sure we will have at least one VMID available
> > after rollover in case we have pinned the max number of VMIDs. I
> > will include that comment to make it clear.
>
> That doesn't really explain the -2. Or is that that we have one for
> the extra empty slot, and one for the reserved?
>
> Jean-Philippe?
Yes, -2 is for ASID#0 and the extra empty slot. A comment higher in
asids_update_limit() hints at that, but it could definitely be clearer
/*
* Expect allocation after rollover to fail if we don't have at least
* one more ASID than CPUs. ASID #0 is reserved for init_mm.
*/
Thanks,
Jean