| Message ID | 20230722022251.3446223-3-rananta@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Add support for FEAT_TLBIRANGE | expand |
On 7/22/23 10:22, Raghavendra Rao Ananta wrote: > Stop depending on CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL and opt to > standardize on kvm_arch_flush_remote_tlbs() since it avoids > duplicating the generic TLB stats across architectures that implement > their own remote TLB flush. > > This adds an extra function call to the ARM64 kvm_flush_remote_tlbs() > path, but that is a small cost in comparison to flushing remote TLBs. > > In addition, instead of just incrementing remote_tlb_flush_requests > stat, the generic interface would also increment the > remote_tlb_flush stat. > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> Reviewed-by: Shaoqin Huang <shahuang@redhat.com> > --- > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/kvm/Kconfig | 1 - > arch/arm64/kvm/mmu.c | 6 +++--- > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 8b6096753740..7281222f24ef 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -1111,6 +1111,9 @@ int __init kvm_set_ipa_limit(void); > #define __KVM_HAVE_ARCH_VM_ALLOC > struct kvm *kvm_arch_alloc_vm(void); > > +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS > +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); > + > static inline bool kvm_vm_is_protected(struct kvm *kvm) > { > return false; > diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig > index f531da6b362e..6b730fcfee37 100644 > --- a/arch/arm64/kvm/Kconfig > +++ b/arch/arm64/kvm/Kconfig > @@ -25,7 +25,6 @@ menuconfig KVM > select MMU_NOTIFIER > select PREEMPT_NOTIFIERS > select HAVE_KVM_CPU_RELAX_INTERCEPT > - select HAVE_KVM_ARCH_TLB_FLUSH_ALL > select KVM_MMIO > select KVM_GENERIC_DIRTYLOG_READ_PROTECT > select KVM_XFER_TO_GUEST_WORK > diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c > index 6db9ef288ec3..0ac721fa27f1 100644 > --- a/arch/arm64/kvm/mmu.c > +++ b/arch/arm64/kvm/mmu.c > @@ -161,15 +161,15 @@ static bool memslot_is_logging(struct kvm_memory_slot *memslot) > } > > /** > - * kvm_flush_remote_tlbs() - flush all VM TLB entries for v7/8 > + * kvm_arch_flush_remote_tlbs() - flush all VM TLB entries for v7/8 > * @kvm: pointer to kvm structure. > * > * Interface to HYP function to flush all VM TLB entries > */ > -void kvm_flush_remote_tlbs(struct kvm *kvm) > +int kvm_arch_flush_remote_tlbs(struct kvm *kvm) > { > - ++kvm->stat.generic.remote_tlb_flush_requests; > kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu); > + return 0; > } > > static bool kvm_is_device_pfn(unsigned long pfn)
On Sat, 22 Jul 2023 03:22:41 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote: > > Stop depending on CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL and opt to > standardize on kvm_arch_flush_remote_tlbs() since it avoids > duplicating the generic TLB stats across architectures that implement > their own remote TLB flush. > > This adds an extra function call to the ARM64 kvm_flush_remote_tlbs() > path, but that is a small cost in comparison to flushing remote TLBs. Well, there is no such thing as a "remote TLB" anyway. We either have a non-shareable or inner-shareable invalidation. The notion of remote would imply that we track who potentially has a TLB, which we obviously don't. More x86 nonsense... > > In addition, instead of just incrementing remote_tlb_flush_requests > stat, the generic interface would also increment the > remote_tlb_flush stat. > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > --- > arch/arm64/include/asm/kvm_host.h | 3 +++ > arch/arm64/kvm/Kconfig | 1 - > arch/arm64/kvm/mmu.c | 6 +++--- > 3 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index 8b6096753740..7281222f24ef 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -1111,6 +1111,9 @@ int __init kvm_set_ipa_limit(void); > #define __KVM_HAVE_ARCH_VM_ALLOC > struct kvm *kvm_arch_alloc_vm(void); > > +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS > +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); See my earlier comment about making this prototype global. M.
On Thu, Jul 27, 2023, Marc Zyngier wrote: > On Sat, 22 Jul 2023 03:22:41 +0100, > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > Stop depending on CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL and opt to > > standardize on kvm_arch_flush_remote_tlbs() since it avoids > > duplicating the generic TLB stats across architectures that implement > > their own remote TLB flush. > > > > This adds an extra function call to the ARM64 kvm_flush_remote_tlbs() > > path, but that is a small cost in comparison to flushing remote TLBs. > > Well, there is no such thing as a "remote TLB" anyway. We either have > a non-shareable or inner-shareable invalidation. The notion of remote > would imply that we track who potentially has a TLB, which we > obviously don't. Maybe kvm_arch_flush_vm_tlbs()? The "remote" part is misleading even on x86 when running on Hyper-V, as the flush may be done via a single hypercall and by kicking "remote" vCPUs.
On Mon, 31 Jul 2023 22:50:07 +0100, Sean Christopherson <seanjc@google.com> wrote: > > On Thu, Jul 27, 2023, Marc Zyngier wrote: > > On Sat, 22 Jul 2023 03:22:41 +0100, > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > Stop depending on CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL and opt to > > > standardize on kvm_arch_flush_remote_tlbs() since it avoids > > > duplicating the generic TLB stats across architectures that implement > > > their own remote TLB flush. > > > > > > This adds an extra function call to the ARM64 kvm_flush_remote_tlbs() > > > path, but that is a small cost in comparison to flushing remote TLBs. > > > > Well, there is no such thing as a "remote TLB" anyway. We either have > > a non-shareable or inner-shareable invalidation. The notion of remote > > would imply that we track who potentially has a TLB, which we > > obviously don't. > > Maybe kvm_arch_flush_vm_tlbs()? The "remote" part is misleading even on x86 when > running on Hyper-V, as the flush may be done via a single hypercall and by kicking > "remote" vCPUs. Yup, this would be much better. Thanks, M.
Sure, I'll change it to kvm_arch_flush_vm_tlbs() in v8. Thanks, Raghavendra On Wed, Aug 2, 2023 at 8:55 AM Marc Zyngier <maz@kernel.org> wrote: > > On Mon, 31 Jul 2023 22:50:07 +0100, > Sean Christopherson <seanjc@google.com> wrote: > > > > On Thu, Jul 27, 2023, Marc Zyngier wrote: > > > On Sat, 22 Jul 2023 03:22:41 +0100, > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > Stop depending on CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL and opt to > > > > standardize on kvm_arch_flush_remote_tlbs() since it avoids > > > > duplicating the generic TLB stats across architectures that implement > > > > their own remote TLB flush. > > > > > > > > This adds an extra function call to the ARM64 kvm_flush_remote_tlbs() > > > > path, but that is a small cost in comparison to flushing remote TLBs. > > > > > > Well, there is no such thing as a "remote TLB" anyway. We either have > > > a non-shareable or inner-shareable invalidation. The notion of remote > > > would imply that we track who potentially has a TLB, which we > > > obviously don't. > > > > Maybe kvm_arch_flush_vm_tlbs()? The "remote" part is misleading even on x86 when > > running on Hyper-V, as the flush may be done via a single hypercall and by kicking > > "remote" vCPUs. > > Yup, this would be much better. > > Thanks, > > M. > > -- > Without deviation from the norm, progress is not possible.
On Wed, Aug 2, 2023 at 4:28 PM Raghavendra Rao Ananta <rananta@google.com> wrote: > > Sure, I'll change it to kvm_arch_flush_vm_tlbs() in v8. > While working on the renaming, I realized that since this function is called from kvm_main.c's kvm_flush_remote_tlbs(). Do we want to rename this and the other kvm_flush_*() functions that the series introduces to match their kvm_arch_flush_*() counterparts? (spiraling more into this, we also have the 'remote_tlb_flush_requests' and 'remote_tlb_flush' stats) Thank you. Raghavendra > Thanks, > Raghavendra > > On Wed, Aug 2, 2023 at 8:55 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Mon, 31 Jul 2023 22:50:07 +0100, > > Sean Christopherson <seanjc@google.com> wrote: > > > > > > On Thu, Jul 27, 2023, Marc Zyngier wrote: > > > > On Sat, 22 Jul 2023 03:22:41 +0100, > > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > > > Stop depending on CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL and opt to > > > > > standardize on kvm_arch_flush_remote_tlbs() since it avoids > > > > > duplicating the generic TLB stats across architectures that implement > > > > > their own remote TLB flush. > > > > > > > > > > This adds an extra function call to the ARM64 kvm_flush_remote_tlbs() > > > > > path, but that is a small cost in comparison to flushing remote TLBs. > > > > > > > > Well, there is no such thing as a "remote TLB" anyway. We either have > > > > a non-shareable or inner-shareable invalidation. The notion of remote > > > > would imply that we track who potentially has a TLB, which we > > > > obviously don't. > > > > > > Maybe kvm_arch_flush_vm_tlbs()? The "remote" part is misleading even on x86 when > > > running on Hyper-V, as the flush may be done via a single hypercall and by kicking > > > "remote" vCPUs. > > > > Yup, this would be much better. > > > > Thanks, > > > > M. > > > > -- > > Without deviation from the norm, progress is not possible.
On Fri, Aug 04, 2023, Raghavendra Rao Ananta wrote: > On Wed, Aug 2, 2023 at 4:28 PM Raghavendra Rao Ananta > <rananta@google.com> wrote: > > > > Sure, I'll change it to kvm_arch_flush_vm_tlbs() in v8. > > > While working on the renaming, I realized that since this function is > called from kvm_main.c's kvm_flush_remote_tlbs(). Do we want to rename > this and the other kvm_flush_*() functions that the series introduces > to match their kvm_arch_flush_*() counterparts? Hmm, if we're going to rename one arch hook, then yes, I think it makes sense to rename all the common APIs and arch hooks to match. However, x86 is rife with the "remote_tlbs" nomenclature, and renaming the common APIs will just push the inconsistencies into x86. While I 100% agree that the current naming is flawed, I am not willing to end up with x86 being partially converted. I think I'm ok renaming all of x86's many hooks? But I'd definitely want input from more x86 folks, and the size and scope of this series would explode. Unless Marc objects and/or has a better idea, the least awful option is probably to ignore the poor "remote_tlbs" naming and tackle it in a separate series. Sorry for not noticiing this earlier, I didn't realize just how much x86 uses remote_tlbs. > (spiraling more into this, we also have the 'remote_tlb_flush_requests' and > 'remote_tlb_flush' stats) Regardless of what we decide for the APIs, definitely leave the stats alone. The names are ABI. We could preserve the names and changes the struct fields, but that would be a net negative IMO.
On Tue, Aug 8, 2023 at 8:07 AM Sean Christopherson <seanjc@google.com> wrote: > > On Fri, Aug 04, 2023, Raghavendra Rao Ananta wrote: > > On Wed, Aug 2, 2023 at 4:28 PM Raghavendra Rao Ananta > > <rananta@google.com> wrote: > > > > > > Sure, I'll change it to kvm_arch_flush_vm_tlbs() in v8. > > > > > While working on the renaming, I realized that since this function is > > called from kvm_main.c's kvm_flush_remote_tlbs(). Do we want to rename > > this and the other kvm_flush_*() functions that the series introduces > > to match their kvm_arch_flush_*() counterparts? > > Hmm, if we're going to rename one arch hook, then yes, I think it makes sense to > rename all the common APIs and arch hooks to match. > > However, x86 is rife with the "remote_tlbs" nomenclature, and renaming the common > APIs will just push the inconsistencies into x86. While I 100% agree that the > current naming is flawed, I am not willing to end up with x86 being partially > converted. > > I think I'm ok renaming all of x86's many hooks? But I'd definitely want input > from more x86 folks, and the size and scope of this series would explode. Unless > Marc objects and/or has a better idea, the least awful option is probably to ignore > the poor "remote_tlbs" naming and tackle it in a separate series. > Sure, I think it's better to do it in a separate series as well. I'm happy to carry out the task after this one gets merged. But, let's wait for Marc and others' opinion on the matter. Thank you. Raghavendra > Sorry for not noticiing this earlier, I didn't realize just how much x86 uses > remote_tlbs. > > > (spiraling more into this, we also have the 'remote_tlb_flush_requests' and > > 'remote_tlb_flush' stats) > > Regardless of what we decide for the APIs, definitely leave the stats alone. The > names are ABI. We could preserve the names and changes the struct fields, but that > would be a net negative IMO.
On 2023-08-08 17:19, Raghavendra Rao Ananta wrote: > On Tue, Aug 8, 2023 at 8:07 AM Sean Christopherson <seanjc@google.com> > wrote: >> >> On Fri, Aug 04, 2023, Raghavendra Rao Ananta wrote: >> > On Wed, Aug 2, 2023 at 4:28 PM Raghavendra Rao Ananta >> > <rananta@google.com> wrote: >> > > >> > > Sure, I'll change it to kvm_arch_flush_vm_tlbs() in v8. >> > > >> > While working on the renaming, I realized that since this function is >> > called from kvm_main.c's kvm_flush_remote_tlbs(). Do we want to rename >> > this and the other kvm_flush_*() functions that the series introduces >> > to match their kvm_arch_flush_*() counterparts? >> >> Hmm, if we're going to rename one arch hook, then yes, I think it >> makes sense to >> rename all the common APIs and arch hooks to match. >> >> However, x86 is rife with the "remote_tlbs" nomenclature, and renaming >> the common >> APIs will just push the inconsistencies into x86. While I 100% agree >> that the >> current naming is flawed, I am not willing to end up with x86 being >> partially >> converted. >> >> I think I'm ok renaming all of x86's many hooks? But I'd definitely >> want input >> from more x86 folks, and the size and scope of this series would >> explode. Unless >> Marc objects and/or has a better idea, the least awful option is >> probably to ignore >> the poor "remote_tlbs" naming and tackle it in a separate series. >> > Sure, I think it's better to do it in a separate series as well. I'm > happy to carry out the task after this one gets merged. But, let's > wait for Marc and others' opinion on the matter. Yeah, let's punt that to a separate series. I'm more interested in getting this code merged than in the inevitable bike-shedding that will result from such a proposal. Raghavendra, any chance you could respin the series this week? I'd really like it to spend some quality time in -next... Thanks, M.
On Tue, Aug 8, 2023 at 9:43 AM Marc Zyngier <maz@kernel.org> wrote: > > On 2023-08-08 17:19, Raghavendra Rao Ananta wrote: > > On Tue, Aug 8, 2023 at 8:07 AM Sean Christopherson <seanjc@google.com> > > wrote: > >> > >> On Fri, Aug 04, 2023, Raghavendra Rao Ananta wrote: > >> > On Wed, Aug 2, 2023 at 4:28 PM Raghavendra Rao Ananta > >> > <rananta@google.com> wrote: > >> > > > >> > > Sure, I'll change it to kvm_arch_flush_vm_tlbs() in v8. > >> > > > >> > While working on the renaming, I realized that since this function is > >> > called from kvm_main.c's kvm_flush_remote_tlbs(). Do we want to rename > >> > this and the other kvm_flush_*() functions that the series introduces > >> > to match their kvm_arch_flush_*() counterparts? > >> > >> Hmm, if we're going to rename one arch hook, then yes, I think it > >> makes sense to > >> rename all the common APIs and arch hooks to match. > >> > >> However, x86 is rife with the "remote_tlbs" nomenclature, and renaming > >> the common > >> APIs will just push the inconsistencies into x86. While I 100% agree > >> that the > >> current naming is flawed, I am not willing to end up with x86 being > >> partially > >> converted. > >> > >> I think I'm ok renaming all of x86's many hooks? But I'd definitely > >> want input > >> from more x86 folks, and the size and scope of this series would > >> explode. Unless > >> Marc objects and/or has a better idea, the least awful option is > >> probably to ignore > >> the poor "remote_tlbs" naming and tackle it in a separate series. > >> > > Sure, I think it's better to do it in a separate series as well. I'm > > happy to carry out the task after this one gets merged. But, let's > > wait for Marc and others' opinion on the matter. > > Yeah, let's punt that to a separate series. I'm more interested in > getting this code merged than in the inevitable bike-shedding that > will result from such a proposal. > > Raghavendra, any chance you could respin the series this week? > I'd really like it to spend some quality time in -next... > No problem. I'll send out v8 by today or tomorrow. Thank you. Raghavendra > Thanks, > > M. > -- > Jazz is not dead. It just smells funny...
diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index 8b6096753740..7281222f24ef 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -1111,6 +1111,9 @@ int __init kvm_set_ipa_limit(void); #define __KVM_HAVE_ARCH_VM_ALLOC struct kvm *kvm_arch_alloc_vm(void); +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); + static inline bool kvm_vm_is_protected(struct kvm *kvm) { return false; diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig index f531da6b362e..6b730fcfee37 100644 --- a/arch/arm64/kvm/Kconfig +++ b/arch/arm64/kvm/Kconfig @@ -25,7 +25,6 @@ menuconfig KVM select MMU_NOTIFIER select PREEMPT_NOTIFIERS select HAVE_KVM_CPU_RELAX_INTERCEPT - select HAVE_KVM_ARCH_TLB_FLUSH_ALL select KVM_MMIO select KVM_GENERIC_DIRTYLOG_READ_PROTECT select KVM_XFER_TO_GUEST_WORK diff --git a/arch/arm64/kvm/mmu.c b/arch/arm64/kvm/mmu.c index 6db9ef288ec3..0ac721fa27f1 100644 --- a/arch/arm64/kvm/mmu.c +++ b/arch/arm64/kvm/mmu.c @@ -161,15 +161,15 @@ static bool memslot_is_logging(struct kvm_memory_slot *memslot) } /** - * kvm_flush_remote_tlbs() - flush all VM TLB entries for v7/8 + * kvm_arch_flush_remote_tlbs() - flush all VM TLB entries for v7/8 * @kvm: pointer to kvm structure. * * Interface to HYP function to flush all VM TLB entries */ -void kvm_flush_remote_tlbs(struct kvm *kvm) +int kvm_arch_flush_remote_tlbs(struct kvm *kvm) { - ++kvm->stat.generic.remote_tlb_flush_requests; kvm_call_hyp(__kvm_tlb_flush_vmid, &kvm->arch.mmu); + return 0; } static bool kvm_is_device_pfn(unsigned long pfn)
Stop depending on CONFIG_HAVE_KVM_ARCH_TLB_FLUSH_ALL and opt to standardize on kvm_arch_flush_remote_tlbs() since it avoids duplicating the generic TLB stats across architectures that implement their own remote TLB flush. This adds an extra function call to the ARM64 kvm_flush_remote_tlbs() path, but that is a small cost in comparison to flushing remote TLBs. In addition, instead of just incrementing remote_tlb_flush_requests stat, the generic interface would also increment the remote_tlb_flush stat. Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> --- arch/arm64/include/asm/kvm_host.h | 3 +++ arch/arm64/kvm/Kconfig | 1 - arch/arm64/kvm/mmu.c | 6 +++--- 3 files changed, 6 insertions(+), 4 deletions(-)