On 22 December 2022 18:53:35 GMT, Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxxxxx>
>> > > Signed-off-by: Yu Zhang <yu.c.zhang@xxxxxxxxxxxxxxx>
>> > > ---
>> > > 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.
