[PATCH v2] KVM: x86/xen: ignore the VCPU_SSHOTTMR_future flag

[PATCH v2] KVM: x86/xen: ignore the VCPU_SSHOTTMR_future flag

[Paul Durrant]

From: Paul Durrant <pdurrant@amazon.com>

Upstream Xen now ignores this flag [1], since the only guest kernel ever to
use it was buggy. By ignoring the flag the guest will always get a callback
if it sets a negative timeout which upstream Xen has determined not to
cause problems for any guest setting the flag.

[1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=19c6cbd909

Signed-off-by: Paul Durrant <pdurrant@amazon.com>
Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
---
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Sean Christopherson <seanjc@google.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: x86@kernel.org

v2:
 - Amend the patch subject line
---
 arch/x86/kvm/xen.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
index 40edf4d1974c..8f1d46df0f3b 100644
--- a/arch/x86/kvm/xen.c
+++ b/arch/x86/kvm/xen.c
@@ -1374,12 +1374,8 @@ static bool kvm_xen_hcall_vcpu_op(struct kvm_vcpu *vcpu, bool longmode, int cmd,
 			return true;
 		}
 
+		/* A delta <= 0 results in an immediate callback, which is what we want */
 		delta = oneshot.timeout_abs_ns - get_kvmclock_ns(vcpu->kvm);
-		if ((oneshot.flags & VCPU_SSHOTTMR_future) && delta < 0) {
-			*r = -ETIME;
-			return true;
-		}
-
 		kvm_xen_start_timer(vcpu, oneshot.timeout_abs_ns, delta);
 		*r = 0;
 		return true;
-- 
2.39.2

Re: [PATCH v2] KVM: x86/xen: ignore the VCPU_SSHOTTMR_future flag

[Sean Christopherson]

On Wed, Oct 04, 2023, Paul Durrant wrote:
> From: Paul Durrant <pdurrant@amazon.com>
> 
> Upstream Xen now ignores this flag [1], since the only guest kernel ever to
> use it was buggy. By ignoring the flag the guest will always get a callback
> if it sets a negative timeout which upstream Xen has determined not to
> cause problems for any guest setting the flag.
> 
> [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=19c6cbd909
> 
> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Sean Christopherson <seanjc@google.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: x86@kernel.org

If you're going to manually Cc folks, put the Cc's in the changelog proper so that
there's a record of who was Cc'd on the patch.

Or even better, just use scripts/get_maintainers.pl and only manually Cc people
when necessary.

Re: [PATCH v2] KVM: x86/xen: ignore the VCPU_SSHOTTMR_future flag

[Sean Christopherson]

On Wed, 04 Oct 2023 17:46:28 +0000, Paul Durrant wrote:
> Upstream Xen now ignores this flag [1], since the only guest kernel ever to
> use it was buggy. By ignoring the flag the guest will always get a callback
> if it sets a negative timeout which upstream Xen has determined not to
> cause problems for any guest setting the flag.
> 
> [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=19c6cbd909
> 
> [...]

Applied to kvm-x86 xen, thanks!

[1/1] KVM: x86/xen: ignore the VCPU_SSHOTTMR_future flag
      https://github.com/kvm-x86/linux/commit/409f2e92a27a

--
https://github.com/kvm-x86/linux/tree/next

Re: [PATCH v2] KVM: x86/xen: ignore the VCPU_SSHOTTMR_future flag

[Paul Durrant]

On 04/10/2023 19:30, Sean Christopherson wrote:
> On Wed, Oct 04, 2023, Paul Durrant wrote:
>> From: Paul Durrant <pdurrant@amazon.com>
>>
>> Upstream Xen now ignores this flag [1], since the only guest kernel ever to
>> use it was buggy. By ignoring the flag the guest will always get a callback
>> if it sets a negative timeout which upstream Xen has determined not to
>> cause problems for any guest setting the flag.
>>
>> [1] https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=19c6cbd909
>>
>> Signed-off-by: Paul Durrant <pdurrant@amazon.com>
>> Reviewed-by: David Woodhouse <dwmw@amazon.co.uk>
>> ---
>> Cc: David Woodhouse <dwmw2@infradead.org>
>> Cc: Sean Christopherson <seanjc@google.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Borislav Petkov <bp@alien8.de>
>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: x86@kernel.org
> 
> If you're going to manually Cc folks, put the Cc's in the changelog proper so that
> there's a record of who was Cc'd on the patch.
> 

FTR, the basic list was generated:

./scripts/get_maintainer.pl --no-rolestats 
0001-KVM-xen-ignore-the-VCPU_SSHOTTMR_future-flag.patch | while read 
line; do echo Cc: $line; done

and then lightly hacked put x86 at the end and remove my own name... so 
not really manual.
Also not entirely sure why you'd want the Cc list making it into the 
actual commit.

> Or even better, just use scripts/get_maintainers.pl and only manually Cc people
> when necessary.

I guess this must be some other way of using get_maintainers.pl that you 
are expecting?

Re: [PATCH v2] KVM: x86/xen: ignore the VCPU_SSHOTTMR_future flag

[Sean Christopherson]

On Thu, Oct 05, 2023, Paul Durrant wrote:
> On 04/10/2023 19:30, Sean Christopherson wrote:
> > On Wed, Oct 04, 2023, Paul Durrant wrote:
> > > ---
> > > Cc: David Woodhouse <dwmw2@infradead.org>
> > > Cc: Sean Christopherson <seanjc@google.com>
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: Ingo Molnar <mingo@redhat.com>
> > > Cc: Borislav Petkov <bp@alien8.de>
> > > Cc: Dave Hansen <dave.hansen@linux.intel.com>
> > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > Cc: x86@kernel.org
> > 
> > If you're going to manually Cc folks, put the Cc's in the changelog proper so that
> > there's a record of who was Cc'd on the patch.
> > 
> 
> FTR, the basic list was generated:
> 
> ./scripts/get_maintainer.pl --no-rolestats
> 0001-KVM-xen-ignore-the-VCPU_SSHOTTMR_future-flag.patch | while read line;
> do echo Cc: $line; done
> 
> and then lightly hacked put x86 at the end and remove my own name... so not
> really manual.
> Also not entirely sure why you'd want the Cc list making it into the actual
> commit.

It's useful for Cc's that *don't* come from get_maintainers, as it provides a
record in the commit of who was Cc'd on a patch. 

E.g. if someone encounters an issue with a commit, the Cc records provide additional
contacts that might be able to help sort things out.

Or if a maintainer further up the stream has questions or concerns about a pull
request, they can use the Cc list to grab the right audience for a discussion,
or be more confident in merging the request because the maintainer knows that the
"right" people at least saw the patch.

Lore links provide much of that functionality, but following a link is almost
always slower, and some maintainers are allergic to web browsers :-)

> > Or even better, just use scripts/get_maintainers.pl and only manually Cc people
> > when necessary.
> 
> I guess this must be some other way of using get_maintainers.pl that you are
> expecting?

Ah, I was just assuming that you were handcoding the Cc "list", but it sounds
like you're piping the results into each patch.  That's fine, just a bit noisy
and uncommon.

FWIW, my scripts gather the To/Cc for all patches in a series, and then use the
results for the entire series, e.g.

  git send-email --confirm=always --suppress-cc=all $to $bcc $cc ...

That way everyone that gets sent mail gets all patches in a series.  Most
contributors, myself included, don't like to receive bits and pieces of a series,
e.g. it makes doing quick triage/reviews annoying, especially if the patches I
didn't receive weren't sent to any of the mailing list to which I'm subscribed.

Re: [PATCH v2] KVM: x86/xen: ignore the VCPU_SSHOTTMR_future flag

[Paul Durrant]

On 06/10/2023 02:48, Sean Christopherson wrote:
> On Thu, Oct 05, 2023, Paul Durrant wrote:
>> On 04/10/2023 19:30, Sean Christopherson wrote:
>>> On Wed, Oct 04, 2023, Paul Durrant wrote:
>>>> ---
>>>> Cc: David Woodhouse <dwmw2@infradead.org>
>>>> Cc: Sean Christopherson <seanjc@google.com>
>>>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>>>> Cc: Thomas Gleixner <tglx@linutronix.de>
>>>> Cc: Ingo Molnar <mingo@redhat.com>
>>>> Cc: Borislav Petkov <bp@alien8.de>
>>>> Cc: Dave Hansen <dave.hansen@linux.intel.com>
>>>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>>>> Cc: x86@kernel.org
>>>
>>> If you're going to manually Cc folks, put the Cc's in the changelog proper so that
>>> there's a record of who was Cc'd on the patch.
>>>
>>
>> FTR, the basic list was generated:
>>
>> ./scripts/get_maintainer.pl --no-rolestats
>> 0001-KVM-xen-ignore-the-VCPU_SSHOTTMR_future-flag.patch | while read line;
>> do echo Cc: $line; done
>>
>> and then lightly hacked put x86 at the end and remove my own name... so not
>> really manual.
>> Also not entirely sure why you'd want the Cc list making it into the actual
>> commit.
> 
> It's useful for Cc's that *don't* come from get_maintainers, as it provides a
> record in the commit of who was Cc'd on a patch.
> 
> E.g. if someone encounters an issue with a commit, the Cc records provide additional
> contacts that might be able to help sort things out.
> 
> Or if a maintainer further up the stream has questions or concerns about a pull
> request, they can use the Cc list to grab the right audience for a discussion,
> or be more confident in merging the request because the maintainer knows that the
> "right" people at least saw the patch.
> 
> Lore links provide much of that functionality, but following a link is almost
> always slower, and some maintainers are allergic to web browsers :-)
> 

Ok... makes sense.

>>> Or even better, just use scripts/get_maintainers.pl and only manually Cc people
>>> when necessary.
>>
>> I guess this must be some other way of using get_maintainers.pl that you are
>> expecting?
> 
> Ah, I was just assuming that you were handcoding the Cc "list", but it sounds
> like you're piping the results into each patch.  That's fine, just a bit noisy
> and uncommon.
> 
> FWIW, my scripts gather the To/Cc for all patches in a series, and then use the
> results for the entire series, e.g.
> 
>    git send-email --confirm=always --suppress-cc=all $to $bcc $cc ...
> 
> That way everyone that gets sent mail gets all patches in a series.  Most
> contributors, myself included, don't like to receive bits and pieces of a series,
> e.g. it makes doing quick triage/reviews annoying, especially if the patches I
> didn't receive weren't sent to any of the mailing list to which I'm subscribed.

Ok, I'll send stuff that way in future. Thanks,

   Paul