Re: [Qemu-arm] [PATCH v2 0/5] target/arm: KVM vs ARMISARegisters

[Mark Rutland]

On Sun, Nov 04, 2018 at 11:25:00AM +0000, Marc Zyngier wrote:
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index 22fbbdbece3c..d50f912d3f4a 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -314,10 +314,15 @@ static bool trap_raz_wi(struct kvm_vcpu *vcpu,
>               return read_zero(vcpu, p);
>  }
>  
> -static bool trap_undef(struct kvm_vcpu *vcpu,
> -                    struct sys_reg_params *p,
> -                    const struct sys_reg_desc *r)

We should probably have a comment here, like:

/*
 * ARMv8.1 mandates at least a trivial LORegion implementation, where
 * all the RW registers are RES0 (which we can implement as RAZ/WI). On
 * an ARMv8.0 system, these registers should UNDEF.
 */

> +static bool trap_loregion(struct kvm_vcpu *vcpu,
> +                       struct sys_reg_params *p,
> +                       const struct sys_reg_desc *r)
>  {
> +     u64 val = read_sanitised_ftr_reg(SYS_ID_AA64MMFR1_EL1);
> +
> +     if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
> +             return trap_raz_wi(vcpu, p, r);
> +
>       kvm_inject_undefined(vcpu);
>       return false;
>  }
> @@ -1040,11 +1045,6 @@ static u64 read_id_reg(struct sys_reg_desc const *r, 
> bool raz)
>                       kvm_debug("SVE unsupported for guests, suppressing\n");
>  
>               val &= ~(0xfUL << ID_AA64PFR0_SVE_SHIFT);
> -     } else if (id == SYS_ID_AA64MMFR1_EL1) {
> -             if (val & (0xfUL << ID_AA64MMFR1_LOR_SHIFT))
> -                     kvm_debug("LORegions unsupported for guests, 
> suppressing\n");
> -
> -             val &= ~(0xfUL << ID_AA64MMFR1_LOR_SHIFT);
>       }
>  
>       return val;
> @@ -1330,11 +1330,11 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>       { SYS_DESC(SYS_MAIR_EL1), access_vm_reg, reset_unknown, MAIR_EL1 },
>       { SYS_DESC(SYS_AMAIR_EL1), access_vm_reg, reset_amair_el1, AMAIR_EL1 },
>  
> -     { SYS_DESC(SYS_LORSA_EL1), trap_undef },
> -     { SYS_DESC(SYS_LOREA_EL1), trap_undef },
> -     { SYS_DESC(SYS_LORN_EL1), trap_undef },
> -     { SYS_DESC(SYS_LORC_EL1), trap_undef },
> -     { SYS_DESC(SYS_LORID_EL1), trap_undef },
> +     { SYS_DESC(SYS_LORSA_EL1), trap_loregion },
> +     { SYS_DESC(SYS_LOREA_EL1), trap_loregion },
> +     { SYS_DESC(SYS_LORN_EL1), trap_loregion },
> +     { SYS_DESC(SYS_LORC_EL1), trap_loregion },
> +     { SYS_DESC(SYS_LORID_EL1), trap_loregion },

LORID_EL1 is always RO, so we need to UNDEF on writes. Either we add a
separate helper, or special-case LORID_EL1 in trap_loregion().

Otherwise, this looks good to me.

Thanks,
Mark.