Hi wanghaibin,
On 08/07/14 09:50, wanghaibin wrote:
> On 2014/6/19 17:45, Andre Przywara wrote:
>
>> Currently we only have one virtual GIC model supported, so all guests
>> use the same emulation code. With the addition of another model we
>> end up with different guests using potentially different vGIC models,
>> so we have to split up some functions to be per VM.
>> Introduce a vgic_vm_ops struct to hold function pointers for those
>> functions that are different and provide the necessary code to
>> initialize them.
>> This includes functions that depend on the emulated GIC model only
>> and functions that depend on the combination of host and guest GIC.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
>> ---
>> include/kvm/arm_vgic.h | 18 +++++++-
>> virt/kvm/arm/vgic-v2.c | 17 +++++++-
>> virt/kvm/arm/vgic-v3.c | 16 ++++++-
>> virt/kvm/arm/vgic.c | 109 +++++++++++++++++++++++++++++++++---------------
>> 4 files changed, 121 insertions(+), 39 deletions(-)
>>
...
>>
>> +static bool init_emulation_ops(struct kvm *kvm, int type)
>> +{
>> + struct vgic_dist *dist = &kvm->arch.vgic;
>> +
>> + switch (type) {
>> + case KVM_DEV_TYPE_ARM_VGIC_V2:
>> + dist->vm_ops.handle_mmio = vgic_v2_handle_mmio;
>> + dist->vm_ops.handle_mmio = vgic_v2_handle_mmio;
>> + dist->vm_ops.unqueue_sgi = vgic_v2_unqueue_sgi;
>> + dist->vm_ops.vgic_init = vgic_v2_init;
>> + return true;
>> + }
>
> How about define (struct vgic_vm_ops *vm_ops (poniter)) in struct vgic_dist and define
> struct vgic_vm_ops gicv2_vm_ops = {
> .handle_mmio = vgic_v2_handle_mmio;
> .vgic_init = vgic_v2_init;
> ......
> }
As I stated in the commit message, some functions depend on the host,
some on the guest GIC version. So we cannot initialize it at compile
time only, but have to adjust it at runtime depending on which GIC
version the user requested. So assigning the handlers statically does
not work, we would have to copy the members of a const struct and fill
in the guest dependent. I found it more straightforward to just set
every single member separately.
>
> and
>
> case KVM_DEV_TYPE_ARM_VGIC_V2:
> dist->vm_ops = &gicv2_vm_ops;
>
> Maybe It seems better..
>
>> + return false;
>> +}
>> +
>> int kvm_vgic_create(struct kvm *kvm, u32 type)
>> {
>> int i, vcpu_lock_idx = -1, ret = 0;
>> @@ -1911,7 +1946,7 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>>
>> mutex_lock(&kvm->lock);
>>
>> - if (kvm->arch.vgic.vctrl_base) {
>> + if (kvm->arch.vgic.in_kernel) {
>> ret = -EEXIST;
>> goto out;
>> }
>> @@ -1934,12 +1969,20 @@ int kvm_vgic_create(struct kvm *kvm, u32 type)
>> }
>> }
>>
>> + if (!vgic->init_emul(kvm, type)) {
>> + ret = -ENODEV;
>> + goto out;
>
>
> There maybe goto out_unlock ....
Indeed! Good catch, thanks!
>
>> + }
>> +
>> spin_lock_init(&kvm->arch.vgic.lock);
>> kvm->arch.vgic.in_kernel = true;
>> + kvm->arch.vgic.vgic_model = type;
>> kvm->arch.vgic.vctrl_base = vgic->vctrl_base;
>> kvm->arch.vgic.vgic_dist_base = VGIC_ADDR_UNDEF;
>> kvm->arch.vgic.vgic_cpu_base = VGIC_ADDR_UNDEF;
>>
>> + init_emulation_ops(kvm, type);
>
>
> It will be better to check the return value of the init_emulation_ops func.
Yes, will do.
Regards,
Andre.
P.S. Can you either CC: or TO: me on replies next time? This makes it
easier for me to catch (and find again) those mails. Thanks!
>
>> +
>> out_unlock:
>> for (; vcpu_lock_idx >= 0; vcpu_lock_idx--) {
>> vcpu = kvm_get_vcpu(kvm, vcpu_lock_idx);
>
>
>
> _______________________________________________
> kvmarm mailing list
> kvmarm@xxxxxxxxxxxxxxxxxxxxx
> https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
