From: Peng Hao <flyingpeng@tencent.com>

kvm->irq_routing is protected by kvm->irq_srcu.

Signed-off-by: Peng Hao <flyingpeng@tencent.com>
---
 virt/kvm/irqchip.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 1e567d1f6d3d..90f54f04e37c 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm,
        }

        mutex_lock(&kvm->irq_lock);
-       old = rcu_dereference_protected(kvm->irq_routing, 1);
+       old = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
+                                       lockdep_is_held(&kvm->irq_lock));
        rcu_assign_pointer(kvm->irq_routing, new);
        kvm_irq_routing_update(kvm);
        kvm_arch_irq_routing_update(kvm);
--
2.27.0

On Thu, Dec 08, 2022, Hao Peng wrote:
> From: Peng Hao <flyingpeng@tencent.com>
> 
> kvm->irq_routing is protected by kvm->irq_srcu.
> 
> Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> ---
>  virt/kvm/irqchip.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index 1e567d1f6d3d..90f54f04e37c 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm,
>         }
> 
>         mutex_lock(&kvm->irq_lock);
> -       old = rcu_dereference_protected(kvm->irq_routing, 1);
> +       old = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
> +                                       lockdep_is_held(&kvm->irq_lock));

Readers of irq_routing are protected via kvm->irq_srcu, but this writer is never
called with kvm->irq_srcu held.  I do like the of replacing '1' with
lockdep_is_held(&kvm->irq_lock) to document the protection, so what about just
doing that?  I.e.

diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
index 1e567d1f6d3d..77a18b4dc103 100644
--- a/virt/kvm/irqchip.c
+++ b/virt/kvm/irqchip.c
@@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm,
        }
 
        mutex_lock(&kvm->irq_lock);
-       old = rcu_dereference_protected(kvm->irq_routing, 1);
+       old = rcu_dereference_protected(kvm->irq_routing,
+                                       lockdep_is_held(&kvm->irq_lock));
        rcu_assign_pointer(kvm->irq_routing, new);
        kvm_irq_routing_update(kvm);
        kvm_arch_irq_routing_update(kvm);


>         rcu_assign_pointer(kvm->irq_routing, new);
>         kvm_irq_routing_update(kvm);
>         kvm_arch_irq_routing_update(kvm);
> --
> 2.27.0

On Fri, Dec 9, 2022 at 9:22 AM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Dec 08, 2022, Hao Peng wrote:
> > From: Peng Hao <flyingpeng@tencent.com>
> >
> > kvm->irq_routing is protected by kvm->irq_srcu.
> >
> > Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> > ---
> >  virt/kvm/irqchip.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> > index 1e567d1f6d3d..90f54f04e37c 100644
> > --- a/virt/kvm/irqchip.c
> > +++ b/virt/kvm/irqchip.c
> > @@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm,
> >         }
> >
> >         mutex_lock(&kvm->irq_lock);
> > -       old = rcu_dereference_protected(kvm->irq_routing, 1);
> > +       old = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
> > +                                       lockdep_is_held(&kvm->irq_lock));
>
> Readers of irq_routing are protected via kvm->irq_srcu, but this writer is never
> called with kvm->irq_srcu held.  I do like the of replacing '1' with
> lockdep_is_held(&kvm->irq_lock) to document the protection, so what about just
> doing that?  I.e.
>

Sorry for the long delay in replying. Although kvm->irq_srcu is not required
to protect irq_routing here, this interface function srcu_dereference_check
indicates that irq_routing is protected by kvm->irq_srcu in the kvm subsystem.
Thanks.

> diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> index 1e567d1f6d3d..77a18b4dc103 100644
> --- a/virt/kvm/irqchip.c
> +++ b/virt/kvm/irqchip.c
> @@ -216,7 +216,8 @@ int kvm_set_irq_routing(struct kvm *kvm,
>         }
>
>         mutex_lock(&kvm->irq_lock);
> -       old = rcu_dereference_protected(kvm->irq_routing, 1);
> +       old = rcu_dereference_protected(kvm->irq_routing,
> +                                       lockdep_is_held(&kvm->irq_lock));
>         rcu_assign_pointer(kvm->irq_routing, new);
>         kvm_irq_routing_update(kvm);
>         kvm_arch_irq_routing_update(kvm);
>
>
> >         rcu_assign_pointer(kvm->irq_routing, new);
> >         kvm_irq_routing_update(kvm);
> >         kvm_arch_irq_routing_update(kvm);
> > --
> > 2.27.0

On 12/20/22 08:47, Hao Peng wrote:
>>> +       old = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
>>> +                                       lockdep_is_held(&kvm->irq_lock));
>> Readers of irq_routing are protected via kvm->irq_srcu, but this writer is never
>> called with kvm->irq_srcu held.  I do like the of replacing '1' with
>> lockdep_is_held(&kvm->irq_lock) to document the protection, so what about just
>> doing that?  I.e.
>>
> Sorry for the long delay in replying. Although kvm->irq_srcu is not required
> to protect irq_routing here, this interface function srcu_dereference_check
> indicates that irq_routing is protected by kvm->irq_srcu in the kvm subsystem.
> Thanks.
> 

I agree, the last two arguments basically are alternative conditions to 
satisfy the check:

#define srcu_dereference_check(p, ssp, c) \
         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
                                 (c) || srcu_read_lock_held(ssp), __rcu)

The idea is to share the code between readers and writers, so what do 
you think of adding a

#define kvm_get_irq_routing(kvm) srcu_dereference_check(...)

macro at the top of virt/kvm/irqchip.c?

Thanks,

Paolo

On Fri, Dec 23, 2022, Paolo Bonzini wrote:
> On 12/20/22 08:47, Hao Peng wrote:
> > > > +       old = srcu_dereference_check(kvm->irq_routing, &kvm->irq_srcu,
> > > > +                                       lockdep_is_held(&kvm->irq_lock));
> > > Readers of irq_routing are protected via kvm->irq_srcu, but this writer is never
> > > called with kvm->irq_srcu held.  I do like the of replacing '1' with
> > > lockdep_is_held(&kvm->irq_lock) to document the protection, so what about just
> > > doing that?  I.e.
> > > 
> > Sorry for the long delay in replying. Although kvm->irq_srcu is not required
> > to protect irq_routing here, this interface function srcu_dereference_check
> > indicates that irq_routing is protected by kvm->irq_srcu in the kvm subsystem.
> > Thanks.
> > 
> 
> I agree, the last two arguments basically are alternative conditions to
> satisfy the check:
> 
> #define srcu_dereference_check(p, ssp, c) \
>         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
>                                 (c) || srcu_read_lock_held(ssp), __rcu)
> 
> The idea is to share the code between readers and writers,

But readers and writers naturally don't share code, and the subsequent
synchronize_srcu_expedited() is what really documents the interaction between
readers and writers.

It's definitely not a sticking point though, and this one does seems to be the
outlier in KVM.

> so what do you think of adding a
> 
> #define kvm_get_irq_routing(kvm) srcu_dereference_check(...)
> 
> macro at the top of virt/kvm/irqchip.c?

I'm fine with any approach, though a macro seems like overkill.