RE: [PATCH 1/1] X86: hyperv: Enable MSR based APIC access

[KY Srinivasan]

> -----Original Message-----
> From: Ingo Molnar [mailto:mingo.kernel.org@xxxxxxxxx] On Behalf Of Ingo
> Molnar
> Sent: Monday, March 16, 2015 12:37 AM
> To: KY Srinivasan
> Cc: x86@xxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; olaf@xxxxxxxxx;
> apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx; tglx@xxxxxxxxxxxxx;
> hpa@xxxxxxxxx
> Subject: Re: [PATCH 1/1] X86: hyperv: Enable MSR based APIC access
> 
> 
> * K. Y. Srinivasan <kys@xxxxxxxxxxxxx> wrote:
> 
> > If the hypervisor supports MSR based access to the APIC registers
> > (EOI, TPR and ICR), implement the MSR based access.
> >
> > Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> > ---
> >  arch/x86/include/uapi/asm/hyperv.h |    5 +++
> >  arch/x86/kernel/cpu/mshyperv.c     |   69
> ++++++++++++++++++++++++++++++++++++
> >  2 files changed, 74 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/x86/include/uapi/asm/hyperv.h
> > b/arch/x86/include/uapi/asm/hyperv.h
> > index 90c458e..6ce69e0 100644
> > --- a/arch/x86/include/uapi/asm/hyperv.h
> > +++ b/arch/x86/include/uapi/asm/hyperv.h
> > @@ -140,6 +140,11 @@
> >   */
> >  #define HV_X64_RELAXED_TIMING_RECOMMENDED	(1 << 5)
> >
> > +/*
> > + * Recommend using x2APIC MSRs.
> 
> So since we are trying to explain things, wouldn't this comment be more
> informative if it explained why we are trying to use the x2APIC facilities of
> Hyper-V?
> 
> I.e. what are the benefits of using the x2apic API towards the hypervisor?
> 
> > + */
> > +#define HV_X64_X2APIC_MSRS_RECOMMENDED       (1 << 8)
> > +
> >  /* MSR used to identify the guest OS. */
> >  #define HV_X64_MSR_GUEST_OS_ID			0x40000000
> >
> > diff --git a/arch/x86/kernel/cpu/mshyperv.c
> > b/arch/x86/kernel/cpu/mshyperv.c index 939155f..dd2eb49 100644
> > --- a/arch/x86/kernel/cpu/mshyperv.c
> > +++ b/arch/x86/kernel/cpu/mshyperv.c
> > @@ -110,6 +110,55 @@ static struct clocksource hyperv_cs = {
> >  	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> >  };
> >
> > +static u64 ms_hv_apic_icr_read(void)
> > +{
> > +	u64 reg_val;
> > +
> > +	rdmsrl(HV_X64_MSR_ICR, reg_val);
> > +	return reg_val;
> > +}
> > +
> > +static void ms_hv_apic_icr_write(u32 low, u32 id) {
> > +	u64 reg_val;
> > +
> > +	reg_val = SET_APIC_DEST_FIELD(id);
> > +	reg_val = (reg_val << 32);
> 
> Those parentheses are not needed.
> 
> > +	reg_val |= low;
> > +
> > +	wrmsrl(HV_X64_MSR_EOI, reg_val);
> > +}
> > +
> > +static u32 ms_hv_apic_read(u32 reg)
> > +{
> > +	u64 reg_val;
> > +
> > +	switch (reg) {
> > +	case APIC_EOI:
> > +	case APIC_TASKPRI:
> > +		rdmsrl(HV_X64_MSR_EOI, reg_val);
> > +		return reg_val;
> 
> So wouldn't it be faster to use u32 for 'reg_val' and rdmsr() instead of u64
> and rdmsrl()? This 64-bit read just throws away the high 32 bits.
> 
> > +
> > +	default:
> > +		return native_apic_mem_read(reg);
> > +	}
> > +}
> > +
> > +static void ms_hv_apic_write(u32 reg, u32 val) {
> > +	u64 reg_val;
> > +
> > +	reg_val =  val;
> > +	switch (reg) {
> > +	case APIC_EOI:
> > +	case APIC_TASKPRI:
> > +		wrmsrl(HV_X64_MSR_EOI, reg_val);
> > +	default:
> > +		native_apic_mem_write(reg, val);
> > +	}
> > +}
> 
> Same observation: it would be faster to use a 32-bit WRMSR.
> 
> > +
> > +
> >  static void __init ms_hyperv_init_platform(void)  {
> >  	/*
> > @@ -143,11 +192,31 @@ static void __init ms_hyperv_init_platform(void)
> >  	no_timer_check = 1;
> >  #endif
> >
> > +	if (ms_hyperv.features & HV_X64_MSR_APIC_ACCESS_AVAILABLE) {
> > +		/*
> > +		 * Setup the hooks for optimized APIC read/write.
> > +		 */
> > +		apic->read = ms_hv_apic_read;
> > +		apic->write = ms_hv_apic_write;
> > +		apic->icr_write = ms_hv_apic_icr_write;
> > +		apic->icr_read = ms_hv_apic_icr_read;
> > +		apic->eoi_write = ms_hv_apic_write;
> 
> Please align the initialization vertically via tabs, like 'x86_hyper_ms_hyperv' is
> initialized.
> 
> > +	}
> > +
> > +}
> > +
> > +static bool ms_hyperv_x2apic(void)
> > +{
> > +	if (ms_hyperv.hints & HV_X64_X2APIC_MSRS_RECOMMENDED)
> > +		return true;
> > +	else
> > +		return false;
> >  }
> 
> Isn't this shorter:
> 
> 	return (ms_hyperv.hints &
> HV_X64_X2APIC_MSRS_RECOMMENDED) != 0;
> 
> ?
> 
> Thanks,

Thank you. I will address your comments and resend the patch.

Regards,

K. Y
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/