On Tue, Mar 21, 2017 at 02:36:00PM +0000, Marc Zyngier wrote:
> On 21/03/17 11:05, Christoffer Dall wrote:
> > There is no common code in the VGIC that uses the VMCR, so we have no
> > use of the intermediate architecture-agnostic representation of the VMCR
> > and might as well manipulate the bits specifically using the logic for
> > the version of the GIC that the code supports.
> >
> > For GICv2, this means translating between the GICH_VMCR register format
> > stored in memory and the GICC_X registers exported to user space, with
> > the annoying quirk around the format of the GICC_PMR, where we export
> > the 8 bit prority field using the lower 5 bits and always assuming
> > bits[2:0] are clear.
> >
> > This now lets us get completely rid of struct vmcr and its common
> > accessor functions.
> >
> > Signed-off-by: Christoffer Dall <cdall@xxxxxxxxxx>
> > ---
> > virt/kvm/arm/vgic/vgic-mmio-v2.c | 51 +++++++++++++++++++++++++++++-----------
> > virt/kvm/arm/vgic/vgic-mmio.c | 16 -------------
> > virt/kvm/arm/vgic/vgic-v2.c | 29 -----------------------
> > virt/kvm/arm/vgic/vgic.h | 14 -----------
> > 4 files changed, 37 insertions(+), 73 deletions(-)
> >
> > diff --git a/virt/kvm/arm/vgic/vgic-mmio-v2.c b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > index a3ad7ff..1bdb990 100644
> > --- a/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > +++ b/virt/kvm/arm/vgic/vgic-mmio-v2.c
> > @@ -219,23 +219,33 @@ static void vgic_mmio_write_sgipends(struct kvm_vcpu *vcpu,
> > static unsigned long vgic_mmio_read_vcpuif(struct kvm_vcpu *vcpu,
> > gpa_t addr, unsigned int len)
> > {
> > - struct vgic_vmcr vmcr;
> > + u32 vmcr = vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr;
> > u32 val;
> >
> > - vgic_get_vmcr(vcpu, &vmcr);
> >
> > switch (addr & 0xff) {
> > case GIC_CPU_CTRL:
> > - val = vmcr.ctlr;
> > + val = (vmcr & GICH_VMCR_CTRL_MASK) >>
> > + GICH_VMCR_CTRL_SHIFT;
> > break;
> > case GIC_CPU_PRIMASK:
> > - val = vmcr.pmr;
> > + /*
> > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the
> > + * the PMR field as GICH_VMCR.VMPriMask rather than
> > + * GICC_PMR.Priority, so we expose the upper five bits of
> > + * priority mask to userspace using the lower bits in the
> > + * 32-bit word provided by userspace.
> > + */
> > + val = (vmcr & GICH_VMCR_PRIMASK_MASK) >>
> > + GICH_VMCR_PRIMASK_SHIFT;
> > break;
> > case GIC_CPU_BINPOINT:
> > - val = vmcr.bpr;
> > + val = (vmcr & GICH_VMCR_BINPOINT_MASK) >>
> > + GICH_VMCR_BINPOINT_SHIFT;
> > break;
> > case GIC_CPU_ALIAS_BINPOINT:
> > - val = vmcr.abpr;
> > + val = (vmcr & GICH_VMCR_ALIAS_BINPOINT_MASK) >>
> > + GICH_VMCR_ALIAS_BINPOINT_SHIFT;
> > break;
> > case GIC_CPU_IDENT:
> > val = ((PRODUCT_ID_KVM << 20) |
> > @@ -253,26 +263,39 @@ static void vgic_mmio_write_vcpuif(struct kvm_vcpu *vcpu,
> > gpa_t addr, unsigned int len,
> > unsigned long val)
> > {
> > - struct vgic_vmcr vmcr;
> > -
> > - vgic_get_vmcr(vcpu, &vmcr);
> > + u32 *vmcr = &vcpu->arch.vgic_cpu.vgic_v2.vgic_vmcr;
> > + u32 mask, field;
> >
> > switch (addr & 0xff) {
> > case GIC_CPU_CTRL:
> > - vmcr.ctlr = val;
> > + mask = GICH_VMCR_CTRL_MASK;
> > + field = val << GICH_VMCR_CTRL_SHIFT;
> > break;
> > case GIC_CPU_PRIMASK:
> > - vmcr.pmr = val;
> > + /*
> > + * Our KVM_DEV_TYPE_ARM_VGIC_V2 device ABI exports the
> > + * the PMR field as GICH_VMCR.VMPriMask rather than
> > + * GICC_PMR.Priority, so we obtain the upper five bits of
> > + * priority mask from userspace using the lower bits in the
> > + * 32-bit word provided by userspace.
> > + */
> > + mask = GICH_VMCR_PRIMASK_MASK;
> > + field = val << GICH_VMCR_PRIMASK_SHIFT;
> > break;
> > case GIC_CPU_BINPOINT:
> > - vmcr.bpr = val;
> > + mask = GICH_VMCR_BINPOINT_MASK;
> > + field = val << GICH_VMCR_BINPOINT_SHIFT;
> > break;
> > case GIC_CPU_ALIAS_BINPOINT:
> > - vmcr.abpr = val;
> > + mask = GICH_VMCR_ALIAS_BINPOINT_MASK;
> > + field = val << GICH_VMCR_ALIAS_BINPOINT_SHIFT;
> > break;
> > + default:
> > + return;
>
>
> Something seems fishy here. If I have a GICv2-on-GICv3, my vmcr is still
> that of a GICv3, not of a GICv2. Here, you're cramming everything into a
> v2 representation, which is not going to work on GICv3.
>
> Or am I missing something?
>
No, you're right, that is the reason for having the indirection, which I
competely missed. I'll drop this series and revert back to my original
patch for the VMCR.
Thanks for having a look and sorry about the noise.
-Christoffer
