On Wed, Jun 27, 2012 at 1:10 PM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> On 22/06/12 21:13, Christoffer Dall wrote:
>> On Mon, May 14, 2012 at 9:05 AM, Marc Zyngier <marc.zyngier at arm.com> wrote:
>>> Add the init code for the hypervisor, the virtual machine, and
>>> the virtual CPUs.
>>>
>>> An interrupt handler is also wired to allow the VGIC maintainance
>>> interrupts, used in case of underflow.
>>>
>>> Signed-off-by: Marc Zyngier <marc.zyngier at arm.com>
>>> ---
>>> ?arch/arm/include/asm/kvm_vgic.h | ? ?3 +
>>> ?arch/arm/kvm/arm.c ? ? ? ? ? ? ?| ? ?4 +
>>> ?arch/arm/kvm/vgic.c ? ? ? ? ? ? | ?120 +++++++++++++++++++++++++++++++++++++++
>>> ?3 files changed, 127 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/include/asm/kvm_vgic.h b/arch/arm/include/asm/kvm_vgic.h
>>> index e293f60..354e528 100644
>>> --- a/arch/arm/include/asm/kvm_vgic.h
>>> +++ b/arch/arm/include/asm/kvm_vgic.h
>>> @@ -138,6 +138,9 @@ struct kvm_vcpu;
>>> ?struct kvm_run;
>>>
>>> ?#ifdef CONFIG_KVM_ARM_VGIC
>>> +int kvm_vgic_hyp_init(void);
>>> +int kvm_vgic_init(struct kvm *kvm);
>>> +void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu);
>>> ?void kvm_vgic_sync_to_cpu(struct kvm_vcpu *vcpu);
>>> ?void kvm_vgic_sync_from_cpu(struct kvm_vcpu *vcpu);
>>> ?int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, const struct kvm_irq_level *irq);
>>> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
>>> index c03e450..7cd8190 100644
>>> --- a/arch/arm/kvm/arm.c
>>> +++ b/arch/arm/kvm/arm.c
>>> @@ -618,6 +618,10 @@ long kvm_arch_vm_ioctl(struct file *filp,
>>> ? ? ? ?void __user *argp = (void __user *)arg;
>>>
>>> ? ? ? ?switch (ioctl) {
>>> +#ifdef CONFIG_KVM_ARM_VGIC
>>> + ? ? ? case KVM_CREATE_IRQCHIP:
>>> + ? ? ? ? ? ? ? return kvm_vgic_init(kvm);
>>> +#endif
>>> ? ? ? ?case KVM_IRQ_LINE: {
>>> ? ? ? ? ? ? ? ?struct kvm_irq_level irq_event;
>>>
>>> diff --git a/arch/arm/kvm/vgic.c b/arch/arm/kvm/vgic.c
>>> index a48b436..b5e6082 100644
>>> --- a/arch/arm/kvm/vgic.c
>>> +++ b/arch/arm/kvm/vgic.c
>>> @@ -21,9 +21,23 @@
>>> ?#include <linux/interrupt.h>
>>> ?#include <linux/io.h>
>>>
>>> +#include <asm/hardware/gic.h>
>>> +#include <asm/kvm_arm.h>
>>> +#include <asm/kvm_mmu.h>
>>> +
>>> ?/* Temporary hacks, need to probe DT instead */
>>> ?#define VGIC_DIST_BASE ? ? ? ? 0x2c001000
>>> ?#define VGIC_DIST_SIZE ? ? ? ? 0x1000
>>> +#define VGIC_CPU_BASE ? ? ? ? ?0x2c002000
>>> +#define VGIC_CPU_SIZE ? ? ? ? ?0x2000
>>> +#define VGIC_VCTRL_BASE ? ? ? ? ? ? ? ?0x2c004000
>>> +#define VGIC_VCTRL_SIZE ? ? ? ? ? ? ? ?0x2000
>>> +#define VGIC_VCPU_BASE ? ? ? ? 0x2c006000
>>> +#define VGIC_VCPU_SIZE ? ? ? ? 0x2000
>>> +#define VGIC_MAINT_IRQ ? ? ? ? 25
>>> +
>>> +static struct kvm_vcpu __percpu **vgic_vcpus;
>>
>> perhaps a comment as to what this is...
>>
>>> +static void __iomem *vgic_vctrl_base;
>>
>> I assume this is the common base address with cpuid redirection -
>> perhaps a comment...
>
> This is the virtual interface control, which is mostly accessed by the
> world-switch code.
>
>>> ?#define ACCESS_READ_VALUE ? ? ?(1 << 0)
>>> ?#define ACCESS_READ_RAZ ? ? ? ? ? ? ? ?(0 << 0)
>>> @@ -718,3 +732,109 @@ int kvm_vgic_inject_irq(struct kvm *kvm, int cpuid, const struct kvm_irq_level *
>>>
>>> ? ? ? ?return 0;
>>> ?}
>>> +
>>> +static irqreturn_t kvm_vgic_maintainance_handler(int irq, void *data)
>>> +{
>>> + ? ? ? struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)data;
>>> +
>>> + ? ? ? WARN(!vcpu,
>>> + ? ? ? ? ? ?"VGIC interrupt on CPU %d with no vcpu\n", smp_processor_id());
>>> + ? ? ? /*
>>> + ? ? ? ?* Not much to do, as we handle everything on world switch.
>>> + ? ? ? ?* It should be possible to do things lazily though, and move
>>> + ? ? ? ?* away from world-switch.
>>> + ? ? ? ?*/
>>> + ? ? ? return IRQ_HANDLED;
>>> +}
>>> +
>>> +void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu)
>>> +{
>>> + ? ? ? struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>>> + ? ? ? struct vgic_dist *dist = &vcpu->kvm->arch.vgic;
>>> +
>>
>> interesting newline
>>
>>> + ? ? ? int i;
>>> +
>>> + ? ? ? if (!irqchip_in_kernel(vcpu->kvm))
>>> + ? ? ? ? ? ? ? return;
>>> +
>>> + ? ? ? spin_lock_init(&vgic_cpu->lock);
>>> + ? ? ? for (i = 0; i < VGIC_NR_IRQS; i++) {
>>> + ? ? ? ? ? ? ? if (i < 16)
>>> + ? ? ? ? ? ? ? ? ? ? ? vgic_bitmap_set_irq_val(&dist->irq_enabled,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? vcpu->vcpu_id, i, 1);
>>
>> why only these 16 (reset: implementation defined?)
>
> Because there is only 16 SGIs. They need to be enabled for SMP boot.
> Otherwise, the secondary CPU never receives the interrupt and stays in
> its WFI loop. The GIC specification says that they can be permanently
> enabled, so we just do that.
>
ok thanks, we could place this little paragraph as a comment.
>>> + ? ? ? ? ? ? ? vgic_cpu->vgic_irq_lr_map[i] = 0xff;
>>> + ? ? ? }
>>> +
>>> + ? ? ? BUG_ON(!vcpu->kvm->arch.vgic.vctrl_base);
>>> + ? ? ? vgic_cpu->nr_lr = (readl_relaxed(vcpu->kvm->arch.vgic.vctrl_base + GICH_VTR) & 0x1f) + 1;
>>
>> please fix the 80 char lines here:
>>
>> vtr = readl_relaxed(...)
>> vgic_cpu->nr_lr = (vtr & 0x1f) + 1;
>>
>>> + ? ? ? for (i = 0; i < vgic_cpu->nr_lr; i++)
>>> + ? ? ? ? ? ? ? vgic_cpu->vgic_lr_irq_map[i] = VGIC_NR_IRQS;
>>> +
>>> + ? ? ? vgic_cpu->vgic_mcr = readl_relaxed(vcpu->kvm->arch.vgic.vctrl_base + GICH_VMCR);
>>
>> same as above
>>
>>> + ? ? ? vgic_cpu->vgic_mcr |= 0xf << 28; /* Priority */
>>> + ? ? ? vgic_cpu->vgic_hcr |= VGIC_HCR_EN; /* Get the show on the road... */
>>> +}
>>> +
>>> +int kvm_vgic_hyp_init(void)
>>> +{
>>> + ? ? ? int ret;
>>> +
>>> + ? ? ? vgic_vcpus = alloc_percpu(struct kvm_vcpu *);
>>> + ? ? ? if (!vgic_vcpus) {
>>> + ? ? ? ? ? ? ? kvm_err("Cannot allocate vgic_vcpus\n");
>>> + ? ? ? ? ? ? ? return -ENOMEM;
>>> + ? ? ? }
>>> +
>>> + ? ? ? ret = request_percpu_irq(VGIC_MAINT_IRQ, kvm_vgic_maintainance_handler,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"vgic", vgic_vcpus);
>>> + ? ? ? if (ret) {
>>> + ? ? ? ? ? ? ? kvm_err("Cannot register interrupt %d\n", VGIC_MAINT_IRQ);
>>> + ? ? ? ? ? ? ? goto out_free_vcpus;
>>> + ? ? ? }
>>> +
>>> + ? ? ? vgic_vctrl_base = ioremap(VGIC_VCTRL_BASE, VGIC_VCTRL_SIZE);
>>> + ? ? ? if (!vgic_vctrl_base) {
>>> + ? ? ? ? ? ? ? kvm_err("Cannot ioremap VCTRL\n");
>>> + ? ? ? ? ? ? ? goto out_free_irq;
>>
>> need to set ret = -EFAULT / -ENOMEM here
>
> Nice catch.
>
I believe you fix it in a later patch.
>>> + ? ? ? }
>>> +
>>> + ? ? ? ret = create_hyp_io_mappings(kvm_hyp_pgd,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?vgic_vctrl_base,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?vgic_vctrl_base + VGIC_VCTRL_SIZE,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?VGIC_VCTRL_BASE);
>>> + ? ? ? if (ret) {
>>> + ? ? ? ? ? ? ? kvm_err("Cannot map VCTRL into hyp\n");
>>> + ? ? ? ? ? ? ? goto out_unmap;
>>> + ? ? ? }
>>> +
>>> + ? ? ? return 0;
>>> +
>>> +out_unmap:
>>> + ? ? ? iounmap(vgic_vctrl_base);
>>> +out_free_irq:
>>> + ? ? ? free_percpu_irq(VGIC_MAINT_IRQ, vgic_vcpus);
>>> +out_free_vcpus:
>>> + ? ? ? free_percpu(vgic_vcpus);
>>> +
>>> + ? ? ? return ret;
>>> +}
>>> +
>>> +int kvm_vgic_init(struct kvm *kvm)
>>> +{
>>> + ? ? ? int ret = -EEXIST;
>>> +
>>> + ? ? ? mutex_lock(&kvm->lock);
>>> + ? ? ? if (atomic_read(&kvm->online_vcpus))
>>> + ? ? ? ? ? ? ? goto out;
>>
>> is -EEXIST the proper error code here? Wouldn't say -EBUSY be more
>> correct, and then do:
>>
>> if (kvm->arch.vgic.vctrl_base) {
>> ? ? ? ? ret = -EBUSY;
>> ? ? ? ? goto out;
>> }
>
> Agreed.
>
>>> +
>>> + ? ? ? spin_lock_init(&kvm->arch.vgic.lock);
>>> + ? ? ? kvm->arch.vgic.vctrl_base = vgic_vctrl_base;
>>> +
>>> + ? ? ? ret = kvm_phys_addr_ioremap(kvm, VGIC_CPU_BASE,
>>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? VGIC_VCPU_BASE, VGIC_CPU_SIZE);
>>> + ? ? ? if (ret)
>>> + ? ? ? ? ? ? ? kvm_err("Unable to remap VGIC CPU to VCPU\n");
>>> +out:
>>> + ? ? ? mutex_unlock(&kvm->lock);
>>> + ? ? ? return ret;
>>> +}
