[PATCH v4 0/2] KVM: MMU: Use 'INVALID_GPA' and 'INVALID_GFN' properly

[PATCH v4 0/2] KVM: MMU: Use 'INVALID_GPA' and 'INVALID_GFN' properly

[Yu Zhang]

Pacth 1 adds a new definition, 'INVALID_GFN', so that GFN
values can use it, instead of the 'GPA_INVALID'.

Pacth 2 makes the definition of 'INVALID_GPA' shared by
different archs.

Both are tested by rebuilding KVM for x86 and for ARM64.

---
V4:
- Put the addition of 'INVALID_GFN' into a seperate patch.
V3:
- Added 'INVALID_GFN' and use it.
v2:
- Renamed 'GPA_INVALID' to 'INVALID_GPA' and modify _those_ users. 
v1:
https://lore.kernel.org/lkml/20221209023622.274715-1-yu.c.zhang@linux.intel.com/

Yu Zhang (2):
  KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values
  KVM: MMU: Make the definition of 'INVALID_GPA' common

 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 ++-
 .../testing/selftests/kvm/x86_64/xen_shinfo_test.c |  4 ++--
 7 files changed, 18 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Yu Zhang]

Currently, KVM xen and its shared info selftest code uses
'GPA_INVALID' for GFN values, but actually it is more accurate
to use the name 'INVALID_GFN'. So just add a new definition
and use it.

No functional changes intended.

Suggested-by: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
---
 arch/x86/kvm/xen.c                                   | 4 ++--
 include/linux/kvm_types.h                            | 1 +
 tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 4 ++--
 3 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index d7af40240248..6908a74ab303 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;
 
diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
index 76de36e56cdf..d21c0d7fee31 100644
--- a/include/linux/kvm_types.h
+++ b/include/linux/kvm_types.h
@@ -41,6 +41,7 @@ typedef u64            gpa_t;
 typedef u64            gfn_t;
 
 #define GPA_INVALID	(~(gpa_t)0)
+#define INVALID_GFN	(~(gfn_t)0)
 
 typedef unsigned long  hva_t;
 typedef u64            hpa_t;
diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index 721f6a693799..d65a23be88b1 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -20,7 +20,7 @@
 #include <sys/eventfd.h>
 
 /* Defined in include/linux/kvm_types.h */
-#define GPA_INVALID		(~(ulong)0)
+#define INVALID_GFN		(~(ulong)0)
 
 #define SHINFO_REGION_GVA	0xc0000000ULL
 #define SHINFO_REGION_GPA	0xc0000000ULL
@@ -419,7 +419,7 @@ static void *juggle_shinfo_state(void *arg)
 
 	struct kvm_xen_hvm_attr cache_destroy = {
 		.type = KVM_XEN_ATTR_TYPE_SHARED_INFO,
-		.u.shared_info.gfn = GPA_INVALID
+		.u.shared_info.gfn = INVALID_GFN
 	};
 
 	for (;;) {
-- 
2.25.1

[PATCH v4 2/2] 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.

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

No functional change intended.

Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
Reviewed-by: Paul Durrant <paul@xen.org>
---
 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                | 10 +++++-----
 include/linux/kvm_types.h         |  2 +-
 6 files changed, 13 insertions(+), 15 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 6908a74ab303..ec893276681c 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -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 d21c0d7fee31..b961043e664a 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)
 #define INVALID_GFN	(~(gfn_t)0)
 
 typedef unsigned long  hva_t;
-- 
2.25.1

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Michal Luczaj]

On 12/16/22 09:59, Yu Zhang wrote:
> +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> @@ -20,7 +20,7 @@
>  #include <sys/eventfd.h>
>  
>  /* Defined in include/linux/kvm_types.h */
> -#define GPA_INVALID		(~(ulong)0)
> +#define INVALID_GFN		(~(ulong)0)


Thank you for fixing the selftest!

Regarding xen_shinfo_test.c, a question to maintainers, would it be ok if I
submit a simple patch fixing the misnamed cache_init/cache_destroy =>
cache_activate/cache_deactivate or it's just not worth the churn now?

Michal

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Sean Christopherson]

On Fri, Dec 16, 2022, Michal Luczaj wrote:
> On 12/16/22 09:59, Yu Zhang wrote:
> > +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
> > @@ -20,7 +20,7 @@
> >  #include <sys/eventfd.h>
> >  
> >  /* Defined in include/linux/kvm_types.h */
> > -#define GPA_INVALID		(~(ulong)0)
> > +#define INVALID_GFN		(~(ulong)0)
> 
> 
> Thank you for fixing the selftest!
> 
> Regarding xen_shinfo_test.c, a question to maintainers, would it be ok if I
> submit a simple patch fixing the misnamed cache_init/cache_destroy =>
> cache_activate/cache_deactivate or it's just not worth the churn now?

It's not too much churn.  My only hesitation would be that chasing KVM names is
usually a fruitless endeavor, but in this case I agree (de)activate is better
terminology even if KVM changes again in the future.

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Sean Christopherson]

On Fri, Dec 16, 2022, Yu Zhang wrote:
> Currently, KVM xen and its shared info selftest code uses
> 'GPA_INVALID' for GFN values, but actually it is more accurate
> to use the name 'INVALID_GFN'. So just add a new definition
> and use it.
> 
> No functional changes intended.
> 
> Suggested-by: David Woodhouse <dwmw2@infradead.org>
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> ---
>  arch/x86/kvm/xen.c                                   | 4 ++--
>  include/linux/kvm_types.h                            | 1 +
>  tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 4 ++--
>  3 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index d7af40240248..6908a74ab303 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) {

Grrr!  This magic value is ABI, as "gfn == -1" yields different behavior than a
random, garbage gfn.
                                                                                
So, sadly, we can't simply introduce INVALID_GFN here, and instead need to do
something like:

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 20522d4ba1e0..2d31caaf812c 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1766,6 +1766,7 @@ struct kvm_xen_hvm_attr {
                __u8 vector;
                __u8 runstate_update_flag;
                struct {
+#define KVM_XEN_INVALID_GFN    (~0ull)
                        __u64 gfn;
                } shared_info;
                struct {

>  		kvm_gpc_deactivate(gpc);
>  		goto out;
>  	}

Re: [PATCH v4 2/2] KVM: MMU: Make the definition of 'INVALID_GPA' common

[Sean Christopherson]

On Fri, Dec 16, 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.

Please wrap closer to ~75 chars, ~60 is too aggressive.

> No functional change intended.
> 
> Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> Reviewed-by: Paul Durrant <paul@xen.org>
> ---

Reviewed-by: Sean Christopherson <seanjc@google.com>

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Michal Luczaj]

On 12/16/22 17:16, Sean Christopherson wrote:
> On Fri, Dec 16, 2022, Michal Luczaj wrote:
>> On 12/16/22 09:59, Yu Zhang wrote:
>>> +++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
>>> @@ -20,7 +20,7 @@
>>>  #include <sys/eventfd.h>
>>>  
>>>  /* Defined in include/linux/kvm_types.h */
>>> -#define GPA_INVALID		(~(ulong)0)
>>> +#define INVALID_GFN		(~(ulong)0)
>>
>>
>> Thank you for fixing the selftest!
>>
>> Regarding xen_shinfo_test.c, a question to maintainers, would it be ok if I
>> submit a simple patch fixing the misnamed cache_init/cache_destroy =>
>> cache_activate/cache_deactivate or it's just not worth the churn now?
> 
> It's not too much churn.  My only hesitation would be that chasing KVM names is
> usually a fruitless endeavor, but in this case I agree (de)activate is better
> terminology even if KVM changes again in the future.

All right, I'll send the fix after Yu's patches land in queue.

Thanks,
Michal

Re: [PATCH v4 2/2] KVM: MMU: Make the definition of 'INVALID_GPA' common

[Huang, Kai]

On Fri, 2022-12-16 at 16:59 +0800, Yu Zhang wrote:
> Also, add definition of 'INVALID_GFN' because it is more
> proper than 'INVALID_GPA' for GFN variables.

This perhaps is a leftover, since it belongs to patch 1 :)

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Yu Zhang]

On Fri, Dec 16, 2022 at 04:34:50PM +0000, Sean Christopherson wrote:
> On Fri, Dec 16, 2022, Yu Zhang wrote:
> > Currently, KVM xen and its shared info selftest code uses
> > 'GPA_INVALID' for GFN values, but actually it is more accurate
> > to use the name 'INVALID_GFN'. So just add a new definition
> > and use it.
> > 
> > No functional changes intended.
> > 
> > Suggested-by: David Woodhouse <dwmw2@infradead.org>
> > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> > ---
> >  arch/x86/kvm/xen.c                                   | 4 ++--
> >  include/linux/kvm_types.h                            | 1 +
> >  tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 4 ++--
> >  3 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > index d7af40240248..6908a74ab303 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) {
> 
> Grrr!  This magic value is ABI, as "gfn == -1" yields different behavior than a
> random, garbage gfn.

Thanks Sean. But I do not get it.
May I ask why ABI usages are different?  Or is there any documentation
describing the requirement? Thanks!

>                                                                                 
> So, sadly, we can't simply introduce INVALID_GFN here, and instead need to do
> something like:
> 
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 20522d4ba1e0..2d31caaf812c 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1766,6 +1766,7 @@ struct kvm_xen_hvm_attr {
>                 __u8 vector;
>                 __u8 runstate_update_flag;
>                 struct {
> +#define KVM_XEN_INVALID_GFN    (~0ull)
>                         __u64 gfn;
>                 } shared_info;

I guess above policy shall also be applied for the gpa inside struct
kvm_xen_vcpu_attr. Instead of using INVALID_GPA (in patch 2), should
be like:

diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 61c052d51a64..c06ef8ed9680 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -1823,6 +1823,7 @@ struct kvm_xen_vcpu_attr {
        __u16 type;
        __u16 pad[3];
        union {
+#define KVM_XEN_INVALID_GPA            (~0ull)
                __u64 gpa;
                __u64 pad[8];
                struct {

Also, xen.c should use KVM_XEN_INVALID_GPA for GPA values...

B.R.
Yu

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[David Woodhouse]

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

On Fri, 2022-12-16 at 16:34 +0000, Sean Christopherson wrote:
> On Fri, Dec 16, 2022, Yu Zhang wrote:
> > Currently, KVM xen and its shared info selftest code uses
> > 'GPA_INVALID' for GFN values, but actually it is more accurate
> > to use the name 'INVALID_GFN'. So just add a new definition
> > and use it.
> > 
> > No functional changes intended.
> > 
> > Suggested-by: David Woodhouse <dwmw2@infradead.org>
> > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> > ---
> >  arch/x86/kvm/xen.c                                   | 4 ++--
> >  include/linux/kvm_types.h                            | 1 +
> >  tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 4 ++--
> >  3 files changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > index d7af40240248..6908a74ab303 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) {
> 
> Grrr!  This magic value is ABI, as "gfn == -1" yields different behavior than a
> random, garbage gfn.
>                                                                                 
> So, sadly, we can't simply introduce INVALID_GFN here, and instead need to do
> something like:
> 

Well... you can still use INVALID_GFN as long as its value remains the
same ((uint64_t)-1).

But yes, the KVM API differs here from Xen because Xen only allows a
guest to *set* these (and later they invented SHUTDOWN_soft_reset).
While KVM lets the userspace VMM handle soft reset, and needs to allow
them to be *unset*. And since zero is a valid GPA/GFN, -1 is a
reasonable value for 'INVALID'. But I do think that detail escaped the
documentation and the uapi headers, so your suggestion below is a good
one, although strictly we need a GPA one too.

> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 20522d4ba1e0..2d31caaf812c 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -1766,6 +1766,7 @@ struct kvm_xen_hvm_attr {
>                 __u8 vector;
>                 __u8 runstate_update_flag;
>                 struct {
> +#define KVM_XEN_INVALID_GFN    (~0ull)
>                         __u64 gfn;
>                 } shared_info;
>                 struct {
> 
> >                 kvm_gpc_deactivate(gpc);
> >                 goto out;
> >         }


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

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Sean Christopherson]

On Tue, Dec 20, 2022, David Woodhouse wrote:
> On Fri, 2022-12-16 at 16:34 +0000, Sean Christopherson wrote:
> > On Fri, Dec 16, 2022, Yu Zhang wrote:
> > > Currently, KVM xen and its shared info selftest code uses
> > > 'GPA_INVALID' for GFN values, but actually it is more accurate
> > > to use the name 'INVALID_GFN'. So just add a new definition
> > > and use it.
> > > 
> > > No functional changes intended.
> > > 
> > > Suggested-by: David Woodhouse <dwmw2@infradead.org>
> > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
> > > ---
> > >  arch/x86/kvm/xen.c                                   | 4 ++--
> > >  include/linux/kvm_types.h                            | 1 +
> > >  tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 4 ++--
> > >  3 files changed, 5 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > > index d7af40240248..6908a74ab303 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) {
> > 
> > Grrr!  This magic value is ABI, as "gfn == -1" yields different behavior than a
> > random, garbage gfn.
> >                                                                                 
> > So, sadly, we can't simply introduce INVALID_GFN here, and instead need to do
> > something like:
> > 
> 
> Well... you can still use INVALID_GFN as long as its value remains the
> same ((uint64_t)-1).
> 
> But yes, the KVM API differs here from Xen because Xen only allows a
> guest to *set* these (and later they invented SHUTDOWN_soft_reset).
> While KVM lets the userspace VMM handle soft reset, and needs to allow
> them to be *unset*. And since zero is a valid GPA/GFN, -1 is a
> reasonable value for 'INVALID'.

Oh, yeah, I'm not arguing against using '-1', just calling out that there is
special meaning given to '-1' and so it needs to be formalized so that KVM doesn't
accidentally break userspace.

> But I do think that detail escaped the documentation and the uapi headers, so
> your suggestion below is a good one, although strictly we need a GPA one too.

Ah, right, for struct kvm_xen_vcpu_attr.

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[David Woodhouse]

On 22 December 2022 18:53:35 GMT, Sean Christopherson <seanjc@google.com> wrote:
>On Tue, Dec 20, 2022, David Woodhouse wrote:
>> On Fri, 2022-12-16 at 16:34 +0000, Sean Christopherson wrote:
>> > On Fri, Dec 16, 2022, Yu Zhang wrote:
>> > > Currently, KVM xen and its shared info selftest code uses
>> > > 'GPA_INVALID' for GFN values, but actually it is more accurate
>> > > to use the name 'INVALID_GFN'. So just add a new definition
>> > > and use it.
>> > > 
>> > > No functional changes intended.
>> > > 
>> > > Suggested-by: David Woodhouse <dwmw2@infradead.org>
>> > > Signed-off-by: Yu Zhang <yu.c.zhang@linux.intel.com>
>> > > ---
>> > >  arch/x86/kvm/xen.c                                   | 4 ++--
>> > >  include/linux/kvm_types.h                            | 1 +
>> > >  tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c | 4 ++--
>> > >  3 files changed, 5 insertions(+), 4 deletions(-)
>> > > 
>> > > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
>> > > index d7af40240248..6908a74ab303 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) {
>> > 
>> > Grrr!  This magic value is ABI, as "gfn == -1" yields different behavior than a
>> > random, garbage gfn.
>> >                                                                                 
>> > So, sadly, we can't simply introduce INVALID_GFN here, and instead need to do
>> > something like:
>> > 
>> 
>> Well... you can still use INVALID_GFN as long as its value remains the
>> same ((uint64_t)-1).
>> 
>> But yes, the KVM API differs here from Xen because Xen only allows a
>> guest to *set* these (and later they invented SHUTDOWN_soft_reset).
>> While KVM lets the userspace VMM handle soft reset, and needs to allow
>> them to be *unset*. And since zero is a valid GPA/GFN, -1 is a
>> reasonable value for 'INVALID'.
>
>Oh, yeah, I'm not arguing against using '-1', just calling out that there is
>special meaning given to '-1' and so it needs to be formalized so that KVM doesn't
>accidentally break userspace.

Indeed. I should make sure the xen_shinfo_test covers it too. We had been fairly diligent about selftests for all this, as I *hadn't* yet got round to posting the updated qemu support to go with it 

Will update the docs and test accordingly. I have a couple of other minor doc fixes which I spotted as I was doing the qemu support. If nobody beats me to the uapi header part, I'll do that too. But I'm not *scheduled* to be in front of a real computer until next year now, and and time I do steal is likely to be spent on qemu itself.

Re: [PATCH v4 1/2] KVM: MMU: Introduce 'INVALID_GFN' and use it for GFN values

[Sean Christopherson]

On Thu, Dec 22, 2022, David Woodhouse wrote:
> Will update the docs and test accordingly. I have a couple of other minor doc
> fixes which I spotted as I was doing the qemu support. If nobody beats me to
> the uapi header part, I'll do that too. But I'm not *scheduled* to be in
> front of a real computer until next year now, and and time I do steal is
> likely to be spent on qemu itself.

No rush, I'm about to disappear until next year too.  I'm guessing Paolo will be
fairly inactive next week too.