[PATCH] x86: Hyper-V: register clocksource only if its advertised

[PATCH] x86: Hyper-V: register clocksource only if its advertised

[Olaf Hering]

Enable hyperv_clocksource only if its advertised as a feature.
XenServer 6 returns the signature which is checked in
ms_hyperv_platform(), but it does not offer all features. Currently the
clocksource is enabled unconditionally in ms_hyperv_init_platform(), and
the result is a hanging guest.

Hyper-V spec Bit 1 indicates the availability of Partition Reference
Counter.  Register the clocksource only if this bit is set.

The guest in question prints this in dmesg:
 [    0.000000] Hypervisor detected: Microsoft HyperV
 [    0.000000] HyperV: features 0x70, hints 0x0

This bug can be reproduced easily be setting 'viridian=1' in a HVM domU
.cfg file. A workaround without this patch is to boot the HVM guest with
'clocksource=jiffies'.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Cc: stable@kernel.org
Cc: KY Srinivasan <kys@microsoft.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
---
 arch/x86/kernel/cpu/mshyperv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index 0a630dd..646d192 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -68,7 +68,8 @@ static void __init ms_hyperv_init_platform(void)
 	printk(KERN_INFO "HyperV: features 0x%x, hints 0x%x\n",
 	       ms_hyperv.features, ms_hyperv.hints);
 
-	clocksource_register_hz(&hyperv_cs, NSEC_PER_SEC/100);
+	if (ms_hyperv.features & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE)
+		clocksource_register_hz(&hyperv_cs, NSEC_PER_SEC/100);
 }
 
 const __refconst struct hypervisor_x86 x86_hyper_ms_hyperv = {
-- 
1.8.1.1

Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Greg KH]

On Fri, Jan 25, 2013 at 02:37:16PM +0100, Olaf Hering wrote:
> Enable hyperv_clocksource only if its advertised as a feature.
> XenServer 6 returns the signature which is checked in
> ms_hyperv_platform(), but it does not offer all features. Currently the
> clocksource is enabled unconditionally in ms_hyperv_init_platform(), and
> the result is a hanging guest.
> 
> Hyper-V spec Bit 1 indicates the availability of Partition Reference
> Counter.  Register the clocksource only if this bit is set.
> 
> The guest in question prints this in dmesg:
>  [    0.000000] Hypervisor detected: Microsoft HyperV
>  [    0.000000] HyperV: features 0x70, hints 0x0
> 
> This bug can be reproduced easily be setting 'viridian=1' in a HVM domU
> .cfg file. A workaround without this patch is to boot the HVM guest with
> 'clocksource=jiffies'.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: stable@kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> ---
>  arch/x86/kernel/cpu/mshyperv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

This needs to go through the x86 maintainers, not me, sorry.

greg k-h

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[KY Srinivasan]

> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Friday, January 25, 2013 8:37 AM
> To: linux-kernel@vger.kernel.org
> Cc: Olaf Hering; stable@kernel.org; KY Srinivasan; Greg KH
> Subject: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> 
> Enable hyperv_clocksource only if its advertised as a feature.
> XenServer 6 returns the signature which is checked in
> ms_hyperv_platform(), but it does not offer all features. Currently the
> clocksource is enabled unconditionally in ms_hyperv_init_platform(), and
> the result is a hanging guest.
> 
> Hyper-V spec Bit 1 indicates the availability of Partition Reference
> Counter.  Register the clocksource only if this bit is set.
> 
> The guest in question prints this in dmesg:
>  [    0.000000] Hypervisor detected: Microsoft HyperV
>  [    0.000000] HyperV: features 0x70, hints 0x0
> 
> This bug can be reproduced easily be setting 'viridian=1' in a HVM domU
> .cfg file. A workaround without this patch is to boot the HVM guest with
> 'clocksource=jiffies'.

I am curious why you are setting viridian=1 for a Linux HVM guest. Initially when
I did the Hyper-V emulation work on Xen, this configuration flag would be set only
for Windows guests since the idea was to run the "enlightened" Windows on Xen.
While the current Xen emulation code does not emulate "Partition Reference Counter"
there is no guarantee that Xen would not do this in the future. Perhaps dealing with this issue
by identifying the guest (via the viridian domain or not) may be a safer approach?

Regards,

K. Y 
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> Cc: stable@kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> ---
>  arch/x86/kernel/cpu/mshyperv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
> index 0a630dd..646d192 100644
> --- a/arch/x86/kernel/cpu/mshyperv.c
> +++ b/arch/x86/kernel/cpu/mshyperv.c
> @@ -68,7 +68,8 @@ static void __init ms_hyperv_init_platform(void)
>  	printk(KERN_INFO "HyperV: features 0x%x, hints 0x%x\n",
>  	       ms_hyperv.features, ms_hyperv.hints);
> 
> -	clocksource_register_hz(&hyperv_cs, NSEC_PER_SEC/100);
> +	if (ms_hyperv.features & HV_X64_MSR_TIME_REF_COUNT_AVAILABLE)
> +		clocksource_register_hz(&hyperv_cs, NSEC_PER_SEC/100);
>  }
> 
>  const __refconst struct hypervisor_x86 x86_hyper_ms_hyperv = {
> --
> 1.8.1.1
> 
> 

Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Olaf Hering]

On Fri, Jan 25, KY Srinivasan wrote:

> 
> 
> > -----Original Message-----
> > From: Olaf Hering [mailto:olaf@aepfle.de]
> > Sent: Friday, January 25, 2013 8:37 AM
> > To: linux-kernel@vger.kernel.org
> > Cc: Olaf Hering; stable@kernel.org; KY Srinivasan; Greg KH
> > Subject: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> > 
> > Enable hyperv_clocksource only if its advertised as a feature.
> > XenServer 6 returns the signature which is checked in
> > ms_hyperv_platform(), but it does not offer all features. Currently the
> > clocksource is enabled unconditionally in ms_hyperv_init_platform(), and
> > the result is a hanging guest.
> > 
> > Hyper-V spec Bit 1 indicates the availability of Partition Reference
> > Counter.  Register the clocksource only if this bit is set.
> > 
> > The guest in question prints this in dmesg:
> >  [    0.000000] Hypervisor detected: Microsoft HyperV
> >  [    0.000000] HyperV: features 0x70, hints 0x0
> > 
> > This bug can be reproduced easily be setting 'viridian=1' in a HVM domU
> > .cfg file. A workaround without this patch is to boot the HVM guest with
> > 'clocksource=jiffies'.
> 
> I am curious why you are setting viridian=1 for a Linux HVM guest. Initially when
> I did the Hyper-V emulation work on Xen, this configuration flag would be set only
> for Windows guests since the idea was to run the "enlightened" Windows on Xen.
> While the current Xen emulation code does not emulate "Partition Reference Counter"
> there is no guarantee that Xen would not do this in the future. Perhaps dealing with this issue
> by identifying the guest (via the viridian domain or not) may be a safer approach?

I'm not sure if XenServer does that per default, but I used the
viridian= flag to reproduce the report.
If Xen starts to emulate the "Partition Reference Counter" it should
also advertise this emulation with the feature flag. Are you saying that
checking for features is not the right approach?

Olaf

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2942 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Friday, January 25, 2013 11:55 AM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; Greg KH
> Subject: Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> 
> On Fri, Jan 25, KY Srinivasan wrote:
> 
> >
> >
> > > -----Original Message-----
> > > From: Olaf Hering [mailto:olaf@aepfle.de]
> > > Sent: Friday, January 25, 2013 8:37 AM
> > > To: linux-kernel@vger.kernel.org
> > > Cc: Olaf Hering; stable@kernel.org; KY Srinivasan; Greg KH
> > > Subject: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> > >
> > > Enable hyperv_clocksource only if its advertised as a feature.
> > > XenServer 6 returns the signature which is checked in
> > > ms_hyperv_platform(), but it does not offer all features. Currently the
> > > clocksource is enabled unconditionally in ms_hyperv_init_platform(), and
> > > the result is a hanging guest.
> > >
> > > Hyper-V spec Bit 1 indicates the availability of Partition Reference
> > > Counter.  Register the clocksource only if this bit is set.
> > >
> > > The guest in question prints this in dmesg:
> > >  [    0.000000] Hypervisor detected: Microsoft HyperV
> > >  [    0.000000] HyperV: features 0x70, hints 0x0
> > >
> > > This bug can be reproduced easily be setting 'viridian=1' in a HVM domU
> > > .cfg file. A workaround without this patch is to boot the HVM guest with
> > > 'clocksource=jiffies'.
> >
> > I am curious why you are setting viridian=1 for a Linux HVM guest. Initially when
> > I did the Hyper-V emulation work on Xen, this configuration flag would be set
> only
> > for Windows guests since the idea was to run the "enlightened" Windows on
> Xen.
> > While the current Xen emulation code does not emulate "Partition Reference
> Counter"
> > there is no guarantee that Xen would not do this in the future. Perhaps dealing
> with this issue
> > by identifying the guest (via the viridian domain or not) may be a safer
> approach?
> 
> I'm not sure if XenServer does that per default, but I used the
> viridian= flag to reproduce the report.
> If Xen starts to emulate the "Partition Reference Counter" it should
> also advertise this emulation with the feature flag. Are you saying that
> checking for features is not the right approach?

My fear is that there is no guarantee that Xen would not emulate this feature
in the spirit of making Hyper-V emulation "more" complete. Since all this problem is
because Xen thinks it is running a viridian domain (when in fact it is running Linux), I felt we
should explore why the viridian tag got set for a Linux VM. If we can fix that we would have
a solution that does not depend upon assuming that Xen would not emulate a particular Hyper-V
feature.

K. Y 
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Olaf Hering]

On Fri, Jan 25, KY Srinivasan wrote:

> My fear is that there is no guarantee that Xen would not emulate this
> feature in the spirit of making Hyper-V emulation "more" complete.
> Since all this problem is because Xen thinks it is running a viridian
> domain (when in fact it is running Linux), I felt we should explore
> why the viridian tag got set for a Linux VM. If we can fix that we
> would have a solution that does not depend upon assuming that Xen
> would not emulate a particular Hyper-V feature.

In my opinion this logic is backwards. A host provides a set of
functionality/features to a given guest, no matter what runs inside the
guest. And the guest can query the feature list and configure itself
accordingly. In this specific case the guest finds feature A (the cpuid
result) and expects that feature B (a clock source) is present as well,
even if feature B has its very own availability flag.

Maybe I miss your point.

Olaf

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2406 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Friday, January 25, 2013 12:19 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; Greg KH; Jan Beulich (JBeulich@suse.com)
> Subject: Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> 
> On Fri, Jan 25, KY Srinivasan wrote:
> 
> > My fear is that there is no guarantee that Xen would not emulate this
> > feature in the spirit of making Hyper-V emulation "more" complete.
> > Since all this problem is because Xen thinks it is running a viridian
> > domain (when in fact it is running Linux), I felt we should explore
> > why the viridian tag got set for a Linux VM. If we can fix that we
> > would have a solution that does not depend upon assuming that Xen
> > would not emulate a particular Hyper-V feature.
> 
> In my opinion this logic is backwards. A host provides a set of
> functionality/features to a given guest, no matter what runs inside the
> guest. And the guest can query the feature list and configure itself
> accordingly. In this specific case the guest finds feature A (the cpuid
> result) and expects that feature B (a clock source) is present as well,
> even if feature B has its very own availability flag.

I would agree with you in the abstract but not in this specific case. First,
the current Hyper-V code in Linux verifies that the underlying hypervisor is
Hyper-V and proceeds to setup state that is needed to run Linux on Hyper-V.
If Xen were not emulating Hyper-V, we would not have any issue here. However,
Xen does emulate Hyper-V and it is emulating Hyper-V not to run Linux, but to run
enlightened Windows (making Windows think it is running on Hyper-V). We have a 
mechanism in place for controlling this Hyper-V emulation in Xen - the viridian flag.
In my view, ensuring that this flag is not set for a non -Windows guest is the only safe
approach to dealing with this issue. Your current approach of using the "Partition Reference
Counter" feature bit is very fragile. Let us assume that future Windows guests depend on this
feature; Xen would certainly emulate this to have a "good" emulation of Hyper-V and we are
back to square one.

Regards,

K. Y
> 
> Maybe I miss your point.
> 
> Olaf
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Olaf Hering]

On Fri, Jan 25, KY Srinivasan wrote:

> Your current approach of using the "Partition Reference Counter"
> feature bit is very fragile. Let us assume that future Windows guests
> depend on this feature; Xen would certainly emulate this to have a
> "good" emulation of Hyper-V and we are back to square one.

If Xen starts to emulate the "Partition Reference Counter" feature,
wouldnt it also advertise this feature?

Olaf

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1047 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Friday, January 25, 2013 3:00 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; Greg KH; Jan Beulich (JBeulich@suse.com)
> Subject: Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> 
> On Fri, Jan 25, KY Srinivasan wrote:
> 
> > Your current approach of using the "Partition Reference Counter"
> > feature bit is very fragile. Let us assume that future Windows guests
> > depend on this feature; Xen would certainly emulate this to have a
> > "good" emulation of Hyper-V and we are back to square one.
> 
> If Xen starts to emulate the "Partition Reference Counter" feature,
> wouldnt it also advertise this feature?

Yes; it has to - if it expects the Windows guests to use it. This would be
no different than the other features that Xen emulates and advertises.

K. Y
 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1151 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Friday, January 25, 2013 3:00 PM
> To: KY Srinivasan
> Cc: linux-kernel@vger.kernel.org; Greg KH; Jan Beulich (JBeulich@suse.com)
> Subject: Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> 
> On Fri, Jan 25, KY Srinivasan wrote:
> 
> > Your current approach of using the "Partition Reference Counter"
> > feature bit is very fragile. Let us assume that future Windows guests
> > depend on this feature; Xen would certainly emulate this to have a
> > "good" emulation of Hyper-V and we are back to square one.
> 
> If Xen starts to emulate the "Partition Reference Counter" feature,
> wouldnt it also advertise this feature?

Olaf, did you get a chance to investigate why the viridian bit was set for the Linux HVM domain.
I am thinking of re-sending the patch for delivering the Hyper-V vmbus on a separate vector without
a run-time check. Let me know.

Regards,

K. Y 
> 
> Olaf
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Olaf Hering]

On Sat, Jan 26, KY Srinivasan wrote:

> 
> 
> > -----Original Message-----
> > From: Olaf Hering [mailto:olaf@aepfle.de]
> > Sent: Friday, January 25, 2013 3:00 PM
> > To: KY Srinivasan
> > Cc: linux-kernel@vger.kernel.org; Greg KH; Jan Beulich (JBeulich@suse.com)
> > Subject: Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> > 
> > On Fri, Jan 25, KY Srinivasan wrote:
> > 
> > > Your current approach of using the "Partition Reference Counter"
> > > feature bit is very fragile. Let us assume that future Windows guests
> > > depend on this feature; Xen would certainly emulate this to have a
> > > "good" emulation of Hyper-V and we are back to square one.
> > 
> > If Xen starts to emulate the "Partition Reference Counter" feature,
> > wouldnt it also advertise this feature?
> 
> Olaf, did you get a chance to investigate why the viridian bit was set
> for the Linux HVM domain.

I dont know, most likely XenServer sets this per default?

> I am thinking of re-sending the patch for delivering the Hyper-V vmbus
> on a separate vector without a run-time check. Let me know.

Are you also acking/nacking this patch?

Olaf

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Stefano Stabellini]

On Fri, 25 Jan 2013, KY Srinivasan wrote:
> > -----Original Message-----
> > From: Olaf Hering [mailto:olaf@aepfle.de]
> > Sent: Friday, January 25, 2013 12:19 PM
> > To: KY Srinivasan
> > Cc: linux-kernel@vger.kernel.org; Greg KH; Jan Beulich (JBeulich@suse.com)
> > Subject: Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> > 
> > On Fri, Jan 25, KY Srinivasan wrote:
> > 
> > > My fear is that there is no guarantee that Xen would not emulate this
> > > feature in the spirit of making Hyper-V emulation "more" complete.
> > > Since all this problem is because Xen thinks it is running a viridian
> > > domain (when in fact it is running Linux), I felt we should explore
> > > why the viridian tag got set for a Linux VM. If we can fix that we
> > > would have a solution that does not depend upon assuming that Xen
> > > would not emulate a particular Hyper-V feature.
> > 
> > In my opinion this logic is backwards. A host provides a set of
> > functionality/features to a given guest, no matter what runs inside the
> > guest. And the guest can query the feature list and configure itself
> > accordingly. In this specific case the guest finds feature A (the cpuid
> > result) and expects that feature B (a clock source) is present as well,
> > even if feature B has its very own availability flag.
> 
> I would agree with you in the abstract but not in this specific case. First,
> the current Hyper-V code in Linux verifies that the underlying hypervisor is
> Hyper-V and proceeds to setup state that is needed to run Linux on Hyper-V.
> If Xen were not emulating Hyper-V, we would not have any issue here. However,
> Xen does emulate Hyper-V and it is emulating Hyper-V not to run Linux, but to run
> enlightened Windows (making Windows think it is running on Hyper-V). We have a 
> mechanism in place for controlling this Hyper-V emulation in Xen - the viridian flag.
> In my view, ensuring that this flag is not set for a non -Windows guest is the only safe
> approach to dealing with this issue. Your current approach of using the "Partition Reference
> Counter" feature bit is very fragile. Let us assume that future Windows guests depend on this
> feature; Xen would certainly emulate this to have a "good" emulation of Hyper-V and we are
> back to square one.

I think that Olaf made his point very clear: a feature A should only be
enabled if the corresponding flag A is set.
In fact it seems to me that this patch is correct on its own merits,
regardless of Xen does or does not.

The Xen tools might or might not know whether a guest is going to be
Linux, Windows, FreeBSD or whatever else people use nowadays.  Setting
viridian=1 is the safe choice, given that it shouldn't create any
issues: after all guests are supposed to check for feature flags before
using them.

If Xen is going to implement "Partition Reference Counter", it is also
going to set the corresponding flag, so the guest OS (Windows, Linux,
my pet OS) can check whether the feature is available and decide whether
it wants to use it.

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Jan Beulich]

>>> On 28.01.13 at 18:44, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> I think that Olaf made his point very clear: a feature A should only be
> enabled if the corresponding flag A is set.
> In fact it seems to me that this patch is correct on its own merits,
> regardless of Xen does or does not.
> 
> The Xen tools might or might not know whether a guest is going to be
> Linux, Windows, FreeBSD or whatever else people use nowadays.  Setting
> viridian=1 is the safe choice, given that it shouldn't create any
> issues: after all guests are supposed to check for feature flags before
> using them.
> 
> If Xen is going to implement "Partition Reference Counter", it is also
> going to set the corresponding flag, so the guest OS (Windows, Linux,
> my pet OS) can check whether the feature is available and decide whether
> it wants to use it.

While I agree in general, the specific case of the callback vector
seems a little more difficult: As KY says, there's no feature flag
for this (or perhaps more precisely for it being deliverable across
all CPUs), and hence there's both the problem of detection and
the problem of disambiguation (as otherwise both the Hyper-V
code and the Xen code in Linux could be trying to use the same
vector).

Jan

Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Olaf Hering]

On Tue, Jan 29, Jan Beulich wrote:

> >>> On 28.01.13 at 18:44, Stefano Stabellini <stefano.stabellini@eu.citrix.com> wrote:
> > I think that Olaf made his point very clear: a feature A should only be
> > enabled if the corresponding flag A is set.
> > In fact it seems to me that this patch is correct on its own merits,
> > regardless of Xen does or does not.
> > 
> > The Xen tools might or might not know whether a guest is going to be
> > Linux, Windows, FreeBSD or whatever else people use nowadays.  Setting
> > viridian=1 is the safe choice, given that it shouldn't create any
> > issues: after all guests are supposed to check for feature flags before
> > using them.
> > 
> > If Xen is going to implement "Partition Reference Counter", it is also
> > going to set the corresponding flag, so the guest OS (Windows, Linux,
> > my pet OS) can check whether the feature is available and decide whether
> > it wants to use it.
> 
> While I agree in general, the specific case of the callback vector
> seems a little more difficult: As KY says, there's no feature flag
> for this (or perhaps more precisely for it being deliverable across
> all CPUs), and hence there's both the problem of detection and
> the problem of disambiguation (as otherwise both the Hyper-V
> code and the Xen code in Linux could be trying to use the same
> vector).

This is true, but belongs to that other thread about the interrupt
vector. I agree that the detection in ms_hyperv_platform() should be
extended, with a DMI check for example.

The patch which started this thread is still valid because it enables
feature B only if the featurebit for B is enabled.

Olaf

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[KY Srinivasan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3018 bytes --]



> -----Original Message-----
> From: Olaf Hering [mailto:olaf@aepfle.de]
> Sent: Tuesday, January 29, 2013 4:27 AM
> To: Jan Beulich
> Cc: Stefano Stabellini; KY Srinivasan; Greg KH; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> 
> On Tue, Jan 29, Jan Beulich wrote:
> 
> > >>> On 28.01.13 at 18:44, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> wrote:
> > > I think that Olaf made his point very clear: a feature A should only be
> > > enabled if the corresponding flag A is set.
> > > In fact it seems to me that this patch is correct on its own merits,
> > > regardless of Xen does or does not.
> > >
> > > The Xen tools might or might not know whether a guest is going to be
> > > Linux, Windows, FreeBSD or whatever else people use nowadays.  Setting
> > > viridian=1 is the safe choice, given that it shouldn't create any
> > > issues: after all guests are supposed to check for feature flags before
> > > using them.
> > >
> > > If Xen is going to implement "Partition Reference Counter", it is also
> > > going to set the corresponding flag, so the guest OS (Windows, Linux,
> > > my pet OS) can check whether the feature is available and decide whether
> > > it wants to use it.
> >
> > While I agree in general, the specific case of the callback vector
> > seems a little more difficult: As KY says, there's no feature flag
> > for this (or perhaps more precisely for it being deliverable across
> > all CPUs), and hence there's both the problem of detection and
> > the problem of disambiguation (as otherwise both the Hyper-V
> > code and the Xen code in Linux could be trying to use the same
> > vector).
> 
> This is true, but belongs to that other thread about the interrupt
> vector. I agree that the detection in ms_hyperv_platform() should be
> extended, with a DMI check for example.
I want to explore extending ms_hyperv_platform() to detect if Hyper-V is being
emulated. Since Hyper-V is never going to emulate Xen, in my view this would be a
better option. 

 
> 
> The patch which started this thread is still valid because it enables
> feature B only if the featurebit for B is enabled.

Why would we need this if we have some other way of detecting that Hyper-V is being
emulated. Furthermore, I am not sure I understand the logic here. The fact that the hypervisor
supports PARTITION_REFERENCE_COUNTER does not necessarily mean that we should register
a clocksource based on that reference counter. It is true that that is the case on Hyper-V today.
However, on other hypervisors emulating Hyper-V, other standard clocksources maybe more
appropriate.

I agree with you that your patch is valid for making the Hyper-V code more robust but not for dealing
with general issue of Xen emulating Hyper-V. I will Ack your patch. 

Regards,

K. Y 
> 
> Olaf
> 

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Stefano Stabellini]

On Tue, 29 Jan 2013, KY Srinivasan wrote:
> > The patch which started this thread is still valid because it enables
> > feature B only if the featurebit for B is enabled.
> 
> Why would we need this if we have some other way of detecting that Hyper-V is being
> emulated.

I don't think that Hyper-V being emulated or not matters here.
What matters is whether the feature is available: only if it is then it
should be used.


> Furthermore, I am not sure I understand the logic here. The fact that the hypervisor
> supports PARTITION_REFERENCE_COUNTER does not necessarily mean that we should register
> a clocksource based on that reference counter. It is true that that is the case on Hyper-V today.
> However, on other hypervisors emulating Hyper-V, other standard clocksources maybe more
> appropriate.

The point is that if a feature is NOT supported, then it should NOT be
used. Only if it is supported, then Linux might consider using it. And
of course can decide not to use it.


> I agree with you that your patch is valid for making the Hyper-V code more robust but not for dealing
> with general issue of Xen emulating Hyper-V.

What "general issue" would that be?

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[KY Srinivasan]

> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabellini@eu.citrix.com]
> Sent: Tuesday, January 29, 2013 10:31 AM
> To: KY Srinivasan
> Cc: Olaf Hering; Jan Beulich; Stefano Stabellini; Greg KH; linux-
> kernel@vger.kernel.org
> Subject: RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised
> 
> On Tue, 29 Jan 2013, KY Srinivasan wrote:
> > > The patch which started this thread is still valid because it enables
> > > feature B only if the featurebit for B is enabled.
> >
> > Why would we need this if we have some other way of detecting that Hyper-V
> is being
> > emulated.
> 
> I don't think that Hyper-V being emulated or not matters here.
> What matters is whether the feature is available: only if it is then it
> should be used.
> 
> 
> > Furthermore, I am not sure I understand the logic here. The fact that the
> hypervisor
> > supports PARTITION_REFERENCE_COUNTER does not necessarily mean that we
> should register
> > a clocksource based on that reference counter. It is true that that is the case on
> Hyper-V today.
> > However, on other hypervisors emulating Hyper-V, other standard
> clocksources maybe more
> > appropriate.
> 
> The point is that if a feature is NOT supported, then it should NOT be
> used. Only if it is supported, then Linux might consider using it. And
> of course can decide not to use it.
> 
> 
> > I agree with you that your patch is valid for making the Hyper-V code more
> robust but not for dealing
> > with general issue of Xen emulating Hyper-V.
> 
> What "general issue" would that be?

Not all Hyper-V specific features are advertised via a feature bit. For instance, we need to reserve
an IDT entry to have VMBUS interrupts delivered on a per-CPU basis and this reservation is done
after confirming that we are running on Hyper-V. With Xen emulating Hyper-V, we have an issue.
Since Hyper-V will never emulate Xen, I am going to add a check in the Hyper-V detection code to
deal with this issue.

Regards,

K. Y
 

RE: [PATCH] x86: Hyper-V: register clocksource only if its advertised

[Stefano Stabellini]

On Tue, 29 Jan 2013, KY Srinivasan wrote:
> > > I agree with you that your patch is valid for making the Hyper-V code more
> > robust but not for dealing
> > > with general issue of Xen emulating Hyper-V.
> > 
> > What "general issue" would that be?
> 
> Not all Hyper-V specific features are advertised via a feature bit. For instance, we need to reserve
> an IDT entry to have VMBUS interrupts delivered on a per-CPU basis and this reservation is done
> after confirming that we are running on Hyper-V. With Xen emulating Hyper-V, we have an issue.
> Since Hyper-V will never emulate Xen, I am going to add a check in the Hyper-V detection code to
> deal with this issue.

I personally think that it would be much nicer to have a feature flag
somewhere to advertise per-CPU VMBUS interrupts.
However adding a check in the Hyper-V detection code is OK too for me.