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 <[email protected]>
---
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)
--
2.41.0.487.g6d72f3e995-goog
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 <[email protected]>
Reviewed-by: Shaoqin Huang <[email protected]>
> ---
> 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)
--
Shaoqin
On Sat, 22 Jul 2023 03:22:41 +0100,
Raghavendra Rao Ananta <[email protected]> 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 <[email protected]>
> ---
> 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.
--
Without deviation from the norm, progress is not possible.
On Thu, Jul 27, 2023, Marc Zyngier wrote:
> On Sat, 22 Jul 2023 03:22:41 +0100,
> Raghavendra Rao Ananta <[email protected]> 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 <[email protected]> wrote:
>
> On Thu, Jul 27, 2023, Marc Zyngier wrote:
> > On Sat, 22 Jul 2023 03:22:41 +0100,
> > Raghavendra Rao Ananta <[email protected]> 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.
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 <[email protected]> wrote:
>
> On Mon, 31 Jul 2023 22:50:07 +0100,
> Sean Christopherson <[email protected]> wrote:
> >
> > On Thu, Jul 27, 2023, Marc Zyngier wrote:
> > > On Sat, 22 Jul 2023 03:22:41 +0100,
> > > Raghavendra Rao Ananta <[email protected]> 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
<[email protected]> 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 <[email protected]> wrote:
> >
> > On Mon, 31 Jul 2023 22:50:07 +0100,
> > Sean Christopherson <[email protected]> wrote:
> > >
> > > On Thu, Jul 27, 2023, Marc Zyngier wrote:
> > > > On Sat, 22 Jul 2023 03:22:41 +0100,
> > > > Raghavendra Rao Ananta <[email protected]> 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
> <[email protected]> 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 <[email protected]> wrote:
>
> On Fri, Aug 04, 2023, Raghavendra Rao Ananta wrote:
> > On Wed, Aug 2, 2023 at 4:28 PM Raghavendra Rao Ananta
> > <[email protected]> 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 <[email protected]>
> wrote:
>>
>> On Fri, Aug 04, 2023, Raghavendra Rao Ananta wrote:
>> > On Wed, Aug 2, 2023 at 4:28 PM Raghavendra Rao Ananta
>> > <[email protected]> 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.
--
Jazz is not dead. It just smells funny...
On Tue, Aug 8, 2023 at 9:43 AM Marc Zyngier <[email protected]> wrote:
>
> On 2023-08-08 17:19, Raghavendra Rao Ananta wrote:
> > On Tue, Aug 8, 2023 at 8:07 AM Sean Christopherson <[email protected]>
> > wrote:
> >>
> >> On Fri, Aug 04, 2023, Raghavendra Rao Ananta wrote:
> >> > On Wed, Aug 2, 2023 at 4:28 PM Raghavendra Rao Ananta
> >> > <[email protected]> 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...