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 <christoffer.dall@xxxxxxxxxx>
> >>> ---
> >>> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
