diff mbox series

KVM: arm64: Add missing ERX*_EL1 registers

Message ID 20240110075739.8291-1-tianruidong@linux.alibaba.com
State New
Headers
Series KVM: arm64: Add missing ERX*_EL1 registers |

Commit Message

Ruidong Tian Jan. 10, 2024, 7:57 a.m. UTC 
  Commit 464f2164da7e ("arm64: Add missing ERX*_EL1 encodings") add some
new RAS registers. Trap them to kvm.

Signed-off-by: Ruidong Tian <tianruidong@linux.alibaba.com>
---
 arch/arm64/kvm/sys_regs.c | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Marc Zyngier Jan. 10, 2024, 12:20 p.m. UTC | #1 
On Wed, 10 Jan 2024 07:57:39 +0000,
Ruidong Tian <tianruidong@linux.alibaba.com> wrote:
> 
> Commit 464f2164da7e ("arm64: Add missing ERX*_EL1 encodings") add some
> new RAS registers. Trap them to kvm.

Well, they *are* already trapped by virtue of HCR_EL2.FIEN being
0. They are lacking a trap handler though.

> 
> Signed-off-by: Ruidong Tian <tianruidong@linux.alibaba.com>
> ---
>  arch/arm64/kvm/sys_regs.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 30253bd19917..76a9ba155d58 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -2389,8 +2389,13 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	{ SYS_DESC(SYS_ERXCTLR_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_ERXSTATUS_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
> +	{ SYS_DESC(SYS_ERXPFGF_EL1), trap_raz_wi },
> +	{ SYS_DESC(SYS_ERXPFGCTL_EL1), trap_raz_wi },
> +	{ SYS_DESC(SYS_ERXPFGCDN_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
>  	{ SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
> +	{ SYS_DESC(SYS_ERXMISC2_EL1), trap_raz_wi },
> +	{ SYS_DESC(SYS_ERXMISC3_EL1), trap_raz_wi },
>  
>  	MTE_REG(TFSR_EL1),
>  	MTE_REG(TFSRE0_EL1),

If my reading of the ARM ARM is correct, these registers only exist if
FEAT_RASv1p1 is implemented. Which means that we shouldn't handle
those as RAZ/WI unconditionally, but instead check for what we
advertise to the guest and handle it accordingly.

Thanks,

	M.
Oliver Upton Jan. 15, 2024, 2:47 p.m. UTC | #2 
On Wed, Jan 10, 2024 at 12:20:30PM +0000, Marc Zyngier wrote:
> If my reading of the ARM ARM is correct, these registers only exist if
> FEAT_RASv1p1 is implemented. Which means that we shouldn't handle
> those as RAZ/WI unconditionally, but instead check for what we
> advertise to the guest and handle it accordingly.

Can we go a step further and just stop advertising RAS to guests? I don't
expect VMs to gain much from our RAZ/WI implementation. Conditional
RAZ/WI would still be helpful in this case for migrated VMs that have
'seen' the feature.
James Morse Jan. 15, 2024, 5:21 p.m. UTC | #3 
Hi Oliver,

On 15/01/2024 14:47, Oliver Upton wrote:
> On Wed, Jan 10, 2024 at 12:20:30PM +0000, Marc Zyngier wrote:
>> If my reading of the ARM ARM is correct, these registers only exist if
>> FEAT_RASv1p1 is implemented. Which means that we shouldn't handle
>> those as RAZ/WI unconditionally, but instead check for what we
>> advertise to the guest and handle it accordingly.
> 
> Can we go a step further and just stop advertising RAS to guests? I don't
> expect VMs to gain much from our RAZ/WI implementation.

These CPU registers would describe the error in a kernel-first setup, but firmware-first
has its own in-memory way of doing that.

The CPU features indicates the IESB feature and ESB-instruction exist to fence errors, and
that the CPU uses the ESR_ELx.{S,A}ET bits to describe the CPU state after an error. These
are all useful as part of the notification of an error, be that kernel-first or
firmware-first.

When its supported by the hardware, the VMM can inject an asynchronous external abort
using KVM_GET_VCPU_EVENTS - otherwise the ESR_ELx.ISS bits are all imp-def, meaning all
errors are catastrophic.

Doing this would skip save/restore of VDISR_EL2, is there any other reason to do it?


> Conditional
> RAZ/WI would still be helpful in this case for migrated VMs that have
> 'seen' the feature.


Thanks,

James
Oliver Upton Jan. 16, 2024, 3:38 p.m. UTC | #4 
Hi James,

On Mon, Jan 15, 2024 at 05:21:19PM +0000, James Morse wrote:
> Hi Oliver,
> 
> On 15/01/2024 14:47, Oliver Upton wrote:
> > On Wed, Jan 10, 2024 at 12:20:30PM +0000, Marc Zyngier wrote:
> >> If my reading of the ARM ARM is correct, these registers only exist if
> >> FEAT_RASv1p1 is implemented. Which means that we shouldn't handle
> >> those as RAZ/WI unconditionally, but instead check for what we
> >> advertise to the guest and handle it accordingly.
> > 
> > Can we go a step further and just stop advertising RAS to guests? I don't
> > expect VMs to gain much from our RAZ/WI implementation.
> 
> These CPU registers would describe the error in a kernel-first setup, but firmware-first
> has its own in-memory way of doing that.
> 
> The CPU features indicates the IESB feature and ESB-instruction exist to fence errors, and
> that the CPU uses the ESR_ELx.{S,A}ET bits to describe the CPU state after an error. These
> are all useful as part of the notification of an error, be that kernel-first or
> firmware-first.
> 
> When its supported by the hardware, the VMM can inject an asynchronous external abort
> using KVM_GET_VCPU_EVENTS - otherwise the ESR_ELx.ISS bits are all imp-def, meaning all
> errors are catastrophic.
> 
> Doing this would skip save/restore of VDISR_EL2, is there any other reason to do it?

Forgive me, had the blinders on and was thinking only of the error
record interface, not ESB/DISR. In that context it makes a lot less
sense to hide RAS from guests, especially if the guest depends on ESB
being a NOP if the hardware doesn't support it.
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index 30253bd19917..76a9ba155d58 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -2389,8 +2389,13 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_ERXCTLR_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_ERXSTATUS_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_ERXADDR_EL1), trap_raz_wi },
+	{ SYS_DESC(SYS_ERXPFGF_EL1), trap_raz_wi },
+	{ SYS_DESC(SYS_ERXPFGCTL_EL1), trap_raz_wi },
+	{ SYS_DESC(SYS_ERXPFGCDN_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_ERXMISC0_EL1), trap_raz_wi },
 	{ SYS_DESC(SYS_ERXMISC1_EL1), trap_raz_wi },
+	{ SYS_DESC(SYS_ERXMISC2_EL1), trap_raz_wi },
+	{ SYS_DESC(SYS_ERXMISC3_EL1), trap_raz_wi },
 
 	MTE_REG(TFSR_EL1),
 	MTE_REG(TFSRE0_EL1),