All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: kvm-riscv@lists.infradead.org
Subject: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param
Date: Fri, 1 Nov 2024 07:48:00 -0700	[thread overview]
Message-ID: <ZyTpwwm0s89iU9Pk@google.com> (raw)
In-Reply-To: <39ea24d8-9dae-447a-ae37-e65878c3806f@sirena.org.uk>

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 at google.com


WARNING: multiple messages have this Message-ID (diff)
From: Sean Christopherson <seanjc@google.com>
To: Mark Brown <broonie@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	 Anup Patel <anup@brainfault.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	 Paolo Bonzini <pbonzini@redhat.com>,
	Christian Borntraeger <borntraeger@linux.ibm.com>,
	 Janosch Frank <frankja@linux.ibm.com>,
	Claudio Imbrenda <imbrenda@linux.ibm.com>,
	 linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	 kvm@vger.kernel.org, kvm-riscv@lists.infradead.org,
	 linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	 Andrew Jones <ajones@ventanamicro.com>,
	James Houghton <jthoughton@google.com>,
	 David Woodhouse <dwmw@amazon.co.uk>,
	linux-next@vger.kernel.org
Subject: Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param
Date: Fri, 1 Nov 2024 07:48:00 -0700	[thread overview]
Message-ID: <ZyTpwwm0s89iU9Pk@google.com> (raw)
In-Reply-To: <39ea24d8-9dae-447a-ae37-e65878c3806f@sirena.org.uk>

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

WARNING: multiple messages have this Message-ID (diff)
From: Sean Christopherson <seanjc@google.com>
To: Mark Brown <broonie@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	 Anup Patel <anup@brainfault.org>,
	Paul Walmsley <paul.walmsley@sifive.com>,
	 Palmer Dabbelt <palmer@dabbelt.com>,
	Albert Ou <aou@eecs.berkeley.edu>,
	 Paolo Bonzini <pbonzini@redhat.com>,
	Christian Borntraeger <borntraeger@linux.ibm.com>,
	 Janosch Frank <frankja@linux.ibm.com>,
	Claudio Imbrenda <imbrenda@linux.ibm.com>,
	 linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	 kvm@vger.kernel.org, kvm-riscv@lists.infradead.org,
	 linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	 Andrew Jones <ajones@ventanamicro.com>,
	James Houghton <jthoughton@google.com>,
	 David Woodhouse <dwmw@amazon.co.uk>,
	linux-next@vger.kernel.org
Subject: Re: [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param
Date: Fri, 1 Nov 2024 07:48:00 -0700	[thread overview]
Message-ID: <ZyTpwwm0s89iU9Pk@google.com> (raw)
In-Reply-To: <39ea24d8-9dae-447a-ae37-e65878c3806f@sirena.org.uk>

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

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2024-11-01 14:48 UTC|newest]

Thread overview: 84+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-10-09 15:49 [PATCH v3 00/14] KVM: selftests: Morph max_guest_mem to mmu_stress Sean Christopherson
2024-10-09 15:49 ` Sean Christopherson
2024-10-09 15:49 ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 01/14] KVM: Move KVM_REG_SIZE() definition to common uAPI header Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-10 11:41   ` Anup Patel
2024-10-10 11:41     ` Anup Patel
2024-10-10 11:41     ` Anup Patel
2024-10-09 15:49 ` [PATCH v3 02/14] KVM: selftests: Disable strict aliasing Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 03/14] KVM: selftests: Return a value from vcpu_get_reg() instead of using an out-param Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-11-01 13:07   ` Mark Brown
2024-11-01 13:07     ` Mark Brown
2024-11-01 13:07     ` Mark Brown
2024-11-01 14:48     ` Sean Christopherson [this message]
2024-11-01 14:48       ` Sean Christopherson
2024-11-01 14:48       ` Sean Christopherson
2024-11-01 15:38       ` Oliver Upton
2024-11-01 15:38         ` Oliver Upton
2024-11-01 15:38         ` Oliver Upton
2024-11-01 15:59         ` Sean Christopherson
2024-11-01 15:59           ` Sean Christopherson
2024-11-01 15:59           ` Sean Christopherson
2024-11-01 16:11           ` Oliver Upton
2024-11-01 16:11             ` Oliver Upton
2024-11-01 16:11             ` Oliver Upton
2024-11-01 16:16             ` Sean Christopherson
2024-11-01 16:16               ` Sean Christopherson
2024-11-01 16:16               ` Sean Christopherson
2024-11-01 16:22               ` Oliver Upton
2024-11-01 16:22                 ` Oliver Upton
2024-11-01 16:22                 ` Oliver Upton
2024-11-01 16:31                 ` Sean Christopherson
2024-11-01 16:31                   ` Sean Christopherson
2024-11-01 16:31                   ` Sean Christopherson
2024-11-01 16:37                 ` Mark Brown
2024-11-01 16:37                   ` Mark Brown
2024-11-01 16:37                   ` Mark Brown
2024-10-09 15:49 ` [PATCH v3 04/14] KVM: selftests: Assert that vcpu_{g,s}et_reg() won't truncate Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 05/14] KVM: selftests: Check for a potential unhandled exception iff KVM_RUN succeeded Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 06/14] KVM: selftests: Rename max_guest_memory_test to mmu_stress_test Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 07/14] KVM: selftests: Only muck with SREGS on x86 in mmu_stress_test Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 08/14] KVM: selftests: Compute number of extra pages needed " Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 09/14] KVM: sefltests: Explicitly include ucall_common.h in mmu_stress_test.c Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
     [not found]   ` <CADrL8HUEwnP8e700y2XYDgVhhUJuj1UEJmd2NLdtZ1dV845DNw@mail.gmail.com>
2024-10-28 20:45     ` Sean Christopherson
2024-10-28 20:45       ` Sean Christopherson
2024-10-28 20:45       ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 10/14] KVM: selftests: Enable mmu_stress_test on arm64 Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 11/14] KVM: selftests: Use vcpu_arch_put_guest() in mmu_stress_test Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 12/14] KVM: selftests: Precisely limit the number of guest loops " Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 13/14] KVM: selftests: Add a read-only mprotect() phase to mmu_stress_test Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49 ` [PATCH v3 14/14] KVM: selftests: Verify KVM correctly handles mprotect(PROT_READ) Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-09 15:49   ` Sean Christopherson
2024-10-31 19:51 ` [PATCH v3 00/14] KVM: selftests: Morph max_guest_mem to mmu_stress Sean Christopherson
2024-10-31 19:51   ` Sean Christopherson
2024-10-31 19:51   ` Sean Christopherson
2024-11-05  5:53   ` Sean Christopherson
2024-11-05  5:53     ` Sean Christopherson
2024-11-05  5:53     ` Sean Christopherson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZyTpwwm0s89iU9Pk@google.com \
    --to=seanjc@google.com \
    --cc=kvm-riscv@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.