Re: [RFC PATCH v3 4/5] arm/arm64: vgic-new: Implement VGICv3 CPU interface access

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Tue, Aug 30, 2016 at 7:15 PM, Christoffer Dall <christoffer.dall@xxxxxxxxxx> wrote:

On Wed, Aug 24, 2016 at 04:50:08PM +0530, vijay.kilari@xxxxxxxxx wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxx>

 

> 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..581d053
> --- /dev/null
> +++ b/virt/kvm/arm/vgic/vgic-sys-reg-v3.c
> @@ -0,0 +1,211 @@
> +#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;
> +
> +     vgic_get_vmcr(vcpu, &vmcr);
> +     if (p->is_write) {
> +             vmcr.ctlr = (u32)p->regval;
> +             vgic_set_vmcr(vcpu, &vmcr);
> +     } else {
> +             p->regval = vmcr.ctlr;
> +     }
> +

really?  Have you looked at the spec and implementation of this or did
you just copy the v2 code?

The ICH_VMCR_EL2 register field mappings are not identical to the ctlr
mappings.  I think this causes some rework for much of this patch, so
I'll have a look at the next revision.

  ICH_VMCR_EL2.VEOIM &  ICH_VMCR_EL2.CBPR holds

ICC_CTLR_EL1.EOImode & ICC_CTLR_EL1.CBPR  respectively.

Remaining fields are Readonly except PHME. AFAIK, vgic is not

holding ICC_CTLR_EL1 except the fields in ICH_VMCR_EL2.

> +     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 = (u32)p->regval;
> +             vgic_set_vmcr(vcpu, &vmcr);
> +     } else {
> +             p->regval = vmcr.pmr;
> +     }
> +
> +     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 = (u32)p->regval;
> +             vgic_set_vmcr(vcpu, &vmcr);
> +     } else {
> +             p->regval = vmcr.bpr;
> +     }
> +
> +     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 = (u32)p->regval;
> +             vgic_set_vmcr(vcpu, &vmcr);
> +     } else {
> +             p->regval = vmcr.abpr;
> +     }
> +
> +     return true;
> +}
> +
> +static bool access_gic_grpen0(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> +                           const struct sys_reg_desc *r)
> +{
> +     struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +     if (p->is_write) {
> +             vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG0;
> +             vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG0_SHIFT) &
> +                                   ICH_VMCR_ENG0;
> +     } else {
> +             p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG0) >>
> +                          ICH_VMCR_ENG0_SHIFT;
> +     }

so for example, why shouldn't these go through the vgic_set/get_vmcr
wrappers?

The vgic_vmcr struct as below is not managing ENG0 and ENG1 fields.

So could not use vgic_set/get wrappers.

struct vgic_vmcr {
        u32     ctlr;
        u32     abpr;
        u32     bpr;
        u32     pmr;
};

 

> +
> +     return true;
> +}
> +
> +static bool access_gic_grpen1(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> +                           const struct sys_reg_desc *r)
> +{
> +     struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +
> +     if (p->is_write) {
> +             vgicv3->vgic_vmcr &= ~ICH_VMCR_ENG1;
> +             vgicv3->vgic_vmcr |= (p->regval << ICH_VMCR_ENG1_SHIFT) &
> +                                   ICH_VMCR_ENG1;
> +     } else {
> +             p->regval = (vgicv3->vgic_vmcr & ICH_VMCR_ENG1) >>
> +                          ICH_VMCR_ENG1_SHIFT;
> +     }
> +
> +     return true;
> +}
> +
> +static bool access_gic_ap0r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> +                         const struct sys_reg_desc *r)
> +{
> +     struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +     u8 idx = r->Op2 & 3;
> +
> +     if (p->is_write)
> +             vgicv3->vgic_ap0r[idx] = p->regval;
> +     else
> +             p->regval = vgicv3->vgic_ap0r[idx];
> +
> +     return true;
> +}
> +
> +static bool access_gic_ap1r(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
> +                         const struct sys_reg_desc *r)
> +{
> +     struct vgic_v3_cpu_if *vgicv3 = &vcpu->arch.vgic_cpu.vgic_v3;
> +     u8 idx = r->Op2 & 3;
> +
> +     if (p->is_write)
> +             vgicv3->vgic_ap1r[idx] = p->regval;
> +     else
> +             p->regval = vgicv3->vgic_ap1r[idx];
> +
> +     return true;
> +}
> +
> +static const struct sys_reg_desc gic_v3_icc_reg_descs[] = {
> +     /* ICC_PMR_EL1 */
> +     { Op0(3), Op1(0), CRn(4), CRm(6), Op2(0), access_gic_pmr },
> +     /* ICC_BPR0_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(3), access_gic_bpr0 },
> +     /* ICC_AP0R0_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(4), access_gic_ap0r },
> +     /* ICC_AP0R1_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(5), access_gic_ap0r },
> +     /* ICC_AP0R2_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(6), access_gic_ap0r },
> +     /* ICC_AP0R3_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(8), Op2(7), access_gic_ap0r },
> +     /* ICC_AP1R0_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(9), Op2(0), access_gic_ap1r },
> +     /* ICC_AP1R1_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(9), Op2(1), access_gic_ap1r },
> +     /* ICC_AP1R2_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(9), Op2(2), access_gic_ap1r },
> +     /* ICC_AP1R3_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(9), Op2(3), access_gic_ap1r },
> +     /* ICC_BPR1_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(3), access_gic_bpr1 },
> +     /* ICC_CTLR_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(4), access_gic_ctlr },
> +     /* ICC_IGRPEN0_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(6), access_gic_grpen0 },
> +     /* ICC_GRPEN1_EL1 */
> +     { Op0(3), Op1(0), CRn(12), CRm(12), Op2(7), access_gic_grpen1 },

Do we need to allow userspace to at least read ICC_SRE_EL1?

 
ICC_SRE_EL1.SRE is enabled at vgic initialization and DIB and FDB are not configured.

    So save and restore does not make any effect.
 

Should we verify that the DIB and FDB fields of that register are
written as the system understands them (clear, WI)?

  What kind of verification we can do on DIB and FDB by userspace?.

> +
> +int vgic_v3_cpu_sysregs_uaccess(struct kvm_vcpu *vcpu, bool is_write, u64 id,
> +                             u64 *reg)
> +{
> +     struct sys_reg_params params;
> +     const struct sys_reg_desc *r;
> +
> +     if (is_write)
> +             params.regval = le64_to_cpu(*reg);

why do we need this conversion here?

   The registers are managed in LE mode. So here the value to be

writtern is coverted to LE mode and similarly on reading back it is converted

from LE.

> +     params.is_write = is_write;
> +     params.is_aarch32 = false;
> +     params.is_32bit = false;
> +
> +     r = find_reg_by_id(id, &params, gic_v3_icc_reg_descs,
> +                        ARRAY_SIZE(gic_v3_icc_reg_descs));
> +     if (!r)
> +             return -ENXIO;
> +
> +     if (!r->access(vcpu, &params, r))
> +             return -EINVAL;
> +
> +     if (!is_write)
> +             *reg = cpu_to_le64(params.regval);

same question as above

_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm