> -----Original Message-----
> From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Sent: Thursday, April 26, 2018 3:17 PM
> To: KY Srinivasan <kys@xxxxxxxxxxxxx>
> Cc: x86@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx;
> apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx; hpa@xxxxxxxxx; Stephen
> Hemminger <sthemmin@xxxxxxxxxxxxx>; Michael Kelley (EOSG)
> <Michael.H.Kelley@xxxxxxxxxxxxx>; vkuznets@xxxxxxxxxx
> Subject: Re: [PATCH 3/5] X86: Hyper-V: Enhanced IPI enlightenment
>
> On Wed, 25 Apr 2018, kys@xxxxxxxxxxxxxxxxx wrote:
> >
> > +struct ipi_arg_ex {
> > + u32 vector;
> > + u32 reserved;
> > + struct hv_vpset vp_set;
>
> Please align that in tabular fashion for easy of reading
>
> u32 vector;
> u32 reserved;
> struct hv_vpset vp_set;
>
> > +};
> > +
> > static struct apic orig_apic;
> >
> > static u64 hv_apic_icr_read(void)
> > @@ -97,6 +103,40 @@ static void hv_apic_eoi_write(u32 reg, u32 val)
> > * IPI implementation on Hyper-V.
> > */
> >
> > +static int __send_ipi_mask_ex(const struct cpumask *mask, int vector)
> > +{
> > + int nr_bank = 0;
> > + struct ipi_arg_ex **arg;
> > + struct ipi_arg_ex *ipi_arg;
> > + int ret = 1;
> > + unsigned long flags;
>
> This is really horrible to read.
>
> struct ipi_arg_ex *ipi_arg;
> struct ipi_arg_ex **arg;
> unsigned long flags;
> bool ret = false;
> int nr_bank = 0;
>
> is really more conveniant for quick reading.
>
> So the other more limited function has a lot more sanity checks vs. vector
> number and other things. Why are they not required here? Comment
> please.
Yes, I will add the comments. This function is called from the other function
after all the sanity checks have been done and hence are not replicated here.
>
> > + local_irq_save(flags);
> > + arg = (struct ipi_arg_ex **)this_cpu_ptr(hyperv_pcpu_input_arg);
> > +
> > + ipi_arg = *arg;
> > + if (unlikely(!ipi_arg))
> > + goto ipi_mask_ex_done;
> > +
> > + ipi_arg->vector = vector;
> > + ipi_arg->reserved = 0;
> > + ipi_arg->vp_set.valid_bank_mask = 0;
> > +
> > + if (!cpumask_equal(mask, cpu_present_mask)) {
> > + ipi_arg->vp_set.format = HV_GENERIC_SET_SPARCE_4K;
> > + nr_bank = cpumask_to_vpset(&(ipi_arg->vp_set), mask);
>
> nr_bank really confused me. bank_nr is what you mean, not number of
> banks,
> right?
It is the number of banks. The hypercall used here is a variable length
hypercall.
Regards,
K. Y
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
