Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Mark Brown]

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

On Wed, Oct 09, 2024 at 08:49:42AM -0700, Sean Christopherson wrote:
> Return a uint64_t from vcpu_get_reg() instead of having the caller provide
> a pointer to storage, as none of the vcpu_get_reg() usage in KVM selftests
> accesses a register larger than 64 bits, and vcpu_set_reg() only accepts a
> 64-bit value.  If a use case comes along that needs to get a register that
> is larger than 64 bits, then a utility can be added to assert success and
> take a void pointer, but until then, forcing an out param yields ugly code
> and prevents feeding the output of vcpu_get_reg() into vcpu_set_reg().

This commit, which is in today's -next as 5c6c7b71a45c9c, breaks the
build on arm64:

aarch64/psci_test.c: In function ‘host_test_system_off2’:
aarch64/psci_test.c:247:9: error: too many arguments to function ‘vcpu_get_reg’
  247 |         vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
      |         ^~~~~~~~~~~~
In file included from aarch64/psci_test.c:18:
include/kvm_util.h:705:24: note: declared here
  705 | static inline uint64_t vcpu_get_reg(struct kvm_vcpu *vcpu, uint64_t id)
      |                        ^~~~~~~~~~~~
At top level:
cc1: note: unrecognized command-line option ‘-Wno-gnu-variable-sized-type-not-at
-end’ may have been intended to silence earlier diagnostics

since the updates done to that file did not take account of 72be5aa6be4
("KVM: selftests: Add test for PSCI SYSTEM_OFF2") which has been merged
in the kvm-arm64 tree.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Sean Christopherson]

On Fri, Nov 01, 2024, Mark Brown wrote:
> On Wed, Oct 09, 2024 at 08:49:42AM -0700, Sean Christopherson wrote:
> > Return a uint64_t from vcpu_get_reg() instead of having the caller provide
> > a pointer to storage, as none of the vcpu_get_reg() usage in KVM selftests
> > accesses a register larger than 64 bits, and vcpu_set_reg() only accepts a
> > 64-bit value.  If a use case comes along that needs to get a register that
> > is larger than 64 bits, then a utility can be added to assert success and
> > take a void pointer, but until then, forcing an out param yields ugly code
> > and prevents feeding the output of vcpu_get_reg() into vcpu_set_reg().
> 
> This commit, which is in today's -next as 5c6c7b71a45c9c, breaks the
> build on arm64:
> 
> aarch64/psci_test.c: In function ‘host_test_system_off2’:
> aarch64/psci_test.c:247:9: error: too many arguments to function ‘vcpu_get_reg’
>   247 |         vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
>       |         ^~~~~~~~~~~~
> In file included from aarch64/psci_test.c:18:
> include/kvm_util.h:705:24: note: declared here
>   705 | static inline uint64_t vcpu_get_reg(struct kvm_vcpu *vcpu, uint64_t id)
>       |                        ^~~~~~~~~~~~
> At top level:
> cc1: note: unrecognized command-line option ‘-Wno-gnu-variable-sized-type-not-at
> -end’ may have been intended to silence earlier diagnostics
> 
> since the updates done to that file did not take account of 72be5aa6be4
> ("KVM: selftests: Add test for PSCI SYSTEM_OFF2") which has been merged
> in the kvm-arm64 tree.

Bugger.  In hindsight, it's obvious that of course arch selftests would add usage
of vcpu_get_reg().

Unless someone has a better idea, I'll drop the series from kvm-x86, post a new
version that applies on linux-next, and then re-apply the series just before the
v6.13 merge window (rinse and repeat as needed if more vcpu_get_reg() users come
along).

That would be a good oppurtunity to do the $(ARCH) directory switch[*] too, e.g.
have a "selftests_late" or whatever topic branch.

Sorry for the pain Mark, you've been playing janitor for us too much lately.

[*] https://lore.kernel.org/all/20240826190116.145945-1-seanjc@google.com

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Oliver Upton]

Hey,

On Fri, Nov 01, 2024 at 07:48:00AM -0700, Sean Christopherson wrote:
> On Fri, Nov 01, 2024, Mark Brown wrote:
> > On Wed, Oct 09, 2024 at 08:49:42AM -0700, Sean Christopherson wrote:
> > > Return a uint64_t from vcpu_get_reg() instead of having the caller provide
> > > a pointer to storage, as none of the vcpu_get_reg() usage in KVM selftests
> > > accesses a register larger than 64 bits, and vcpu_set_reg() only accepts a
> > > 64-bit value.  If a use case comes along that needs to get a register that
> > > is larger than 64 bits, then a utility can be added to assert success and
> > > take a void pointer, but until then, forcing an out param yields ugly code
> > > and prevents feeding the output of vcpu_get_reg() into vcpu_set_reg().
> > 
> > This commit, which is in today's -next as 5c6c7b71a45c9c, breaks the
> > build on arm64:
> > 
> > aarch64/psci_test.c: In function ‘host_test_system_off2’:
> > aarch64/psci_test.c:247:9: error: too many arguments to function ‘vcpu_get_reg’
> >   247 |         vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
> >       |         ^~~~~~~~~~~~
> > In file included from aarch64/psci_test.c:18:
> > include/kvm_util.h:705:24: note: declared here
> >   705 | static inline uint64_t vcpu_get_reg(struct kvm_vcpu *vcpu, uint64_t id)
> >       |                        ^~~~~~~~~~~~
> > At top level:
> > cc1: note: unrecognized command-line option ‘-Wno-gnu-variable-sized-type-not-at
> > -end’ may have been intended to silence earlier diagnostics
> > 
> > since the updates done to that file did not take account of 72be5aa6be4
> > ("KVM: selftests: Add test for PSCI SYSTEM_OFF2") which has been merged
> > in the kvm-arm64 tree.
> 
> Bugger.  In hindsight, it's obvious that of course arch selftests would add usage
> of vcpu_get_reg().
> 
> Unless someone has a better idea, I'll drop the series from kvm-x86, post a new
> version that applies on linux-next, and then re-apply the series just before the
> v6.13 merge window (rinse and repeat as needed if more vcpu_get_reg() users come
> along).

Can you instead just push out a topic branch and let the affected
maintainers deal with it? This is the usual way we handle conflicts
between trees...

> That would be a good oppurtunity to do the $(ARCH) directory switch[*] too, e.g.
> have a "selftests_late" or whatever topic branch.

The right time to do KVM-wide changes (even selftests) is *early* in the
development cycle, not last minute. It gives us plenty of time to iron out
the wrinkles.

> Sorry for the pain Mark, you've been playing janitor for us too much lately.

+1, appreciate your help on this.

-- 
Thanks,
Oliver

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Sean Christopherson]

On Fri, Nov 01, 2024, Oliver Upton wrote:
> Hey,
> 
> On Fri, Nov 01, 2024 at 07:48:00AM -0700, Sean Christopherson wrote:
> > On Fri, Nov 01, 2024, Mark Brown wrote:
> > > On Wed, Oct 09, 2024 at 08:49:42AM -0700, Sean Christopherson wrote:
> > > > Return a uint64_t from vcpu_get_reg() instead of having the caller provide
> > > > a pointer to storage, as none of the vcpu_get_reg() usage in KVM selftests
> > > > accesses a register larger than 64 bits, and vcpu_set_reg() only accepts a
> > > > 64-bit value.  If a use case comes along that needs to get a register that
> > > > is larger than 64 bits, then a utility can be added to assert success and
> > > > take a void pointer, but until then, forcing an out param yields ugly code
> > > > and prevents feeding the output of vcpu_get_reg() into vcpu_set_reg().
> > > 
> > > This commit, which is in today's -next as 5c6c7b71a45c9c, breaks the
> > > build on arm64:
> > > 
> > > aarch64/psci_test.c: In function ‘host_test_system_off2’:
> > > aarch64/psci_test.c:247:9: error: too many arguments to function ‘vcpu_get_reg’
> > >   247 |         vcpu_get_reg(target, KVM_REG_ARM_PSCI_VERSION, &psci_version);
> > >       |         ^~~~~~~~~~~~
> > > In file included from aarch64/psci_test.c:18:
> > > include/kvm_util.h:705:24: note: declared here
> > >   705 | static inline uint64_t vcpu_get_reg(struct kvm_vcpu *vcpu, uint64_t id)
> > >       |                        ^~~~~~~~~~~~
> > > At top level:
> > > cc1: note: unrecognized command-line option ‘-Wno-gnu-variable-sized-type-not-at
> > > -end’ may have been intended to silence earlier diagnostics
> > > 
> > > since the updates done to that file did not take account of 72be5aa6be4
> > > ("KVM: selftests: Add test for PSCI SYSTEM_OFF2") which has been merged
> > > in the kvm-arm64 tree.
> > 
> > Bugger.  In hindsight, it's obvious that of course arch selftests would add usage
> > of vcpu_get_reg().
> > 
> > Unless someone has a better idea, I'll drop the series from kvm-x86, post a new
> > version that applies on linux-next, and then re-apply the series just before the
> > v6.13 merge window (rinse and repeat as needed if more vcpu_get_reg() users come
> > along).
> 
> Can you instead just push out a topic branch and let the affected
> maintainers deal with it? This is the usual way we handle conflicts
> between trees...

That'd work too, but as you note below, doing that now throws a wrench in things
because essentially all arch maintainers would need merge that topic branch,
otherwise linux-next would end up in the same state.

> > That would be a good oppurtunity to do the $(ARCH) directory switch[*] too, e.g.
> > have a "selftests_late" or whatever topic branch.
> 
> The right time to do KVM-wide changes (even selftests) is *early* in the
> development cycle, not last minute. It gives us plenty of time to iron out
> the wrinkles.

Yeah, that was the original plan, then the stupid strict aliasing bug happened,
and I honestly forgot the vcpu_get_reg() changes would need to be consumed by
other architectures.

Other than letting me forget about this mess a few weeks earlier, there's no
good reason to force this into 6.13.  So, I'll drop the series from 6.13, post
new versions of the this and the $(ARCH) series just before the merge window,
and then either send a pull request to Paolo for 6.14 as soon as the 6.13 merge
window closes, or ask/bribe Paolo to apply everything directly.

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Oliver Upton]

On Fri, Nov 01, 2024 at 08:59:16AM -0700, Sean Christopherson wrote:
> > Can you instead just push out a topic branch and let the affected
> > maintainers deal with it? This is the usual way we handle conflicts
> > between trees...
> 
> That'd work too, but as you note below, doing that now throws a wrench in things
> because essentially all arch maintainers would need merge that topic branch,
> otherwise linux-next would end up in the same state.

TBH, I'm quite happy with that. Recent history has not been particularly
convinincing to me that folks are actually testing arm64, let alone
compiling for it when applying selftests patches.

Otherwise, the alternative of respinning the global change for every -next
breakage adds a decent amount of toil and gives the wrong impression of how
long our patches have actually been baking in -next.

> > > That would be a good oppurtunity to do the $(ARCH) directory switch[*] too, e.g.
> > > have a "selftests_late" or whatever topic branch.
> > 
> > The right time to do KVM-wide changes (even selftests) is *early* in the
> > development cycle, not last minute. It gives us plenty of time to iron out
> > the wrinkles.
> 
> Yeah, that was the original plan, then the stupid strict aliasing bug happened,
> and I honestly forgot the vcpu_get_reg() changes would need to be consumed by
> other architectures.
> 
> Other than letting me forget about this mess a few weeks earlier, there's no
> good reason to force this into 6.13.  So, I'll drop the series from 6.13, post
> new versions of the this and the $(ARCH) series just before the merge window,
> and then either send a pull request to Paolo for 6.14 as soon as the 6.13 merge
> window closes, or ask/bribe Paolo to apply everything directly.

Sounds good, punting to 6.14 seems reasonable.

-- 
Thanks,
Oliver

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Sean Christopherson]

On Fri, Nov 01, 2024, Oliver Upton wrote:
> On Fri, Nov 01, 2024 at 08:59:16AM -0700, Sean Christopherson wrote:
> > > Can you instead just push out a topic branch and let the affected
> > > maintainers deal with it? This is the usual way we handle conflicts
> > > between trees...
> > 
> > That'd work too, but as you note below, doing that now throws a wrench in things
> > because essentially all arch maintainers would need merge that topic branch,
> > otherwise linux-next would end up in the same state.
> 
> TBH, I'm quite happy with that. Recent history has not been particularly
> convinincing to me that folks are actually testing arm64, let alone
> compiling for it when applying selftests patches.

FWIW, I did compile all patches on all KVM architectures, including selftests.
But my base obviously didn't include the kvm-arm64 branch :-/

One thing I'll add to my workflow would be to do a local merge (and smoke test)
of linux-next into kvm-x86 next before pushing it out.  This isn't the only snafu
this cycle where such a sanity check would have saved me and others a bit of pain.

https://lore.kernel.org/all/20241101153857.GAZyT2EdLXKs7ZmDFx@fat_crate.local

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Oliver Upton]

On Fri, Nov 01, 2024 at 09:16:42AM -0700, Sean Christopherson wrote:
> On Fri, Nov 01, 2024, Oliver Upton wrote:
> > On Fri, Nov 01, 2024 at 08:59:16AM -0700, Sean Christopherson wrote:
> > > > Can you instead just push out a topic branch and let the affected
> > > > maintainers deal with it? This is the usual way we handle conflicts
> > > > between trees...
> > > 
> > > That'd work too, but as you note below, doing that now throws a wrench in things
> > > because essentially all arch maintainers would need merge that topic branch,
> > > otherwise linux-next would end up in the same state.
> > 
> > TBH, I'm quite happy with that. Recent history has not been particularly
> > convinincing to me that folks are actually testing arm64, let alone
> > compiling for it when applying selftests patches.
> 
> FWIW, I did compile all patches on all KVM architectures, including selftests.
> But my base obviously didn't include the kvm-arm64 branch :-/

Oh, that rip wasn't aimed at you, commit 76f972c2cfdf ("KVM: selftests: Fix build
on architectures other than x86_64") just came to mind.

> One thing I'll add to my workflow would be to do a local merge (and smoke test)
> of linux-next into kvm-x86 next before pushing it out.  This isn't the only snafu
> this cycle where such a sanity check would have saved me and others a bit of pain.

Eh, shit happens, that's what -next is for :)

The only point I wanted to make was that it is perfectly fine by me to
spread the workload w/ a topic branch if things blow up sometime after
your changes show up in -next.

-- 
Thanks,
Oliver

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Sean Christopherson]

On Fri, Nov 01, 2024, Oliver Upton wrote:
> On Fri, Nov 01, 2024 at 09:16:42AM -0700, Sean Christopherson wrote:
> > One thing I'll add to my workflow would be to do a local merge (and smoke test)
> > of linux-next into kvm-x86 next before pushing it out.  This isn't the only snafu
> > this cycle where such a sanity check would have saved me and others a bit of pain.
> 
> Eh, shit happens, that's what -next is for :)

Heh, but I also don't actually test -next, which was another snafu (not my fault
this time!) from this cycle[*].  Testing 6.12-next prior to the merge window
wouldn't have made that any less painful to bisect, but I think it would at least
have allowed me to detect that the issue specifically came in from linux-next,
and the bug report would have gotten to PeterZ almost two months earlier.

https://lore.kernel.org/all/ZwdA0sbA2tJA3IKh@google.com

Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param

[Mark Brown]

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

On Fri, Nov 01, 2024 at 09:22:30AM -0700, Oliver Upton wrote:
> On Fri, Nov 01, 2024 at 09:16:42AM -0700, Sean Christopherson wrote:

> > One thing I'll add to my workflow would be to do a local merge (and smoke test)
> > of linux-next into kvm-x86 next before pushing it out.  This isn't the only snafu
> > this cycle where such a sanity check would have saved me and others a bit of pain.

> Eh, shit happens, that's what -next is for :)

> The only point I wanted to make was that it is perfectly fine by me to
> spread the workload w/ a topic branch if things blow up sometime after
> your changes show up in -next.

Yeah, the -next breakage is a bit annoying but so long as it gets fixed
promptly it's kind of what it's there for.  It's much more of an issue
when things make it into mainline, and can be very problematic if it
makes it into a tagged -rc (especially -rc1) or something.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]