| Message ID | 20221216085928.1671901-2-yu.c.zhang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: MMU: Use 'INVALID_GPA' and 'INVALID_GFN' properly | expand |
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
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.
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; > }
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
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
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; > > }
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.
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.
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.
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 (;;) {
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(-)