From: Yi Wang <up2wing@gmail.com>
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
Subject: Re: [v4 RESEND 1/3] KVM: setup empty irq routing when create vm
Date: Sun, 5 May 2024 12:56:39 +0800 [thread overview]
Message-ID: <CAN35MuTTy+ZxVHfFjPAGVDpqA_teGBg7vi9btCL8oQe+iUqnHg@mail.gmail.com> (raw)
In-Reply-To: <ZjVgWvrXyyVYXoxj@google.com>
Hi Sean,
Thanks so much for your patient reply.
On Sat, May 4, 2024 at 6:08 AM Sean Christopherson <seanjc@google.com> 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 = 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
next prev parent reply other threads:[~2024-05-05 4:56 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-26 4:15 [v4 RESEND 0/3] KVM: irqchip: synchronize srcu only if needed Yi Wang
2024-04-26 4:15 ` [v4 RESEND 1/3] KVM: setup empty irq routing when create vm Yi Wang
2024-05-03 22:08 ` Sean Christopherson
2024-05-05 4:56 ` Yi Wang [this message]
2024-04-26 4:15 ` [v4 RESEND 2/3] KVM: x86: don't setup empty irq routing when KVM_CAP_SPLIT_IRQCHIP Yi Wang
2024-04-26 4:15 ` [v4 RESEND 3/3] KVM: s390: don't setup dummy routing when KVM_CREATE_IRQCHIP Yi Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAN35MuTTy+ZxVHfFjPAGVDpqA_teGBg7vi9btCL8oQe+iUqnHg@mail.gmail.com \
--to=up2wing@gmail.com \
--cc=anup@brainfault.org \
--cc=atishp@atishpatra.org \
--cc=borntraeger@linux.ibm.com \
--cc=bp@alien8.de \
--cc=dave.hansen@linux.intel.com \
--cc=foxywang@tencent.com \
--cc=frankja@linux.ibm.com \
--cc=hpa@zytor.com \
--cc=imbrenda@linux.ibm.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=maz@kernel.org \
--cc=mingo@redhat.com \
--cc=oliver.upton@linux.dev \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=tglx@linutronix.de \
--cc=wanpengli@tencent.com \
--cc=weijiang.yang@intel.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).