Date: Sun, 5 May 2024 12:56:39 +0800
From: Yi Wang <up2wing@...il.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: pbonzini@...hat.com, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com, 
	kvm@...r.kernel.org, linux-kernel@...r.kernel.org, wanpengli@...cent.com, 
	foxywang@...cent.com, oliver.upton@...ux.dev, maz@...nel.org, 
	anup@...infault.org, atishp@...shpatra.org, borntraeger@...ux.ibm.com, 
	frankja@...ux.ibm.com, imbrenda@...ux.ibm.com, weijiang.yang@...el.com
Subject: Re: [v4 RESEND 1/3] KVM: setup empty irq routing when create vm

Hi Sean,

Thanks so much for your patient reply.

On Sat, May 4, 2024 at 6:08 AM Sean Christopherson <seanjc@...gle.com> wrote:
>
> On Fri, Apr 26, 2024, Yi Wang wrote:
> > From: Yi Wang <foxywang@...cent.com>
> >
> > Add a new function to setup empty irq routing in kvm path, which
> > can be invoded in non-architecture-specific functions. The difference
> > compared to the kvm_setup_empty_irq_routing() is this function just
> > alloc the empty irq routing and does not need synchronize srcu, as
> > we will call it in kvm_create_vm().
> >
> > Using the new adding function, we can setup empty irq routing when
> > kvm_create_vm(), so that x86 and s390 no longer need to set
> > empty/dummy irq routing when creating an IRQCHIP 'cause it avoid
> > an synchronize_srcu.
> >
> > Signed-off-by: Yi Wang <foxywang@...cent.com>
> > ---
> >  include/linux/kvm_host.h |  1 +
> >  virt/kvm/irqchip.c       | 19 +++++++++++++++++++
> >  virt/kvm/kvm_main.c      |  4 ++++
> >  3 files changed, 24 insertions(+)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 48f31dcd318a..9256539139ef 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -2100,6 +2100,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
> >                       const struct kvm_irq_routing_entry *entries,
> >                       unsigned nr,
> >                       unsigned flags);
> > +int kvm_setup_empty_irq_routing_lockless(struct kvm *kvm);
>
> This is in an #ifdef, the #else needs a stub (for MIPS).

Okay, I'll update the patch.

>
> >  int kvm_set_routing_entry(struct kvm *kvm,
> >                         struct kvm_kernel_irq_routing_entry *e,
> >                         const struct kvm_irq_routing_entry *ue);
> > diff --git a/virt/kvm/irqchip.c b/virt/kvm/irqchip.c
> > index 1e567d1f6d3d..266bab99a8a8 100644
> > --- a/virt/kvm/irqchip.c
> > +++ b/virt/kvm/irqchip.c
> > @@ -237,3 +237,22 @@ int kvm_set_irq_routing(struct kvm *kvm,
> >
> >       return r;
> >  }
> > +
> > +int kvm_setup_empty_irq_routing_lockless(struct kvm *kvm)
>
> I vote for kvm_init_irq_routing() to make it more obvious that the API is purely
> for initializing the routing, i.e. only to be used at VM creation.  Then the
> "lockless" tag is largely redundant.  And then maybe add a comment about how this
> creates an empty routing table?  Because every time I look at this code, it takes
> me a few seconds to remember how this is actually an empty table.

That sounds reasonable to me.

>
> > +{
> > +     struct kvm_irq_routing_table *new;
> > +     int chip_size;
> > +
> > +     new = kzalloc(struct_size(new, map, 1), GFP_KERNEL_ACCOUNT);
> > +     if (!new)
> > +             return -ENOMEM;
> > +
> > +     new->nr_rt_entries = 1;
> > +
> > +     chip_size = sizeof(int) * KVM_NR_IRQCHIPS * KVM_IRQCHIP_NUM_PINS;
> > +     memset(new->chip, -1, chip_size);
> > +
> > +     RCU_INIT_POINTER(kvm->irq_routing, new);
> > +
> > +     return 0;
> > +}
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index ff0a20565f90..b5f4fa9d050d 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -1285,6 +1285,10 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
> >       if (r)
> >               goto out_err;
> >
> > +     r = kvm_setup_empty_irq_routing_lockless(kvm);
> > +     if (r)
> > +             goto out_err;
>
> This is too late.  It might not matter in practice, but the call before this is
> to kvm_arch_post_init_vm(), which quite strongly suggests that *all* common setup
> is done before that arch hook is invoked.
>
> Calling this right before kvm_arch_init_vm() seems like the best/easiest fit, e.g.

Got it. I will update the patch ASAP, before that I will do some tests.

Thanks again, Sean.

>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2e388972d856..ab607441686f 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1197,9 +1197,13 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
>                 rcu_assign_pointer(kvm->buses[i],
>                         kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACCOUNT));
>                 if (!kvm->buses[i])
> -                       goto out_err_no_arch_destroy_vm;
> +                       goto out_err_no_irq_routing;
>         }
>
> +       r = kvm_init_irq_routing(kvm);
> +       if (r)
> +               goto out_err_no_irq_routing;
> +
>         r = kvm_arch_init_vm(kvm, type);
>         if (r)
>                 goto out_err_no_arch_destroy_vm;
> @@ -1254,6 +1258,8 @@ static struct kvm *kvm_create_vm(unsigned long type, const char *fdname)
>         WARN_ON_ONCE(!refcount_dec_and_test(&kvm->users_count));
>         for (i = 0; i < KVM_NR_BUSES; i++)
>                 kfree(kvm_get_bus(kvm, i));
> +       kvm_free_irq_routing(kvm);
> +out_err_no_irq_routing:
>         cleanup_srcu_struct(&kvm->irq_srcu);
>  out_err_no_irq_srcu:
>         cleanup_srcu_struct(&kvm->srcu);
>


-- 
---
Best wishes
Yi Wang

Powered by blists - more mailing lists