LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/1] X86: hyperv: Enable MSR based APIC access
@ 2015-03-16  6:12 K. Y. Srinivasan
  2015-03-16  7:36 ` Ingo Molnar
  0 siblings, 1 reply; 3+ messages in thread
From: K. Y. Srinivasan @ 2015-03-16  6:12 UTC (permalink / raw)
  To: x86, gregkh, linux-kernel, devel, olaf, apw, jasowang, tglx, hpa
  Cc: K. Y. Srinivasan

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@microsoft.com>
---
 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.
+ */
+#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);
+	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;
+
+	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);
+	}
+}
+
+
 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;
+	}
+
+}
+
+static bool ms_hyperv_x2apic(void)
+{
+	if (ms_hyperv.hints & HV_X64_X2APIC_MSRS_RECOMMENDED)
+		return true;
+	else
+		return false;
 }
 
 const __refconst struct hypervisor_x86 x86_hyper_ms_hyperv = {
 	.name			= "Microsoft HyperV",
 	.detect			= ms_hyperv_platform,
 	.init_platform		= ms_hyperv_init_platform,
+	.x2apic_available	= ms_hyperv_x2apic
 };
 EXPORT_SYMBOL(x86_hyper_ms_hyperv);
-- 
1.7.4.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH 1/1] X86: hyperv: Enable MSR based APIC access
  2015-03-16  6:12 [PATCH 1/1] X86: hyperv: Enable MSR based APIC access K. Y. Srinivasan
@ 2015-03-16  7:36 ` Ingo Molnar
  2015-03-16 14:12   ` KY Srinivasan
  0 siblings, 1 reply; 3+ messages in thread
From: Ingo Molnar @ 2015-03-16  7:36 UTC (permalink / raw)
  To: K. Y. Srinivasan
  Cc: x86, gregkh, linux-kernel, devel, olaf, apw, jasowang, tglx, hpa


* K. Y. Srinivasan <kys@microsoft.com> 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@microsoft.com>
> ---
>  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,

	Ingo

^ permalink raw reply	[flat|nested] 3+ messages in thread

* RE: [PATCH 1/1] X86: hyperv: Enable MSR based APIC access
  2015-03-16  7:36 ` Ingo Molnar
@ 2015-03-16 14:12   ` KY Srinivasan
  0 siblings, 0 replies; 3+ messages in thread
From: KY Srinivasan @ 2015-03-16 14:12 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: x86, gregkh, linux-kernel, devel, olaf, apw, jasowang, tglx, hpa



> -----Original Message-----
> From: Ingo Molnar [mailto:mingo.kernel.org@gmail.com] On Behalf Of Ingo
> Molnar
> Sent: Monday, March 16, 2015 12:37 AM
> To: KY Srinivasan
> Cc: x86@kernel.org; gregkh@linuxfoundation.org; linux-
> kernel@vger.kernel.org; devel@linuxdriverproject.org; olaf@aepfle.de;
> apw@canonical.com; jasowang@redhat.com; tglx@linutronix.de;
> hpa@zytor.com
> Subject: Re: [PATCH 1/1] X86: hyperv: Enable MSR based APIC access
> 
> 
> * K. Y. Srinivasan <kys@microsoft.com> 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@microsoft.com>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2015-03-16 14:12 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-16  6:12 [PATCH 1/1] X86: hyperv: Enable MSR based APIC access K. Y. Srinivasan
2015-03-16  7:36 ` Ingo Molnar
2015-03-16 14:12   ` KY Srinivasan

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 NNTP newsgroup(s).