On 2014/11/17 18:51, Andre Przywara wrote:
> Hi,
>
> On 17/11/14 09:27, wanghaibin wrote:
>> Add a out_unlock flag, for the different unusual can goto the
>> corresponding flag.
>>
>> Signed-off-by: wanghaibin <wanghaibin.wang@xxxxxxxxxx>
>> ---
>> virt/kvm/arm/vgic.c | 8 +++++---
>> 1 files changed, 5 insertions(+), 3 deletions(-)
>>
>> diff --git a/virt/kvm/arm/vgic.c b/virt/kvm/arm/vgic.c
>> index bd4b104..d6a6c05 100644
>> --- a/virt/kvm/arm/vgic.c
>> +++ b/virt/kvm/arm/vgic.c
>> @@ -1897,19 +1897,19 @@ int kvm_vgic_init(struct kvm *kvm)
>> mutex_lock(&kvm->lock);
>>
>> if (vgic_initialized(kvm))
>> - goto out;
>> + goto out_unlock;
>
> Why this? ret is 0 at this point, so we will not call kvm_vgic_destroy()
> below.
>
>>
>> if (IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_dist_base) ||
>> IS_VGIC_ADDR_UNDEF(kvm->arch.vgic.vgic_cpu_base)) {
>> kvm_err("Need to set vgic cpu and dist addresses first\n");
>> ret = -ENXIO;
>> - goto out;
>> + goto out_unlock;
>> }
>>
>> ret = vgic_init_maps(kvm);
>> if (ret) {
>> kvm_err("Unable to allocate maps\n");
>> - goto out;
>> + goto out_unlock;
>
> These two make some more sense, but what does this fix?
> kvm_vgic_destroy() is safe to be called with not allocated VGIC memory
> or to be called multiple times (by using kfree(), checking for NULL
> pointers before referencing the irq_spi_target[] array and setting
> pointers explicitly to NULL after freeing them).
> If you want to improve robustness I'd rather leave the call to
> kvm_vgic_destroy() in than to avoid it.
>
> So can you explain why we need this?
> Have you seen any real bugs caused by this?
> Is there some code out there that will need these fixes? If yes, please
> show it.
>
> Actually, if vgic_init_maps() fails that's due to a failing kalloc(), in
> which case we are in deep trouble anyway and don't need to care about
> such optimizations.
>
> I am a bit wary of changing random places in the VGIC code for no good
> reasons (this applies to most of the other patches as well).
>
Sorry about this, it is my fault, I just read this code, and according to the
code logic, I just attempt to avoid call the kvm_vgic_destroy multiple times.
Sorry, again.
> Regards,
> Andre.
>
>> }
>>
>> ret = kvm_phys_addr_ioremap(kvm, kvm->arch.vgic.vgic_cpu_base,
>> @@ -1927,6 +1927,8 @@ int kvm_vgic_init(struct kvm *kvm)
>> out:
>> if (ret)
>> kvm_vgic_destroy(kvm);
>> +
>> +out_unlock:
>> mutex_unlock(&kvm->lock);
>> return ret;
>> }
>>
>
> .
>
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
