On Thu, Sep 26, 2013 at 12:37:03AM +0200, Alexander Graf wrote: > > On 25.09.2013, at 23:30, Christoffer Dall wrote: > > > On Sun, Aug 25, 2013 at 04:24:20PM +0100, Alexander Graf wrote: > >> > >> On 23.08.2013, at 20:20, Christoffer Dall wrote: > >> > >>> Implement support for the CPU interface register access driven by MMIO > >>> address offsets from the CPU interface base address. Useful for user > >>> space to support save/restore of the VGIC state. > >>> > >>> This commit adds support only for the same logic as the current VGIC > >>> support, and no more. For example, the active priority registers are > >>> handled as RAZ/WI, just like setting priorities on the emulated > >>> distributor. > >>> > >>> Signed-off-by: Christoffer Dall <[email protected]> > >>> --- > >>> virt/kvm/arm/vgic.c | 66 > >>> +++++++++++++++++++++++++++++++++++++++++++++++---- > >>> 1 file changed, 62 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > >>> index d44b5a1..257dbae 100644 > >>> --- a/virt/kvm/arm/vgic.c > >>> +++ b/virt/kvm/arm/vgic.c > >>> @@ -1684,9 +1684,67 @@ int kvm_vgic_addr(struct kvm *kvm, unsigned long > >>> type, u64 *addr, bool write) > >>> static bool handle_cpu_mmio_misc(struct kvm_vcpu *vcpu, > >>> struct kvm_exit_mmio *mmio, phys_addr_t offset) > >>> { > >>> - return true; > >>> + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > >>> + u32 reg, mask = 0, shift = 0; > >>> + bool updated = false; > >>> + > >>> + switch (offset & ~0x3) { > >>> + case GIC_CPU_CTRL: > >>> + mask = GICH_VMCR_CTRL_MASK; > >>> + shift = GICH_VMCR_CTRL_SHIFT; > >>> + break; > >>> + case GIC_CPU_PRIMASK: > >>> + mask = GICH_VMCR_PRIMASK_MASK; > >>> + shift = GICH_VMCR_PRIMASK_SHIFT; > >>> + break; > >>> + case GIC_CPU_BINPOINT: > >>> + mask = GICH_VMCR_BINPOINT_MASK; > >>> + shift = GICH_VMCR_BINPOINT_SHIFT; > >>> + break; > >>> + case GIC_CPU_ALIAS_BINPOINT: > >>> + mask = GICH_VMCR_ALIAS_BINPOINT_MASK; > >>> + shift = GICH_VMCR_ALIAS_BINPOINT_SHIFT; > >>> + break; > >>> + } > >>> + > >>> + if (!mmio->is_write) { > >>> + reg = (vgic_cpu->vgic_vmcr & mask) >> shift; > >>> + memcpy(mmio->data, ®, sizeof(reg)); > >>> + } else { > >>> + memcpy(®, mmio->data, sizeof(reg)); > >>> + reg = (reg << shift) & mask; > >>> + if (reg != (vgic_cpu->vgic_vmcr & mask)) > >>> + updated = true; > >>> + vgic_cpu->vgic_vmcr &= ~mask; > >>> + vgic_cpu->vgic_vmcr |= reg; > >>> + } > >>> + return updated; > >>> +} > >>> + > >>> +static bool handle_mmio_abpr(struct kvm_vcpu *vcpu, > >>> + struct kvm_exit_mmio *mmio, phys_addr_t offset) > >>> +{ > >>> + return handle_cpu_mmio_misc(vcpu, mmio, GIC_CPU_ALIAS_BINPOINT); > >>> +} > >>> + > >>> +static bool handle_cpu_mmio_ident(struct kvm_vcpu *vcpu, > >>> + struct kvm_exit_mmio *mmio, > >>> + phys_addr_t offset) > >>> +{ > >>> + u32 reg; > >>> + > >>> + if (mmio->is_write) > >>> + return false; > >>> + > >>> + reg = 0x0002043B; > >> > >> This wants a comment and probably also a #define :). > >> > > > > Marc, where does the 0x4b0 product id code come from for the distributor > > IIDR? > > > > Would this be satisfying? > > > > > > diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c > > index 5214424..558be38 100644 > > --- a/virt/kvm/arm/vgic.c > > +++ b/virt/kvm/arm/vgic.c > > @@ -71,6 +71,9 @@ > > #define VGIC_ADDR_UNDEF (-1) > > #define IS_VGIC_ADDR_UNDEF(_x) ((_x) == VGIC_ADDR_UNDEF) > > > > +#define GIC_PRODUCT_ID 0x4b0 > > This is a specific GIC version. PL390 for example is 0x3b0: > > > http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.ddi0416b/Beiggeba.html > > That should be reflected in the #define. If it means "GICv2" then it should > be GICV2_PRODUCT_ID for example. >
I know what field in the register it is thanks :) But I couldn't find 0x4b0 anywhere in the docs, so I'm asking Marc where he got it from. I don't believe it means GICv2, but a specific implementation of a GICv2, and once I have more info I can change the define name, I suspect this is potentially something made-up to indicate that this is the KVM VGIC... > > +#define ARM_JEP106_IMPLEMENTER 0x43b > > I think naming this JEP106_IMPLEMENTER_ARM makes it more obvious that we're > talking about the implementer code for ARM. Or maybe just only > IMPLEMENTER_ARM. > ok > > + > > /* Physical address of vgic virtual cpu interface */ > > static phys_addr_t vgic_vcpu_base; > > > > @@ -331,7 +334,7 @@ static bool handle_mmio_misc(struct kvm_vcpu *vcpu, > > break; > > > > case 8: /* IIDR */ > > - reg = 0x4B00043B; > > + reg = (GIC_PRODUCT_ID << 20) | ARM_JEP106_IMPLEMENTER; > > vgic_reg_access(mmio, ®, word_offset, > > ACCESS_READ_VALUE | ACCESS_WRITE_IGNORED); > > break; > > @@ -1734,7 +1737,7 @@ static bool handle_cpu_mmio_ident(struct kvm_vcpu > > *vcpu, > > if (mmio->is_write) > > return false; > > > > - reg = 0x0002043B; > > + reg = (GIC_PRODUCT_ID << 20) | (0x2 << 16) | ARM_JEP106_IMPLEMENTER; > > What is the 0x2 here? > > GICv2, and now you're going to tell me to create a define for it right? Which of course I can, but it's getting a bit silly, because then you can ask me what is the field shifted at 16 bits, and the next question is what is the GICC_IIDR, and at some point you're just going to have to look up the definition of this specific register in the GIC specs. -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
