2023-10-04 17:46:48

by Paul Durrant

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

From: Paul Durrant <[email protected]>

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 <[email protected]>
Reviewed-by: David Woodhouse <[email protected]>
---
Cc: David Woodhouse <[email protected]>
Cc: Sean Christopherson <[email protected]>
Cc: Paolo Bonzini <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Borislav Petkov <[email protected]>
Cc: Dave Hansen <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]

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


2023-10-04 18:31:45

by Sean Christopherson

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

On Wed, Oct 04, 2023, Paul Durrant wrote:
> From: Paul Durrant <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: David Woodhouse <[email protected]>
> ---
> Cc: David Woodhouse <[email protected]>
> Cc: Sean Christopherson <[email protected]>
> Cc: Paolo Bonzini <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Borislav Petkov <[email protected]>
> Cc: Dave Hansen <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]

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.

2023-10-05 01:31:51

by Sean Christopherson

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

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

2023-10-05 13:56:48

by Paul Durrant

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

On 04/10/2023 19:30, Sean Christopherson wrote:
> On Wed, Oct 04, 2023, Paul Durrant wrote:
>> From: Paul Durrant <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: David Woodhouse <[email protected]>
>> ---
>> Cc: David Woodhouse <[email protected]>
>> Cc: Sean Christopherson <[email protected]>
>> Cc: Paolo Bonzini <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: Borislav Petkov <[email protected]>
>> Cc: Dave Hansen <[email protected]>
>> Cc: "H. Peter Anvin" <[email protected]>
>> Cc: [email protected]
>
> 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?

2023-10-06 01:49:17

by Sean Christopherson

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

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 <[email protected]>
> > > Cc: Sean Christopherson <[email protected]>
> > > Cc: Paolo Bonzini <[email protected]>
> > > Cc: Thomas Gleixner <[email protected]>
> > > Cc: Ingo Molnar <[email protected]>
> > > Cc: Borislav Petkov <[email protected]>
> > > Cc: Dave Hansen <[email protected]>
> > > Cc: "H. Peter Anvin" <[email protected]>
> > > Cc: [email protected]
> >
> > 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.

2023-10-06 08:41:56

by Paul Durrant

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

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 <[email protected]>
>>>> Cc: Sean Christopherson <[email protected]>
>>>> Cc: Paolo Bonzini <[email protected]>
>>>> Cc: Thomas Gleixner <[email protected]>
>>>> Cc: Ingo Molnar <[email protected]>
>>>> Cc: Borislav Petkov <[email protected]>
>>>> Cc: Dave Hansen <[email protected]>
>>>> Cc: "H. Peter Anvin" <[email protected]>
>>>> Cc: [email protected]
>>>
>>> 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