On Tue, Aug 30, 2016 at 7:15 PM, Christoffer Dall < [email protected]> wrote:
> On Wed, Aug 24, 2016 at 04:50:08PM +0530, [email protected] wrote: > > From: Vijaya Kumar K <[email protected]> > > > 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, ¶ms, gic_v3_icc_reg_descs, > > + ARRAY_SIZE(gic_v3_icc_reg_descs)); > > + if (!r) > > + return -ENXIO; > > + > > + if (!r->access(vcpu, ¶ms, r)) > > + return -EINVAL; > > + > > + if (!is_write) > > + *reg = cpu_to_le64(params.regval); > > same question as above > >
_______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
