On Tue, Mar 21, 2017 at 12:05:30PM +0000, Marc Zyngier wrote: > On 20/03/17 10:58, Christoffer Dall wrote: > > Implement early initialization for both the distributor and the CPU > > interfaces. The basic idea is that even though the VGIC is not > > functional or not requested from user space, the critical path of the > > run loop can still call VGIC functions that just won't do anything, > > without them having to check additional initialization flags to ensure > > they don't look at uninitialized data structures. > > > > Signed-off-by: Christoffer Dall <[email protected]> > > --- > > virt/kvm/arm/vgic/vgic-init.c | 96 > > +++++++++++++++++++++++++------------------ > > 1 file changed, 56 insertions(+), 40 deletions(-) > > > > diff --git a/virt/kvm/arm/vgic/vgic-init.c b/virt/kvm/arm/vgic/vgic-init.c > > index 3762fd1..25fd1b9 100644 > > --- a/virt/kvm/arm/vgic/vgic-init.c > > +++ b/virt/kvm/arm/vgic/vgic-init.c > > @@ -24,7 +24,12 @@ > > > > /* > > * Initialization rules: there are multiple stages to the vgic > > - * initialization, both for the distributor and the CPU interfaces. > > + * initialization, both for the distributor and the CPU interfaces. The > > basic > > + * idea is that even though the VGIC is not functional or not requested > > from > > + * user space, the critical path of the run loop can still call VGIC > > functions > > + * that just won't do anything, without them having to check additional > > + * initialization flags to ensure they don't look at uninitialized data > > + * structures. > > * > > * Distributor: > > * > > @@ -39,23 +44,67 @@ > > * > > * CPU Interface: > > * > > - * - kvm_vgic_cpu_early_init(): initialization of static data that > > + * - kvm_vgic_vcpu_early_init(): initialization of static data that > > * doesn't depend on any sizing information or emulation type. No > > * allocation is allowed there. > > */ > > > > /* EARLY INIT */ > > > > -/* > > - * Those 2 functions should not be needed anymore but they > > - * still are called from arm.c > > +/** > > + * kvm_vgic_early_init() - Initialize static VGIC VCPU data structures > > + * @kvm: The VM whose VGIC districutor should be initialized > > + * > > + * Only do initialization of static structures that don't require any > > + * allocation or sizing information from userspace. vgic_init() called > > + * kvm_vgic_dist_init() which takes care of the rest. > > */ > > void kvm_vgic_early_init(struct kvm *kvm) > > { > > + struct vgic_dist *dist = &kvm->arch.vgic; > > + > > + INIT_LIST_HEAD(&dist->lpi_list_head); > > + spin_lock_init(&dist->lpi_list_lock); > > } > > > > +/** > > + * kvm_vgic_vcpu_early_init() - Initialize static VGIC VCPU data structures > > + * @vcpu: The VCPU whose VGIC data structures whould be initialized > > + * > > + * Only do initialization, but do not actually enable the VGIC CPU > > interface > > + * yet. > > + */ > > void kvm_vgic_vcpu_early_init(struct kvm_vcpu *vcpu) > > { > > + struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > > + int i; > > + > > + INIT_LIST_HEAD(&vgic_cpu->ap_list_head); > > + spin_lock_init(&vgic_cpu->ap_list_lock); > > + > > + /* > > + * Enable and configure all SGIs to be edge-triggered and > > + * configure all PPIs as level-triggered. > > + */ > > + for (i = 0; i < VGIC_NR_PRIVATE_IRQS; i++) { > > + struct vgic_irq *irq = &vgic_cpu->private_irqs[i]; > > + > > + INIT_LIST_HEAD(&irq->ap_list); > > + spin_lock_init(&irq->irq_lock); > > + irq->intid = i; > > + irq->vcpu = NULL; > > + irq->target_vcpu = vcpu; > > + irq->targets = 1U << vcpu->vcpu_id; > > + kref_init(&irq->refcount); > > + if (vgic_irq_is_sgi(i)) { > > + /* SGIs */ > > + irq->enabled = 1; > > + irq->config = VGIC_CONFIG_EDGE; > > + } else { > > + /* PPIs */ > > + irq->config = VGIC_CONFIG_LEVEL; > > + } > > + } > > } > > > > /* CREATION */ > > @@ -148,9 +197,6 @@ static int kvm_vgic_dist_init(struct kvm *kvm, unsigned > > int nr_spis) > > struct kvm_vcpu *vcpu0 = kvm_get_vcpu(kvm, 0); > > int i; > > > > - INIT_LIST_HEAD(&dist->lpi_list_head); > > - spin_lock_init(&dist->lpi_list_lock); > > - > > dist->spis = kcalloc(nr_spis, sizeof(struct vgic_irq), GFP_KERNEL); > > if (!dist->spis) > > return -ENOMEM; > > @@ -181,41 +227,11 @@ static int kvm_vgic_dist_init(struct kvm *kvm, > > unsigned int nr_spis) > > } > > > > /** > > - * kvm_vgic_vcpu_init: initialize the vcpu data structures and > > - * enable the VCPU interface > > - * @vcpu: the VCPU which's VGIC should be initialized > > + * kvm_vgic_vcpu_init() - Enable the VCPU interface > > + * @vcpu: the VCPU which's VGIC should be enabled > > */ > > static void kvm_vgic_vcpu_init(struct kvm_vcpu *vcpu) > > { > > - struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > > - int i; > > - > > - INIT_LIST_HEAD(&vgic_cpu->ap_list_head); > > - spin_lock_init(&vgic_cpu->ap_list_lock); > > - > > - /* > > - * Enable and configure all SGIs to be edge-triggered and > > - * configure all PPIs as level-triggered. > > - */ > > - for (i = 0; i < VGIC_NR_PRIVATE_IRQS; i++) { > > - struct vgic_irq *irq = &vgic_cpu->private_irqs[i]; > > - > > - INIT_LIST_HEAD(&irq->ap_list); > > - spin_lock_init(&irq->irq_lock); > > - irq->intid = i; > > - irq->vcpu = NULL; > > - irq->target_vcpu = vcpu; > > - irq->targets = 1U << vcpu->vcpu_id; > > - kref_init(&irq->refcount); > > - if (vgic_irq_is_sgi(i)) { > > - /* SGIs */ > > - irq->enabled = 1; > > - irq->config = VGIC_CONFIG_EDGE; > > - } else { > > - /* PPIs */ > > - irq->config = VGIC_CONFIG_LEVEL; > > - } > > - } > > if (kvm_vgic_global_state.type == VGIC_V2) > > vgic_v2_enable(vcpu); > > else > > > > Since this function is now about enabling the vgic on a given vcpu, > shouldn't the name reflect this? Other than that:
The v3 version does something which is a bit of a mix between initialization and enabling, plus it would add even more churn, so, meh, not really sure. If you feel it should be kvm_vgic_vcpu_enable(), I'm okay with changing it. > > Acked-by: Marc Zyngier <[email protected]> > Thanks, -Christoffer _______________________________________________ kvmarm mailing list [email protected] https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
