On Mon, 2 Jul 2018 at 18:26, Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>
> Wanpeng Li <kernellwp@xxxxxxxxx> writes:
>
> > From: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> >
> > Implement PV IPIs in guest kernel.
> >
> > Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> > Cc: Radim Krčmář <rkrcmar@xxxxxxxxxx>
> > Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> > Signed-off-by: Wanpeng Li <wanpengli@xxxxxxxxxxx>
> > ---
> > arch/x86/kernel/kvm.c | 99 +++++++++++++++++++++++++++++++++++++++++++
> > include/uapi/linux/kvm_para.h | 1 +
> > 2 files changed, 100 insertions(+)
> >
> > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> > index 5b2300b..7e3ee25 100644
> > --- a/arch/x86/kernel/kvm.c
> > +++ b/arch/x86/kernel/kvm.c
> > @@ -47,6 +47,7 @@
> > #include <asm/hypervisor.h>
> > #include <asm/kvm_guest.h>
> >
> > +static struct apic orig_apic;
> > static int kvmapf = 1;
> >
> > static int __init parse_no_kvmapf(char *arg)
> > @@ -454,6 +455,89 @@ static void __init sev_map_percpu_data(void)
> > }
> >
> > #ifdef CONFIG_SMP
> > +
> > +#ifdef CONFIG_X86_64
> > +static bool __send_ipi_mask(const struct cpumask *mask, int vector)
> > +{
> > + unsigned long flags, ipi_bitmap_low = 0, ipi_bitmap_high = 0, icr = 0;
> > + int cpu, apic_id, ret = 1;
> > +
> > + if (cpumask_empty(mask))
> > + return true;
> > +
> > + local_irq_save(flags);
> > +
> > + for_each_cpu(cpu, mask) {
> > + apic_id = per_cpu(x86_cpu_to_apicid, cpu);
> > + if (apic_id < BITS_PER_LONG)
> > + __set_bit(apic_id, &ipi_bitmap_low);
> > + else if (apic_id < 2 * BITS_PER_LONG)
> > + __set_bit(apic_id - BITS_PER_LONG, &ipi_bitmap_high);
> > + else
> > + goto ipi_mask_done;
>
> Nit:
>
> Both the fact that we don't set 'ret' here and the fact that the label
> is named 'ipi_mask_done' -- which sounds like 'all OK' at least to me --
> contribute to the feeling that we just skip sending IPIs in some cases.
>
> I would prefer to see something like
>
> else {
> ret = -EFAULT;
> goto irq_restore_exit;
> }
>
> > + }
> > +
> > + switch (vector) {
> > + default:
> > + icr = APIC_DM_FIXED | vector;
> > + break;
> > + case NMI_VECTOR:
> > + icr = APIC_DM_NMI;
> > + break;
> > + }
> > +
> > + ret = kvm_hypercall3(KVM_HC_SEND_IPI, ipi_bitmap_low, ipi_bitmap_high, icr);
> > +
> > +ipi_mask_done:
> > + local_irq_restore(flags);
> > + return ((ret == 0) ? true : false);
>
> ... and why in the first place do we need to make this function return
> 'bool' then? Let's just make it return 'int'.
Thanks for the comments, will do in v3. Btw, on my haswell desktop(i7
8 HT), there is a 2.5 times performance boot for the IPI
microbenchmark(https://lkml.org/lkml/2017/12/19/141). (8 vCPUs guest,
x2apic physical mode, I will retest on Skylake server w/ 64 vCPUs
x2apic cluster mode guest tomorrow):
Before:
Dry-run: 0, 1885493 ns
Self-IPI: 7071403, 14711151 ns
Normal IPI: 204453899, 219896346 ns
Broadcast IPI: 0, 2213679722 ns
Broadcast lock: 0, 2241226307 ns
After:
Dry-run: 0, 1752903 ns
Self-IPI: 4944737, 10434149 ns
Normal IPI: 202351280, 220807969 ns
Broadcast IPI: 0, 872991742 ns
=> 2.5 times boost
Broadcast lock: 0, 879995113 ns
Regards,
Wanpeng Li
