[PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Marc Zyngier]

Here's a handful of random fixes I've queued locally that didn't have
a chance to make it in 4.11.

The first two patches avoid stack-protector messing with the HYP code,
as this ends up being a complete disaster.

The following two patches fix a bug introduced in the new vgic, where
we may queue HW interrupts with the Pending+Active state, which is
illegal.

The final patch fixes a misinterpretation of the spec, where we
compute the number of APxRn register based on the number of priorities
instead of using the number of preemption levels.

I've tagged the first 4 patches for stable, given that we're doing
something potentially harmful. The last patch is more of a theoretical
issue at this stage, so probably need for a backport.

Marc Zyngier (5):
  arm64: KVM: Do not use stack-protector to compile EL2 code
  arm: KVM: Do not use stack-protector to compile HYP code
  KVM: arm/arm64: vgic-v2: Do not use Active+Pending state for a HW
    interrupt
  KVM: arm/arm64: vgic-v3: Do not use Active+Pending state for a HW
    interrupt
  KVM: arm/arm64: vgic-v3: Use PREbits to infer the number of
    ICH_APxRn_EL2 registers

 arch/arm/kvm/hyp/Makefile     |  2 ++
 arch/arm64/kvm/hyp/Makefile   |  2 ++
 virt/kvm/arm/hyp/vgic-v3-sr.c | 18 +++++++++---------
 virt/kvm/arm/vgic/vgic-v2.c   |  7 +++++++
 virt/kvm/arm/vgic/vgic-v3.c   |  7 +++++++
 5 files changed, 27 insertions(+), 9 deletions(-)

-- 
2.11.0

[PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Marc Zyngier]

Here's a handful of random fixes I've queued locally that didn't have
a chance to make it in 4.11.

The first two patches avoid stack-protector messing with the HYP code,
as this ends up being a complete disaster.

The following two patches fix a bug introduced in the new vgic, where
we may queue HW interrupts with the Pending+Active state, which is
illegal.

The final patch fixes a misinterpretation of the spec, where we
compute the number of APxRn register based on the number of priorities
instead of using the number of preemption levels.

I've tagged the first 4 patches for stable, given that we're doing
something potentially harmful. The last patch is more of a theoretical
issue at this stage, so probably need for a backport.

Marc Zyngier (5):
  arm64: KVM: Do not use stack-protector to compile EL2 code
  arm: KVM: Do not use stack-protector to compile HYP code
  KVM: arm/arm64: vgic-v2: Do not use Active+Pending state for a HW
    interrupt
  KVM: arm/arm64: vgic-v3: Do not use Active+Pending state for a HW
    interrupt
  KVM: arm/arm64: vgic-v3: Use PREbits to infer the number of
    ICH_APxRn_EL2 registers

 arch/arm/kvm/hyp/Makefile     |  2 ++
 arch/arm64/kvm/hyp/Makefile   |  2 ++
 virt/kvm/arm/hyp/vgic-v3-sr.c | 18 +++++++++---------
 virt/kvm/arm/vgic/vgic-v2.c   |  7 +++++++
 virt/kvm/arm/vgic/vgic-v3.c   |  7 +++++++
 5 files changed, 27 insertions(+), 9 deletions(-)

-- 
2.11.0

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

We like living dangerously. Nothing explicitely forbids stack-protector
to be used in the EL2 code, while distributions routinely compile their
kernel with it. We're just lucky that no code actually triggers the
instrumentation.

Let's not try our luck for much longer, and disable stack-protector
for code living at EL2.

Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index aaf42ae8d8c3..14c4e3b14bcb 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -2,6 +2,8 @@
 # Makefile for Kernel-based Virtual Machine module, HYP part
 #
 
+ccflags-y += -fno-stack-protector
+
 KVM=../../../../virt/kvm
 
 obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
-- 
2.11.0

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

We like living dangerously. Nothing explicitely forbids stack-protector
to be used in the EL2 code, while distributions routinely compile their
kernel with it. We're just lucky that no code actually triggers the
instrumentation.

Let's not try our luck for much longer, and disable stack-protector
for code living at EL2.

Cc: stable at vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/kvm/hyp/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index aaf42ae8d8c3..14c4e3b14bcb 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -2,6 +2,8 @@
 # Makefile for Kernel-based Virtual Machine module, HYP part
 #
 
+ccflags-y += -fno-stack-protector
+
 KVM=../../../../virt/kvm
 
 obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
-- 
2.11.0

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Catalin Marinas]

On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
> We like living dangerously. Nothing explicitely forbids stack-protector
> to be used in the EL2 code, while distributions routinely compile their
> kernel with it. We're just lucky that no code actually triggers the
> instrumentation.
> 
> Let's not try our luck for much longer, and disable stack-protector
> for code living at EL2.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/hyp/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index aaf42ae8d8c3..14c4e3b14bcb 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -2,6 +2,8 @@
>  # Makefile for Kernel-based Virtual Machine module, HYP part
>  #
>  
> +ccflags-y += -fno-stack-protector
> +

While you are at it, should we have a -fpic here as well? The hyp code
runs at a different location than the rest of the kernel.

-- 
Catalin

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Catalin Marinas]

On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
> We like living dangerously. Nothing explicitely forbids stack-protector
> to be used in the EL2 code, while distributions routinely compile their
> kernel with it. We're just lucky that no code actually triggers the
> instrumentation.
> 
> Let's not try our luck for much longer, and disable stack-protector
> for code living at EL2.
> 
> Cc: stable at vger.kernel.org
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  arch/arm64/kvm/hyp/Makefile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
> index aaf42ae8d8c3..14c4e3b14bcb 100644
> --- a/arch/arm64/kvm/hyp/Makefile
> +++ b/arch/arm64/kvm/hyp/Makefile
> @@ -2,6 +2,8 @@
>  # Makefile for Kernel-based Virtual Machine module, HYP part
>  #
>  
> +ccflags-y += -fno-stack-protector
> +

While you are at it, should we have a -fpic here as well? The hyp code
runs at a different location than the rest of the kernel.

-- 
Catalin

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 2 May 2017 at 15:40, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>> We like living dangerously. Nothing explicitely forbids stack-protector
>> to be used in the EL2 code, while distributions routinely compile their
>> kernel with it. We're just lucky that no code actually triggers the
>> instrumentation.
>>
>> Let's not try our luck for much longer, and disable stack-protector
>> for code living at EL2.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>> --- a/arch/arm64/kvm/hyp/Makefile
>> +++ b/arch/arm64/kvm/hyp/Makefile
>> @@ -2,6 +2,8 @@
>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>  #
>>
>> +ccflags-y += -fno-stack-protector
>> +
>
> While you are at it, should we have a -fpic here as well? The hyp code
> runs at a different location than the rest of the kernel.
>

-fpic almost guarantees you will get position dependent but runtime
relocatable code (i.e., symbol references indirected via GOT entries
which need to be fixed up at runtime etc), unless you play around with
hidden visibility etc. For the same reason, the EFI stub does not
support being built with -fpic either.

Adding -mcmodel=small explicitly is much more likely to do anything
meaningful here, but only in case we need to set it to 'large'
globally in the future.

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 2 May 2017 at 15:40, Catalin Marinas <catalin.marinas@arm.com> wrote:
> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>> We like living dangerously. Nothing explicitely forbids stack-protector
>> to be used in the EL2 code, while distributions routinely compile their
>> kernel with it. We're just lucky that no code actually triggers the
>> instrumentation.
>>
>> Let's not try our luck for much longer, and disable stack-protector
>> for code living at EL2.
>>
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>> --- a/arch/arm64/kvm/hyp/Makefile
>> +++ b/arch/arm64/kvm/hyp/Makefile
>> @@ -2,6 +2,8 @@
>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>  #
>>
>> +ccflags-y += -fno-stack-protector
>> +
>
> While you are at it, should we have a -fpic here as well? The hyp code
> runs at a different location than the rest of the kernel.
>

-fpic almost guarantees you will get position dependent but runtime
relocatable code (i.e., symbol references indirected via GOT entries
which need to be fixed up at runtime etc), unless you play around with
hidden visibility etc. For the same reason, the EFI stub does not
support being built with -fpic either.

Adding -mcmodel=small explicitly is much more likely to do anything
meaningful here, but only in case we need to set it to 'large'
globally in the future.

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

On 02/05/17 15:40, Catalin Marinas wrote:
> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>> We like living dangerously. Nothing explicitely forbids stack-protector
>> to be used in the EL2 code, while distributions routinely compile their
>> kernel with it. We're just lucky that no code actually triggers the
>> instrumentation.
>>
>> Let's not try our luck for much longer, and disable stack-protector
>> for code living at EL2.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>> --- a/arch/arm64/kvm/hyp/Makefile
>> +++ b/arch/arm64/kvm/hyp/Makefile
>> @@ -2,6 +2,8 @@
>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>  #
>>  
>> +ccflags-y += -fno-stack-protector
>> +
> 
> While you are at it, should we have a -fpic here as well? The hyp code
> runs at a different location than the rest of the kernel.

We definitely should. I've just tried this, and this doesn't seem to
work very well. At least this seems to break our jump label
implementation. I need to page in that part of the code base and see
what happens.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

On 02/05/17 15:40, Catalin Marinas wrote:
> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>> We like living dangerously. Nothing explicitely forbids stack-protector
>> to be used in the EL2 code, while distributions routinely compile their
>> kernel with it. We're just lucky that no code actually triggers the
>> instrumentation.
>>
>> Let's not try our luck for much longer, and disable stack-protector
>> for code living at EL2.
>>
>> Cc: stable at vger.kernel.org
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>> --- a/arch/arm64/kvm/hyp/Makefile
>> +++ b/arch/arm64/kvm/hyp/Makefile
>> @@ -2,6 +2,8 @@
>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>  #
>>  
>> +ccflags-y += -fno-stack-protector
>> +
> 
> While you are at it, should we have a -fpic here as well? The hyp code
> runs at a different location than the rest of the kernel.

We definitely should. I've just tried this, and this doesn't seem to
work very well. At least this seems to break our jump label
implementation. I need to page in that part of the code base and see
what happens.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

On 02/05/17 15:50, Marc Zyngier wrote:
> On 02/05/17 15:40, Catalin Marinas wrote:
>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>> to be used in the EL2 code, while distributions routinely compile their
>>> kernel with it. We're just lucky that no code actually triggers the
>>> instrumentation.
>>>
>>> Let's not try our luck for much longer, and disable stack-protector
>>> for code living at EL2.
>>>
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>> --- a/arch/arm64/kvm/hyp/Makefile
>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>> @@ -2,6 +2,8 @@
>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>  #
>>>  
>>> +ccflags-y += -fno-stack-protector
>>> +
>>
>> While you are at it, should we have a -fpic here as well? The hyp code
>> runs at a different location than the rest of the kernel.
> 
> We definitely should. I've just tried this, and this doesn't seem to
> work very well. At least this seems to break our jump label
> implementation. I need to page in that part of the code base and see
> what happens.

So here's the issue:

  CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
In file included from ./include/linux/jump_label.h:120:0,
                 from ./include/linux/dynamic_debug.h:5,
                 from ./include/linux/printk.h:329,
                 from ./include/linux/kernel.h:13,
                 from ./include/asm-generic/bug.h:15,
                 from ./arch/arm64/include/asm/bug.h:66,
                 from ./include/linux/bug.h:4,
                 from ./include/linux/mmdebug.h:4,
                 from ./include/linux/mm.h:8,
                 from ./arch/arm64/include/asm/cacheflush.h:22,
                 from ./arch/arm64/include/asm/arch_gicv3.h:27,
                 from ./include/linux/irqchip/arm-gic-v3.h:453,
                 from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
  asm goto("1: nop\n\t"
  ^~~
./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
  asm goto("1: nop\n\t"
  ^~~
scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed

The corresponding code does this:

static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
{
	asm goto("1: nop\n\t"
		 ".pushsection __jump_table,  \"aw\"\n\t"
		 ".align 3\n\t"
		 ".quad 1b, %l[l_yes], %c0\n\t"
		 ".popsection\n\t"
		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);

	return false;
l_yes:
	return true;
}

and the problem lies in the evaluation of "key", which probably
cannot be guaranteed a constant at that point. There is also the
issue that even if it was known, the branch cannot be easily 
patched in from the rest of the kernel (how is the l_yes address
represented?).

It looks to me like we either need to rewrite the whole of our
static key infrastructure to cope with PIC, or switch over to
the hyp_alternate_select() hack, which I'd rather avoid spreading
further.

In the end, I wonder if that's even worth it...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

On 02/05/17 15:50, Marc Zyngier wrote:
> On 02/05/17 15:40, Catalin Marinas wrote:
>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>> to be used in the EL2 code, while distributions routinely compile their
>>> kernel with it. We're just lucky that no code actually triggers the
>>> instrumentation.
>>>
>>> Let's not try our luck for much longer, and disable stack-protector
>>> for code living at EL2.
>>>
>>> Cc: stable at vger.kernel.org
>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>> ---
>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>> --- a/arch/arm64/kvm/hyp/Makefile
>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>> @@ -2,6 +2,8 @@
>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>  #
>>>  
>>> +ccflags-y += -fno-stack-protector
>>> +
>>
>> While you are at it, should we have a -fpic here as well? The hyp code
>> runs at a different location than the rest of the kernel.
> 
> We definitely should. I've just tried this, and this doesn't seem to
> work very well. At least this seems to break our jump label
> implementation. I need to page in that part of the code base and see
> what happens.

So here's the issue:

  CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
In file included from ./include/linux/jump_label.h:120:0,
                 from ./include/linux/dynamic_debug.h:5,
                 from ./include/linux/printk.h:329,
                 from ./include/linux/kernel.h:13,
                 from ./include/asm-generic/bug.h:15,
                 from ./arch/arm64/include/asm/bug.h:66,
                 from ./include/linux/bug.h:4,
                 from ./include/linux/mmdebug.h:4,
                 from ./include/linux/mm.h:8,
                 from ./arch/arm64/include/asm/cacheflush.h:22,
                 from ./arch/arm64/include/asm/arch_gicv3.h:27,
                 from ./include/linux/irqchip/arm-gic-v3.h:453,
                 from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
  asm goto("1: nop\n\t"
  ^~~
./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
  asm goto("1: nop\n\t"
  ^~~
scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed

The corresponding code does this:

static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
{
	asm goto("1: nop\n\t"
		 ".pushsection __jump_table,  \"aw\"\n\t"
		 ".align 3\n\t"
		 ".quad 1b, %l[l_yes], %c0\n\t"
		 ".popsection\n\t"
		 :  :  "i"(&((char *)key)[branch]) :  : l_yes);

	return false;
l_yes:
	return true;
}

and the problem lies in the evaluation of "key", which probably
cannot be guaranteed a constant at that point. There is also the
issue that even if it was known, the branch cannot be easily 
patched in from the rest of the kernel (how is the l_yes address
represented?).

It looks to me like we either need to rewrite the whole of our
static key infrastructure to cope with PIC, or switch over to
the hyp_alternate_select() hack, which I'd rather avoid spreading
further.

In the end, I wonder if that's even worth it...

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 02/05/17 15:50, Marc Zyngier wrote:
>> On 02/05/17 15:40, Catalin Marinas wrote:
>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>> to be used in the EL2 code, while distributions routinely compile their
>>>> kernel with it. We're just lucky that no code actually triggers the
>>>> instrumentation.
>>>>
>>>> Let's not try our luck for much longer, and disable stack-protector
>>>> for code living at EL2.
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>> @@ -2,6 +2,8 @@
>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>  #
>>>>
>>>> +ccflags-y += -fno-stack-protector
>>>> +
>>>
>>> While you are at it, should we have a -fpic here as well? The hyp code
>>> runs at a different location than the rest of the kernel.
>>
>> We definitely should. I've just tried this, and this doesn't seem to
>> work very well. At least this seems to break our jump label
>> implementation. I need to page in that part of the code base and see
>> what happens.
>
> So here's the issue:
>
>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
> In file included from ./include/linux/jump_label.h:120:0,
>                  from ./include/linux/dynamic_debug.h:5,
>                  from ./include/linux/printk.h:329,
>                  from ./include/linux/kernel.h:13,
>                  from ./include/asm-generic/bug.h:15,
>                  from ./arch/arm64/include/asm/bug.h:66,
>                  from ./include/linux/bug.h:4,
>                  from ./include/linux/mmdebug.h:4,
>                  from ./include/linux/mm.h:8,
>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>   asm goto("1: nop\n\t"
>   ^~~
> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>   asm goto("1: nop\n\t"
>   ^~~
> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>
> The corresponding code does this:
>
> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
> {
>         asm goto("1: nop\n\t"
>                  ".pushsection __jump_table,  \"aw\"\n\t"
>                  ".align 3\n\t"
>                  ".quad 1b, %l[l_yes], %c0\n\t"
>                  ".popsection\n\t"
>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>
>         return false;
> l_yes:
>         return true;
> }
>
> and the problem lies in the evaluation of "key", which probably
> cannot be guaranteed a constant at that point. There is also the
> issue that even if it was known, the branch cannot be easily
> patched in from the rest of the kernel (how is the l_yes address
> represented?).
>
> It looks to me like we either need to rewrite the whole of our
> static key infrastructure to cope with PIC, or switch over to
> the hyp_alternate_select() hack, which I'd rather avoid spreading
> further.
>
> In the end, I wonder if that's even worth it...
>

Could you check if it builds with

>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"

instead? We'd still need to update the code that interprets the
__jump_table fields, but it changes the references into relative ones,
which also reduces the size as a bonus.

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 02/05/17 15:50, Marc Zyngier wrote:
>> On 02/05/17 15:40, Catalin Marinas wrote:
>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>> to be used in the EL2 code, while distributions routinely compile their
>>>> kernel with it. We're just lucky that no code actually triggers the
>>>> instrumentation.
>>>>
>>>> Let's not try our luck for much longer, and disable stack-protector
>>>> for code living at EL2.
>>>>
>>>> Cc: stable at vger.kernel.org
>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>> ---
>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>> @@ -2,6 +2,8 @@
>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>  #
>>>>
>>>> +ccflags-y += -fno-stack-protector
>>>> +
>>>
>>> While you are at it, should we have a -fpic here as well? The hyp code
>>> runs at a different location than the rest of the kernel.
>>
>> We definitely should. I've just tried this, and this doesn't seem to
>> work very well. At least this seems to break our jump label
>> implementation. I need to page in that part of the code base and see
>> what happens.
>
> So here's the issue:
>
>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
> In file included from ./include/linux/jump_label.h:120:0,
>                  from ./include/linux/dynamic_debug.h:5,
>                  from ./include/linux/printk.h:329,
>                  from ./include/linux/kernel.h:13,
>                  from ./include/asm-generic/bug.h:15,
>                  from ./arch/arm64/include/asm/bug.h:66,
>                  from ./include/linux/bug.h:4,
>                  from ./include/linux/mmdebug.h:4,
>                  from ./include/linux/mm.h:8,
>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>   asm goto("1: nop\n\t"
>   ^~~
> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>   asm goto("1: nop\n\t"
>   ^~~
> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>
> The corresponding code does this:
>
> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
> {
>         asm goto("1: nop\n\t"
>                  ".pushsection __jump_table,  \"aw\"\n\t"
>                  ".align 3\n\t"
>                  ".quad 1b, %l[l_yes], %c0\n\t"
>                  ".popsection\n\t"
>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>
>         return false;
> l_yes:
>         return true;
> }
>
> and the problem lies in the evaluation of "key", which probably
> cannot be guaranteed a constant at that point. There is also the
> issue that even if it was known, the branch cannot be easily
> patched in from the rest of the kernel (how is the l_yes address
> represented?).
>
> It looks to me like we either need to rewrite the whole of our
> static key infrastructure to cope with PIC, or switch over to
> the hyp_alternate_select() hack, which I'd rather avoid spreading
> further.
>
> In the end, I wonder if that's even worth it...
>

Could you check if it builds with

>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"

instead? We'd still need to update the code that interprets the
__jump_table fields, but it changes the references into relative ones,
which also reduces the size as a bonus.

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 02/05/17 15:50, Marc Zyngier wrote:
>>> On 02/05/17 15:40, Catalin Marinas wrote:
>>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>>> to be used in the EL2 code, while distributions routinely compile their
>>>>> kernel with it. We're just lucky that no code actually triggers the
>>>>> instrumentation.
>>>>>
>>>>> Let's not try our luck for much longer, and disable stack-protector
>>>>> for code living at EL2.
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> ---
>>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>>> @@ -2,6 +2,8 @@
>>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>>  #
>>>>>
>>>>> +ccflags-y += -fno-stack-protector
>>>>> +
>>>>
>>>> While you are at it, should we have a -fpic here as well? The hyp code
>>>> runs at a different location than the rest of the kernel.
>>>
>>> We definitely should. I've just tried this, and this doesn't seem to
>>> work very well. At least this seems to break our jump label
>>> implementation. I need to page in that part of the code base and see
>>> what happens.
>>
>> So here's the issue:
>>
>>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
>> In file included from ./include/linux/jump_label.h:120:0,
>>                  from ./include/linux/dynamic_debug.h:5,
>>                  from ./include/linux/printk.h:329,
>>                  from ./include/linux/kernel.h:13,
>>                  from ./include/asm-generic/bug.h:15,
>>                  from ./arch/arm64/include/asm/bug.h:66,
>>                  from ./include/linux/bug.h:4,
>>                  from ./include/linux/mmdebug.h:4,
>>                  from ./include/linux/mm.h:8,
>>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>   asm goto("1: nop\n\t"
>>   ^~~
>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>   asm goto("1: nop\n\t"
>>   ^~~
>> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
>> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
>> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>>
>> The corresponding code does this:
>>
>> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>> {
>>         asm goto("1: nop\n\t"
>>                  ".pushsection __jump_table,  \"aw\"\n\t"
>>                  ".align 3\n\t"
>>                  ".quad 1b, %l[l_yes], %c0\n\t"
>>                  ".popsection\n\t"
>>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>>
>>         return false;
>> l_yes:
>>         return true;
>> }
>>
>> and the problem lies in the evaluation of "key", which probably
>> cannot be guaranteed a constant at that point. There is also the
>> issue that even if it was known, the branch cannot be easily
>> patched in from the rest of the kernel (how is the l_yes address
>> represented?).
>>
>> It looks to me like we either need to rewrite the whole of our
>> static key infrastructure to cope with PIC, or switch over to
>> the hyp_alternate_select() hack, which I'd rather avoid spreading
>> further.
>>
>> In the end, I wonder if that's even worth it...
>>
>
> Could you check if it builds with
>
>>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
>
> instead? We'd still need to update the code that interprets the
> __jump_table fields, but it changes the references into relative ones,
> which also reduces the size as a bonus.

OK, strike that, this is more tricky than I thought. I am failing to
reproduce this locally, though. Which gcc and tree are you using?

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 02/05/17 15:50, Marc Zyngier wrote:
>>> On 02/05/17 15:40, Catalin Marinas wrote:
>>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>>> to be used in the EL2 code, while distributions routinely compile their
>>>>> kernel with it. We're just lucky that no code actually triggers the
>>>>> instrumentation.
>>>>>
>>>>> Let's not try our luck for much longer, and disable stack-protector
>>>>> for code living at EL2.
>>>>>
>>>>> Cc: stable at vger.kernel.org
>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>> ---
>>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>>> @@ -2,6 +2,8 @@
>>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>>  #
>>>>>
>>>>> +ccflags-y += -fno-stack-protector
>>>>> +
>>>>
>>>> While you are at it, should we have a -fpic here as well? The hyp code
>>>> runs at a different location than the rest of the kernel.
>>>
>>> We definitely should. I've just tried this, and this doesn't seem to
>>> work very well. At least this seems to break our jump label
>>> implementation. I need to page in that part of the code base and see
>>> what happens.
>>
>> So here's the issue:
>>
>>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
>> In file included from ./include/linux/jump_label.h:120:0,
>>                  from ./include/linux/dynamic_debug.h:5,
>>                  from ./include/linux/printk.h:329,
>>                  from ./include/linux/kernel.h:13,
>>                  from ./include/asm-generic/bug.h:15,
>>                  from ./arch/arm64/include/asm/bug.h:66,
>>                  from ./include/linux/bug.h:4,
>>                  from ./include/linux/mmdebug.h:4,
>>                  from ./include/linux/mm.h:8,
>>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>   asm goto("1: nop\n\t"
>>   ^~~
>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>   asm goto("1: nop\n\t"
>>   ^~~
>> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
>> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
>> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>>
>> The corresponding code does this:
>>
>> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>> {
>>         asm goto("1: nop\n\t"
>>                  ".pushsection __jump_table,  \"aw\"\n\t"
>>                  ".align 3\n\t"
>>                  ".quad 1b, %l[l_yes], %c0\n\t"
>>                  ".popsection\n\t"
>>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>>
>>         return false;
>> l_yes:
>>         return true;
>> }
>>
>> and the problem lies in the evaluation of "key", which probably
>> cannot be guaranteed a constant at that point. There is also the
>> issue that even if it was known, the branch cannot be easily
>> patched in from the rest of the kernel (how is the l_yes address
>> represented?).
>>
>> It looks to me like we either need to rewrite the whole of our
>> static key infrastructure to cope with PIC, or switch over to
>> the hyp_alternate_select() hack, which I'd rather avoid spreading
>> further.
>>
>> In the end, I wonder if that's even worth it...
>>
>
> Could you check if it builds with
>
>>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
>
> instead? We'd still need to update the code that interprets the
> __jump_table fields, but it changes the references into relative ones,
> which also reduces the size as a bonus.

OK, strike that, this is more tricky than I thought. I am failing to
reproduce this locally, though. Which gcc and tree are you using?

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

On 11/05/17 17:36, Ard Biesheuvel wrote:
> On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 02/05/17 15:50, Marc Zyngier wrote:
>>>> On 02/05/17 15:40, Catalin Marinas wrote:
>>>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>>>> to be used in the EL2 code, while distributions routinely compile their
>>>>>> kernel with it. We're just lucky that no code actually triggers the
>>>>>> instrumentation.
>>>>>>
>>>>>> Let's not try our luck for much longer, and disable stack-protector
>>>>>> for code living at EL2.
>>>>>>
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>>> ---
>>>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>>>> @@ -2,6 +2,8 @@
>>>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>>>  #
>>>>>>
>>>>>> +ccflags-y += -fno-stack-protector
>>>>>> +
>>>>>
>>>>> While you are at it, should we have a -fpic here as well? The hyp code
>>>>> runs at a different location than the rest of the kernel.
>>>>
>>>> We definitely should. I've just tried this, and this doesn't seem to
>>>> work very well. At least this seems to break our jump label
>>>> implementation. I need to page in that part of the code base and see
>>>> what happens.
>>>
>>> So here's the issue:
>>>
>>>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
>>> In file included from ./include/linux/jump_label.h:120:0,
>>>                  from ./include/linux/dynamic_debug.h:5,
>>>                  from ./include/linux/printk.h:329,
>>>                  from ./include/linux/kernel.h:13,
>>>                  from ./include/asm-generic/bug.h:15,
>>>                  from ./arch/arm64/include/asm/bug.h:66,
>>>                  from ./include/linux/bug.h:4,
>>>                  from ./include/linux/mmdebug.h:4,
>>>                  from ./include/linux/mm.h:8,
>>>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>>>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>>>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>>>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>   asm goto("1: nop\n\t"
>>>   ^~~
>>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>   asm goto("1: nop\n\t"
>>>   ^~~
>>> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
>>> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
>>> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>>>
>>> The corresponding code does this:
>>>
>>> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>>> {
>>>         asm goto("1: nop\n\t"
>>>                  ".pushsection __jump_table,  \"aw\"\n\t"
>>>                  ".align 3\n\t"
>>>                  ".quad 1b, %l[l_yes], %c0\n\t"
>>>                  ".popsection\n\t"
>>>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>>>
>>>         return false;
>>> l_yes:
>>>         return true;
>>> }
>>>
>>> and the problem lies in the evaluation of "key", which probably
>>> cannot be guaranteed a constant at that point. There is also the
>>> issue that even if it was known, the branch cannot be easily
>>> patched in from the rest of the kernel (how is the l_yes address
>>> represented?).
>>>
>>> It looks to me like we either need to rewrite the whole of our
>>> static key infrastructure to cope with PIC, or switch over to
>>> the hyp_alternate_select() hack, which I'd rather avoid spreading
>>> further.
>>>
>>> In the end, I wonder if that's even worth it...
>>>
>>
>> Could you check if it builds with
>>
>>>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
>>
>> instead? We'd still need to update the code that interprets the
>> __jump_table fields, but it changes the references into relative ones,
>> which also reduces the size as a bonus.
> 
> OK, strike that, this is more tricky than I thought. I am failing to
> reproduce this locally, though. Which gcc and tree are you using?

That's current mainline + a number of patches which I don't think are
relevant to this discussion, and -fPIC added to
arch/arm64/kvm/hyp/Makefile. You should see it exploding in timer-sr.c
because of the has_vhe() helper.

GCC is "aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016".

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

On 11/05/17 17:36, Ard Biesheuvel wrote:
> On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>> On 02/05/17 15:50, Marc Zyngier wrote:
>>>> On 02/05/17 15:40, Catalin Marinas wrote:
>>>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>>>> to be used in the EL2 code, while distributions routinely compile their
>>>>>> kernel with it. We're just lucky that no code actually triggers the
>>>>>> instrumentation.
>>>>>>
>>>>>> Let's not try our luck for much longer, and disable stack-protector
>>>>>> for code living at EL2.
>>>>>>
>>>>>> Cc: stable at vger.kernel.org
>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>>> ---
>>>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>>>> @@ -2,6 +2,8 @@
>>>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>>>  #
>>>>>>
>>>>>> +ccflags-y += -fno-stack-protector
>>>>>> +
>>>>>
>>>>> While you are at it, should we have a -fpic here as well? The hyp code
>>>>> runs at a different location than the rest of the kernel.
>>>>
>>>> We definitely should. I've just tried this, and this doesn't seem to
>>>> work very well. At least this seems to break our jump label
>>>> implementation. I need to page in that part of the code base and see
>>>> what happens.
>>>
>>> So here's the issue:
>>>
>>>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
>>> In file included from ./include/linux/jump_label.h:120:0,
>>>                  from ./include/linux/dynamic_debug.h:5,
>>>                  from ./include/linux/printk.h:329,
>>>                  from ./include/linux/kernel.h:13,
>>>                  from ./include/asm-generic/bug.h:15,
>>>                  from ./arch/arm64/include/asm/bug.h:66,
>>>                  from ./include/linux/bug.h:4,
>>>                  from ./include/linux/mmdebug.h:4,
>>>                  from ./include/linux/mm.h:8,
>>>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>>>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>>>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>>>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>   asm goto("1: nop\n\t"
>>>   ^~~
>>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>   asm goto("1: nop\n\t"
>>>   ^~~
>>> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
>>> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
>>> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>>>
>>> The corresponding code does this:
>>>
>>> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>>> {
>>>         asm goto("1: nop\n\t"
>>>                  ".pushsection __jump_table,  \"aw\"\n\t"
>>>                  ".align 3\n\t"
>>>                  ".quad 1b, %l[l_yes], %c0\n\t"
>>>                  ".popsection\n\t"
>>>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>>>
>>>         return false;
>>> l_yes:
>>>         return true;
>>> }
>>>
>>> and the problem lies in the evaluation of "key", which probably
>>> cannot be guaranteed a constant at that point. There is also the
>>> issue that even if it was known, the branch cannot be easily
>>> patched in from the rest of the kernel (how is the l_yes address
>>> represented?).
>>>
>>> It looks to me like we either need to rewrite the whole of our
>>> static key infrastructure to cope with PIC, or switch over to
>>> the hyp_alternate_select() hack, which I'd rather avoid spreading
>>> further.
>>>
>>> In the end, I wonder if that's even worth it...
>>>
>>
>> Could you check if it builds with
>>
>>>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
>>
>> instead? We'd still need to update the code that interprets the
>> __jump_table fields, but it changes the references into relative ones,
>> which also reduces the size as a bonus.
> 
> OK, strike that, this is more tricky than I thought. I am failing to
> reproduce this locally, though. Which gcc and tree are you using?

That's current mainline + a number of patches which I don't think are
relevant to this discussion, and -fPIC added to
arch/arm64/kvm/hyp/Makefile. You should see it exploding in timer-sr.c
because of the has_vhe() helper.

GCC is "aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016".

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 11 May 2017 at 17:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 11/05/17 17:36, Ard Biesheuvel wrote:
>> On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> On 02/05/17 15:50, Marc Zyngier wrote:
>>>>> On 02/05/17 15:40, Catalin Marinas wrote:
>>>>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>>>>> to be used in the EL2 code, while distributions routinely compile their
>>>>>>> kernel with it. We're just lucky that no code actually triggers the
>>>>>>> instrumentation.
>>>>>>>
>>>>>>> Let's not try our luck for much longer, and disable stack-protector
>>>>>>> for code living at EL2.
>>>>>>>
>>>>>>> Cc: stable@vger.kernel.org
>>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>> ---
>>>>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>>>>> @@ -2,6 +2,8 @@
>>>>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>>>>  #
>>>>>>>
>>>>>>> +ccflags-y += -fno-stack-protector
>>>>>>> +
>>>>>>
>>>>>> While you are at it, should we have a -fpic here as well? The hyp code
>>>>>> runs at a different location than the rest of the kernel.
>>>>>
>>>>> We definitely should. I've just tried this, and this doesn't seem to
>>>>> work very well. At least this seems to break our jump label
>>>>> implementation. I need to page in that part of the code base and see
>>>>> what happens.
>>>>
>>>> So here's the issue:
>>>>
>>>>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
>>>> In file included from ./include/linux/jump_label.h:120:0,
>>>>                  from ./include/linux/dynamic_debug.h:5,
>>>>                  from ./include/linux/printk.h:329,
>>>>                  from ./include/linux/kernel.h:13,
>>>>                  from ./include/asm-generic/bug.h:15,
>>>>                  from ./arch/arm64/include/asm/bug.h:66,
>>>>                  from ./include/linux/bug.h:4,
>>>>                  from ./include/linux/mmdebug.h:4,
>>>>                  from ./include/linux/mm.h:8,
>>>>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>>>>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>>>>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>>>>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
>>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
>>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>>   asm goto("1: nop\n\t"
>>>>   ^~~
>>>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
>>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
>>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>>   asm goto("1: nop\n\t"
>>>>   ^~~
>>>> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
>>>> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
>>>> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>>>>
>>>> The corresponding code does this:
>>>>
>>>> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>>>> {
>>>>         asm goto("1: nop\n\t"
>>>>                  ".pushsection __jump_table,  \"aw\"\n\t"
>>>>                  ".align 3\n\t"
>>>>                  ".quad 1b, %l[l_yes], %c0\n\t"
>>>>                  ".popsection\n\t"
>>>>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>>>>
>>>>         return false;
>>>> l_yes:
>>>>         return true;
>>>> }
>>>>
>>>> and the problem lies in the evaluation of "key", which probably
>>>> cannot be guaranteed a constant at that point. There is also the
>>>> issue that even if it was known, the branch cannot be easily
>>>> patched in from the rest of the kernel (how is the l_yes address
>>>> represented?).
>>>>
>>>> It looks to me like we either need to rewrite the whole of our
>>>> static key infrastructure to cope with PIC, or switch over to
>>>> the hyp_alternate_select() hack, which I'd rather avoid spreading
>>>> further.
>>>>
>>>> In the end, I wonder if that's even worth it...
>>>>
>>>
>>> Could you check if it builds with
>>>
>>>>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
>>>
>>> instead? We'd still need to update the code that interprets the
>>> __jump_table fields, but it changes the references into relative ones,
>>> which also reduces the size as a bonus.
>>
>> OK, strike that, this is more tricky than I thought. I am failing to
>> reproduce this locally, though. Which gcc and tree are you using?
>
> That's current mainline + a number of patches which I don't think are
> relevant to this discussion, and -fPIC added to
> arch/arm64/kvm/hyp/Makefile. You should see it exploding in timer-sr.c
> because of the has_vhe() helper.
>
> GCC is "aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016".
>

Nope, builds fine, with Linaro GCC 5.4.0 and 'ccflags-y += -fPIC'
added to arch/arm64/kvm/hyp/Makefile.

In any case, it is worth trying whether -fpie behaves differently: as
per my other reply, aarch64 small model code is already mostly
position independent anyway, and so -fpic (which is intended for
dynamic linking under ELF preemption rules*) is more likely to emit
absolute symbol references than ordinary code. -fpie is supposed to be
the middle ground here, but I dismissed it for the EFI stub because I
could not get it to work at the time.

*) Preemption in ELF means any externally visible symbol can be
overridden by the main executable, in which case the shared library
must update all its internal references as well. In this particular
case, if the key argument to arch_static_branch() refers to a static
key that is part of an externally visible structure, its address is
preemptible at load time, which I suspect may be causing the error you
are seeing.

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Ard Biesheuvel]

On 11 May 2017 at 17:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 11/05/17 17:36, Ard Biesheuvel wrote:
>> On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>> On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>> On 02/05/17 15:50, Marc Zyngier wrote:
>>>>> On 02/05/17 15:40, Catalin Marinas wrote:
>>>>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>>>>> to be used in the EL2 code, while distributions routinely compile their
>>>>>>> kernel with it. We're just lucky that no code actually triggers the
>>>>>>> instrumentation.
>>>>>>>
>>>>>>> Let's not try our luck for much longer, and disable stack-protector
>>>>>>> for code living at EL2.
>>>>>>>
>>>>>>> Cc: stable at vger.kernel.org
>>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>> ---
>>>>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>>>>> @@ -2,6 +2,8 @@
>>>>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>>>>  #
>>>>>>>
>>>>>>> +ccflags-y += -fno-stack-protector
>>>>>>> +
>>>>>>
>>>>>> While you are at it, should we have a -fpic here as well? The hyp code
>>>>>> runs at a different location than the rest of the kernel.
>>>>>
>>>>> We definitely should. I've just tried this, and this doesn't seem to
>>>>> work very well. At least this seems to break our jump label
>>>>> implementation. I need to page in that part of the code base and see
>>>>> what happens.
>>>>
>>>> So here's the issue:
>>>>
>>>>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
>>>> In file included from ./include/linux/jump_label.h:120:0,
>>>>                  from ./include/linux/dynamic_debug.h:5,
>>>>                  from ./include/linux/printk.h:329,
>>>>                  from ./include/linux/kernel.h:13,
>>>>                  from ./include/asm-generic/bug.h:15,
>>>>                  from ./arch/arm64/include/asm/bug.h:66,
>>>>                  from ./include/linux/bug.h:4,
>>>>                  from ./include/linux/mmdebug.h:4,
>>>>                  from ./include/linux/mm.h:8,
>>>>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>>>>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>>>>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>>>>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
>>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
>>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>>   asm goto("1: nop\n\t"
>>>>   ^~~
>>>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
>>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
>>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>>   asm goto("1: nop\n\t"
>>>>   ^~~
>>>> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
>>>> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
>>>> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>>>>
>>>> The corresponding code does this:
>>>>
>>>> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>>>> {
>>>>         asm goto("1: nop\n\t"
>>>>                  ".pushsection __jump_table,  \"aw\"\n\t"
>>>>                  ".align 3\n\t"
>>>>                  ".quad 1b, %l[l_yes], %c0\n\t"
>>>>                  ".popsection\n\t"
>>>>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>>>>
>>>>         return false;
>>>> l_yes:
>>>>         return true;
>>>> }
>>>>
>>>> and the problem lies in the evaluation of "key", which probably
>>>> cannot be guaranteed a constant at that point. There is also the
>>>> issue that even if it was known, the branch cannot be easily
>>>> patched in from the rest of the kernel (how is the l_yes address
>>>> represented?).
>>>>
>>>> It looks to me like we either need to rewrite the whole of our
>>>> static key infrastructure to cope with PIC, or switch over to
>>>> the hyp_alternate_select() hack, which I'd rather avoid spreading
>>>> further.
>>>>
>>>> In the end, I wonder if that's even worth it...
>>>>
>>>
>>> Could you check if it builds with
>>>
>>>>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
>>>
>>> instead? We'd still need to update the code that interprets the
>>> __jump_table fields, but it changes the references into relative ones,
>>> which also reduces the size as a bonus.
>>
>> OK, strike that, this is more tricky than I thought. I am failing to
>> reproduce this locally, though. Which gcc and tree are you using?
>
> That's current mainline + a number of patches which I don't think are
> relevant to this discussion, and -fPIC added to
> arch/arm64/kvm/hyp/Makefile. You should see it exploding in timer-sr.c
> because of the has_vhe() helper.
>
> GCC is "aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016".
>

Nope, builds fine, with Linaro GCC 5.4.0 and 'ccflags-y += -fPIC'
added to arch/arm64/kvm/hyp/Makefile.

In any case, it is worth trying whether -fpie behaves differently: as
per my other reply, aarch64 small model code is already mostly
position independent anyway, and so -fpic (which is intended for
dynamic linking under ELF preemption rules*) is more likely to emit
absolute symbol references than ordinary code. -fpie is supposed to be
the middle ground here, but I dismissed it for the EFI stub because I
could not get it to work at the time.

*) Preemption in ELF means any externally visible symbol can be
overridden by the main executable, in which case the shared library
must update all its internal references as well. In this particular
case, if the key argument to arch_static_branch() refers to a static
key that is part of an externally visible structure, its address is
preemptible at load time, which I suspect may be causing the error you
are seeing.

Re: [PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

On 11/05/17 18:01, Ard Biesheuvel wrote:
> On 11 May 2017 at 17:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 11/05/17 17:36, Ard Biesheuvel wrote:
>>> On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> On 02/05/17 15:50, Marc Zyngier wrote:
>>>>>> On 02/05/17 15:40, Catalin Marinas wrote:
>>>>>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>>>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>>>>>> to be used in the EL2 code, while distributions routinely compile their
>>>>>>>> kernel with it. We're just lucky that no code actually triggers the
>>>>>>>> instrumentation.
>>>>>>>>
>>>>>>>> Let's not try our luck for much longer, and disable stack-protector
>>>>>>>> for code living at EL2.
>>>>>>>>
>>>>>>>> Cc: stable@vger.kernel.org
>>>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>>> ---
>>>>>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>>>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>>>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>>>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>>>>>> @@ -2,6 +2,8 @@
>>>>>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>>>>>  #
>>>>>>>>
>>>>>>>> +ccflags-y += -fno-stack-protector
>>>>>>>> +
>>>>>>>
>>>>>>> While you are at it, should we have a -fpic here as well? The hyp code
>>>>>>> runs at a different location than the rest of the kernel.
>>>>>>
>>>>>> We definitely should. I've just tried this, and this doesn't seem to
>>>>>> work very well. At least this seems to break our jump label
>>>>>> implementation. I need to page in that part of the code base and see
>>>>>> what happens.
>>>>>
>>>>> So here's the issue:
>>>>>
>>>>>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
>>>>> In file included from ./include/linux/jump_label.h:120:0,
>>>>>                  from ./include/linux/dynamic_debug.h:5,
>>>>>                  from ./include/linux/printk.h:329,
>>>>>                  from ./include/linux/kernel.h:13,
>>>>>                  from ./include/asm-generic/bug.h:15,
>>>>>                  from ./arch/arm64/include/asm/bug.h:66,
>>>>>                  from ./include/linux/bug.h:4,
>>>>>                  from ./include/linux/mmdebug.h:4,
>>>>>                  from ./include/linux/mm.h:8,
>>>>>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>>>>>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>>>>>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>>>>>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
>>>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
>>>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>>>   asm goto("1: nop\n\t"
>>>>>   ^~~
>>>>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
>>>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
>>>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>>>   asm goto("1: nop\n\t"
>>>>>   ^~~
>>>>> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
>>>>> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
>>>>> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>>>>>
>>>>> The corresponding code does this:
>>>>>
>>>>> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>>>>> {
>>>>>         asm goto("1: nop\n\t"
>>>>>                  ".pushsection __jump_table,  \"aw\"\n\t"
>>>>>                  ".align 3\n\t"
>>>>>                  ".quad 1b, %l[l_yes], %c0\n\t"
>>>>>                  ".popsection\n\t"
>>>>>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>>>>>
>>>>>         return false;
>>>>> l_yes:
>>>>>         return true;
>>>>> }
>>>>>
>>>>> and the problem lies in the evaluation of "key", which probably
>>>>> cannot be guaranteed a constant at that point. There is also the
>>>>> issue that even if it was known, the branch cannot be easily
>>>>> patched in from the rest of the kernel (how is the l_yes address
>>>>> represented?).
>>>>>
>>>>> It looks to me like we either need to rewrite the whole of our
>>>>> static key infrastructure to cope with PIC, or switch over to
>>>>> the hyp_alternate_select() hack, which I'd rather avoid spreading
>>>>> further.
>>>>>
>>>>> In the end, I wonder if that's even worth it...
>>>>>
>>>>
>>>> Could you check if it builds with
>>>>
>>>>>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
>>>>
>>>> instead? We'd still need to update the code that interprets the
>>>> __jump_table fields, but it changes the references into relative ones,
>>>> which also reduces the size as a bonus.
>>>
>>> OK, strike that, this is more tricky than I thought. I am failing to
>>> reproduce this locally, though. Which gcc and tree are you using?
>>
>> That's current mainline + a number of patches which I don't think are
>> relevant to this discussion, and -fPIC added to
>> arch/arm64/kvm/hyp/Makefile. You should see it exploding in timer-sr.c
>> because of the has_vhe() helper.
>>
>> GCC is "aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016".
>>
> 
> Nope, builds fine, with Linaro GCC 5.4.0 and 'ccflags-y += -fPIC'
> added to arch/arm64/kvm/hyp/Makefile.

Weird. I can't get it to build (just tried with GCC 5.4.1 as well):

  CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/timer-sr.o
In file included from ./include/linux/jump_label.h:120:0,
                 from ./include/linux/static_key.h:1,
                 from ./include/linux/context_tracking_state.h:5,
                 from ./include/linux/vtime.h:4,
                 from ./include/linux/hardirq.h:7,
                 from ./include/linux/kvm_host.h:10,
                 from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/timer-sr.c:20:
./arch/arm64/include/asm/jump_label.h: In function ‘__timer_save_state’:
./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn’t match constraints
  asm goto("1: nop\n\t"
  ^
./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in ‘asm’
./arch/arm64/include/asm/jump_label.h: In function ‘__timer_restore_state’:
./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn’t match constraints
  asm goto("1: nop\n\t"
  ^
scripts/Makefile.build:302: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/timer-sr.o' failed

(defconfig build)

> In any case, it is worth trying whether -fpie behaves differently: as
> per my other reply, aarch64 small model code is already mostly
> position independent anyway, and so -fpic (which is intended for
> dynamic linking under ELF preemption rules*) is more likely to emit
> absolute symbol references than ordinary code. -fpie is supposed to be
> the middle ground here, but I dismissed it for the EFI stub because I
> could not get it to work at the time.

-fpie have the same effect here. I really wonder what's wrong with my setup.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 1/5] arm64: KVM: Do not use stack-protector to compile EL2 code

[Marc Zyngier]

On 11/05/17 18:01, Ard Biesheuvel wrote:
> On 11 May 2017 at 17:42, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 11/05/17 17:36, Ard Biesheuvel wrote:
>>> On 11 May 2017 at 17:11, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>> On 11 May 2017 at 17:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>>>> On 02/05/17 15:50, Marc Zyngier wrote:
>>>>>> On 02/05/17 15:40, Catalin Marinas wrote:
>>>>>>> On Tue, May 02, 2017 at 02:30:37PM +0100, Marc Zyngier wrote:
>>>>>>>> We like living dangerously. Nothing explicitely forbids stack-protector
>>>>>>>> to be used in the EL2 code, while distributions routinely compile their
>>>>>>>> kernel with it. We're just lucky that no code actually triggers the
>>>>>>>> instrumentation.
>>>>>>>>
>>>>>>>> Let's not try our luck for much longer, and disable stack-protector
>>>>>>>> for code living at EL2.
>>>>>>>>
>>>>>>>> Cc: stable at vger.kernel.org
>>>>>>>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>>>>>>>> ---
>>>>>>>>  arch/arm64/kvm/hyp/Makefile | 2 ++
>>>>>>>>  1 file changed, 2 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
>>>>>>>> index aaf42ae8d8c3..14c4e3b14bcb 100644
>>>>>>>> --- a/arch/arm64/kvm/hyp/Makefile
>>>>>>>> +++ b/arch/arm64/kvm/hyp/Makefile
>>>>>>>> @@ -2,6 +2,8 @@
>>>>>>>>  # Makefile for Kernel-based Virtual Machine module, HYP part
>>>>>>>>  #
>>>>>>>>
>>>>>>>> +ccflags-y += -fno-stack-protector
>>>>>>>> +
>>>>>>>
>>>>>>> While you are at it, should we have a -fpic here as well? The hyp code
>>>>>>> runs at a different location than the rest of the kernel.
>>>>>>
>>>>>> We definitely should. I've just tried this, and this doesn't seem to
>>>>>> work very well. At least this seems to break our jump label
>>>>>> implementation. I need to page in that part of the code base and see
>>>>>> what happens.
>>>>>
>>>>> So here's the issue:
>>>>>
>>>>>   CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o
>>>>> In file included from ./include/linux/jump_label.h:120:0,
>>>>>                  from ./include/linux/dynamic_debug.h:5,
>>>>>                  from ./include/linux/printk.h:329,
>>>>>                  from ./include/linux/kernel.h:13,
>>>>>                  from ./include/asm-generic/bug.h:15,
>>>>>                  from ./arch/arm64/include/asm/bug.h:66,
>>>>>                  from ./include/linux/bug.h:4,
>>>>>                  from ./include/linux/mmdebug.h:4,
>>>>>                  from ./include/linux/mm.h:8,
>>>>>                  from ./arch/arm64/include/asm/cacheflush.h:22,
>>>>>                  from ./arch/arm64/include/asm/arch_gicv3.h:27,
>>>>>                  from ./include/linux/irqchip/arm-gic-v3.h:453,
>>>>>                  from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.c:19:
>>>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_save_state':
>>>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>>>   asm goto("1: nop\n\t"
>>>>>   ^~~
>>>>> ./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in 'asm'
>>>>> ./arch/arm64/include/asm/jump_label.h: In function '__vgic_v3_restore_state':
>>>>> ./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn't match constraints
>>>>>   asm goto("1: nop\n\t"
>>>>>   ^~~
>>>>> scripts/Makefile.build:294: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o' failed
>>>>> make[1]: *** [arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/vgic-v3-sr.o] Error 1
>>>>> Makefile:1664: recipe for target 'arch/arm64/kvm/hyp/' failed
>>>>>
>>>>> The corresponding code does this:
>>>>>
>>>>> static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
>>>>> {
>>>>>         asm goto("1: nop\n\t"
>>>>>                  ".pushsection __jump_table,  \"aw\"\n\t"
>>>>>                  ".align 3\n\t"
>>>>>                  ".quad 1b, %l[l_yes], %c0\n\t"
>>>>>                  ".popsection\n\t"
>>>>>                  :  :  "i"(&((char *)key)[branch]) :  : l_yes);
>>>>>
>>>>>         return false;
>>>>> l_yes:
>>>>>         return true;
>>>>> }
>>>>>
>>>>> and the problem lies in the evaluation of "key", which probably
>>>>> cannot be guaranteed a constant at that point. There is also the
>>>>> issue that even if it was known, the branch cannot be easily
>>>>> patched in from the rest of the kernel (how is the l_yes address
>>>>> represented?).
>>>>>
>>>>> It looks to me like we either need to rewrite the whole of our
>>>>> static key infrastructure to cope with PIC, or switch over to
>>>>> the hyp_alternate_select() hack, which I'd rather avoid spreading
>>>>> further.
>>>>>
>>>>> In the end, I wonder if that's even worth it...
>>>>>
>>>>
>>>> Could you check if it builds with
>>>>
>>>>>                  ".long 1b - ., %l[l_yes] - ., %c0 - .\n\t"
>>>>
>>>> instead? We'd still need to update the code that interprets the
>>>> __jump_table fields, but it changes the references into relative ones,
>>>> which also reduces the size as a bonus.
>>>
>>> OK, strike that, this is more tricky than I thought. I am failing to
>>> reproduce this locally, though. Which gcc and tree are you using?
>>
>> That's current mainline + a number of patches which I don't think are
>> relevant to this discussion, and -fPIC added to
>> arch/arm64/kvm/hyp/Makefile. You should see it exploding in timer-sr.c
>> because of the has_vhe() helper.
>>
>> GCC is "aarch64-linux-gnu-gcc (Linaro GCC 6.2-2016.11) 6.2.1 20161016".
>>
> 
> Nope, builds fine, with Linaro GCC 5.4.0 and 'ccflags-y += -fPIC'
> added to arch/arm64/kvm/hyp/Makefile.

Weird. I can't get it to build (just tried with GCC 5.4.1 as well):

  CC      arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/timer-sr.o
In file included from ./include/linux/jump_label.h:120:0,
                 from ./include/linux/static_key.h:1,
                 from ./include/linux/context_tracking_state.h:5,
                 from ./include/linux/vtime.h:4,
                 from ./include/linux/hardirq.h:7,
                 from ./include/linux/kvm_host.h:10,
                 from arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/timer-sr.c:20:
./arch/arm64/include/asm/jump_label.h: In function ?__timer_save_state?:
./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn?t match constraints
  asm goto("1: nop\n\t"
  ^
./arch/arm64/include/asm/jump_label.h:31:2: error: impossible constraint in ?asm?
./arch/arm64/include/asm/jump_label.h: In function ?__timer_restore_state?:
./arch/arm64/include/asm/jump_label.h:31:2: warning: asm operand 0 probably doesn?t match constraints
  asm goto("1: nop\n\t"
  ^
scripts/Makefile.build:302: recipe for target 'arch/arm64/kvm/hyp/../../../../virt/kvm/arm/hyp/timer-sr.o' failed

(defconfig build)

> In any case, it is worth trying whether -fpie behaves differently: as
> per my other reply, aarch64 small model code is already mostly
> position independent anyway, and so -fpic (which is intended for
> dynamic linking under ELF preemption rules*) is more likely to emit
> absolute symbol references than ordinary code. -fpie is supposed to be
> the middle ground here, but I dismissed it for the EFI stub because I
> could not get it to work at the time.

-fpie have the same effect here. I really wonder what's wrong with my setup.

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 2/5] arm: KVM: Do not use stack-protector to compile HYP code

[Marc Zyngier]

We like living dangerously. Nothing explicitely forbids stack-protector
to be used in the HYP code, while distributions routinely compile their
kernel with it. We're just lucky that no code actually triggers the
instrumentation.

Let's not try our luck for much longer, and disable stack-protector
for code living at HYP.

Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/hyp/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
index 3023bb530edf..8679405b0b2b 100644
--- a/arch/arm/kvm/hyp/Makefile
+++ b/arch/arm/kvm/hyp/Makefile
@@ -2,6 +2,8 @@
 # Makefile for Kernel-based Virtual Machine module, HYP part
 #
 
+ccflags-y += -fno-stack-protector
+
 KVM=../../../../virt/kvm
 
 obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
-- 
2.11.0

[PATCH 2/5] arm: KVM: Do not use stack-protector to compile HYP code

[Marc Zyngier]

We like living dangerously. Nothing explicitely forbids stack-protector
to be used in the HYP code, while distributions routinely compile their
kernel with it. We're just lucky that no code actually triggers the
instrumentation.

Let's not try our luck for much longer, and disable stack-protector
for code living at HYP.

Cc: stable at vger.kernel.org
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/kvm/hyp/Makefile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
index 3023bb530edf..8679405b0b2b 100644
--- a/arch/arm/kvm/hyp/Makefile
+++ b/arch/arm/kvm/hyp/Makefile
@@ -2,6 +2,8 @@
 # Makefile for Kernel-based Virtual Machine module, HYP part
 #
 
+ccflags-y += -fno-stack-protector
+
 KVM=../../../../virt/kvm
 
 obj-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/hyp/vgic-v2-sr.o
-- 
2.11.0

[PATCH 3/5] KVM: arm/arm64: vgic-v2: Do not use Active+Pending state for a HW interrupt

[Marc Zyngier]

When an interrupt is injected with the HW bit set (indicating that
deactivation should be propagated to the physical distributor),
special care must be taken so that we never mark the corresponding
LR with the Active+Pending state (as the pending state is kept in
the physycal distributor).

Cc: stable@vger.kernel.org
Fixes: 140b086dd197 ("KVM: arm/arm64: vgic-new: Add GICv2 world switch backend")
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v2.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index a65757aab6d3..504b4bd0d651 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -149,6 +149,13 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 	if (irq->hw) {
 		val |= GICH_LR_HW;
 		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
+		/*
+		 * Never set pending+active on a HW interrupt, as the
+		 * pending state is kept at the physical distributor
+		 * level.
+		 */
+		if (irq->active && irq_is_pending(irq))
+			val &= ~GICH_LR_PENDING_BIT;
 	} else {
 		if (irq->config == VGIC_CONFIG_LEVEL)
 			val |= GICH_LR_EOI;
-- 
2.11.0

[PATCH 3/5] KVM: arm/arm64: vgic-v2: Do not use Active+Pending state for a HW interrupt

[Marc Zyngier]

When an interrupt is injected with the HW bit set (indicating that
deactivation should be propagated to the physical distributor),
special care must be taken so that we never mark the corresponding
LR with the Active+Pending state (as the pending state is kept in
the physycal distributor).

Cc: stable at vger.kernel.org
Fixes: 140b086dd197 ("KVM: arm/arm64: vgic-new: Add GICv2 world switch backend")
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v2.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
index a65757aab6d3..504b4bd0d651 100644
--- a/virt/kvm/arm/vgic/vgic-v2.c
+++ b/virt/kvm/arm/vgic/vgic-v2.c
@@ -149,6 +149,13 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 	if (irq->hw) {
 		val |= GICH_LR_HW;
 		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
+		/*
+		 * Never set pending+active on a HW interrupt, as the
+		 * pending state is kept@the physical distributor
+		 * level.
+		 */
+		if (irq->active && irq_is_pending(irq))
+			val &= ~GICH_LR_PENDING_BIT;
 	} else {
 		if (irq->config == VGIC_CONFIG_LEVEL)
 			val |= GICH_LR_EOI;
-- 
2.11.0

Re: [PATCH 3/5] KVM: arm/arm64: vgic-v2: Do not use Active+Pending state for a HW interrupt

[Christoffer Dall]

On Tue, May 02, 2017 at 02:30:39PM +0100, Marc Zyngier wrote:
> When an interrupt is injected with the HW bit set (indicating that
> deactivation should be propagated to the physical distributor),
> special care must be taken so that we never mark the corresponding
> LR with the Active+Pending state (as the pending state is kept in
> the physycal distributor).
> 
> Cc: stable@vger.kernel.org
> Fixes: 140b086dd197 ("KVM: arm/arm64: vgic-new: Add GICv2 world switch backend")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  virt/kvm/arm/vgic/vgic-v2.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index a65757aab6d3..504b4bd0d651 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -149,6 +149,13 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  	if (irq->hw) {
>  		val |= GICH_LR_HW;
>  		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
> +		/*
> +		 * Never set pending+active on a HW interrupt, as the
> +		 * pending state is kept at the physical distributor
> +		 * level.
> +		 */
> +		if (irq->active && irq_is_pending(irq))
> +			val &= ~GICH_LR_PENDING_BIT;
>  	} else {
>  		if (irq->config == VGIC_CONFIG_LEVEL)
>  			val |= GICH_LR_EOI;
> -- 
> 2.11.0
> 

[PATCH 3/5] KVM: arm/arm64: vgic-v2: Do not use Active+Pending state for a HW interrupt

[Christoffer Dall]

On Tue, May 02, 2017 at 02:30:39PM +0100, Marc Zyngier wrote:
> When an interrupt is injected with the HW bit set (indicating that
> deactivation should be propagated to the physical distributor),
> special care must be taken so that we never mark the corresponding
> LR with the Active+Pending state (as the pending state is kept in
> the physycal distributor).
> 
> Cc: stable at vger.kernel.org
> Fixes: 140b086dd197 ("KVM: arm/arm64: vgic-new: Add GICv2 world switch backend")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  virt/kvm/arm/vgic/vgic-v2.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v2.c b/virt/kvm/arm/vgic/vgic-v2.c
> index a65757aab6d3..504b4bd0d651 100644
> --- a/virt/kvm/arm/vgic/vgic-v2.c
> +++ b/virt/kvm/arm/vgic/vgic-v2.c
> @@ -149,6 +149,13 @@ void vgic_v2_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  	if (irq->hw) {
>  		val |= GICH_LR_HW;
>  		val |= irq->hwintid << GICH_LR_PHYSID_CPUID_SHIFT;
> +		/*
> +		 * Never set pending+active on a HW interrupt, as the
> +		 * pending state is kept at the physical distributor
> +		 * level.
> +		 */
> +		if (irq->active && irq_is_pending(irq))
> +			val &= ~GICH_LR_PENDING_BIT;
>  	} else {
>  		if (irq->config == VGIC_CONFIG_LEVEL)
>  			val |= GICH_LR_EOI;
> -- 
> 2.11.0
> 

[PATCH 4/5] KVM: arm/arm64: vgic-v3: Do not use Active+Pending state for a HW interrupt

[Marc Zyngier]

When an interrupt is injected with the HW bit set (indicating that
deactivation should be propagated to the physical distributor),
special care must be taken so that we never mark the corresponding
LR with the Active+Pending state (as the pending state is kept in
the physycal distributor).

Cc: stable@vger.kernel.org
Fixes: 59529f69f504 ("KVM: arm/arm64: vgic-new: Add GICv3 world switch backend")
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v3.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index df1503650300..393779ebe87c 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -127,6 +127,13 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 	if (irq->hw) {
 		val |= ICH_LR_HW;
 		val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
+		/*
+		 * Never set pending+active on a HW interrupt, as the
+		 * pending state is kept at the physical distributor
+		 * level.
+		 */
+		if (irq->active && irq_is_pending(irq))
+			val &= ~ICH_LR_PENDING_BIT;
 	} else {
 		if (irq->config == VGIC_CONFIG_LEVEL)
 			val |= ICH_LR_EOI;
-- 
2.11.0

[PATCH 4/5] KVM: arm/arm64: vgic-v3: Do not use Active+Pending state for a HW interrupt

[Marc Zyngier]

When an interrupt is injected with the HW bit set (indicating that
deactivation should be propagated to the physical distributor),
special care must be taken so that we never mark the corresponding
LR with the Active+Pending state (as the pending state is kept in
the physycal distributor).

Cc: stable at vger.kernel.org
Fixes: 59529f69f504 ("KVM: arm/arm64: vgic-new: Add GICv3 world switch backend")
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v3.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index df1503650300..393779ebe87c 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -127,6 +127,13 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
 	if (irq->hw) {
 		val |= ICH_LR_HW;
 		val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
+		/*
+		 * Never set pending+active on a HW interrupt, as the
+		 * pending state is kept@the physical distributor
+		 * level.
+		 */
+		if (irq->active && irq_is_pending(irq))
+			val &= ~ICH_LR_PENDING_BIT;
 	} else {
 		if (irq->config == VGIC_CONFIG_LEVEL)
 			val |= ICH_LR_EOI;
-- 
2.11.0

Re: [PATCH 4/5] KVM: arm/arm64: vgic-v3: Do not use Active+Pending state for a HW interrupt

[Christoffer Dall]

On Tue, May 02, 2017 at 02:30:40PM +0100, Marc Zyngier wrote:
> When an interrupt is injected with the HW bit set (indicating that
> deactivation should be propagated to the physical distributor),
> special care must be taken so that we never mark the corresponding
> LR with the Active+Pending state (as the pending state is kept in
> the physycal distributor).
> 
> Cc: stable@vger.kernel.org
> Fixes: 59529f69f504 ("KVM: arm/arm64: vgic-new: Add GICv3 world switch backend")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index df1503650300..393779ebe87c 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -127,6 +127,13 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  	if (irq->hw) {
>  		val |= ICH_LR_HW;
>  		val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
> +		/*
> +		 * Never set pending+active on a HW interrupt, as the
> +		 * pending state is kept at the physical distributor
> +		 * level.
> +		 */
> +		if (irq->active && irq_is_pending(irq))
> +			val &= ~ICH_LR_PENDING_BIT;
>  	} else {
>  		if (irq->config == VGIC_CONFIG_LEVEL)
>  			val |= ICH_LR_EOI;
> -- 
> 2.11.0
> 

[PATCH 4/5] KVM: arm/arm64: vgic-v3: Do not use Active+Pending state for a HW interrupt

[Christoffer Dall]

On Tue, May 02, 2017 at 02:30:40PM +0100, Marc Zyngier wrote:
> When an interrupt is injected with the HW bit set (indicating that
> deactivation should be propagated to the physical distributor),
> special care must be taken so that we never mark the corresponding
> LR with the Active+Pending state (as the pending state is kept in
> the physycal distributor).
> 
> Cc: stable at vger.kernel.org
> Fixes: 59529f69f504 ("KVM: arm/arm64: vgic-new: Add GICv3 world switch backend")
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <cdall@linaro.org>

> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index df1503650300..393779ebe87c 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -127,6 +127,13 @@ void vgic_v3_populate_lr(struct kvm_vcpu *vcpu, struct vgic_irq *irq, int lr)
>  	if (irq->hw) {
>  		val |= ICH_LR_HW;
>  		val |= ((u64)irq->hwintid) << ICH_LR_PHYS_ID_SHIFT;
> +		/*
> +		 * Never set pending+active on a HW interrupt, as the
> +		 * pending state is kept at the physical distributor
> +		 * level.
> +		 */
> +		if (irq->active && irq_is_pending(irq))
> +			val &= ~ICH_LR_PENDING_BIT;
>  	} else {
>  		if (irq->config == VGIC_CONFIG_LEVEL)
>  			val |= ICH_LR_EOI;
> -- 
> 2.11.0
> 

[PATCH 5/5] KVM: arm/arm64: vgic-v3: Use PREbits to infer the number of ICH_APxRn_EL2 registers

[Marc Zyngier]

The GICv3 documentation is extremely confusing, as it talks about
the number of priorities represented by the ICH_APxRn_EL2 registers,
while it should really talk about the number of preemption levels.

This leads to a bug where we may access undefined ICH_APxRn_EL2
registers, since PREbits is allowed to be smaller than PRIbits.
Thankfully, nobody seem to have taken this path so far...

The fix is to use ICH_VTR_EL2.PREbits instead.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/hyp/vgic-v3-sr.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
index bce6037cf01d..32c3295929b0 100644
--- a/virt/kvm/arm/hyp/vgic-v3-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
@@ -22,7 +22,7 @@
 #include <asm/kvm_hyp.h>
 
 #define vtr_to_max_lr_idx(v)		((v) & 0xf)
-#define vtr_to_nr_pri_bits(v)		(((u32)(v) >> 29) + 1)
+#define vtr_to_nr_pre_bits(v)		(((u32)(v) >> 26) + 1)
 
 static u64 __hyp_text __gic_v3_get_lr(unsigned int lr)
 {
@@ -135,13 +135,13 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 
 	if (used_lrs) {
 		int i;
-		u32 nr_pri_bits;
+		u32 nr_pre_bits;
 
 		cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2);
 
 		write_gicreg(0, ICH_HCR_EL2);
 		val = read_gicreg(ICH_VTR_EL2);
-		nr_pri_bits = vtr_to_nr_pri_bits(val);
+		nr_pre_bits = vtr_to_nr_pre_bits(val);
 
 		for (i = 0; i < used_lrs; i++) {
 			if (cpu_if->vgic_elrsr & (1 << i))
@@ -152,7 +152,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 			__gic_v3_set_lr(0, i);
 		}
 
-		switch (nr_pri_bits) {
+		switch (nr_pre_bits) {
 		case 7:
 			cpu_if->vgic_ap0r[3] = read_gicreg(ICH_AP0R3_EL2);
 			cpu_if->vgic_ap0r[2] = read_gicreg(ICH_AP0R2_EL2);
@@ -162,7 +162,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 			cpu_if->vgic_ap0r[0] = read_gicreg(ICH_AP0R0_EL2);
 		}
 
-		switch (nr_pri_bits) {
+		switch (nr_pre_bits) {
 		case 7:
 			cpu_if->vgic_ap1r[3] = read_gicreg(ICH_AP1R3_EL2);
 			cpu_if->vgic_ap1r[2] = read_gicreg(ICH_AP1R2_EL2);
@@ -198,7 +198,7 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
 	u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs;
 	u64 val;
-	u32 nr_pri_bits;
+	u32 nr_pre_bits;
 	int i;
 
 	/*
@@ -217,12 +217,12 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	}
 
 	val = read_gicreg(ICH_VTR_EL2);
-	nr_pri_bits = vtr_to_nr_pri_bits(val);
+	nr_pre_bits = vtr_to_nr_pre_bits(val);
 
 	if (used_lrs) {
 		write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2);
 
-		switch (nr_pri_bits) {
+		switch (nr_pre_bits) {
 		case 7:
 			write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2);
 			write_gicreg(cpu_if->vgic_ap0r[2], ICH_AP0R2_EL2);
@@ -232,7 +232,7 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 			write_gicreg(cpu_if->vgic_ap0r[0], ICH_AP0R0_EL2);
 		}
 
-		switch (nr_pri_bits) {
+		switch (nr_pre_bits) {
 		case 7:
 			write_gicreg(cpu_if->vgic_ap1r[3], ICH_AP1R3_EL2);
 			write_gicreg(cpu_if->vgic_ap1r[2], ICH_AP1R2_EL2);
-- 
2.11.0

[PATCH 5/5] KVM: arm/arm64: vgic-v3: Use PREbits to infer the number of ICH_APxRn_EL2 registers

[Marc Zyngier]

The GICv3 documentation is extremely confusing, as it talks about
the number of priorities represented by the ICH_APxRn_EL2 registers,
while it should really talk about the number of preemption levels.

This leads to a bug where we may access undefined ICH_APxRn_EL2
registers, since PREbits is allowed to be smaller than PRIbits.
Thankfully, nobody seem to have taken this path so far...

The fix is to use ICH_VTR_EL2.PREbits instead.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/hyp/vgic-v3-sr.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
index bce6037cf01d..32c3295929b0 100644
--- a/virt/kvm/arm/hyp/vgic-v3-sr.c
+++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
@@ -22,7 +22,7 @@
 #include <asm/kvm_hyp.h>
 
 #define vtr_to_max_lr_idx(v)		((v) & 0xf)
-#define vtr_to_nr_pri_bits(v)		(((u32)(v) >> 29) + 1)
+#define vtr_to_nr_pre_bits(v)		(((u32)(v) >> 26) + 1)
 
 static u64 __hyp_text __gic_v3_get_lr(unsigned int lr)
 {
@@ -135,13 +135,13 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 
 	if (used_lrs) {
 		int i;
-		u32 nr_pri_bits;
+		u32 nr_pre_bits;
 
 		cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2);
 
 		write_gicreg(0, ICH_HCR_EL2);
 		val = read_gicreg(ICH_VTR_EL2);
-		nr_pri_bits = vtr_to_nr_pri_bits(val);
+		nr_pre_bits = vtr_to_nr_pre_bits(val);
 
 		for (i = 0; i < used_lrs; i++) {
 			if (cpu_if->vgic_elrsr & (1 << i))
@@ -152,7 +152,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 			__gic_v3_set_lr(0, i);
 		}
 
-		switch (nr_pri_bits) {
+		switch (nr_pre_bits) {
 		case 7:
 			cpu_if->vgic_ap0r[3] = read_gicreg(ICH_AP0R3_EL2);
 			cpu_if->vgic_ap0r[2] = read_gicreg(ICH_AP0R2_EL2);
@@ -162,7 +162,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
 			cpu_if->vgic_ap0r[0] = read_gicreg(ICH_AP0R0_EL2);
 		}
 
-		switch (nr_pri_bits) {
+		switch (nr_pre_bits) {
 		case 7:
 			cpu_if->vgic_ap1r[3] = read_gicreg(ICH_AP1R3_EL2);
 			cpu_if->vgic_ap1r[2] = read_gicreg(ICH_AP1R2_EL2);
@@ -198,7 +198,7 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
 	u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs;
 	u64 val;
-	u32 nr_pri_bits;
+	u32 nr_pre_bits;
 	int i;
 
 	/*
@@ -217,12 +217,12 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 	}
 
 	val = read_gicreg(ICH_VTR_EL2);
-	nr_pri_bits = vtr_to_nr_pri_bits(val);
+	nr_pre_bits = vtr_to_nr_pre_bits(val);
 
 	if (used_lrs) {
 		write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2);
 
-		switch (nr_pri_bits) {
+		switch (nr_pre_bits) {
 		case 7:
 			write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2);
 			write_gicreg(cpu_if->vgic_ap0r[2], ICH_AP0R2_EL2);
@@ -232,7 +232,7 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
 			write_gicreg(cpu_if->vgic_ap0r[0], ICH_AP0R0_EL2);
 		}
 
-		switch (nr_pri_bits) {
+		switch (nr_pre_bits) {
 		case 7:
 			write_gicreg(cpu_if->vgic_ap1r[3], ICH_AP1R3_EL2);
 			write_gicreg(cpu_if->vgic_ap1r[2], ICH_AP1R2_EL2);
-- 
2.11.0

Re: [PATCH 5/5] KVM: arm/arm64: vgic-v3: Use PREbits to infer the number of ICH_APxRn_EL2 registers

[Christoffer Dall]

On Tue, May 02, 2017 at 02:30:41PM +0100, Marc Zyngier wrote:
> The GICv3 documentation is extremely confusing, as it talks about
> the number of priorities represented by the ICH_APxRn_EL2 registers,
> while it should really talk about the number of preemption levels.
> 
> This leads to a bug where we may access undefined ICH_APxRn_EL2
> registers, since PREbits is allowed to be smaller than PRIbits.

How does this work from the guest's point of view?  If I read the spec
correctly, software can derive the number of supported priority levels
(and thereby the minimal value of ICC_BPR0_EL1.BinaryPoint field) from
the number of priority bits implemented, which is exposed via
ICC_CTLR_EL1.PRIbits.

If that minimum value can be higher when running in a VM, does that mean
that an OS that wants to support running in a VM and on real hardware,
must adjust its expectations by writing to the BinaryPoint and read back
the value?  Otherwise it seems to me it won't get the preemption it
asked for.

> Thankfully, nobody seem to have taken this path so far...
> 
> The fix is to use ICH_VTR_EL2.PREbits instead.

Strictly speaking, I cannot find anything in the spec that says that
this is the way things are connected, although it seems to me that it's
the most obvious thing.  Is there a plan to clarify the spec?

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

I'm going to say that I reviewed this patch because you're potentially a
better authority in this area than the spec.

Reviewed-by: Christoffer Dall <cdall@linaro.org>


> ---
>  virt/kvm/arm/hyp/vgic-v3-sr.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> index bce6037cf01d..32c3295929b0 100644
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> @@ -22,7 +22,7 @@
>  #include <asm/kvm_hyp.h>
>  
>  #define vtr_to_max_lr_idx(v)		((v) & 0xf)
> -#define vtr_to_nr_pri_bits(v)		(((u32)(v) >> 29) + 1)
> +#define vtr_to_nr_pre_bits(v)		(((u32)(v) >> 26) + 1)
>  
>  static u64 __hyp_text __gic_v3_get_lr(unsigned int lr)
>  {
> @@ -135,13 +135,13 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  
>  	if (used_lrs) {
>  		int i;
> -		u32 nr_pri_bits;
> +		u32 nr_pre_bits;
>  
>  		cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2);
>  
>  		write_gicreg(0, ICH_HCR_EL2);
>  		val = read_gicreg(ICH_VTR_EL2);
> -		nr_pri_bits = vtr_to_nr_pri_bits(val);
> +		nr_pre_bits = vtr_to_nr_pre_bits(val);
>  
>  		for (i = 0; i < used_lrs; i++) {
>  			if (cpu_if->vgic_elrsr & (1 << i))
> @@ -152,7 +152,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  			__gic_v3_set_lr(0, i);
>  		}
>  
> -		switch (nr_pri_bits) {
> +		switch (nr_pre_bits) {
>  		case 7:
>  			cpu_if->vgic_ap0r[3] = read_gicreg(ICH_AP0R3_EL2);
>  			cpu_if->vgic_ap0r[2] = read_gicreg(ICH_AP0R2_EL2);
> @@ -162,7 +162,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  			cpu_if->vgic_ap0r[0] = read_gicreg(ICH_AP0R0_EL2);
>  		}
>  
> -		switch (nr_pri_bits) {
> +		switch (nr_pre_bits) {
>  		case 7:
>  			cpu_if->vgic_ap1r[3] = read_gicreg(ICH_AP1R3_EL2);
>  			cpu_if->vgic_ap1r[2] = read_gicreg(ICH_AP1R2_EL2);
> @@ -198,7 +198,7 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>  	u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs;
>  	u64 val;
> -	u32 nr_pri_bits;
> +	u32 nr_pre_bits;
>  	int i;
>  
>  	/*
> @@ -217,12 +217,12 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  	}
>  
>  	val = read_gicreg(ICH_VTR_EL2);
> -	nr_pri_bits = vtr_to_nr_pri_bits(val);
> +	nr_pre_bits = vtr_to_nr_pre_bits(val);
>  
>  	if (used_lrs) {
>  		write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2);
>  
> -		switch (nr_pri_bits) {
> +		switch (nr_pre_bits) {
>  		case 7:
>  			write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2);
>  			write_gicreg(cpu_if->vgic_ap0r[2], ICH_AP0R2_EL2);
> @@ -232,7 +232,7 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  			write_gicreg(cpu_if->vgic_ap0r[0], ICH_AP0R0_EL2);
>  		}
>  
> -		switch (nr_pri_bits) {
> +		switch (nr_pre_bits) {
>  		case 7:
>  			write_gicreg(cpu_if->vgic_ap1r[3], ICH_AP1R3_EL2);
>  			write_gicreg(cpu_if->vgic_ap1r[2], ICH_AP1R2_EL2);
> -- 
> 2.11.0
> 

[PATCH 5/5] KVM: arm/arm64: vgic-v3: Use PREbits to infer the number of ICH_APxRn_EL2 registers

[Christoffer Dall]

On Tue, May 02, 2017 at 02:30:41PM +0100, Marc Zyngier wrote:
> The GICv3 documentation is extremely confusing, as it talks about
> the number of priorities represented by the ICH_APxRn_EL2 registers,
> while it should really talk about the number of preemption levels.
> 
> This leads to a bug where we may access undefined ICH_APxRn_EL2
> registers, since PREbits is allowed to be smaller than PRIbits.

How does this work from the guest's point of view?  If I read the spec
correctly, software can derive the number of supported priority levels
(and thereby the minimal value of ICC_BPR0_EL1.BinaryPoint field) from
the number of priority bits implemented, which is exposed via
ICC_CTLR_EL1.PRIbits.

If that minimum value can be higher when running in a VM, does that mean
that an OS that wants to support running in a VM and on real hardware,
must adjust its expectations by writing to the BinaryPoint and read back
the value?  Otherwise it seems to me it won't get the preemption it
asked for.

> Thankfully, nobody seem to have taken this path so far...
> 
> The fix is to use ICH_VTR_EL2.PREbits instead.

Strictly speaking, I cannot find anything in the spec that says that
this is the way things are connected, although it seems to me that it's
the most obvious thing.  Is there a plan to clarify the spec?

> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

I'm going to say that I reviewed this patch because you're potentially a
better authority in this area than the spec.

Reviewed-by: Christoffer Dall <cdall@linaro.org>


> ---
>  virt/kvm/arm/hyp/vgic-v3-sr.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> index bce6037cf01d..32c3295929b0 100644
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> @@ -22,7 +22,7 @@
>  #include <asm/kvm_hyp.h>
>  
>  #define vtr_to_max_lr_idx(v)		((v) & 0xf)
> -#define vtr_to_nr_pri_bits(v)		(((u32)(v) >> 29) + 1)
> +#define vtr_to_nr_pre_bits(v)		(((u32)(v) >> 26) + 1)
>  
>  static u64 __hyp_text __gic_v3_get_lr(unsigned int lr)
>  {
> @@ -135,13 +135,13 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  
>  	if (used_lrs) {
>  		int i;
> -		u32 nr_pri_bits;
> +		u32 nr_pre_bits;
>  
>  		cpu_if->vgic_elrsr = read_gicreg(ICH_ELSR_EL2);
>  
>  		write_gicreg(0, ICH_HCR_EL2);
>  		val = read_gicreg(ICH_VTR_EL2);
> -		nr_pri_bits = vtr_to_nr_pri_bits(val);
> +		nr_pre_bits = vtr_to_nr_pre_bits(val);
>  
>  		for (i = 0; i < used_lrs; i++) {
>  			if (cpu_if->vgic_elrsr & (1 << i))
> @@ -152,7 +152,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  			__gic_v3_set_lr(0, i);
>  		}
>  
> -		switch (nr_pri_bits) {
> +		switch (nr_pre_bits) {
>  		case 7:
>  			cpu_if->vgic_ap0r[3] = read_gicreg(ICH_AP0R3_EL2);
>  			cpu_if->vgic_ap0r[2] = read_gicreg(ICH_AP0R2_EL2);
> @@ -162,7 +162,7 @@ void __hyp_text __vgic_v3_save_state(struct kvm_vcpu *vcpu)
>  			cpu_if->vgic_ap0r[0] = read_gicreg(ICH_AP0R0_EL2);
>  		}
>  
> -		switch (nr_pri_bits) {
> +		switch (nr_pre_bits) {
>  		case 7:
>  			cpu_if->vgic_ap1r[3] = read_gicreg(ICH_AP1R3_EL2);
>  			cpu_if->vgic_ap1r[2] = read_gicreg(ICH_AP1R2_EL2);
> @@ -198,7 +198,7 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  	struct vgic_v3_cpu_if *cpu_if = &vcpu->arch.vgic_cpu.vgic_v3;
>  	u64 used_lrs = vcpu->arch.vgic_cpu.used_lrs;
>  	u64 val;
> -	u32 nr_pri_bits;
> +	u32 nr_pre_bits;
>  	int i;
>  
>  	/*
> @@ -217,12 +217,12 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  	}
>  
>  	val = read_gicreg(ICH_VTR_EL2);
> -	nr_pri_bits = vtr_to_nr_pri_bits(val);
> +	nr_pre_bits = vtr_to_nr_pre_bits(val);
>  
>  	if (used_lrs) {
>  		write_gicreg(cpu_if->vgic_hcr, ICH_HCR_EL2);
>  
> -		switch (nr_pri_bits) {
> +		switch (nr_pre_bits) {
>  		case 7:
>  			write_gicreg(cpu_if->vgic_ap0r[3], ICH_AP0R3_EL2);
>  			write_gicreg(cpu_if->vgic_ap0r[2], ICH_AP0R2_EL2);
> @@ -232,7 +232,7 @@ void __hyp_text __vgic_v3_restore_state(struct kvm_vcpu *vcpu)
>  			write_gicreg(cpu_if->vgic_ap0r[0], ICH_AP0R0_EL2);
>  		}
>  
> -		switch (nr_pri_bits) {
> +		switch (nr_pre_bits) {
>  		case 7:
>  			write_gicreg(cpu_if->vgic_ap1r[3], ICH_AP1R3_EL2);
>  			write_gicreg(cpu_if->vgic_ap1r[2], ICH_AP1R2_EL2);
> -- 
> 2.11.0
> 

Re: [PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Paolo Bonzini]

On 02/05/2017 15:30, Marc Zyngier wrote:
> Here's a handful of random fixes I've queued locally that didn't have
> a chance to make it in 4.11.
> 
> The first two patches avoid stack-protector messing with the HYP code,
> as this ends up being a complete disaster.
> 
> The following two patches fix a bug introduced in the new vgic, where
> we may queue HW interrupts with the Pending+Active state, which is
> illegal.
> 
> The final patch fixes a misinterpretation of the spec, where we
> compute the number of APxRn register based on the number of priorities
> instead of using the number of preemption levels.
> 
> I've tagged the first 4 patches for stable, given that we're doing
> something potentially harmful. The last patch is more of a theoretical
> issue at this stage, so probably need for a backport.

Would you like me to apply them, or are you looking for reviews and
going to send them in a pull request?

I can wait a couple days before sending my own pull request to Linus.

Paolo

[PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Paolo Bonzini]

On 02/05/2017 15:30, Marc Zyngier wrote:
> Here's a handful of random fixes I've queued locally that didn't have
> a chance to make it in 4.11.
> 
> The first two patches avoid stack-protector messing with the HYP code,
> as this ends up being a complete disaster.
> 
> The following two patches fix a bug introduced in the new vgic, where
> we may queue HW interrupts with the Pending+Active state, which is
> illegal.
> 
> The final patch fixes a misinterpretation of the spec, where we
> compute the number of APxRn register based on the number of priorities
> instead of using the number of preemption levels.
> 
> I've tagged the first 4 patches for stable, given that we're doing
> something potentially harmful. The last patch is more of a theoretical
> issue at this stage, so probably need for a backport.

Would you like me to apply them, or are you looking for reviews and
going to send them in a pull request?

I can wait a couple days before sending my own pull request to Linus.

Paolo

Re: [PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Marc Zyngier]

On 02/05/17 15:44, Paolo Bonzini wrote:
> 
> 
> On 02/05/2017 15:30, Marc Zyngier wrote:
>> Here's a handful of random fixes I've queued locally that didn't have
>> a chance to make it in 4.11.
>>
>> The first two patches avoid stack-protector messing with the HYP code,
>> as this ends up being a complete disaster.
>>
>> The following two patches fix a bug introduced in the new vgic, where
>> we may queue HW interrupts with the Pending+Active state, which is
>> illegal.
>>
>> The final patch fixes a misinterpretation of the spec, where we
>> compute the number of APxRn register based on the number of priorities
>> instead of using the number of preemption levels.
>>
>> I've tagged the first 4 patches for stable, given that we're doing
>> something potentially harmful. The last patch is more of a theoretical
>> issue at this stage, so probably need for a backport.
> 
> Would you like me to apply them, or are you looking for reviews and
> going to send them in a pull request?
> 
> I can wait a couple days before sending my own pull request to Linus.

Christoffer is in charge of the tree at the moment, so I'll leave it up
to him to decide. But my guess is that he will send a PR some time
later, with the rest of the fixes that have been posted lately.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Marc Zyngier]

On 02/05/17 15:44, Paolo Bonzini wrote:
> 
> 
> On 02/05/2017 15:30, Marc Zyngier wrote:
>> Here's a handful of random fixes I've queued locally that didn't have
>> a chance to make it in 4.11.
>>
>> The first two patches avoid stack-protector messing with the HYP code,
>> as this ends up being a complete disaster.
>>
>> The following two patches fix a bug introduced in the new vgic, where
>> we may queue HW interrupts with the Pending+Active state, which is
>> illegal.
>>
>> The final patch fixes a misinterpretation of the spec, where we
>> compute the number of APxRn register based on the number of priorities
>> instead of using the number of preemption levels.
>>
>> I've tagged the first 4 patches for stable, given that we're doing
>> something potentially harmful. The last patch is more of a theoretical
>> issue at this stage, so probably need for a backport.
> 
> Would you like me to apply them, or are you looking for reviews and
> going to send them in a pull request?
> 
> I can wait a couple days before sending my own pull request to Linus.

Christoffer is in charge of the tree at the moment, so I'll leave it up
to him to decide. But my guess is that he will send a PR some time
later, with the rest of the fixes that have been posted lately.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Christoffer Dall]

On Tue, May 02, 2017 at 04:00:49PM +0100, Marc Zyngier wrote:
> On 02/05/17 15:44, Paolo Bonzini wrote:
> > 
> > 
> > On 02/05/2017 15:30, Marc Zyngier wrote:
> >> Here's a handful of random fixes I've queued locally that didn't have
> >> a chance to make it in 4.11.
> >>
> >> The first two patches avoid stack-protector messing with the HYP code,
> >> as this ends up being a complete disaster.
> >>
> >> The following two patches fix a bug introduced in the new vgic, where
> >> we may queue HW interrupts with the Pending+Active state, which is
> >> illegal.
> >>
> >> The final patch fixes a misinterpretation of the spec, where we
> >> compute the number of APxRn register based on the number of priorities
> >> instead of using the number of preemption levels.
> >>
> >> I've tagged the first 4 patches for stable, given that we're doing
> >> something potentially harmful. The last patch is more of a theoretical
> >> issue at this stage, so probably need for a backport.
> > 
> > Would you like me to apply them, or are you looking for reviews and
> > going to send them in a pull request?
> > 
> > I can wait a couple days before sending my own pull request to Linus.
> 
> Christoffer is in charge of the tree at the moment, so I'll leave it up
> to him to decide. But my guess is that he will send a PR some time
> later, with the rest of the fixes that have been posted lately.
> 

Yes, I have some other fixes that I'll send together with these as soon
as -rc1 hits.  And I plan on reviewing these.

Thanks,
-Christoffer

[PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Christoffer Dall]

On Tue, May 02, 2017 at 04:00:49PM +0100, Marc Zyngier wrote:
> On 02/05/17 15:44, Paolo Bonzini wrote:
> > 
> > 
> > On 02/05/2017 15:30, Marc Zyngier wrote:
> >> Here's a handful of random fixes I've queued locally that didn't have
> >> a chance to make it in 4.11.
> >>
> >> The first two patches avoid stack-protector messing with the HYP code,
> >> as this ends up being a complete disaster.
> >>
> >> The following two patches fix a bug introduced in the new vgic, where
> >> we may queue HW interrupts with the Pending+Active state, which is
> >> illegal.
> >>
> >> The final patch fixes a misinterpretation of the spec, where we
> >> compute the number of APxRn register based on the number of priorities
> >> instead of using the number of preemption levels.
> >>
> >> I've tagged the first 4 patches for stable, given that we're doing
> >> something potentially harmful. The last patch is more of a theoretical
> >> issue at this stage, so probably need for a backport.
> > 
> > Would you like me to apply them, or are you looking for reviews and
> > going to send them in a pull request?
> > 
> > I can wait a couple days before sending my own pull request to Linus.
> 
> Christoffer is in charge of the tree at the moment, so I'll leave it up
> to him to decide. But my guess is that he will send a PR some time
> later, with the rest of the fixes that have been posted lately.
> 

Yes, I have some other fixes that I'll send together with these as soon
as -rc1 hits.  And I plan on reviewing these.

Thanks,
-Christoffer

Re: [PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Christoffer Dall]

Hi Marc,

On Tue, May 02, 2017 at 02:30:36PM +0100, Marc Zyngier wrote:
> Here's a handful of random fixes I've queued locally that didn't have
> a chance to make it in 4.11.
> 
> The first two patches avoid stack-protector messing with the HYP code,
> as this ends up being a complete disaster.
> 
> The following two patches fix a bug introduced in the new vgic, where
> we may queue HW interrupts with the Pending+Active state, which is
> illegal.
> 
> The final patch fixes a misinterpretation of the spec, where we
> compute the number of APxRn register based on the number of priorities
> instead of using the number of preemption levels.
> 
> I've tagged the first 4 patches for stable, given that we're doing
> something potentially harmful. The last patch is more of a theoretical
> issue at this stage, so probably need for a backport.

I've applied these to kvmarm/master including the initial patches to
disable the stack-protecter for EL2/HYP.

Once you figure out what to do with -fpic/-fpie/-mcmodel=small we can
deal with that as follow-up patches.

Thanks,
-Christoffer

[PATCH 0/5] KVM/ARM: Fixes for 4.12-rc1

[Christoffer Dall]

Hi Marc,

On Tue, May 02, 2017 at 02:30:36PM +0100, Marc Zyngier wrote:
> Here's a handful of random fixes I've queued locally that didn't have
> a chance to make it in 4.11.
> 
> The first two patches avoid stack-protector messing with the HYP code,
> as this ends up being a complete disaster.
> 
> The following two patches fix a bug introduced in the new vgic, where
> we may queue HW interrupts with the Pending+Active state, which is
> illegal.
> 
> The final patch fixes a misinterpretation of the spec, where we
> compute the number of APxRn register based on the number of priorities
> instead of using the number of preemption levels.
> 
> I've tagged the first 4 patches for stable, given that we're doing
> something potentially harmful. The last patch is more of a theoretical
> issue at this stage, so probably need for a backport.

I've applied these to kvmarm/master including the initial patches to
disable the stack-protecter for EL2/HYP.

Once you figure out what to do with -fpic/-fpie/-mcmodel=small we can
deal with that as follow-up patches.

Thanks,
-Christoffer