Received-SPF: pass (google.com: domain of linux-kernel+bounces-168885-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240426041559.3717884-1-foxywang@tencent.com>
 <20240426041559.3717884-2-foxywang@tencent.com> <ZjVgWvrXyyVYXoxj@google.com>
In-Reply-To: <ZjVgWvrXyyVYXoxj@google.com>
From: Yi Wang <up2wing@gmail.com>
Date: Sun, 5 May 2024 12:56:39 +0800
Message-ID: <CAN35MuTTy+ZxVHfFjPAGVDpqA_teGBg7vi9btCL8oQe+iUqnHg@mail.gmail.com>
Subject: Re: [v4 RESEND 1/3] KVM: setup empty irq routing when create vm
To: Sean Christopherson <seanjc@google.com>
Cc: pbonzini@redhat.com, tglx@linutronix.de, mingo@redhat.com, bp@alien8.de, 
	dave.hansen@linux.intel.com, x86@kernel.org, hpa@zytor.com, 
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wanpengli@tencent.com, 
	foxywang@tencent.com, oliver.upton@linux.dev, maz@kernel.org, 
	anup@brainfault.org, atishp@atishpatra.org, borntraeger@linux.ibm.com, 
	frankja@linux.ibm.com, imbrenda@linux.ibm.com, weijiang.yang@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Sean,

Thanks so much for your patient reply.

On Sat, May 4, 2024 at 6:08=E2=80=AFAM Sean Christopherson <seanjc@google.c=
om> wrote:
>
> On Fri, Apr 26, 2024, Yi Wang wrote:
> > From: Yi Wang <foxywang@tencent.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@tencent.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 =3D kzalloc(struct_size(new, map, 1), GFP_KERNEL_ACCOUNT);
> > +     if (!new)
> > +             return -ENOMEM;
> > +
> > +     new->nr_rt_entries =3D 1;
> > +
> > +     chip_size =3D sizeof(int) * KVM_NR_IRQCHIPS * KVM_IRQCHIP_NUM_PIN=
S;
> > +     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 t=
ype, const char *fdname)
> >       if (r)
> >               goto out_err;
> >
> > +     r =3D 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 t=
his is
> to kvm_arch_post_init_vm(), which quite strongly suggests that *all* comm=
on 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 typ=
e, const char *fdname)
>                 rcu_assign_pointer(kvm->buses[i],
>                         kzalloc(sizeof(struct kvm_io_bus), GFP_KERNEL_ACC=
OUNT));
>                 if (!kvm->buses[i])
> -                       goto out_err_no_arch_destroy_vm;
> +                       goto out_err_no_irq_routing;
>         }
>
> +       r =3D kvm_init_irq_routing(kvm);
> +       if (r)
> +               goto out_err_no_irq_routing;
> +
>         r =3D 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 =3D 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);
>


--=20
---
Best wishes
Yi Wang