| Message ID | 20201201150157.223625-4-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: Miscellaneous improvements | expand |
Hi Alexandru, On 12/1/20 4:01 PM, Alexandru Elisei wrote: > kvm_vgic_map_resources() is called when a VCPU if first run and it maps all > the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the > ready variable to keep track of the state of resources and the global KVM > mutex to protect against concurrent accesses. After the lock is taken, the > variable is checked again in case another VCPU took the lock between the > current VCPU reading ready equals false and taking the lock. > > The double-checked lock pattern is spread across four different functions: > in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in > vgic_{v2,v3}_map_resources(), which makes it hard to reason about and > introduces minor code duplication. Consolidate the checks in > kvm_vgic_map_resources(), where the lock is taken. > > No functional change intended. > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > --- > arch/arm64/kvm/arm.c | 8 +++----- > arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++ > arch/arm64/kvm/vgic/vgic-v2.c | 3 --- > arch/arm64/kvm/vgic/vgic-v3.c | 3 --- > 4 files changed, 9 insertions(+), 11 deletions(-) > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > index 9d69d2bf6943..65a5e89f5133 100644 > --- a/arch/arm64/kvm/arm.c > +++ b/arch/arm64/kvm/arm.c > @@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) > * Map the VGIC hardware resources before running a vcpu the > * first time on this VM. > */ > - if (unlikely(!vgic_ready(kvm))) { > - ret = kvm_vgic_map_resources(kvm); > - if (ret) > - return ret; > - } > + ret = kvm_vgic_map_resources(kvm); > + if (ret) > + return ret; > } else { > /* > * Tell the rest of the code that there are userspace irqchip > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c > index 32e32d67a127..a2f4d1c85f00 100644 > --- a/arch/arm64/kvm/vgic/vgic-init.c > +++ b/arch/arm64/kvm/vgic/vgic-init.c > @@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm) > struct vgic_dist *dist = &kvm->arch.vgic; > int ret = 0; > > + if (likely(vgic_ready(kvm))) > + return 0; > + > mutex_lock(&kvm->lock); > + if (vgic_ready(kvm)) > + goto out; > + While we are at it, the setting of dist->ready may be moved in kvm_vgic_map_resources() too. Thanks Eric > if (!irqchip_in_kernel(kvm)) > goto out; > > diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c > index ebf53a4e1296..7f38c1a93639 100644 > --- a/arch/arm64/kvm/vgic/vgic-v2.c > +++ b/arch/arm64/kvm/vgic/vgic-v2.c > @@ -306,9 +306,6 @@ int vgic_v2_map_resources(struct kvm *kvm) > struct vgic_dist *dist = &kvm->arch.vgic; > int ret = 0; > > - if (vgic_ready(kvm)) > - goto out; > - > if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) || > IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) { > kvm_err("Need to set vgic cpu and dist addresses first\n"); > diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c > index 9cdf39a94a63..35029c5cb0f1 100644 > --- a/arch/arm64/kvm/vgic/vgic-v3.c > +++ b/arch/arm64/kvm/vgic/vgic-v3.c > @@ -500,9 +500,6 @@ int vgic_v3_map_resources(struct kvm *kvm) > int ret = 0; > int c; > > - if (vgic_ready(kvm)) > - goto out; > - > kvm_for_each_vcpu(c, vcpu, kvm) { > struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu; > >
On Mon, 14 Dec 2020 12:55:51 +0000, Auger Eric <eric.auger@redhat.com> wrote: > > Hi Alexandru, > > On 12/1/20 4:01 PM, Alexandru Elisei wrote: > > kvm_vgic_map_resources() is called when a VCPU if first run and it maps all > > the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the > > ready variable to keep track of the state of resources and the global KVM > > mutex to protect against concurrent accesses. After the lock is taken, the > > variable is checked again in case another VCPU took the lock between the > > current VCPU reading ready equals false and taking the lock. > > > > The double-checked lock pattern is spread across four different functions: > > in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in > > vgic_{v2,v3}_map_resources(), which makes it hard to reason about and > > introduces minor code duplication. Consolidate the checks in > > kvm_vgic_map_resources(), where the lock is taken. > > > > No functional change intended. > > > > Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> > > --- > > arch/arm64/kvm/arm.c | 8 +++----- > > arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++ > > arch/arm64/kvm/vgic/vgic-v2.c | 3 --- > > arch/arm64/kvm/vgic/vgic-v3.c | 3 --- > > 4 files changed, 9 insertions(+), 11 deletions(-) > > > > diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c > > index 9d69d2bf6943..65a5e89f5133 100644 > > --- a/arch/arm64/kvm/arm.c > > +++ b/arch/arm64/kvm/arm.c > > @@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) > > * Map the VGIC hardware resources before running a vcpu the > > * first time on this VM. > > */ > > - if (unlikely(!vgic_ready(kvm))) { > > - ret = kvm_vgic_map_resources(kvm); > > - if (ret) > > - return ret; > > - } > > + ret = kvm_vgic_map_resources(kvm); > > + if (ret) > > + return ret; > > } else { > > /* > > * Tell the rest of the code that there are userspace irqchip > > diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c > > index 32e32d67a127..a2f4d1c85f00 100644 > > --- a/arch/arm64/kvm/vgic/vgic-init.c > > +++ b/arch/arm64/kvm/vgic/vgic-init.c > > @@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm) > > struct vgic_dist *dist = &kvm->arch.vgic; > > int ret = 0; > > > > + if (likely(vgic_ready(kvm))) > > + return 0; > > + > > mutex_lock(&kvm->lock); > > + if (vgic_ready(kvm)) > > + goto out; > > + > While we are at it, the setting of dist->ready may be moved in > kvm_vgic_map_resources() too. Good point. FWIW, I have added an extra patch on top to that effect. Thanks, M.
diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c index 9d69d2bf6943..65a5e89f5133 100644 --- a/arch/arm64/kvm/arm.c +++ b/arch/arm64/kvm/arm.c @@ -530,11 +530,9 @@ static int kvm_vcpu_first_run_init(struct kvm_vcpu *vcpu) * Map the VGIC hardware resources before running a vcpu the * first time on this VM. */ - if (unlikely(!vgic_ready(kvm))) { - ret = kvm_vgic_map_resources(kvm); - if (ret) - return ret; - } + ret = kvm_vgic_map_resources(kvm); + if (ret) + return ret; } else { /* * Tell the rest of the code that there are userspace irqchip diff --git a/arch/arm64/kvm/vgic/vgic-init.c b/arch/arm64/kvm/vgic/vgic-init.c index 32e32d67a127..a2f4d1c85f00 100644 --- a/arch/arm64/kvm/vgic/vgic-init.c +++ b/arch/arm64/kvm/vgic/vgic-init.c @@ -428,7 +428,13 @@ int kvm_vgic_map_resources(struct kvm *kvm) struct vgic_dist *dist = &kvm->arch.vgic; int ret = 0; + if (likely(vgic_ready(kvm))) + return 0; + mutex_lock(&kvm->lock); + if (vgic_ready(kvm)) + goto out; + if (!irqchip_in_kernel(kvm)) goto out; diff --git a/arch/arm64/kvm/vgic/vgic-v2.c b/arch/arm64/kvm/vgic/vgic-v2.c index ebf53a4e1296..7f38c1a93639 100644 --- a/arch/arm64/kvm/vgic/vgic-v2.c +++ b/arch/arm64/kvm/vgic/vgic-v2.c @@ -306,9 +306,6 @@ int vgic_v2_map_resources(struct kvm *kvm) struct vgic_dist *dist = &kvm->arch.vgic; int ret = 0; - if (vgic_ready(kvm)) - goto out; - if (IS_VGIC_ADDR_UNDEF(dist->vgic_dist_base) || IS_VGIC_ADDR_UNDEF(dist->vgic_cpu_base)) { kvm_err("Need to set vgic cpu and dist addresses first\n"); diff --git a/arch/arm64/kvm/vgic/vgic-v3.c b/arch/arm64/kvm/vgic/vgic-v3.c index 9cdf39a94a63..35029c5cb0f1 100644 --- a/arch/arm64/kvm/vgic/vgic-v3.c +++ b/arch/arm64/kvm/vgic/vgic-v3.c @@ -500,9 +500,6 @@ int vgic_v3_map_resources(struct kvm *kvm) int ret = 0; int c; - if (vgic_ready(kvm)) - goto out; - kvm_for_each_vcpu(c, vcpu, kvm) { struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
kvm_vgic_map_resources() is called when a VCPU if first run and it maps all the VGIC MMIO regions. To prevent double-initialization, the VGIC uses the ready variable to keep track of the state of resources and the global KVM mutex to protect against concurrent accesses. After the lock is taken, the variable is checked again in case another VCPU took the lock between the current VCPU reading ready equals false and taking the lock. The double-checked lock pattern is spread across four different functions: in kvm_vcpu_first_run_init(), in kvm_vgic_map_resource() and in vgic_{v2,v3}_map_resources(), which makes it hard to reason about and introduces minor code duplication. Consolidate the checks in kvm_vgic_map_resources(), where the lock is taken. No functional change intended. Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com> --- arch/arm64/kvm/arm.c | 8 +++----- arch/arm64/kvm/vgic/vgic-init.c | 6 ++++++ arch/arm64/kvm/vgic/vgic-v2.c | 3 --- arch/arm64/kvm/vgic/vgic-v3.c | 3 --- 4 files changed, 9 insertions(+), 11 deletions(-)