[v3] KVM: MMU: Make the definition of 'INVALID_GPA' common.

Commit Message

Yu Zhang Dec. 15, 2022, 9:57 a.m. UTC

KVM already has a 'GPA_INVALID' defined as (~(gpa_t)0) in
kvm_types.h, and it is used by ARM and X86 xen code. We do
not need a specific definition of 'INVALID_GPA' for X86.

Instead of using the common 'GPA_INVALID' for X86, replace
the definition of 'GPA_INVALID' with 'INVALID_GPA', and
change the users of 'GPA_INVALID', so that the diff can be
smaller. Also because the name 'INVALID_GPA' tells the user
we are using an invalid GPA, while the name 'GPA_INVALID'
is emphasizing the GPA is an invalid one.

Also, add definition of 'INVALID_GFN' because it is more
proper than 'INVALID_GPA' for GFN variables.

Tested by rebuilding KVM for x86 and for ARM64.

No functional change intended.

Signed-off-by: Yu Zhang <[e-mail]>
Reviewed-by: Paul Durrant <[e-mail]>
---
V3:
Followed David and Sean's comment to add 'INVALID_GFN'. Also,
changed the commit message.
v2:
Followed Sean's comments to rename GPA_INVALID to INVALID_GPA
and modify _those_ users. Also, changed the commit message.
v1:
https://lore.kernel.org/lkml/[e-mail]/
---
 arch/arm64/include/asm/kvm_host.h |  4 ++--
 arch/arm64/kvm/hypercalls.c       |  2 +-
 arch/arm64/kvm/pvtime.c           |  8 ++++----
 arch/x86/include/asm/kvm_host.h   |  2 --
 arch/x86/kvm/xen.c                | 14 +++++++-------
 include/linux/kvm_types.h         |  3 ++-
 6 files changed, 16 insertions(+), 17 deletions(-)

Comments

Sean Christopherson Dec. 15, 2022, 6:12 p.m. UTC | #1

Nit, terminating punction (the period) isn't usually included in the shortlog.

On Thu, Dec 15, 2022, Yu Zhang wrote:
> KVM already has a 'GPA_INVALID' defined as (~(gpa_t)0) in
> kvm_types.h, and it is used by ARM and X86 xen code. We do
> not need a specific definition of 'INVALID_GPA' for X86.
> 
> Instead of using the common 'GPA_INVALID' for X86, replace
> the definition of 'GPA_INVALID' with 'INVALID_GPA', and
> change the users of 'GPA_INVALID', so that the diff can be
> smaller. Also because the name 'INVALID_GPA' tells the user
> we are using an invalid GPA, while the name 'GPA_INVALID'
> is emphasizing the GPA is an invalid one.
> 
> Also, add definition of 'INVALID_GFN' because it is more
> proper than 'INVALID_GPA' for GFN variables.

This should be a separate commit.  Yes, it's trivial and a nop, but there's no
reason to surprise readers/blamers that assumed the shortlog tells the whole
story.  E.g. add and use INVALID_GFN where appropriate in patch 1, then switch
to INVALID_GPA in patch 2.  Then you can also add a "Suggested-by: David ..." for
patch 1.

Yu Zhang Dec. 16, 2022, 3:03 a.m. UTC | #2

On Thu, Dec 15, 2022 at 06:12:05PM +0000, Sean Christopherson wrote:
> Nit, terminating punction (the period) isn't usually included in the shortlog.
> 
> On Thu, Dec 15, 2022, Yu Zhang wrote:
> > KVM already has a 'GPA_INVALID' defined as (~(gpa_t)0) in
> > kvm_types.h, and it is used by ARM and X86 xen code. We do
> > not need a specific definition of 'INVALID_GPA' for X86.
> > 
> > Instead of using the common 'GPA_INVALID' for X86, replace
> > the definition of 'GPA_INVALID' with 'INVALID_GPA', and
> > change the users of 'GPA_INVALID', so that the diff can be
> > smaller. Also because the name 'INVALID_GPA' tells the user
> > we are using an invalid GPA, while the name 'GPA_INVALID'
> > is emphasizing the GPA is an invalid one.
> > 
> > Also, add definition of 'INVALID_GFN' because it is more
> > proper than 'INVALID_GPA' for GFN variables.
> 
> This should be a separate commit.  Yes, it's trivial and a nop, but there's no
> reason to surprise readers/blamers that assumed the shortlog tells the whole
> story.  E.g. add and use INVALID_GFN where appropriate in patch 1, then switch
> to INVALID_GPA in patch 2.  Then you can also add a "Suggested-by: David ..." for
> patch 1.

Good point! Thanks! :)

B.R.
Yu

Patch

diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
index 001c8abe87fc..fcf96e9cc8cd 100644
--- a/arch/arm64/include/asm/kvm_host.h
+++ b/arch/arm64/include/asm/kvm_host.h
@@ -906,12 +906,12 @@  void kvm_arm_vmid_clear_active(void);
 
 static inline void kvm_arm_pvtime_vcpu_init(struct kvm_vcpu_arch *vcpu_arch)
 {
-	vcpu_arch->steal.base = GPA_INVALID;
+	vcpu_arch->steal.base = INVALID_GPA;
 }
 
 static inline bool kvm_arm_is_pvtime_enabled(struct kvm_vcpu_arch *vcpu_arch)
 {
-	return (vcpu_arch->steal.base != GPA_INVALID);
+	return (vcpu_arch->steal.base != INVALID_GPA);
 }
 
 void kvm_set_sei_esr(struct kvm_vcpu *vcpu, u64 syndrome);
diff --git a/arch/arm64/kvm/hypercalls.c b/arch/arm64/kvm/hypercalls.c
index c9f401fa01a9..64c086c02c60 100644
--- a/arch/arm64/kvm/hypercalls.c
+++ b/arch/arm64/kvm/hypercalls.c
@@ -198,7 +198,7 @@  int kvm_hvc_call_handler(struct kvm_vcpu *vcpu)
 		break;
 	case ARM_SMCCC_HV_PV_TIME_ST:
 		gpa = kvm_init_stolen_time(vcpu);
-		if (gpa != GPA_INVALID)
+		if (gpa != INVALID_GPA)
 			val[0] = gpa;
 		break;
 	case ARM_SMCCC_VENDOR_HYP_CALL_UID_FUNC_ID:
diff --git a/arch/arm64/kvm/pvtime.c b/arch/arm64/kvm/pvtime.c
index 78a09f7a6637..4ceabaa4c30b 100644
--- a/arch/arm64/kvm/pvtime.c
+++ b/arch/arm64/kvm/pvtime.c
@@ -19,7 +19,7 @@  void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
 	u64 steal = 0;
 	int idx;
 
-	if (base == GPA_INVALID)
+	if (base == INVALID_GPA)
 		return;
 
 	idx = srcu_read_lock(&kvm->srcu);
@@ -40,7 +40,7 @@  long kvm_hypercall_pv_features(struct kvm_vcpu *vcpu)
 	switch (feature) {
 	case ARM_SMCCC_HV_PV_TIME_FEATURES:
 	case ARM_SMCCC_HV_PV_TIME_ST:
-		if (vcpu->arch.steal.base != GPA_INVALID)
+		if (vcpu->arch.steal.base != INVALID_GPA)
 			val = SMCCC_RET_SUCCESS;
 		break;
 	}
@@ -54,7 +54,7 @@  gpa_t kvm_init_stolen_time(struct kvm_vcpu *vcpu)
 	struct kvm *kvm = vcpu->kvm;
 	u64 base = vcpu->arch.steal.base;
 
-	if (base == GPA_INVALID)
+	if (base == INVALID_GPA)
 		return base;
 
 	/*
@@ -89,7 +89,7 @@  int kvm_arm_pvtime_set_attr(struct kvm_vcpu *vcpu,
 		return -EFAULT;
 	if (!IS_ALIGNED(ipa, 64))
 		return -EINVAL;
-	if (vcpu->arch.steal.base != GPA_INVALID)
+	if (vcpu->arch.steal.base != INVALID_GPA)
 		return -EEXIST;
 
 	/* Check the address is in a valid memslot */
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index f35f1ff4427b..46e50cb6c9ca 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -134,8 +134,6 @@ 
 #define INVALID_PAGE (~(hpa_t)0)
 #define VALID_PAGE(x) ((x) != INVALID_PAGE)
 
-#define INVALID_GPA (~(gpa_t)0)
-
 /* KVM Hugepage definitions for x86 */
 #define KVM_MAX_HUGEPAGE_LEVEL	PG_LEVEL_1G
 #define KVM_NR_PAGE_SIZES	(KVM_MAX_HUGEPAGE_LEVEL - PG_LEVEL_4K + 1)
diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index d7af40240248..ec893276681c 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -41,7 +41,7 @@  static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn)
 	int ret = 0;
 	int idx = srcu_read_lock(&kvm->srcu);
 
-	if (gfn == GPA_INVALID) {
+	if (gfn == INVALID_GFN) {
 		kvm_gpc_deactivate(gpc);
 		goto out;
 	}
@@ -659,7 +659,7 @@  int kvm_xen_hvm_get_attr(struct kvm *kvm, struct kvm_xen_hvm_attr *data)
 		if (kvm->arch.xen.shinfo_cache.active)
 			data->u.shared_info.gfn = gpa_to_gfn(kvm->arch.xen.shinfo_cache.gpa);
 		else
-			data->u.shared_info.gfn = GPA_INVALID;
+			data->u.shared_info.gfn = INVALID_GFN;
 		r = 0;
 		break;
 
@@ -705,7 +705,7 @@  int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 		BUILD_BUG_ON(offsetof(struct vcpu_info, time) !=
 			     offsetof(struct compat_vcpu_info, time));
 
-		if (data->u.gpa == GPA_INVALID) {
+		if (data->u.gpa == INVALID_GPA) {
 			kvm_gpc_deactivate(&vcpu->arch.xen.vcpu_info_cache);
 			r = 0;
 			break;
@@ -719,7 +719,7 @@  int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 		break;
 
 	case KVM_XEN_VCPU_ATTR_TYPE_VCPU_TIME_INFO:
-		if (data->u.gpa == GPA_INVALID) {
+		if (data->u.gpa == INVALID_GPA) {
 			kvm_gpc_deactivate(&vcpu->arch.xen.vcpu_time_info_cache);
 			r = 0;
 			break;
@@ -739,7 +739,7 @@  int kvm_xen_vcpu_set_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 			r = -EOPNOTSUPP;
 			break;
 		}
-		if (data->u.gpa == GPA_INVALID) {
+		if (data->u.gpa == INVALID_GPA) {
 			r = 0;
 		deactivate_out:
 			kvm_gpc_deactivate(&vcpu->arch.xen.runstate_cache);
@@ -937,7 +937,7 @@  int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 		if (vcpu->arch.xen.vcpu_info_cache.active)
 			data->u.gpa = vcpu->arch.xen.vcpu_info_cache.gpa;
 		else
-			data->u.gpa = GPA_INVALID;
+			data->u.gpa = INVALID_GPA;
 		r = 0;
 		break;
 
@@ -945,7 +945,7 @@  int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
 		if (vcpu->arch.xen.vcpu_time_info_cache.active)
 			data->u.gpa = vcpu->arch.xen.vcpu_time_info_cache.gpa;
 		else
-			data->u.gpa = GPA_INVALID;
+			data->u.gpa = INVALID_GPA;
 		r = 0;
 		break;
 
diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
index 76de36e56cdf..b961043e664a 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -40,7 +40,8 @@  typedef unsigned long  gva_t;
 typedef u64            gpa_t;
 typedef u64            gfn_t;
 
-#define GPA_INVALID	(~(gpa_t)0)
+#define INVALID_GPA	(~(gpa_t)0)
+#define INVALID_GFN	(~(gfn_t)0)
 
 typedef unsigned long  hva_t;
 typedef u64            hpa_t;