| Message ID | 20230722022251.3446223-2-rananta@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | KVM: arm64: Add support for FEAT_TLBIRANGE | expand |
On Sat, 22 Jul 2023 03:22:40 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote: > > From: David Matlack <dmatlack@google.com> > > Rename kvm_arch_flush_remote_tlb() and the associated macro > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB to kvm_arch_flush_remote_tlbs() and > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS respectively. > > Making the name plural matches kvm_flush_remote_tlbs() and makes it more > clear that this function can affect more than one remote TLB. > > No functional change intended. > > Signed-off-by: David Matlack <dmatlack@google.com> > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > Reviewed-by: Gavin Shan <gshan@redhat.com> > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > Reviewed-by: Shaoqin Huang <shahuang@redhat.com> > --- > arch/mips/include/asm/kvm_host.h | 4 ++-- > arch/mips/kvm/mips.c | 2 +- > arch/x86/include/asm/kvm_host.h | 4 ++-- > include/linux/kvm_host.h | 4 ++-- > virt/kvm/kvm_main.c | 2 +- > 5 files changed, 8 insertions(+), 8 deletions(-) > > diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h > index 04cedf9f8811..9b0ad8f3bf32 100644 > --- a/arch/mips/include/asm/kvm_host.h > +++ b/arch/mips/include/asm/kvm_host.h > @@ -896,7 +896,7 @@ static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > > -#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB > -int kvm_arch_flush_remote_tlb(struct kvm *kvm); > +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS > +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); How about making this prototype global? I don't see a point in having it per-architecture, specially as you are adding arm64 to that mix in the following patch. M.
On Thu, Jul 27, 2023 at 3:24 AM Marc Zyngier <maz@kernel.org> wrote: > > On Sat, 22 Jul 2023 03:22:40 +0100, > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > From: David Matlack <dmatlack@google.com> > > > > Rename kvm_arch_flush_remote_tlb() and the associated macro > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB to kvm_arch_flush_remote_tlbs() and > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS respectively. > > > > Making the name plural matches kvm_flush_remote_tlbs() and makes it more > > clear that this function can affect more than one remote TLB. > > > > No functional change intended. > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > Reviewed-by: Gavin Shan <gshan@redhat.com> > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > Reviewed-by: Shaoqin Huang <shahuang@redhat.com> > > --- > > arch/mips/include/asm/kvm_host.h | 4 ++-- > > arch/mips/kvm/mips.c | 2 +- > > arch/x86/include/asm/kvm_host.h | 4 ++-- > > include/linux/kvm_host.h | 4 ++-- > > virt/kvm/kvm_main.c | 2 +- > > 5 files changed, 8 insertions(+), 8 deletions(-) > > > > diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h > > index 04cedf9f8811..9b0ad8f3bf32 100644 > > --- a/arch/mips/include/asm/kvm_host.h > > +++ b/arch/mips/include/asm/kvm_host.h > > @@ -896,7 +896,7 @@ static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > > > > -#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB > > -int kvm_arch_flush_remote_tlb(struct kvm *kvm); > > +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS > > +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); > > How about making this prototype global? I don't see a point in having > it per-architecture, specially as you are adding arm64 to that mix in > the following patch. > We can make it global, but I'm not sure what was the intention of the original author. My guess is that he was following the same style that we have for some of the other kvm_arch_*() functions (kvm_arch_free_vm() for example)? - Raghavendra > M. > > -- > Without deviation from the norm, progress is not possible.
On Mon, Jul 31, 2023, Raghavendra Rao Ananta wrote: > On Thu, Jul 27, 2023 at 3:24 AM Marc Zyngier <maz@kernel.org> wrote: > > > > On Sat, 22 Jul 2023 03:22:40 +0100, > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > From: David Matlack <dmatlack@google.com> > > > > > > Rename kvm_arch_flush_remote_tlb() and the associated macro > > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB to kvm_arch_flush_remote_tlbs() and > > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS respectively. > > > > > > Making the name plural matches kvm_flush_remote_tlbs() and makes it more > > > clear that this function can affect more than one remote TLB. > > > > > > No functional change intended. > > > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > > Reviewed-by: Gavin Shan <gshan@redhat.com> > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > Reviewed-by: Shaoqin Huang <shahuang@redhat.com> > > > --- > > > arch/mips/include/asm/kvm_host.h | 4 ++-- > > > arch/mips/kvm/mips.c | 2 +- > > > arch/x86/include/asm/kvm_host.h | 4 ++-- > > > include/linux/kvm_host.h | 4 ++-- > > > virt/kvm/kvm_main.c | 2 +- > > > 5 files changed, 8 insertions(+), 8 deletions(-) > > > > > > diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h > > > index 04cedf9f8811..9b0ad8f3bf32 100644 > > > --- a/arch/mips/include/asm/kvm_host.h > > > +++ b/arch/mips/include/asm/kvm_host.h > > > @@ -896,7 +896,7 @@ static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > > > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > > > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > > > > > > -#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB > > > -int kvm_arch_flush_remote_tlb(struct kvm *kvm); > > > +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS > > > +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); > > > > How about making this prototype global? I don't see a point in having > > it per-architecture, specially as you are adding arm64 to that mix in > > the following patch. > > > We can make it global, but I'm not sure what was the intention of the > original author. My guess is that he was following the same style that > we have for some of the other kvm_arch_*() functions > (kvm_arch_free_vm() for example)? Heh, KVM has a *lot* of code that was written with questionable style. I agree with Marc, I can't think of a single reason not to have the definition in common code. Declaring the function doesn't preclude a "static inline" implementation, and we could even keep the prototype under an #ifdef, e.g. diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 9d3ac7720da9..5ac64f933547 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1484,6 +1484,8 @@ static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm) { return -ENOTSUPP; } +#else +int kvm_arch_flush_remote_tlb(struct kvm *kvm); #endif #ifdef __KVM_HAVE_ARCH_NONCOHERENT_DMA
On Mon, Jul 31, 2023 at 2:42 PM Sean Christopherson <seanjc@google.com> wrote: > > On Mon, Jul 31, 2023, Raghavendra Rao Ananta wrote: > > On Thu, Jul 27, 2023 at 3:24 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > On Sat, 22 Jul 2023 03:22:40 +0100, > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > From: David Matlack <dmatlack@google.com> > > > > > > > > Rename kvm_arch_flush_remote_tlb() and the associated macro > > > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB to kvm_arch_flush_remote_tlbs() and > > > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS respectively. > > > > > > > > Making the name plural matches kvm_flush_remote_tlbs() and makes it more > > > > clear that this function can affect more than one remote TLB. > > > > > > > > No functional change intended. > > > > > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > > > Reviewed-by: Gavin Shan <gshan@redhat.com> > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > Reviewed-by: Shaoqin Huang <shahuang@redhat.com> > > > > --- > > > > arch/mips/include/asm/kvm_host.h | 4 ++-- > > > > arch/mips/kvm/mips.c | 2 +- > > > > arch/x86/include/asm/kvm_host.h | 4 ++-- > > > > include/linux/kvm_host.h | 4 ++-- > > > > virt/kvm/kvm_main.c | 2 +- > > > > 5 files changed, 8 insertions(+), 8 deletions(-) > > > > > > > > diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h > > > > index 04cedf9f8811..9b0ad8f3bf32 100644 > > > > --- a/arch/mips/include/asm/kvm_host.h > > > > +++ b/arch/mips/include/asm/kvm_host.h > > > > @@ -896,7 +896,7 @@ static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > > > > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > > > > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > > > > > > > > -#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB > > > > -int kvm_arch_flush_remote_tlb(struct kvm *kvm); > > > > +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS > > > > +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); > > > > > > How about making this prototype global? I don't see a point in having > > > it per-architecture, specially as you are adding arm64 to that mix in > > > the following patch. > > > > > We can make it global, but I'm not sure what was the intention of the > > original author. My guess is that he was following the same style that > > we have for some of the other kvm_arch_*() functions > > (kvm_arch_free_vm() for example)? > > Heh, KVM has a *lot* of code that was written with questionable style. I agree > with Marc, I can't think of a single reason not to have the definition in common > code. Declaring the function doesn't preclude a "static inline" implementation, > and we could even keep the prototype under an #ifdef, e.g. > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > index 9d3ac7720da9..5ac64f933547 100644 > --- a/include/linux/kvm_host.h > +++ b/include/linux/kvm_host.h > @@ -1484,6 +1484,8 @@ static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm) > { > return -ENOTSUPP; > } > +#else > +int kvm_arch_flush_remote_tlb(struct kvm *kvm); > #endif > > #ifdef __KVM_HAVE_ARCH_NONCOHERENT_DMA > Thanks for the suggestions; I can go with a common declaration. Along with that, do we want to keep defining __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS in the arch code that supports it or convert it into a CONFIG_? - Raghavendra
On Tue, 01 Aug 2023 01:42:54 +0100, Raghavendra Rao Ananta <rananta@google.com> wrote: > > On Mon, Jul 31, 2023 at 2:42 PM Sean Christopherson <seanjc@google.com> wrote: > > > > On Mon, Jul 31, 2023, Raghavendra Rao Ananta wrote: > > > On Thu, Jul 27, 2023 at 3:24 AM Marc Zyngier <maz@kernel.org> wrote: > > > > > > > > On Sat, 22 Jul 2023 03:22:40 +0100, > > > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > > > > > > > > From: David Matlack <dmatlack@google.com> > > > > > > > > > > Rename kvm_arch_flush_remote_tlb() and the associated macro > > > > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB to kvm_arch_flush_remote_tlbs() and > > > > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS respectively. > > > > > > > > > > Making the name plural matches kvm_flush_remote_tlbs() and makes it more > > > > > clear that this function can affect more than one remote TLB. > > > > > > > > > > No functional change intended. > > > > > > > > > > Signed-off-by: David Matlack <dmatlack@google.com> > > > > > Signed-off-by: Raghavendra Rao Ananta <rananta@google.com> > > > > > Reviewed-by: Gavin Shan <gshan@redhat.com> > > > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> > > > > > Reviewed-by: Shaoqin Huang <shahuang@redhat.com> > > > > > --- > > > > > arch/mips/include/asm/kvm_host.h | 4 ++-- > > > > > arch/mips/kvm/mips.c | 2 +- > > > > > arch/x86/include/asm/kvm_host.h | 4 ++-- > > > > > include/linux/kvm_host.h | 4 ++-- > > > > > virt/kvm/kvm_main.c | 2 +- > > > > > 5 files changed, 8 insertions(+), 8 deletions(-) > > > > > > > > > > diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h > > > > > index 04cedf9f8811..9b0ad8f3bf32 100644 > > > > > --- a/arch/mips/include/asm/kvm_host.h > > > > > +++ b/arch/mips/include/asm/kvm_host.h > > > > > @@ -896,7 +896,7 @@ static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} > > > > > static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} > > > > > static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} > > > > > > > > > > -#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB > > > > > -int kvm_arch_flush_remote_tlb(struct kvm *kvm); > > > > > +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS > > > > > +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); > > > > > > > > How about making this prototype global? I don't see a point in having > > > > it per-architecture, specially as you are adding arm64 to that mix in > > > > the following patch. > > > > > > > We can make it global, but I'm not sure what was the intention of the > > > original author. My guess is that he was following the same style that > > > we have for some of the other kvm_arch_*() functions > > > (kvm_arch_free_vm() for example)? > > > > Heh, KVM has a *lot* of code that was written with questionable style. I agree > > with Marc, I can't think of a single reason not to have the definition in common > > code. Declaring the function doesn't preclude a "static inline" implementation, > > and we could even keep the prototype under an #ifdef, e.g. > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index 9d3ac7720da9..5ac64f933547 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -1484,6 +1484,8 @@ static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm) > > { > > return -ENOTSUPP; > > } > > +#else > > +int kvm_arch_flush_remote_tlb(struct kvm *kvm); > > #endif > > > > #ifdef __KVM_HAVE_ARCH_NONCOHERENT_DMA > > > Thanks for the suggestions; I can go with a common declaration. Along > with that, do we want to keep defining > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS in the arch code that supports it or > convert it into a CONFIG_? This isn't something that a user can select, more something that is an architectural decision. Maybe in a later patch if there is a consensus around that, but probably not as part of this series. Thanks, M.
On Wed, Aug 02, 2023, Marc Zyngier wrote: > On Tue, 01 Aug 2023 01:42:54 +0100, > Raghavendra Rao Ananta <rananta@google.com> wrote: > > Thanks for the suggestions; I can go with a common declaration. Along > > with that, do we want to keep defining > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS in the arch code that supports it or > > convert it into a CONFIG_? > > This isn't something that a user can select, more something that is an > architectural decision. Maybe in a later patch if there is a consensus > around that, but probably not as part of this series. +1. I agree it's annoying that KVM uses a mix of Kconfigs and manual #defines for the various "KVM_HAVE" knobs, but we have so many of both that one-off conversions without a real need don't make much sense.
Okay, so just the #define in the respective arch header with a global declaration. I'll consider this in v8. Thanks, Raghavendra On Wed, Aug 2, 2023 at 9:10 AM Sean Christopherson <seanjc@google.com> wrote: > > On Wed, Aug 02, 2023, Marc Zyngier wrote: > > On Tue, 01 Aug 2023 01:42:54 +0100, > > Raghavendra Rao Ananta <rananta@google.com> wrote: > > > Thanks for the suggestions; I can go with a common declaration. Along > > > with that, do we want to keep defining > > > __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS in the arch code that supports it or > > > convert it into a CONFIG_? > > > > This isn't something that a user can select, more something that is an > > architectural decision. Maybe in a later patch if there is a consensus > > around that, but probably not as part of this series. > > +1. I agree it's annoying that KVM uses a mix of Kconfigs and manual #defines > for the various "KVM_HAVE" knobs, but we have so many of both that one-off > conversions without a real need don't make much sense.
diff --git a/arch/mips/include/asm/kvm_host.h b/arch/mips/include/asm/kvm_host.h index 04cedf9f8811..9b0ad8f3bf32 100644 --- a/arch/mips/include/asm/kvm_host.h +++ b/arch/mips/include/asm/kvm_host.h @@ -896,7 +896,7 @@ static inline void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu) {} static inline void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu) {} static inline void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu) {} -#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB -int kvm_arch_flush_remote_tlb(struct kvm *kvm); +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS +int kvm_arch_flush_remote_tlbs(struct kvm *kvm); #endif /* __MIPS_KVM_HOST_H__ */ diff --git a/arch/mips/kvm/mips.c b/arch/mips/kvm/mips.c index aa5583a7b05b..4b7bc39a4173 100644 --- a/arch/mips/kvm/mips.c +++ b/arch/mips/kvm/mips.c @@ -981,7 +981,7 @@ void kvm_arch_sync_dirty_log(struct kvm *kvm, struct kvm_memory_slot *memslot) } -int kvm_arch_flush_remote_tlb(struct kvm *kvm) +int kvm_arch_flush_remote_tlbs(struct kvm *kvm) { kvm_mips_callbacks->prepare_flush_shadow(kvm); return 1; diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 28bd38303d70..a2d3cfc2eb75 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -1794,8 +1794,8 @@ static inline struct kvm *kvm_arch_alloc_vm(void) #define __KVM_HAVE_ARCH_VM_FREE void kvm_arch_free_vm(struct kvm *kvm); -#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB -static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm) +#define __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS +static inline int kvm_arch_flush_remote_tlbs(struct kvm *kvm) { if (kvm_x86_ops.flush_remote_tlbs && !static_call(kvm_x86_flush_remote_tlbs)(kvm)) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 9d3ac7720da9..e3f968b38ae9 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1479,8 +1479,8 @@ static inline void kvm_arch_free_vm(struct kvm *kvm) } #endif -#ifndef __KVM_HAVE_ARCH_FLUSH_REMOTE_TLB -static inline int kvm_arch_flush_remote_tlb(struct kvm *kvm) +#ifndef __KVM_HAVE_ARCH_FLUSH_REMOTE_TLBS +static inline int kvm_arch_flush_remote_tlbs(struct kvm *kvm) { return -ENOTSUPP; } diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index dfbaafbe3a00..70e5479797ac 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -361,7 +361,7 @@ void kvm_flush_remote_tlbs(struct kvm *kvm) * kvm_make_all_cpus_request() reads vcpu->mode. We reuse that * barrier here. */ - if (!kvm_arch_flush_remote_tlb(kvm) + if (!kvm_arch_flush_remote_tlbs(kvm) || kvm_make_all_cpus_request(kvm, KVM_REQ_TLB_FLUSH)) ++kvm->stat.generic.remote_tlb_flush; }