RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Michael Kelley (EOSG)]

Thomas Gleixner  writes:

> On Tue, 31 Oct 2017, mikel...@exchange.microsoft.com wrote:
> > diff --git a/arch/x86/include/uapi/asm/hyperv.h
> > b/arch/x86/include/uapi/asm/hyperv.h
> > index f65d125..408cf3e 100644
> > --- a/arch/x86/include/uapi/asm/hyperv.h
> > +++ b/arch/x86/include/uapi/asm/hyperv.h
> > @@ -112,6 +112,22 @@
> >  #define HV_X64_GUEST_IDLE_STATE_AVAILABLE  (1 << 5)
> >  /* Guest crash data handler available */
> >  #define HV_X64_GUEST_CRASH_MSR_AVAILABLE   (1 << 10)
> > +/* Debug MSRs available */
> > +#define HV_X64_DEBUG_MSR_AVAILABLE (1 << 11)
> > +/* Support for Non-Privileged Instruction Execution Prevention is 
> > available */
> > +#define HV_X64_NPIEP_AVAILABLE (1 << 12)
> > +/* Support for DisableHypervisor is available */
> > +#define HV_X64_DISABLE_HYPERVISOR_AVAILABLE(1 << 13)
> > +/* Extended GVA Ranges for Flush Virtual Address list is available */
> > +#define HV_X64_EXTENDED_GVA_RANGE_AVAILABLE(1 << 14)
> > +/* Return Hypercall output via XMM registers is available */
> > +#define HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE  (1 << 15)
> > +/* SINT polling mode available */
> > +#define HV_X64_SINT_POLLING_MODE_AVAILABLE (1 << 17)
> > +/* Hypercall MSR lock is available */
> > +#define HV_X64_HYPERCALL_MSR_LOCK_AVAILABLE(1 << 18)
> > +/* stimer direct mode is available */
> > +#define HV_X64_STIMER_DIRECT_MODE_AVAILABLE(1 << 19)
> 
> How are all these defines (except the last one related to that patch?

Will move to a separate patch.

> 
> > +/* Hardware IRQ number to use for stimer0 in Direct Mode.  This IRQ
> > +is a fake
> > + * because stimer's in Direct Mode simply interrupt on the specified
> > +vector,
> > + * without using a particular IOAPIC pin. But we use the IRQ
> > +allocation
> > + * machinery, so we need a hardware IRQ #.  This value is somewhat
> > +arbitrary,
> > + * but it should not be a legacy IRQ (0 to 15), and should fit within
> > +the
> > + * single IOAPIC (0 to 23) that Hyper-V provides to a guest VM. So
> > +any value
> > + * between 16 and 23 should be good.
> > + */
> > +#define HV_STIMER0_IRQNR   18
> 
> Why would you want abuse an IOAPIC interrupt if all you need is a vector?

Allocating a vector up-front like the existing HYPERVISOR_CALLBACK_VECTOR would 
certainly be more straightforward, and in fact, my original internal testing of 
stimer Direct Mode used that approach.   Vectors are a limited resource, so I 
wanted to find a way to allocate one on-the-fly rather than use fixed 
pre-allocation, even under CONFIG_HYPERV.   But I've got no problem with 
allocating a vector up-front and skipping all the irq/APIC manipulation and 
related issues.

Any guidance on which vector to use?

> 
> > +/* Routines to do per-architecture handling of stimer0 when in Direct
> > +Mode */
> > +
> > +void hv_ack_stimer0_interrupt(struct irq_desc *desc) {
> > +   ack_APIC_irq();
> > +}
> > +
> > +static void allonline_vector_allocation_domain(int cpu, struct cpumask 
> > *retmask,
> > +   const struct cpumask *mask)
> > +{
> > +   cpumask_copy(retmask, cpu_online_mask); }
> > +
> > +int hv_allocate_stimer0_irq(int *irq, int *vector) {
> > +   int localirq;
> > +   int result;
> > +   struct irq_data *irq_data;
> > +
> > +   /* The normal APIC vector allocation domain allows allocation of
> > +vectors
> 
> Please fix you comment style. Multi line comments are:
> 
>/*
> * Bla
>   * foo...
>   */
> 

Will do.

> > +* only for the calling CPU.  So we change the allocation domain to one
> > +* that allows vectors to be allocated in all online CPUs.  This
> > +* change is fine in a Hyper-V VM because VMs don't have the usual
> > +* complement of interrupting devices.
> > +*/
> > +   apic->vector_allocation_domain = allonline_vector_allocation_domain;
> 
> This does not work anymore. vector_allocation_domain is gone as of 4.15. 
> Please check the
> vector rework in
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip x86/apic
> 
> Aside of that what guarantees that all cpus are online at the point where you 
> allocate that
> interrupt? Nothing, so the vector will be not reserved or allocated on 
> offline CPUs. Now guess
> what happens if you bring the offline CPUs online later, it will drown in 
> spurious interrupts.
> Worse, the vector can also be reused for a device interrupt. Great plan.
> 
> > +   localirq = acpi_register_gsi(NULL, HV_STIMER0_IRQNR,
> > +   ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
> > +   if (localirq < 0) {
> > +   pr_err("Cannot register stimer0 gsi. Error %d", localirq);
> > +   return -1;
> > +   }
> > +
> > +   /* We pass in a dummy IRQ handler because architecture independent code
> > +* will later override the IRQ domain 

RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Michael Kelley (EOSG)]

Vitaly Kuznetsov  writes:

> Vitaly Kuznetsov  writes:
> 
> > mikel...@exchange.microsoft.com writes:
> >
> >> From: Michael Kelley 
> >>
> >> The 2016 version of Hyper-V offers the option to operate the guest VM
> >> per-vcpu stimer's in Direct Mode, which means the timer interupts on
> >> its own vector rather than queueing a VMbus message.  Direct Mode
> >> reduces timer processing overhead in both the hypervisor and the
> >> guest, and avoids having timer interrupts pollute the VMbus interrupt
> >> stream for the synthetic NIC and storage.  This patch enables Direct
> >> Mode by default on stimer0 (which is the only stimer used in Linux on
> >> Hyper-V) when running on a version of Hyper-V that supports it, with
> >> a hv_vmbus module parameter for disabling Direct Mode and reverting
> >> to the old behavior.
> >>
> >> Signed-off-by: Michael Kelley 
> >> ---
> >>  arch/x86/include/asm/mshyperv.h| 14 
> >>  arch/x86/include/uapi/asm/hyperv.h | 26 ++
> >>  arch/x86/kernel/cpu/mshyperv.c | 64 +-
> >>  drivers/hv/hv.c| 71 
> >> --
> >>  drivers/hv/hyperv_vmbus.h  |  4 ++-
> >>  5 files changed, 175 insertions(+), 4 deletions(-)
> >>
> 
> (in addition to my previous comment)
> 
> This patch is also x86-heavy so it probably makes sense to make x86 
> maintainers (Thomas,
> Peter, Ingo) aware no matter which tree it will go through.
> 
> CC: x...@kernel.org
> 
> >> diff --git a/arch/x86/include/asm/mshyperv.h
> >> b/arch/x86/include/asm/mshyperv.h index 740dc97..1bba1d7 100644
> >> --- a/arch/x86/include/asm/mshyperv.h
> >> +++ b/arch/x86/include/asm/mshyperv.h
> >> @@ -4,6 +4,8 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> >> +#include 
> >>  #include 
> >>  #include 
> >>
> >> @@ -374,4 +376,16 @@ static inline struct ms_hyperv_tsc_page 
> >> *hv_get_tsc_page(void)
> >>return NULL;
> >>  }
> >>  #endif
> >> +
> >> +/* Per architecture routines for stimer0 Direct Mode handling.  On
> >> +x86/x64
> >> + * there are no percpu actions to take.
> >> + */
> >> +#if IS_ENABLED(CONFIG_HYPERV)
> >> +static inline void hv_enable_stimer0_percpu_irq(int irq) { } static
> >> +inline void hv_disable_stimer0_percpu_irq(int irq) { } extern int
> >> +hv_allocate_stimer0_irq(int *irq, int *vector); extern void
> >> +hv_deallocate_stimer0_irq(int irq); extern void
> >> +hv_ack_stimer0_interrupt(struct irq_desc *desc); #endif
> >> +
> >>  #endif
> >> diff --git a/arch/x86/include/uapi/asm/hyperv.h
> >> b/arch/x86/include/uapi/asm/hyperv.h
> >> index f65d125..408cf3e 100644
> >> --- a/arch/x86/include/uapi/asm/hyperv.h
> >> +++ b/arch/x86/include/uapi/asm/hyperv.h
> >> @@ -112,6 +112,22 @@
> >>  #define HV_X64_GUEST_IDLE_STATE_AVAILABLE (1 << 5)
> >>  /* Guest crash data handler available */
> >>  #define HV_X64_GUEST_CRASH_MSR_AVAILABLE  (1 << 10)
> >> +/* Debug MSRs available */
> >> +#define HV_X64_DEBUG_MSR_AVAILABLE(1 << 11)
> >> +/* Support for Non-Privileged Instruction Execution Prevention is 
> >> available */
> >> +#define HV_X64_NPIEP_AVAILABLE(1 << 12)
> >> +/* Support for DisableHypervisor is available */
> >> +#define HV_X64_DISABLE_HYPERVISOR_AVAILABLE   (1 << 13)
> >> +/* Extended GVA Ranges for Flush Virtual Address list is available */
> >> +#define HV_X64_EXTENDED_GVA_RANGE_AVAILABLE   (1 << 14)
> >> +/* Return Hypercall output via XMM registers is available */
> >> +#define HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE (1 << 15)
> >> +/* SINT polling mode available */
> >> +#define HV_X64_SINT_POLLING_MODE_AVAILABLE(1 << 17)
> >> +/* Hypercall MSR lock is available */
> >> +#define HV_X64_HYPERCALL_MSR_LOCK_AVAILABLE   (1 << 18)
> >> +/* stimer direct mode is available */
> >> +#define HV_X64_STIMER_DIRECT_MODE_AVAILABLE   (1 << 19)
> >>
> >>  /*
> >>   * Implementation recommendations. Indicates which behaviors the
> >> hypervisor @@ -300,6 +316,16 @@ enum HV_GENERIC_SET_FORMAT {
> >>
> >>  #define HV_SYNIC_STIMER_COUNT (4)
> >>
> >> +/* Hardware IRQ number to use for stimer0 in Direct Mode.  This IRQ
> >> +is a fake
> >> + * because stimer's in Direct Mode simply interrupt on the specified
> >> +vector,
> >> + * without using a particular IOAPIC pin. But we use the IRQ
> >> +allocation
> >> + * machinery, so we need a hardware IRQ #.  This value is somewhat
> >> +arbitrary,
> >> + * but it should not be a legacy IRQ (0 to 15), and should fit
> >> +within the
> >> + * single IOAPIC (0 to 23) that Hyper-V provides to a guest VM. So
> >> +any value
> >> + * between 16 and 23 should be good.
> >> + */
> >
> > (nitpick): all comments in the patch are in 'net' style:
> >
> >  /* This is a
> >   * comment.
> >   */
> >
> > While 

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[kbuild test robot]

Hi Michael,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/master]

url:
https://github.com/0day-ci/linux/commits/mikelley-exchange-microsoft-com/Drivers-hv-vmbus-Implement-Direct-Mode-for-stimer0/20171103-214519
config: x86_64-randconfig-h0-11040619 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/linux/kasan.h:16:0,
from include/linux/slab.h:120,
from include/linux/irq.h:25,
from arch/x86/include/asm/mshyperv.h:7,
from arch/x86/entry/vdso/vdso32/../vclock_gettime.c:20,
from arch/x86/entry/vdso/vdso32/vclock_gettime.c:32:
   arch/x86/include/asm/pgtable.h: In function 'pte_flags_pkey':
>> arch/x86/include/asm/pgtable.h:1199:2: warning: left shift count >= width of 
>> type
 return (pte_flags & _PAGE_PKEY_MASK) >> _PAGE_BIT_PKEY_BIT0;
 ^
>> arch/x86/include/asm/pgtable.h:1199:2: warning: left shift count >= width of 
>> type
>> arch/x86/include/asm/pgtable.h:1199:2: warning: left shift count >= width of 
>> type
>> arch/x86/include/asm/pgtable.h:1199:2: warning: left shift count >= width of 
>> type
   arch/x86/include/asm/pgtable.h:1199:2: warning: right shift count >= width 
of type

vim +1199 arch/x86/include/asm/pgtable.h

33a709b2 Dave Hansen 2016-02-12  1194  
33a709b2 Dave Hansen 2016-02-12  1195  static inline u16 
pte_flags_pkey(unsigned long pte_flags)
33a709b2 Dave Hansen 2016-02-12  1196  {
33a709b2 Dave Hansen 2016-02-12  1197  #ifdef 
CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
33a709b2 Dave Hansen 2016-02-12  1198   /* ifdef to avoid doing 59-bit shift on 
32-bit values */
33a709b2 Dave Hansen 2016-02-12 @1199   return (pte_flags & _PAGE_PKEY_MASK) >> 
_PAGE_BIT_PKEY_BIT0;
33a709b2 Dave Hansen 2016-02-12  1200  #else
33a709b2 Dave Hansen 2016-02-12  1201   return 0;
33a709b2 Dave Hansen 2016-02-12  1202  #endif
33a709b2 Dave Hansen 2016-02-12  1203  }
33a709b2 Dave Hansen 2016-02-12  1204  

:: The code at line 1199 was first introduced by commit
:: 33a709b25a760b91184bb335cf7d7c32b8123013 mm/gup, x86/mm/pkeys: Check 
VMAs and PTEs for protection keys

:: TO: Dave Hansen 
:: CC: Ingo Molnar 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[kbuild test robot]

Hi Michael,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/master]

url:
https://github.com/0day-ci/linux/commits/mikelley-exchange-microsoft-com/Drivers-hv-vmbus-Implement-Direct-Mode-for-stimer0/20171103-214519
config: x86_64-rhel (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64 

All errors (new ones prefixed by >>):

>> ERROR: "hv_deallocate_stimer0_irq" [drivers/hv/hv_vmbus.ko] undefined!
>> ERROR: "hv_allocate_stimer0_irq" [drivers/hv/hv_vmbus.ko] undefined!
>> ERROR: "hv_ack_stimer0_interrupt" [drivers/hv/hv_vmbus.ko] undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

RE: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Michael Kelley (EOSG)]

It's fixed.

Michael

-Original Message-
From: Thomas Gleixner [mailto:t...@linutronix.de] 
Sent: Wednesday, November 1, 2017 9:20 AM
To: Michael Kelley (EOSG) <michael.h.kel...@microsoft.com>
Cc: gre...@linuxfoundation.org; LKML <linux-ker...@vger.kernel.org>; 
de...@linuxdriverproject.org; o...@aepfle.de; a...@canonical.com; 
vkuzn...@redhat.com; jasow...@redhat.com; leann.ogasaw...@canonical.com; 
marcelo.ce...@canonical.com; Stephen Hemminger <sthem...@microsoft.com>; KY 
Srinivasan <k...@microsoft.com>
Subject: Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode 
for stimer0

On Wed, 1 Nov 2017, Thomas Gleixner wrote:

> On Tue, 31 Oct 2017, mikel...@exchange.microsoft.com wrote:

   ^^^

Please fix your mail process. mikel...@exchange.microsoft.com is not your real 
mail address, but shows up as From and obviously the reply bounces:

  mikel...@exchange.microsoft.com
  Remote Server returned '550 5.1.10 RESOLVER.ADR.RecipientNotFound;
  Recipient not found by SMTP address lookup'

Sigh,

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

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Thomas Gleixner]

On Wed, 1 Nov 2017, Thomas Gleixner wrote:

> On Tue, 31 Oct 2017, mikel...@exchange.microsoft.com wrote:

   ^^^

Please fix your mail process. mikel...@exchange.microsoft.com is not your
real mail address, but shows up as From and obviously the reply bounces:

  mikel...@exchange.microsoft.com
  Remote Server returned '550 5.1.10 RESOLVER.ADR.RecipientNotFound;
  Recipient not found by SMTP address lookup'

Sigh,

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

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Thomas Gleixner]

On Tue, 31 Oct 2017, mikel...@exchange.microsoft.com wrote:
> diff --git a/arch/x86/include/uapi/asm/hyperv.h 
> b/arch/x86/include/uapi/asm/hyperv.h
> index f65d125..408cf3e 100644
> --- a/arch/x86/include/uapi/asm/hyperv.h
> +++ b/arch/x86/include/uapi/asm/hyperv.h
> @@ -112,6 +112,22 @@
>  #define HV_X64_GUEST_IDLE_STATE_AVAILABLE(1 << 5)
>  /* Guest crash data handler available */
>  #define HV_X64_GUEST_CRASH_MSR_AVAILABLE (1 << 10)
> +/* Debug MSRs available */
> +#define HV_X64_DEBUG_MSR_AVAILABLE   (1 << 11)
> +/* Support for Non-Privileged Instruction Execution Prevention is available 
> */
> +#define HV_X64_NPIEP_AVAILABLE   (1 << 12)
> +/* Support for DisableHypervisor is available */
> +#define HV_X64_DISABLE_HYPERVISOR_AVAILABLE  (1 << 13)
> +/* Extended GVA Ranges for Flush Virtual Address list is available */
> +#define HV_X64_EXTENDED_GVA_RANGE_AVAILABLE  (1 << 14)
> +/* Return Hypercall output via XMM registers is available */
> +#define HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE(1 << 15)
> +/* SINT polling mode available */
> +#define HV_X64_SINT_POLLING_MODE_AVAILABLE   (1 << 17)
> +/* Hypercall MSR lock is available */
> +#define HV_X64_HYPERCALL_MSR_LOCK_AVAILABLE  (1 << 18)
> +/* stimer direct mode is available */
> +#define HV_X64_STIMER_DIRECT_MODE_AVAILABLE  (1 << 19)

How are all these defines (except the last one related to that patch?

> +/* Hardware IRQ number to use for stimer0 in Direct Mode.  This IRQ is a fake
> + * because stimer's in Direct Mode simply interrupt on the specified vector,
> + * without using a particular IOAPIC pin. But we use the IRQ allocation
> + * machinery, so we need a hardware IRQ #.  This value is somewhat arbitrary,
> + * but it should not be a legacy IRQ (0 to 15), and should fit within the
> + * single IOAPIC (0 to 23) that Hyper-V provides to a guest VM. So any value
> + * between 16 and 23 should be good.
> + */
> +#define HV_STIMER0_IRQNR 18

Why would you want abuse an IOAPIC interrupt if all you need is a vector?

> +/* Routines to do per-architecture handling of stimer0 when in Direct Mode */
> +
> +void hv_ack_stimer0_interrupt(struct irq_desc *desc)
> +{
> + ack_APIC_irq();
> +}
> +
> +static void allonline_vector_allocation_domain(int cpu, struct cpumask 
> *retmask,
> + const struct cpumask *mask)
> +{
> + cpumask_copy(retmask, cpu_online_mask);
> +}
> +
> +int hv_allocate_stimer0_irq(int *irq, int *vector)
> +{
> + int localirq;
> + int result;
> + struct irq_data *irq_data;
> +
> + /* The normal APIC vector allocation domain allows allocation of vectors

Please fix you comment style. Multi line comments are:

   /*
* Bla
* foo...
*/  

> +  * only for the calling CPU.  So we change the allocation domain to one
> +  * that allows vectors to be allocated in all online CPUs.  This
> +  * change is fine in a Hyper-V VM because VMs don't have the usual
> +  * complement of interrupting devices.
> +  */
> + apic->vector_allocation_domain = allonline_vector_allocation_domain;

This does not work anymore. vector_allocation_domain is gone as of
4.15. Please check the vector rework in

  git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip x86/apic

Aside of that what guarantees that all cpus are online at the point where
you allocate that interrupt? Nothing, so the vector will be not reserved or
allocated on offline CPUs. Now guess what happens if you bring the offline
CPUs online later, it will drown in spurious interrupts. Worse, the vector
can also be reused for a device interrupt. Great plan.

> + localirq = acpi_register_gsi(NULL, HV_STIMER0_IRQNR,
> + ACPI_LEVEL_SENSITIVE, ACPI_ACTIVE_HIGH);
> + if (localirq < 0) {
> + pr_err("Cannot register stimer0 gsi. Error %d", localirq);
> + return -1;
> + }
> +
> + /* We pass in a dummy IRQ handler because architecture independent code
> +  * will later override the IRQ domain interrupt handler and set it to a
> +  * Hyper-V specific handler.
> +  */
> + result = request_irq(localirq, (irq_handler_t)(-1), 0,
"Hyper-V stimer0", NULL);

That's a crude hack. Really. 

> + if (result) {
> + pr_err("Cannot request stimer0 irq. Error %d", result);
> + acpi_unregister_gsi(localirq);
> + return -1;
> + }
> + irq_data = irq_domain_get_irq_data(x86_vector_domain, localirq);
> + *vector = irqd_cfg(irq_data)->vector;
> + *irq = localirq;

Uurgh, no. This is even more of a layering violation. Grab random data from
wherever it comes and then expect that it works. This will simply fall
apart the moment someone changes the affinity of this interrupt. It will
move to some 

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Vitaly Kuznetsov]

Vitaly Kuznetsov  writes:

> mikel...@exchange.microsoft.com writes:
>
>> From: Michael Kelley 
>>
>> The 2016 version of Hyper-V offers the option to operate the guest VM
>> per-vcpu stimer's in Direct Mode, which means the timer interupts on its
>> own vector rather than queueing a VMbus message.  Direct Mode reduces
>> timer processing overhead in both the hypervisor and the guest, and
>> avoids having timer interrupts pollute the VMbus interrupt stream for
>> the synthetic NIC and storage.  This patch enables Direct Mode by
>> default on stimer0 (which is the only stimer used in Linux on Hyper-V)
>> when running on a version of Hyper-V that supports it, with a hv_vmbus
>> module parameter for disabling Direct Mode and reverting to the old
>> behavior.
>>
>> Signed-off-by: Michael Kelley 
>> ---
>>  arch/x86/include/asm/mshyperv.h| 14 
>>  arch/x86/include/uapi/asm/hyperv.h | 26 ++
>>  arch/x86/kernel/cpu/mshyperv.c | 64 +-
>>  drivers/hv/hv.c| 71 
>> --
>>  drivers/hv/hyperv_vmbus.h  |  4 ++-
>>  5 files changed, 175 insertions(+), 4 deletions(-)
>>

(in addition to my previous comment)

This patch is also x86-heavy so it probably makes sense to make x86
maintainers (Thomas, Peter, Ingo) aware no matter which tree it will go
through. 

CC: x...@kernel.org

>> diff --git a/arch/x86/include/asm/mshyperv.h 
>> b/arch/x86/include/asm/mshyperv.h
>> index 740dc97..1bba1d7 100644
>> --- a/arch/x86/include/asm/mshyperv.h
>> +++ b/arch/x86/include/asm/mshyperv.h
>> @@ -4,6 +4,8 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>> +#include 
>>  #include 
>>  #include 
>>
>> @@ -374,4 +376,16 @@ static inline struct ms_hyperv_tsc_page 
>> *hv_get_tsc_page(void)
>>  return NULL;
>>  }
>>  #endif
>> +
>> +/* Per architecture routines for stimer0 Direct Mode handling.  On x86/x64
>> + * there are no percpu actions to take.
>> + */
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +static inline void hv_enable_stimer0_percpu_irq(int irq) { }
>> +static inline void hv_disable_stimer0_percpu_irq(int irq) { }
>> +extern int hv_allocate_stimer0_irq(int *irq, int *vector);
>> +extern void hv_deallocate_stimer0_irq(int irq);
>> +extern void hv_ack_stimer0_interrupt(struct irq_desc *desc);
>> +#endif
>> +
>>  #endif
>> diff --git a/arch/x86/include/uapi/asm/hyperv.h 
>> b/arch/x86/include/uapi/asm/hyperv.h
>> index f65d125..408cf3e 100644
>> --- a/arch/x86/include/uapi/asm/hyperv.h
>> +++ b/arch/x86/include/uapi/asm/hyperv.h
>> @@ -112,6 +112,22 @@
>>  #define HV_X64_GUEST_IDLE_STATE_AVAILABLE   (1 << 5)
>>  /* Guest crash data handler available */
>>  #define HV_X64_GUEST_CRASH_MSR_AVAILABLE(1 << 10)
>> +/* Debug MSRs available */
>> +#define HV_X64_DEBUG_MSR_AVAILABLE  (1 << 11)
>> +/* Support for Non-Privileged Instruction Execution Prevention is available 
>> */
>> +#define HV_X64_NPIEP_AVAILABLE  (1 << 12)
>> +/* Support for DisableHypervisor is available */
>> +#define HV_X64_DISABLE_HYPERVISOR_AVAILABLE (1 << 13)
>> +/* Extended GVA Ranges for Flush Virtual Address list is available */
>> +#define HV_X64_EXTENDED_GVA_RANGE_AVAILABLE (1 << 14)
>> +/* Return Hypercall output via XMM registers is available */
>> +#define HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE   (1 << 15)
>> +/* SINT polling mode available */
>> +#define HV_X64_SINT_POLLING_MODE_AVAILABLE  (1 << 17)
>> +/* Hypercall MSR lock is available */
>> +#define HV_X64_HYPERCALL_MSR_LOCK_AVAILABLE (1 << 18)
>> +/* stimer direct mode is available */
>> +#define HV_X64_STIMER_DIRECT_MODE_AVAILABLE (1 << 19)
>>
>>  /*
>>   * Implementation recommendations. Indicates which behaviors the hypervisor
>> @@ -300,6 +316,16 @@ enum HV_GENERIC_SET_FORMAT {
>>
>>  #define HV_SYNIC_STIMER_COUNT   (4)
>>
>> +/* Hardware IRQ number to use for stimer0 in Direct Mode.  This IRQ is a 
>> fake
>> + * because stimer's in Direct Mode simply interrupt on the specified vector,
>> + * without using a particular IOAPIC pin. But we use the IRQ allocation
>> + * machinery, so we need a hardware IRQ #.  This value is somewhat 
>> arbitrary,
>> + * but it should not be a legacy IRQ (0 to 15), and should fit within the
>> + * single IOAPIC (0 to 23) that Hyper-V provides to a guest VM. So any value
>> + * between 16 and 23 should be good.
>> + */
>
> (nitpick): all comments in the patch are in 'net' style:
>
>  /* This is a
>   * comment.
>   */
>
> While in Hyper-V files (and x86 in general) the 'normal' style is used:
>
>  /*
>   * This is a
>   * comment.
>   */
>
> But checkpatch.pl doesn't complain.
>
>> +#define HV_STIMER0_IRQNR18
>> +
>>  /* Define synthetic interrupt controller message constants. */
>>  #define HV_MESSAGE_SIZE (256)
>>  

Re: [PATCH char-misc 1/1] Drivers: hv: vmbus: Implement Direct Mode for stimer0

[Vitaly Kuznetsov]

mikel...@exchange.microsoft.com writes:

> From: Michael Kelley 
>
> The 2016 version of Hyper-V offers the option to operate the guest VM
> per-vcpu stimer's in Direct Mode, which means the timer interupts on its
> own vector rather than queueing a VMbus message.  Direct Mode reduces
> timer processing overhead in both the hypervisor and the guest, and
> avoids having timer interrupts pollute the VMbus interrupt stream for
> the synthetic NIC and storage.  This patch enables Direct Mode by
> default on stimer0 (which is the only stimer used in Linux on Hyper-V)
> when running on a version of Hyper-V that supports it, with a hv_vmbus
> module parameter for disabling Direct Mode and reverting to the old
> behavior.
>
> Signed-off-by: Michael Kelley 
> ---
>  arch/x86/include/asm/mshyperv.h| 14 
>  arch/x86/include/uapi/asm/hyperv.h | 26 ++
>  arch/x86/kernel/cpu/mshyperv.c | 64 +-
>  drivers/hv/hv.c| 71 
> --
>  drivers/hv/hyperv_vmbus.h  |  4 ++-
>  5 files changed, 175 insertions(+), 4 deletions(-)
>
> diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h
> index 740dc97..1bba1d7 100644
> --- a/arch/x86/include/asm/mshyperv.h
> +++ b/arch/x86/include/asm/mshyperv.h
> @@ -4,6 +4,8 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
>  #include 
>
> @@ -374,4 +376,16 @@ static inline struct ms_hyperv_tsc_page 
> *hv_get_tsc_page(void)
>   return NULL;
>  }
>  #endif
> +
> +/* Per architecture routines for stimer0 Direct Mode handling.  On x86/x64
> + * there are no percpu actions to take.
> + */
> +#if IS_ENABLED(CONFIG_HYPERV)
> +static inline void hv_enable_stimer0_percpu_irq(int irq) { }
> +static inline void hv_disable_stimer0_percpu_irq(int irq) { }
> +extern int hv_allocate_stimer0_irq(int *irq, int *vector);
> +extern void hv_deallocate_stimer0_irq(int irq);
> +extern void hv_ack_stimer0_interrupt(struct irq_desc *desc);
> +#endif
> +
>  #endif
> diff --git a/arch/x86/include/uapi/asm/hyperv.h 
> b/arch/x86/include/uapi/asm/hyperv.h
> index f65d125..408cf3e 100644
> --- a/arch/x86/include/uapi/asm/hyperv.h
> +++ b/arch/x86/include/uapi/asm/hyperv.h
> @@ -112,6 +112,22 @@
>  #define HV_X64_GUEST_IDLE_STATE_AVAILABLE(1 << 5)
>  /* Guest crash data handler available */
>  #define HV_X64_GUEST_CRASH_MSR_AVAILABLE (1 << 10)
> +/* Debug MSRs available */
> +#define HV_X64_DEBUG_MSR_AVAILABLE   (1 << 11)
> +/* Support for Non-Privileged Instruction Execution Prevention is available 
> */
> +#define HV_X64_NPIEP_AVAILABLE   (1 << 12)
> +/* Support for DisableHypervisor is available */
> +#define HV_X64_DISABLE_HYPERVISOR_AVAILABLE  (1 << 13)
> +/* Extended GVA Ranges for Flush Virtual Address list is available */
> +#define HV_X64_EXTENDED_GVA_RANGE_AVAILABLE  (1 << 14)
> +/* Return Hypercall output via XMM registers is available */
> +#define HV_X64_HYPERCALL_XMM_OUTPUT_AVAILABLE(1 << 15)
> +/* SINT polling mode available */
> +#define HV_X64_SINT_POLLING_MODE_AVAILABLE   (1 << 17)
> +/* Hypercall MSR lock is available */
> +#define HV_X64_HYPERCALL_MSR_LOCK_AVAILABLE  (1 << 18)
> +/* stimer direct mode is available */
> +#define HV_X64_STIMER_DIRECT_MODE_AVAILABLE  (1 << 19)
>
>  /*
>   * Implementation recommendations. Indicates which behaviors the hypervisor
> @@ -300,6 +316,16 @@ enum HV_GENERIC_SET_FORMAT {
>
>  #define HV_SYNIC_STIMER_COUNT(4)
>
> +/* Hardware IRQ number to use for stimer0 in Direct Mode.  This IRQ is a fake
> + * because stimer's in Direct Mode simply interrupt on the specified vector,
> + * without using a particular IOAPIC pin. But we use the IRQ allocation
> + * machinery, so we need a hardware IRQ #.  This value is somewhat arbitrary,
> + * but it should not be a legacy IRQ (0 to 15), and should fit within the
> + * single IOAPIC (0 to 23) that Hyper-V provides to a guest VM. So any value
> + * between 16 and 23 should be good.
> + */

(nitpick): all comments in the patch are in 'net' style:

 /* This is a
  * comment.
  */

While in Hyper-V files (and x86 in general) the 'normal' style is used:

 /*
  * This is a
  * comment.
  */

But checkpatch.pl doesn't complain.

> +#define HV_STIMER0_IRQNR 18
> +
>  /* Define synthetic interrupt controller message constants. */
>  #define HV_MESSAGE_SIZE  (256)
>  #define HV_MESSAGE_PAYLOAD_BYTE_COUNT(240)
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 236324e8..88dc243 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -19,7 +19,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
>  #include 
> +#include 
>  #include 
>  #include 
>