> -----Original Message----- > From: Ingo Molnar [mailto:mingo.kernel....@gmail.com] On Behalf Of Ingo > Molnar > Sent: Monday, March 16, 2015 12:37 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 1/1] X86: hyperv: Enable MSR based APIC access > > > * K. Y. Srinivasan <k...@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 <k...@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 _______________________________________________ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
