KVM ARM Archive on lore.kernel.org
 help / color / Atom feed
From: Alexandru Elisei <alexandru.elisei@arm.com>
To: Andrew Jones <drjones@redhat.com>
Cc: kvm@vger.kernel.org, maz@kernel.org, andre.przywara@arm.com,
	pbonzini@redhat.com, kvmarm@lists.cs.columbia.edu
Subject: Re: [kvm-unit-tests RFC PATCH 02/16] arm/arm64: psci: Don't run C code without stack or vectors
Date: Wed, 28 Aug 2019 16:14:57 +0100
Message-ID: <a2da5efd-b466-3fc2-f8a3-eb9a852f2fdc@arm.com> (raw)
In-Reply-To: <20190828144522.qkmckjcmrdayfq7r@kamzik.brq.redhat.com>

On 8/28/19 3:45 PM, Andrew Jones wrote:
> On Wed, Aug 28, 2019 at 02:38:17PM +0100, Alexandru Elisei wrote:
>> The psci test performs a series of CPU_ON/CPU_OFF cycles for CPU 1. This is
>> done by setting the entry point for the CPU_ON call to the physical address
>> of the C function cpu_psci_cpu_die.
>>
>> The compiler is well within its rights to use the stack when generating
>> code for cpu_psci_cpu_die.  However, because no stack initialization has
>> been done, the stack pointer is zero, as set by KVM when creating the VCPU.
>> This causes a data abort without a change in exception level. The VBAR_EL1
>> register is also zero (the KVM reset value for VBAR_EL1), the MMU is off,
>> and we end up trying to fetch instructions from address 0x200.
>>
>> At this point, a stage 2 instruction abort is generated which is taken to
>> KVM. KVM interprets this as an instruction fetch from an I/O region, and
>> injects a prefetch abort into the guest. Prefetch abort is a synchronous
>> exception, and on guest return the VCPU PC will be set to VBAR_EL1 + 0x200,
>> which is...  0x200. The VCPU ends up in an infinite loop causing a prefetch
>> abort while fetching the instruction to service the said abort.
>>
>> cpu_psci_cpu_die is basically a wrapper over the HVC instruction, so
>> provide an assembly implementation for the function which will serve as the
>> entry point for CPU_ON.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arm/cstart.S   | 7 +++++++
>>  arm/cstart64.S | 7 +++++++
>>  arm/psci.c     | 5 +++--
>>  3 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/arm/cstart.S b/arm/cstart.S
>> index 114726feab82..5d4fe4b1570b 100644
>> --- a/arm/cstart.S
>> +++ b/arm/cstart.S
>> @@ -7,6 +7,7 @@
>>   */
>>  #define __ASSEMBLY__
>>  #include <auxinfo.h>
>> +#include <linux/psci.h>
>>  #include <asm/thread_info.h>
>>  #include <asm/asm-offsets.h>
>>  #include <asm/ptrace.h>
>> @@ -138,6 +139,12 @@ secondary_entry:
>>  	blx	r0
>>  	b	do_idle
>>  
>> +.global asm_cpu_psci_cpu_die
>> +asm_cpu_psci_cpu_die:
>> +	ldr	r0, =PSCI_0_2_FN_CPU_OFF
>> +	hvc	#0
>> +	b	halt
> Shouldn't we load PSCI_POWER_STATE_TYPE_POWER_DOWN into r1 and
> zero out r2 and r3, as cpu_psci_cpu_die() does? And maybe we
> should just do a 'b .' here instead of 'b halt' in order to
> avoid confusion as to how we ended up in halt(), if the psci
> invocation were to ever fail.

Not really, I'm not really sure where the extra parameter in cpu_psci_cpu_die
comes from. I have looked at ARM DEN 0022D, section 5.1.3, and the CPU_OFFcall
has exactly one parameter, the function id. I've also looked at how KVM handles
this call, and it doesn't use anything else other than the function id. Please
correct me if I missed something.

As for zero'ing the extra registers, this is not part of the SMC calling
convention, this is just something that the C code for psci does. The SMC
calling convention states that registers 0-3 will be modified after the call, so
having 4 arguments to the psci_invoke function will tell the compiler to
save/restore the registers in the caller.

As for doing 'b .' instead of branching to halt, that's a good idea, I'll do
that. But it will only be useful if the last CPU_OFF call fails.

Thanks,
Alex
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-28 13:38 [kvm-unit-tests RFC PATCH 00/16] arm64: Run at EL2 Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 01/16] arm: selftest.c: Remove redundant check for Exception Level Alexandru Elisei
2019-08-28 14:32   ` Andrew Jones
2019-08-28 15:39     ` Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 02/16] arm/arm64: psci: Don't run C code without stack or vectors Alexandru Elisei
2019-08-28 14:45   ` Andrew Jones
2019-08-28 15:14     ` Alexandru Elisei [this message]
2019-09-02 14:55       ` Alexandru Elisei
2019-09-03  6:37         ` Andrew Jones
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 03/16] lib: arm/arm64: Add missing include for alloc_page.h in pgtable.h Alexandru Elisei
2019-08-28 14:47   ` Andrew Jones
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 04/16] arm/arm64: selftest: Add prefetch abort test Alexandru Elisei
2019-08-28 14:09   ` Mark Rutland
2019-08-29  8:18     ` Alexandru Elisei
2019-08-29 10:19       ` Mark Rutland
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 05/16] arm64: timer: Write to ICENABLER to disable timer IRQ Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 06/16] arm64: timer: EOIR the interrupt after masking the timer Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 07/16] arm64: timer: Test behavior when timer disabled or masked Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 08/16] lib: arm/arm64: Refuse to disable the MMU with non-identity stack pointer Alexandru Elisei
2019-08-28 14:55   ` Andrew Jones
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 09/16] lib: arm/arm64: Invalidate TLB before enabling MMU Alexandru Elisei
2019-08-28 14:59   ` Andrew Jones
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 10/16] lib: Add UL and ULL definitions to linux/const.h Alexandru Elisei
2019-08-28 15:10   ` Andrew Jones
2019-08-28 15:46     ` Alexandru Elisei
2019-08-28 16:19       ` Andrew Jones
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 11/16] lib: arm64: Run existing tests at EL2 Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 12/16] arm64: timer: Add test for EL2 timers Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 13/16] arm64: selftest: Add basic test for EL2 Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 14/16] lib: arm64: Add support for disabling and re-enabling VHE Alexandru Elisei
2019-08-28 14:19   ` Mark Rutland
2019-08-29  8:36     ` Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 15/16] arm64: selftest: Expand EL2 test to disable and re-enable VHE Alexandru Elisei
2019-08-28 13:38 ` [kvm-unit-tests RFC PATCH 16/16] arm64: timer: Run tests with VHE disabled Alexandru Elisei

Reply instructions:

You may reply publically 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=a2da5efd-b466-3fc2-f8a3-eb9a852f2fdc@arm.com \
    --to=alexandru.elisei@arm.com \
    --cc=andre.przywara@arm.com \
    --cc=drjones@redhat.com \
    --cc=kvm@vger.kernel.org \
    --cc=kvmarm@lists.cs.columbia.edu \
    --cc=maz@kernel.org \
    --cc=pbonzini@redhat.com \
    /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

KVM ARM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvmarm/0 kvmarm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvmarm kvmarm/ https://lore.kernel.org/kvmarm \
		kvmarm@lists.cs.columbia.edu kvmarm@archiver.kernel.org
	public-inbox-index kvmarm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/edu.columbia.cs.lists.kvmarm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox