Re: [PATCH v11 6/6] target-arm: kvm64: Handle SError interrupt for the guest OS

From: Peter Maydell
Date: Tue Sep 05 2017 - 13:51:03 EST


On 18 August 2017 at 15:23, Dongjiu Geng <gengdongjiu@xxxxxxxxxx> wrote:
> When guest OS happens SError interrupt(SEI), it will trap to host.
> Host firstly calls memory failure to deal with this error and decide
> whether it needs to deliver SIGBUS signal to userspace. The advantage
> that using signal to notify is that it can make the notification method
> is general, non-KVM user can also use it. when userspace gets this
> signal and knows this is SError interrupt, it will translate the
> delivered host VA to PA and record this PA to GHES.
>
> Because ARMv8.2 adds an extension to RAS to allow system software insert
> implicit Error Synchronization Barrier operations to isolate the error and
> allow passes specified syndrome to guest OS, so after record the CPER, user
> space calls IOCTL to pass a specified syndrome to KVM, then switch to guest
> OS, guest OS can use the recorded CPER record and syndrome information to
> do the recovery.
>
> The steps are shown below:
> 1. translate the host VA to guest OS PA and record this error PA to HEST table.
> 2. set specified virtual SError syndrome and pass the value to KVM.
>
> Signed-off-by: Dongjiu Geng <gengdongjiu@xxxxxxxxxx>
> Signed-off-by: Quanming Wu <wuquanming@xxxxxxxxxx>
> ---
> linux-headers/linux/kvm.h | 1 +
> target/arm/internals.h | 1 +
> target/arm/kvm64.c | 28 ++++++++++++++++++++++++++++
> 3 files changed, 30 insertions(+)
>
> diff --git a/linux-headers/linux/kvm.h b/linux-headers/linux/kvm.h
> index 2aa176e..10dfcab 100644
> --- a/linux-headers/linux/kvm.h
> +++ b/linux-headers/linux/kvm.h
> @@ -1356,6 +1356,7 @@ struct kvm_s390_ucas_mapping {
> /* Available with KVM_CAP_S390_CMMA_MIGRATION */
> #define KVM_S390_GET_CMMA_BITS _IOWR(KVMIO, 0xb8, struct kvm_s390_cmma_log)
> #define KVM_S390_SET_CMMA_BITS _IOW(KVMIO, 0xb9, struct kvm_s390_cmma_log)
> +#define KVM_ARM_SEI _IO(KVMIO, 0xb10)
>
> #define KVM_DEV_ASSIGN_ENABLE_IOMMU (1 << 0)
> #define KVM_DEV_ASSIGN_PCI_2_3 (1 << 1)
> diff --git a/target/arm/internals.h b/target/arm/internals.h
> index fc0ad6d..18b1cbc 100644
> --- a/target/arm/internals.h
> +++ b/target/arm/internals.h
> @@ -237,6 +237,7 @@ enum arm_exception_class {
> #define ARM_EL_ISV (1 << ARM_EL_ISV_SHIFT)
> #define ARM_EL_EC_MASK ((0x3F) << ARM_EL_EC_SHIFT)
> #define ARM_EL_FSC_TYPE (0x3C)
> +#define ARM_EL_ISS_MASK ((1 << ARM_EL_IL_SHIFT) - 1)
>
> #define FSC_SEA (0x10)
> #define FSC_SEA_TTW0 (0x14)
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index d3bdab2..b84cb49 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -616,6 +616,22 @@ static int kvm_arm_cpreg_value(ARMCPU *cpu, ptrdiff_t fieldoffset)
> return -EINVAL;
> }
>
> +static int kvm_inject_arm_sei(CPUState *cs)
> +{
> + ARMCPU *cpu = ARM_CPU(cs);
> + CPUARMState *env = &cpu->env;
> +
> + unsigned long syndrome = env->exception.vaddress;
> + /* set virtual SError syndrome */
> + if (arm_feature(env, ARM_FEATURE_RAS_EXTENSION)) {
> + syndrome = syndrome & ARM_EL_ISS_MASK;
> + } else {
> + syndrome = 0;
> + }
> +
> + return kvm_vcpu_ioctl(CPU(cpu), KVM_ARM_SEI, &syndrome);

This looks odd. If we don't have the RAS extension why do
we need to do anything at all here ?

> +}
> +
> /* Inject synchronous external abort */
> static int kvm_inject_arm_sea(CPUState *c)
> {
> @@ -1007,6 +1023,15 @@ static bool is_abort_sea(unsigned long syndrome)
> }
> }
>
> +static bool is_abort_sei(unsigned long syndrome)
> +{
> + uint8_t ec = ((syndrome & ARM_EL_EC_MASK) >> ARM_EL_EC_SHIFT);

You don't need to bother masking here -- in other places in
QEMU we assume that the EC field is at the top of the word,
so just "syndrome >> ARM_EL_EC_SHIFT" is sufficient.

> + if ((ec != EC_SERROR))
> + return false;
> + else
> + return true;

scripts/checkpatch.pl should tell you that this if needs braces
(it's good to get in the habit of running it on all patches; it
is not always correct, so judgement is required, but it will flag
up some common mistakes).

In this particular case, you should just
return ec == EC_SERROR;
though.

> +}
> +
> void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> {
> ram_addr_t ram_addr;
> @@ -1024,6 +1049,9 @@ void kvm_arch_on_sigbus_vcpu(CPUState *c, int code, void *addr)
> if (is_abort_sea(env->exception.syndrome)) {
> ghes_update_guest(ACPI_HEST_NOTIFY_SEA, paddr);
> kvm_inject_arm_sea(c);
> + } else if (is_abort_sei(env->exception.syndrome)) {
> + ghes_update_guest(ACPI_HEST_NOTIFY_SEI, paddr);
> + kvm_inject_arm_sei(c);
> }
> return;
> }
> --
> 1.8.3.1

thanks
-- PMM