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

[KY Srinivasan]

> -Original Message-
> From: Ingo Molnar [mailto:mingo.kernel@gmail.com] On Behalf Of Ingo
> Molnar
> Sent: Wednesday, March 25, 2015 5:15 AM
> To: KY Srinivasan
> Cc: x...@kernel.org; gre...@linuxfoundation.org; linux-
> ker...@vger.kernel.org; de...@linuxdriverproject.org; o...@aepfle.de;
> a...@canonical.com; jasow...@redhat.com; t...@linutronix.de;
> h...@zytor.com
> Subject: Re: [PATCH V2 1/1] X86: hyperv: Enable MSR based APIC access
> 
> 
> * K. Y. Srinivasan  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 
> > ---
> >  Changes from V1: Addressed comments from Ingo Molnar
> 
> 
> You didn't address my primary feedback:
> 
> > > 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?

Ingo,

I added the code for advertising support for x2APIC since I saw the feature 
flag in
the public documentation of Hyper-V. Subsequently, when I checked with the 
Hyper-V
developers, I learned that Hyper-V does not support x2APIC for guests. So, I 
got rid
of the code that checked for this flag as that would be dead code.  
> > >
> > > I.e. what are the benefits of using the x2apic API towards the
> > > hypervisor?
> 
> please explain the purpose, context and benefits, not just what the
> patch does - even if the comment from your first patch is now gone.

The MSR based access can be a little more efficient than the mmio based access
to the select APIC registers. That is the intent of this patch. Do you want me 
to resubmit
the patch with these additional details.

Regards,

K. Y
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Ingo Molnar]

* K. Y. Srinivasan  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 
> ---
>  Changes from V1: Addressed comments from Ingo Molnar 
> 

You didn't address my primary feedback:

> > 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?

please explain the purpose, context and benefits, not just what the 
patch does - even if the comment from your first patch is now gone.

Thanks,

Ingo
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[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 
---
 Changes from V1: Addressed comments from Ingo Molnar 


 arch/x86/kernel/cpu/mshyperv.c |   62 
 1 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 939155f..76833c6 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -110,6 +110,57 @@ 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_ICR, reg_val);
+}
+
+static u32 ms_hv_apic_read(u32 reg)
+{
+   u32 reg_val, hi;
+
+   switch (reg) {
+   case APIC_EOI:
+   rdmsr(HV_X64_MSR_EOI, reg_val, hi);
+   return reg_val;
+   case APIC_TASKPRI:
+   rdmsr(HV_X64_MSR_TPR, reg_val, hi);
+   return reg_val;
+
+   default:
+   return native_apic_mem_read(reg);
+   }
+}
+
+static void ms_hv_apic_write(u32 reg, u32 val)
+{
+   switch (reg) {
+   case APIC_EOI:
+   wrmsr(HV_X64_MSR_EOI, val, 0);
+   break;
+   case APIC_TASKPRI:
+   wrmsr(HV_X64_MSR_TPR, val, 0);
+   break;
+   default:
+   native_apic_mem_write(reg, val);
+   }
+}
+
+
 static void __init ms_hyperv_init_platform(void)
 {
/*
@@ -143,6 +194,17 @@ 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;
+   }
+
 }
 
 const __refconst struct hypervisor_x86 x86_hyper_ms_hyperv = {
-- 
1.7.4.1

___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel