On Thu, Jun 01, 2017 at 11:20:57AM +0100, Marc Zyngier wrote:
> Add a handler for reading/writing the guest's view of the ICC_BPR1_EL1
> register, which is located in the ICH_VMCR_EL2.BPR1 field.
>
> Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx>
> Signed-off-by: Marc Zyngier <marc.zyngier@xxxxxxx>
> ---
> virt/kvm/arm/hyp/vgic-v3-sr.c | 51 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/virt/kvm/arm/hyp/vgic-v3-sr.c b/virt/kvm/arm/hyp/vgic-v3-sr.c
> index 943bf11252d9..6254eaf72a77 100644
> --- a/virt/kvm/arm/hyp/vgic-v3-sr.c
> +++ b/virt/kvm/arm/hyp/vgic-v3-sr.c
> @@ -375,6 +375,51 @@ void __hyp_text __vgic_v3_write_vmcr(u32 vmcr)
>
> #ifdef CONFIG_ARM64
>
> +static unsigned int __hyp_text __vgic_v3_get_bpr0(u32 vmcr)
> +{
> + return (vmcr & ICH_VMCR_BPR0_MASK) >> ICH_VMCR_BPR0_SHIFT;
> +}
> +
> +static unsigned int __hyp_text __vgic_v3_get_bpr1(u32 vmcr)
> +{
> + unsigned int bpr;
> +
> + if (vmcr & ICH_VMCR_CBPR_MASK) {
> + bpr = __vgic_v3_get_bpr0(vmcr);
> + if (bpr < 7)
> + bpr++;
> + } else {
> + bpr = (vmcr & ICH_VMCR_BPR1_MASK) >> ICH_VMCR_BPR1_SHIFT;
> + }
> +
> + return bpr;
> +}
> +
> +static void __hyp_text __vgic_v3_read_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
> +{
> + vcpu_set_reg(vcpu, rt, __vgic_v3_get_bpr1(vmcr));
> +}
> +
> +static void __hyp_text __vgic_v3_write_bpr1(struct kvm_vcpu *vcpu, u32 vmcr, int rt)
> +{
> + u64 val = vcpu_get_reg(vcpu, rt);
> + u8 bpr_min = 8 - vtr_to_nr_pre_bits(read_gicreg(ICH_VTR_EL2));
I can't seem to find where this behavior is documented, is it that 8 is
the theoretical max, and it's the upper preemption levels that apply, so
it must be 8 - number supported?
> +
> + if (vmcr & ICH_VMCR_CBPR_MASK)
> + return;
> +
> + /* Enforce BPR limiting */
> + if (val < bpr_min)
> + val = bpr_min;
Are we not implying that the reset value here also means bpr_min? I
also can't seem to find this behavior in the spec and it appears we rely
on the underlying hardware to set a reset value (by setting vmcr=0 on
first run). Could this result in a guest observing one reset value,
writing 0 to this register, and observing another one?
> +
> + val <<= ICH_VMCR_BPR1_SHIFT;
> + val &= ICH_VMCR_BPR1_MASK;
> + vmcr &= ~ICH_VMCR_BPR1_MASK;
> + vmcr |= val;
> +
> + __vgic_v3_write_vmcr(vmcr);
> +}
> +
> int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
> {
> int rt;
> @@ -397,6 +442,12 @@ int __hyp_text __vgic_v3_perform_cpuif_access(struct kvm_vcpu *vcpu)
> is_read = (esr & ESR_ELx_SYS64_ISS_DIR_MASK) == ESR_ELx_SYS64_ISS_DIR_READ;
>
> switch (sysreg) {
> + case SYS_ICC_BPR1_EL1:
> + if (is_read)
> + fn = __vgic_v3_read_bpr1;
> + else
> + fn = __vgic_v3_write_bpr1;
> + break;
Did you consider a lookup table with the sysreg encoding, the read
function, and the right function as each entry? It may compress the
code a bit, but I'm not sure if it's nicer or worth it.
> default:
> return 0;
> }
> --
> 2.11.0
>
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
