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

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

[Yu Zhang]

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.

Tested by rebuilding KVM for x86 and for ARM64.

No functional change intended.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
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/20221209023622.274715-1-yu.c.zhang@linux.intel.com/
---
 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         |  2 +-
 6 files changed, 15 insertions(+), 17 deletions(-)

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..988d2d7762f3 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_GPA) {
 		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_GPA;
 		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..2728d49bbdf6 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -40,7 +40,7 @@ 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)
 
 typedef unsigned long  hva_t;
 typedef u64            hpa_t;
-- 
2.25.1

Re: [PATCH v2] KVM: MMU: Make the definition of 'INVALID_GPA' common.

[Paul Durrant]

On 13/12/2022 09:04, 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.
> 
> Tested by rebuilding KVM for x86 and for ARM64.
> 
> No functional change intended.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> ---
> 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/20221209023622.274715-1-yu.c.zhang@linux.intel.com/
> ---
>   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         |  2 +-
>   6 files changed, 15 insertions(+), 17 deletions(-)
> 

Reviewed-by: Paul Durrant <paul@xen.org>

Re: [PATCH v2] KVM: MMU: Make the definition of 'INVALID_GPA' common.

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Tue, 2022-12-13 at 17:04 +0800, Yu Zhang wrote:
> --- 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_GPA) {
>                 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_GPA;
>                 r = 0;
>                 break;

Strictly, those are INVALID_GFN not INVALID_GPA but I have so far
managed to pretend not to notice...

If we're bikeshedding the naming then I might have suggested
INVALID_PAGE but that already exists as an hpa_t type.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5965 bytes --]

Re: [PATCH v2] KVM: MMU: Make the definition of 'INVALID_GPA' common.

[Yu Zhang]

On Wed, Dec 14, 2022 at 11:10:54AM +0000, David Woodhouse wrote:
> On Tue, 2022-12-13 at 17:04 +0800, Yu Zhang wrote:
> > --- 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_GPA) {
> >                 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_GPA;
> >                 r = 0;
> >                 break;
> 
> Strictly, those are INVALID_GFN not INVALID_GPA but I have so far
> managed to pretend not to notice...
> 
> If we're bikeshedding the naming then I might have suggested
> INVALID_PAGE but that already exists as an hpa_t type.

Thanks, David. INVALID_GFN sounds more reasonable for me.

But I am not sure if adding INVALID_GFN is necessary. Because for now
only kvm_xen_shared_info_init() and kvm_xen_hvm_get_attr() use INVALID_GPA
as a GFN. 

Any suggestion? Thanks!

B.R.
Yu

Re: [PATCH v2] KVM: MMU: Make the definition of 'INVALID_GPA' common.

[Sean Christopherson]

On Wed, Dec 14, 2022, Yu Zhang wrote:
> On Wed, Dec 14, 2022 at 11:10:54AM +0000, David Woodhouse wrote:
> > On Tue, 2022-12-13 at 17:04 +0800, Yu Zhang wrote:
> > > --- 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_GPA) {
> > >                 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_GPA;
> > >                 r = 0;
> > >                 break;
> > 
> > Strictly, those are INVALID_GFN not INVALID_GPA but I have so far
> > managed to pretend not to notice...
> > 
> > If we're bikeshedding the naming then I might have suggested
> > INVALID_PAGE but that already exists as an hpa_t type.
> 
> Thanks, David. INVALID_GFN sounds more reasonable for me.
> 
> But I am not sure if adding INVALID_GFN is necessary. Because for now
> only kvm_xen_shared_info_init() and kvm_xen_hvm_get_attr() use INVALID_GPA
> as a GFN. 
> 
> Any suggestion? Thanks!

It's not strictly necessary, but it's an easy change and I can't think of any
reason not to add INVALID_GFN.