[PATCH 5/6] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

[marc.zyngier@arm.com (Marc Zyngier)]

On Sun, 18 Sep 2016 12:00:01 +0530
Vijay Kilari <vijay.kilari@gmail.com> wrote:

> Hi Marc,
> 
> On Fri, Sep 16, 2016 at 10:37 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > On 16/09/16 17:57, Vijay Kilari wrote:  
> >> On Fri, Sep 16, 2016 at 8:06 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:  
> >>> On 16/09/16 13:20, vijay.kilari at gmail.com wrote:  
> >>>> From: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >>>>
> >>>> VGICv3 CPU interface registers are accessed using
> >>>> KVM_DEV_ARM_VGIC_CPU_SYSREGS ioctl. These registers are accessed
> >>>> as 64-bit. The cpu MPIDR value is passed along with register id.
> >>>> is used to identify the cpu for registers access.
> >>>>
> >>>> The version of VGIC v3 specification is define here
> >>>> http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/445611.html
> >>>>
> >>>> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> >>>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@cavium.com>
> >>>> ---
> >>>>  arch/arm64/include/uapi/asm/kvm.h   |   3 +
> >>>>  arch/arm64/kvm/Makefile             |   1 +
> >>>>  include/linux/irqchip/arm-gic-v3.h  |  30 ++++
> >>>>  virt/kvm/arm/vgic/vgic-kvm-device.c |  27 ++++
> >>>>  virt/kvm/arm/vgic/vgic-mmio-v3.c    |  18 +++
> >>>>  virt/kvm/arm/vgic/vgic-sys-reg-v3.c | 296 ++++++++++++++++++++++++++++++++++++
> >>>>  virt/kvm/arm/vgic/vgic.h            |  10 ++
> >>>>  7 files changed, 385 insertions(+)  
> >
> > [...]
> >  
> >>>> diff --git a/virt/kvm/arm/vgic/vgic-sys-reg-v3.c b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> >>>> new file mode 100644
> >>>> index 0000000..8e4f403
> >>>> --- /dev/null
> >>>> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> >>>> @@ -0,0 +1,296 @@
> >>>> +#include <linux/irqchip/arm-gic-v3.h>
> >>>> +#include <linux/kvm.h>
> >>>> +#include <linux/kvm_host.h>
> >>>> +#include <kvm/iodev.h>
> >>>> +#include <kvm/arm_vgic.h>
> >>>> +#include <asm/kvm_emulate.h>
> >>>> +#include <asm/kvm_arm.h>
> >>>> +#include <asm/kvm_mmu.h>
> >>>> +
> >>>> +#include "vgic.h"
> >>>> +#include "vgic-mmio.h"
> >>>> +#include "sys_regs.h"
> >>>> +
> >>>> +static bool access_gic_ctlr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>>> +                         const struct sys_reg_desc *r)
> >>>> +{
> >>>> +     struct vgic_vmcr vmcr;
> >>>> +     u64 val;
> >>>> +     u32 ich_vtr;
> >>>> +
> >>>> +     vgic_get_vmcr(vcpu, &vmcr);
> >>>> +     if (p->is_write) {
> >>>> +             val = p->regval;
> >>>> +             vmcr.ctlr &= ~(ICH_VMCR_CBPR_MASK | ICH_VMCR_EOIM_MASK);
> >>>> +             vmcr.ctlr |= ((val & ICC_CTLR_EL1_CBPR_MASK) >>
> >>>> +                           ICC_CTLR_EL1_CBPR_SHIFT) << ICH_VMCR_CBPR_SHIFT;
> >>>> +             vmcr.ctlr |= ((val & ICC_CTLR_EL1_EOImode_MASK) >>
> >>>> +                          ICC_CTLR_EL1_EOImode_SHIFT) << ICH_VMCR_EOIM_SHIFT;
> >>>> +             vgic_set_vmcr(vcpu, &vmcr);  
> >>>
> >>> You've ignored my comments again: "What if userspace writes something
> >>> that is incompatible with the current configuration? Wrong number of ID
> >>> bits, or number of priorities?"  
> >>
> >> IMO, In case of incompatibility,
> >> If ID bits and PRI bits are less than HW supported, it is ok.  
> >
> > Yes. But you also need to track of what the guest has programmed in
> > order to be able to migrate it back to its original configuration.
> >  
> >> If ID bits and PRI bits are greater than HW supported, then warn would be good
> >> enough. Please suggest the behaviour that you think it should be.  
> >
> > No, it is an error, plain and simple. You cannot run in this condition.
> >  
> >>  
> >>>  
> >>>> +     } else {
> >>>> +             ich_vtr = kvm_call_hyp(__vgic_v3_get_ich_vtr_el2);
> >>>> +
> >>>> +             val = 0;
> >>>> +             val |= ((ich_vtr & ICH_VTR_PRI_BITS_MASK) >>
> >>>> +                     ICH_VTR_PRI_BITS_SHIFT) << ICC_CTLR_EL1_PRI_BITS_SHIFT;
> >>>> +             val |= ((ich_vtr & ICH_VTR_ID_BITS_MASK) >>
> >>>> +                     ICH_VTR_ID_BITS_SHIFT) << ICC_CTLR_EL1_ID_BITS_SHIFT;
> >>>> +             val |= ((ich_vtr & ICH_VTR_SEIS_MASK) >> ICH_VTR_SEIS_SHIFT)
> >>>> +                     << ICC_CTLR_EL1_SEIS_SHIFT;
> >>>> +             val |= ((ich_vtr & ICH_VTR_A3V_MASK) >> ICH_VTR_A3V_SHIFT)
> >>>> +                     << ICC_CTLR_EL1_A3V_SHIFT;
> >>>> +             val |= ((vmcr.ctlr & ICH_VMCR_CBPR_MASK) >>
> >>>> +                     ICH_VMCR_CBPR_SHIFT) << ICC_CTLR_EL1_CBPR_SHIFT;
> >>>> +             val |= ((vmcr.ctlr & ICH_VMCR_EOIM_MASK) >>
> >>>> +                     ICH_VMCR_EOIM_SHIFT) << ICC_CTLR_EL1_EOImode_SHIFT;
> >>>> +
> >>>> +             p->regval = val;
> >>>> +     }
> >>>> +
> >>>> +     return true;
> >>>> +}
> >>>> +
> >>>> +static bool access_gic_pmr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>>> +                        const struct sys_reg_desc *r)
> >>>> +{
> >>>> +     struct vgic_vmcr vmcr;
> >>>> +
> >>>> +     vgic_get_vmcr(vcpu, &vmcr);
> >>>> +     if (p->is_write) {
> >>>> +             vmcr.pmr = (p->regval & ICC_PMR_EL1_MASK) >> ICC_PMR_EL1_SHIFT;
> >>>> +             vgic_set_vmcr(vcpu, &vmcr);
> >>>> +     } else {
> >>>> +             p->regval = (vmcr.pmr << ICC_PMR_EL1_SHIFT) & ICC_PMR_EL1_MASK;
> >>>> +     }
> >>>> +
> >>>> +     return true;
> >>>> +}
> >>>> +
> >>>> +static bool access_gic_bpr0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>>> +                         const struct sys_reg_desc *r)
> >>>> +{
> >>>> +     struct vgic_vmcr vmcr;
> >>>> +
> >>>> +     vgic_get_vmcr(vcpu, &vmcr);
> >>>> +     if (p->is_write) {
> >>>> +             vmcr.bpr = (p->regval & ICC_BPR0_EL1_MASK) >>
> >>>> +                         ICC_BPR0_EL1_SHIFT;
> >>>> +             vgic_set_vmcr(vcpu, &vmcr);
> >>>> +     } else {
> >>>> +             p->regval = (vmcr.bpr << ICC_BPR0_EL1_SHIFT) &
> >>>> +                          ICC_BPR0_EL1_MASK;
> >>>> +     }
> >>>> +
> >>>> +     return true;
> >>>> +}
> >>>> +
> >>>> +static bool access_gic_bpr1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>>> +                         const struct sys_reg_desc *r)
> >>>> +{
> >>>> +     struct vgic_vmcr vmcr;
> >>>> +
> >>>> +     vgic_get_vmcr(vcpu, &vmcr);
> >>>> +     if (p->is_write) {
> >>>> +             vmcr.abpr = (p->regval & ICC_BPR1_EL1_MASK) >>
> >>>> +                          ICC_BPR1_EL1_SHIFT;
> >>>> +             vgic_set_vmcr(vcpu, &vmcr);
> >>>> +     } else {
> >>>> +             p->regval = (vmcr.abpr << ICC_BPR1_EL1_SHIFT) &
> >>>> +                          ICC_BPR1_EL1_MASK;
> >>>> +     }
> >>>> +
> >>>> +     return true;
> >>>> +}
> >>>> +
> >>>> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>>> +                           const struct sys_reg_desc *r)
> >>>> +{
> >>>> +     struct vgic_vmcr vmcr;
> >>>> +
> >>>> +     vgic_get_vmcr(vcpu, &vmcr);
> >>>> +     if (p->is_write) {
> >>>> +             vmcr.grpen0 = (p->regval & ICC_IGRPEN0_EL1_MASK) >>
> >>>> +                                   ICC_IGRPEN0_EL1_SHIFT;
> >>>> +             vgic_set_vmcr(vcpu, &vmcr);
> >>>> +     } else {
> >>>> +             p->regval = (vmcr.grpen0 << ICC_IGRPEN0_EL1_SHIFT) &
> >>>> +                          ICC_IGRPEN0_EL1_MASK;
> >>>> +     }
> >>>> +
> >>>> +     return true;
> >>>> +}
> >>>> +
> >>>> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> >>>> +                           const struct sys_reg_desc *r)
> >>>> +{
> >>>> +     struct vgic_vmcr vmcr;
> >>>> +
> >>>> +     vgic_get_vmcr(vcpu, &vmcr);
> >>>> +     if (p->is_write) {
> >>>> +             vmcr.grpen1 = (p->regval & ICC_IGRPEN1_EL1_MASK) >>
> >>>> +                                   ICC_IGRPEN1_EL1_SHIFT;
> >>>> +             vgic_set_vmcr(vcpu, &vmcr);
> >>>> +     } else {
> >>>> +             p->regval = (vmcr.grpen1 << ICC_IGRPEN1_EL1_SHIFT) &
> >>>> +                          ICC_IGRPEN1_EL1_MASK;  
> >>>
> >>> From the previous review comments: "Shouldn't this account for the
> >>> ICC_CTLR_EL1.CBPR setting?"  
> >>
> >>  Ok. I think this comment is for ICC_BPR1_EL1 access.  
> >
> > Yes, sorry about the misplaced comment.
> >  
> >> I will make a check on ICC_CTLR.EL1.CBPR for accessing ICC_BPR1_EL1.  
> >
> > The reverse is also true: you also need to account the value of
> > ICC_BPR1_EL1 when accessing ICC_CTLR_EL1.
> >  
> 
>   ICC_CTLR_EL1 reg does not hold BPR1 value to account value of ICC_BPR1_EL1.

Oh please. What do you think ICC_CTLR_EL1.CPBR is for? It ties
ICC_BPR1_EL1 to ICC_BPR0_EL1. So if you have the following sequence:

	Set ICC_BPR0_EL1 to p0
	Set ICC_BPR1_EL1 to p1
	Set ICC_CTLR_EL1.CBPR to 1

you must ensure that ICC_BPR1_EL1 is now min(p0 + 1, 7), no matter what
p1 is. ICC_CTLR_EL1 may not hold the BPR1 value, but it directly
controls it.

Effectively, you must always consider BPR0, BPR1 and CBPR together.
Please read the architecture spec, it is one of the few things that are
easy actually to understand.

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