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).
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
