Re: CPUID in amd64/i386 boot code
This is the intended use, avoid busy-polling of BIOS PC console if
running on HV. Avoid 100% CPU usage at boot prompt on hypervisors.
Originally I unconditionally reverted the polling part, which was
introduced only for Intel Mac Mini. After brief discussion with mikeb@,
I decided to leave that hack by checking HV bit in CPUID (cpu_ecxfeature).
diff --git a/sys/arch/amd64/stand/libsa/bioscons.c
b/sys/arch/amd64/stand/libsa/bioscons.c
index bdff9a4..fa42751 100644
--- a/sys/arch/amd64/stand/libsa/bioscons.c
+++ b/sys/arch/amd64/stand/libsa/bioscons.c
@@ -30,6 +30,7 @@
#include
#include
+#include
#include
#include
@@ -37,7 +38,7 @@
#include
#include
-#include
+#include
#include "biosdev.h"
@@ -82,14 +83,17 @@ pc_getc(dev_t dev)
return (rv & 0xff);
}
- /*
-* Wait for a character to actually become available. Appears to
-* be necessary on (at least) the Intel Mac Mini.
-*/
- do {
- __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
- "0" (0x100) : "%ecx", "%edx", "cc" );
- } while ((rv & 0xff) == 0);
+ if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
+ /*
+ * Wait for a character to actually become available.
+ * Appears to be necessary on (at least) the Intel Mac
+ * Mini.
+*/
+ do {
+ __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
+ "0" (0x100) : "%ecx", "%edx", "cc" );
+ } while ((rv & 0xff) == 0);
+ }
__asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
"%ecx", "%edx", "cc" );
diff --git a/sys/arch/i386/stand/libsa/bioscons.c
b/sys/arch/i386/stand/libsa/bioscons.c
index 028bef3..b53abcf 100644
--- a/sys/arch/i386/stand/libsa/bioscons.c
+++ b/sys/arch/i386/stand/libsa/bioscons.c
@@ -27,15 +27,20 @@
*/
#include
+
#include
#include
+#include
+
#include
#include
#include
#include
/* #include */
#include
-#include
+
+#include
+
#include "debug.h"
#include "biosdev.h"
@@ -80,14 +85,17 @@ pc_getc(dev_t dev)
return (rv & 0xff);
}
- /*
-* Wait for a character to actually become available. Appears to
-* be necessary on (at least) the Intel Mac Mini.
-*/
- do {
- __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
- "0" (0x100) : "%ecx", "%edx", "cc" );
- } while ((rv & 0xff) == 0);
+ if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
+ /*
+ * Wait for a character to actually become available.
+ * Appears to be necessary on (at least) the Intel Mac
+ * Mini.
+*/
+ do {
+ __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
+ "0" (0x100) : "%ecx", "%edx", "cc" );
+ } while ((rv & 0xff) == 0);
+ }
__asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
"%ecx", "%edx", "cc" );
On Thu, Mar 17, 2016 at 02:58:58PM +0900, Masao Uebayashi wrote:
> Factor out CPUID code in mdrandom(), invoke it once, then save the
> result. I'll use it to switch behavior depending on HV or not.
>
> efiboot is not tested.
>
> Comments?
> >From 104cb04bbbd0f7e40758938cc3103b2370a2285c Mon Sep 17 00:00:00 2001
> From: Masao Uebayashi
> Date: Thu, 10 Mar 2016 21:03:07 +0900
> Subject: [PATCH 1/3] Factor out CPUID instruction in amd64 boot code
>
> ---
> sys/arch/amd64/stand/boot/Makefile | 1 +
> sys/arch/amd64/stand/boot/srt0.S | 1 +
> sys/arch/amd64/stand/cdboot/Makefile | 1 +
> sys/arch/amd64/stand/cdboot/srt0.S | 2 +
> sys/arch/amd64/stand/libsa/cpuid.S | 69
>
> sys/arch/amd64/stand/libsa/libsa.h | 7
> sys/arch/amd64/stand/libsa/random_i386.S | 22 +-
> sys/arch/amd64/stand/pxeboot/Makefile| 1 +
> sys/arch/amd64/stand/pxeboot/srt0.S | 2 +
> 9 files changed, 86 insertions(+), 20 deletions(-)
> create mode 100644 sys/arch/amd64/stand/libsa/cpuid.S
>
> diff --git a/sys/arch/amd64/stand/boot/Makefile
> b/sys/arch/amd64/stand/boot/Makefile
> index 359ea31..5811646 100644
> --- a/sys/arch/amd64/stand/boot/Makefile
> +++ b/sys/arch/amd64/stand/boot/Makefile
> @@ -26,6 +26,7 @@ SRCS+= boot.c bootarg.c cmd.c vars.c
>
> .PATH: ${SADIR}/libsa
> SRCS+= gidt.S random_i386.S
> +SRCS+= cpuid.S
> SRCS+= cmd_i386.c dev_i386.c exec_i386.c gateA20.c machdep.c
> SRCS+= bioscons.c biosdev.c diskprobe.c memprobe.c time.c
> .if ${SOFTRAID:L} == "yes"
> diff --git a/sys/arch/amd64/stand/boot/srt0.S
> b/sys/arch/amd64/stand/boot/srt0.S
> index 9e1ede6..c2a5b2a 100644
> --- a/sys/arch/amd64/stand/boot/srt0.S
> +++ b/sys/arch/amd64/stand/boot/srt0.S
> @@ -87,6 +87,7 @@ _start:
>
Re: CPUID in amd64/i386 boot code
On Thu, Mar 17, 2016 at 03:31:45PM +0900, Masao Uebayashi wrote:
> On Wed, Mar 16, 2016 at 11:26:39PM -0700, Mike Larkin wrote:
> > On Thu, Mar 17, 2016 at 03:15:07PM +0900, Masao Uebayashi wrote:
> > > This is the intended use, avoid busy-polling of BIOS PC console if
> > > running on HV. Avoid 100% CPU usage at boot prompt on hypervisors.
> > >
> >
> > Which hypervisors have you tested this on?
>
> VMware ESXi
> KVM
> QEMU
While I share some of Theo's concerns about testing this, this particular
diff seems ok to me. You probably at least need to get someone to test on
Hyper-V, and run these new bootblocks on a variety of machines (especially
older non-Intel/AMD i386 machines).
-ml
>
> > -ml
> >
> > > Originally I unconditionally reverted the polling part, which was
> > > introduced only for Intel Mac Mini. After brief discussion with mikeb@,
> > > I decided to leave that hack by checking HV bit in CPUID (cpu_ecxfeature).
> > >
> > > diff --git a/sys/arch/amd64/stand/libsa/bioscons.c
> > > b/sys/arch/amd64/stand/libsa/bioscons.c
> > > index bdff9a4..fa42751 100644
> > > --- a/sys/arch/amd64/stand/libsa/bioscons.c
> > > +++ b/sys/arch/amd64/stand/libsa/bioscons.c
> > > @@ -30,6 +30,7 @@
> > >
> > > #include
> > > #include
> > > +#include
> > >
> > > #include
> > > #include
> > > @@ -37,7 +38,7 @@
> > > #include
> > > #include
> > >
> > > -#include
> > > +#include
> > >
> > > #include "biosdev.h"
> > >
> > > @@ -82,14 +83,17 @@ pc_getc(dev_t dev)
> > > return (rv & 0xff);
> > > }
> > >
> > > - /*
> > > - * Wait for a character to actually become available. Appears to
> > > - * be necessary on (at least) the Intel Mac Mini.
> > > - */
> > > - do {
> > > - __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > > - "0" (0x100) : "%ecx", "%edx", "cc" );
> > > - } while ((rv & 0xff) == 0);
> > > + if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
> > > + /*
> > > + * Wait for a character to actually become available.
> > > + * Appears to be necessary on (at least) the Intel Mac
> > > + * Mini.
> > > + */
> > > + do {
> > > + __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > > + "0" (0x100) : "%ecx", "%edx", "cc" );
> > > + } while ((rv & 0xff) == 0);
> > > + }
> > >
> > > __asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
> > > "%ecx", "%edx", "cc" );
> > > diff --git a/sys/arch/i386/stand/libsa/bioscons.c
> > > b/sys/arch/i386/stand/libsa/bioscons.c
> > > index 028bef3..b53abcf 100644
> > > --- a/sys/arch/i386/stand/libsa/bioscons.c
> > > +++ b/sys/arch/i386/stand/libsa/bioscons.c
> > > @@ -27,15 +27,20 @@
> > > */
> > >
> > > #include
> > > +
> > > #include
> > > #include
> > > +#include
> > > +
> > > #include
> > > #include
> > > #include
> > > #include
> > > /* #include */
> > > #include
> > > -#include
> > > +
> > > +#include
> > > +
> > > #include "debug.h"
> > > #include "biosdev.h"
> > >
> > > @@ -80,14 +85,17 @@ pc_getc(dev_t dev)
> > > return (rv & 0xff);
> > > }
> > >
> > > - /*
> > > - * Wait for a character to actually become available. Appears to
> > > - * be necessary on (at least) the Intel Mac Mini.
> > > - */
> > > - do {
> > > - __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > > - "0" (0x100) : "%ecx", "%edx", "cc" );
> > > - } while ((rv & 0xff) == 0);
> > > + if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
> > > + /*
> > > + * Wait for a character to actually become available.
> > > + * Appears to be necessary on (at least) the Intel Mac
> > > + * Mini.
> > > + */
> > > + do {
> > > + __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > > + "0" (0x100) : "%ecx", "%edx", "cc" );
> > > + } while ((rv & 0xff) == 0);
> > > + }
> > >
> > > __asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
> > > "%ecx", "%edx", "cc" );
> > >
> > > On Thu, Mar 17, 2016 at 02:58:58PM +0900, Masao Uebayashi wrote:
> > > > Factor out CPUID code in mdrandom(), invoke it once, then save the
> > > > result. I'll use it to switch behavior depending on HV or not.
> > > >
> > > > efiboot is not tested.
> > > >
> > > > Comments?
> > >
> > > > >From 104cb04bbbd0f7e40758938cc3103b2370a2285c Mon Sep 17 00:00:00 2001
> > > > From: Masao Uebayashi
> > > > Date: Thu, 10 Mar 2016 21:03:07 +0900
> > > > Subject: [PATCH 1/3] Factor out CPUID instruction in amd64 boot code
> > > >
> > > > ---
> > > > sys/arch/amd64/stand/boot/Makefile | 1 +
> > > > sys/arch/amd64/stand/boot/srt0.S | 1 +
> > > > sys/arch/amd64/stand/cdboot/Makefile | 1 +
> > > > sys/arch/amd64/stand/cdboot/srt0.S | 2 +
> > > > sys/arch/amd64/stand/libsa/cpuid.S | 69
> > > >
CPUID in amd64/i386 boot code
Factor out CPUID code in mdrandom(), invoke it once, then save the
result. I'll use it to switch behavior depending on HV or not.
efiboot is not tested.
Comments?
>From 104cb04bbbd0f7e40758938cc3103b2370a2285c Mon Sep 17 00:00:00 2001
From: Masao Uebayashi
Date: Thu, 10 Mar 2016 21:03:07 +0900
Subject: [PATCH 1/3] Factor out CPUID instruction in amd64 boot code
---
sys/arch/amd64/stand/boot/Makefile | 1 +
sys/arch/amd64/stand/boot/srt0.S | 1 +
sys/arch/amd64/stand/cdboot/Makefile | 1 +
sys/arch/amd64/stand/cdboot/srt0.S | 2 +
sys/arch/amd64/stand/libsa/cpuid.S | 69
sys/arch/amd64/stand/libsa/libsa.h | 7
sys/arch/amd64/stand/libsa/random_i386.S | 22 +-
sys/arch/amd64/stand/pxeboot/Makefile| 1 +
sys/arch/amd64/stand/pxeboot/srt0.S | 2 +
9 files changed, 86 insertions(+), 20 deletions(-)
create mode 100644 sys/arch/amd64/stand/libsa/cpuid.S
diff --git a/sys/arch/amd64/stand/boot/Makefile
b/sys/arch/amd64/stand/boot/Makefile
index 359ea31..5811646 100644
--- a/sys/arch/amd64/stand/boot/Makefile
+++ b/sys/arch/amd64/stand/boot/Makefile
@@ -26,6 +26,7 @@ SRCS+=boot.c bootarg.c cmd.c vars.c
.PATH: ${SADIR}/libsa
SRCS+= gidt.S random_i386.S
+SRCS+= cpuid.S
SRCS+= cmd_i386.c dev_i386.c exec_i386.c gateA20.c machdep.c
SRCS+= bioscons.c biosdev.c diskprobe.c memprobe.c time.c
.if ${SOFTRAID:L} == "yes"
diff --git a/sys/arch/amd64/stand/boot/srt0.S b/sys/arch/amd64/stand/boot/srt0.S
index 9e1ede6..c2a5b2a 100644
--- a/sys/arch/amd64/stand/boot/srt0.S
+++ b/sys/arch/amd64/stand/boot/srt0.S
@@ -87,6 +87,7 @@ _start:
rep;stosb
call_ASM_LABEL(pmm_init)
+ call_C_LABEL(initcpuid)
call_C_LABEL(boot)
jmp _C_LABEL(_rtt)
diff --git a/sys/arch/amd64/stand/cdboot/Makefile
b/sys/arch/amd64/stand/cdboot/Makefile
index 23a261f..04ba1e9 100644
--- a/sys/arch/amd64/stand/cdboot/Makefile
+++ b/sys/arch/amd64/stand/cdboot/Makefile
@@ -19,6 +19,7 @@ BINMODE=644
SRCS+= machdep.c dev_i386.c exec_i386.c cmd_i386.c
SRCS+= gidt.S random_i386.S biosdev.c bioscons.c gateA20.c \
memprobe.c diskprobe.c time.c
+SRCS+= cpuid.S
SRCS+= softraid.c
.PATH: ${S}/stand/boot
diff --git a/sys/arch/amd64/stand/cdboot/srt0.S
b/sys/arch/amd64/stand/cdboot/srt0.S
index 82ff009..abe40d4 100644
--- a/sys/arch/amd64/stand/cdboot/srt0.S
+++ b/sys/arch/amd64/stand/cdboot/srt0.S
@@ -177,6 +177,8 @@ relocated:
movl%eax, _C_LABEL(bios_bootdev)
movl%eax, _C_LABEL(bios_cddev)
+ call_C_LABEL(initcpuid)
+
/*
* Now call "main()".
*
diff --git a/sys/arch/amd64/stand/libsa/cpuid.S
b/sys/arch/amd64/stand/libsa/cpuid.S
new file mode 100644
index 000..c3ca1ac
--- /dev/null
+++ b/sys/arch/amd64/stand/libsa/cpuid.S
@@ -0,0 +1,69 @@
+/* $OpenBSD$ */
+
+/*
+ * Copyright (c) 2016 Masao Uebayashi
+ * Copyright (c) 2013 Joel Sing
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include
+#include
+#include
+#include
+
+ .data
+
+ .globl _C_LABEL(cpu_id)
+ .globl _C_LABEL(cpu_feature)
+ .globl _C_LABEL(cpu_ebxfeature)
+ .globl _C_LABEL(cpu_ecxfeature)
+
+_C_LABEL(cpu_id): .long 0
+_C_LABEL(cpu_feature): .long 0
+_C_LABEL(cpu_ebxfeature): .long 0
+_C_LABEL(cpu_ecxfeature): .long 0
+
+ .text
+
+ENTRY(initcpuid)
+ pushal
+
+ // See if we have CPU identification.
+ pushfl
+ popl%eax
+ movl%eax, %ecx
+ orl $PSL_ID, %eax
+ pushl %eax
+ popfl
+ pushfl
+ popl%eax
+ pushl %ecx
+ popfl
+ andl$PSL_ID, %eax
+ jz done
+
+ // CPUID leaf = 1, subleaf = 0
+ movl$1, %eax
+ movl$0, %ecx
+ cpuid
+
+ movl%eax, _C_LABEL(cpu_id)
+ movl%ebx, _C_LABEL(cpu_ebxfeature)
+ movl%ecx, _C_LABEL(cpu_ecxfeature)
+ movl%edx, _C_LABEL(cpu_feature)
+
+done:
+ popal
+ ret
+END(initcpuid)
diff --git a/sys/arch/amd64/stand/libsa/libsa.h
b/sys/arch/amd64/stand/libsa/libsa.h
index fc65c6f..1934078 100644
--- a/sys/arch/amd64/stand/libsa/libsa.h
+++ b/sys/arch/amd64/stand/libsa/libsa
Re: CPUID in amd64/i386 boot code
On Thu, Mar 17, 2016 at 03:15:07PM +0900, Masao Uebayashi wrote:
> This is the intended use, avoid busy-polling of BIOS PC console if
> running on HV. Avoid 100% CPU usage at boot prompt on hypervisors.
>
Which hypervisors have you tested this on?
-ml
> Originally I unconditionally reverted the polling part, which was
> introduced only for Intel Mac Mini. After brief discussion with mikeb@,
> I decided to leave that hack by checking HV bit in CPUID (cpu_ecxfeature).
>
> diff --git a/sys/arch/amd64/stand/libsa/bioscons.c
> b/sys/arch/amd64/stand/libsa/bioscons.c
> index bdff9a4..fa42751 100644
> --- a/sys/arch/amd64/stand/libsa/bioscons.c
> +++ b/sys/arch/amd64/stand/libsa/bioscons.c
> @@ -30,6 +30,7 @@
>
> #include
> #include
> +#include
>
> #include
> #include
> @@ -37,7 +38,7 @@
> #include
> #include
>
> -#include
> +#include
>
> #include "biosdev.h"
>
> @@ -82,14 +83,17 @@ pc_getc(dev_t dev)
> return (rv & 0xff);
> }
>
> - /*
> - * Wait for a character to actually become available. Appears to
> - * be necessary on (at least) the Intel Mac Mini.
> - */
> - do {
> - __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> - "0" (0x100) : "%ecx", "%edx", "cc" );
> - } while ((rv & 0xff) == 0);
> + if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
> + /*
> + * Wait for a character to actually become available.
> + * Appears to be necessary on (at least) the Intel Mac
> + * Mini.
> + */
> + do {
> + __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> + "0" (0x100) : "%ecx", "%edx", "cc" );
> + } while ((rv & 0xff) == 0);
> + }
>
> __asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
> "%ecx", "%edx", "cc" );
> diff --git a/sys/arch/i386/stand/libsa/bioscons.c
> b/sys/arch/i386/stand/libsa/bioscons.c
> index 028bef3..b53abcf 100644
> --- a/sys/arch/i386/stand/libsa/bioscons.c
> +++ b/sys/arch/i386/stand/libsa/bioscons.c
> @@ -27,15 +27,20 @@
> */
>
> #include
> +
> #include
> #include
> +#include
> +
> #include
> #include
> #include
> #include
> /* #include */
> #include
> -#include
> +
> +#include
> +
> #include "debug.h"
> #include "biosdev.h"
>
> @@ -80,14 +85,17 @@ pc_getc(dev_t dev)
> return (rv & 0xff);
> }
>
> - /*
> - * Wait for a character to actually become available. Appears to
> - * be necessary on (at least) the Intel Mac Mini.
> - */
> - do {
> - __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> - "0" (0x100) : "%ecx", "%edx", "cc" );
> - } while ((rv & 0xff) == 0);
> + if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
> + /*
> + * Wait for a character to actually become available.
> + * Appears to be necessary on (at least) the Intel Mac
> + * Mini.
> + */
> + do {
> + __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> + "0" (0x100) : "%ecx", "%edx", "cc" );
> + } while ((rv & 0xff) == 0);
> + }
>
> __asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
> "%ecx", "%edx", "cc" );
>
> On Thu, Mar 17, 2016 at 02:58:58PM +0900, Masao Uebayashi wrote:
> > Factor out CPUID code in mdrandom(), invoke it once, then save the
> > result. I'll use it to switch behavior depending on HV or not.
> >
> > efiboot is not tested.
> >
> > Comments?
>
> > >From 104cb04bbbd0f7e40758938cc3103b2370a2285c Mon Sep 17 00:00:00 2001
> > From: Masao Uebayashi
> > Date: Thu, 10 Mar 2016 21:03:07 +0900
> > Subject: [PATCH 1/3] Factor out CPUID instruction in amd64 boot code
> >
> > ---
> > sys/arch/amd64/stand/boot/Makefile | 1 +
> > sys/arch/amd64/stand/boot/srt0.S | 1 +
> > sys/arch/amd64/stand/cdboot/Makefile | 1 +
> > sys/arch/amd64/stand/cdboot/srt0.S | 2 +
> > sys/arch/amd64/stand/libsa/cpuid.S | 69
> >
> > sys/arch/amd64/stand/libsa/libsa.h | 7
> > sys/arch/amd64/stand/libsa/random_i386.S | 22 +-
> > sys/arch/amd64/stand/pxeboot/Makefile| 1 +
> > sys/arch/amd64/stand/pxeboot/srt0.S | 2 +
> > 9 files changed, 86 insertions(+), 20 deletions(-)
> > create mode 100644 sys/arch/amd64/stand/libsa/cpuid.S
> >
> > diff --git a/sys/arch/amd64/stand/boot/Makefile
> > b/sys/arch/amd64/stand/boot/Makefile
> > index 359ea31..5811646 100644
> > --- a/sys/arch/amd64/stand/boot/Makefile
> > +++ b/sys/arch/amd64/stand/boot/Makefile
> > @@ -26,6 +26,7 @@ SRCS+=boot.c bootarg.c cmd.c vars.c
> >
> > .PATH: ${SADIR}/libsa
> > SRCS+= gidt.S random_i386.S
> > +SRCS+= cpuid.S
> > SRCS+= cmd_i386.c dev_i386.c exec_i386.c gateA20.c machdep.c
> > SRCS+
Re: CPUID in amd64/i386 boot code
On Wed, Mar 16, 2016 at 23:47 -0700, Mike Larkin wrote:
> On Thu, Mar 17, 2016 at 03:31:45PM +0900, Masao Uebayashi wrote:
> > On Wed, Mar 16, 2016 at 11:26:39PM -0700, Mike Larkin wrote:
> > > On Thu, Mar 17, 2016 at 03:15:07PM +0900, Masao Uebayashi wrote:
> > > > This is the intended use, avoid busy-polling of BIOS PC console if
> > > > running on HV. Avoid 100% CPU usage at boot prompt on hypervisors.
> > > >
> > >
> > > Which hypervisors have you tested this on?
> >
> > VMware ESXi
> > KVM
> > QEMU
>
> While I share some of Theo's concerns about testing this, this particular
> diff seems ok to me. You probably at least need to get someone to test on
> Hyper-V, and run these new bootblocks on a variety of machines (especially
> older non-Intel/AMD i386 machines).
>
> -ml
>
I've tested the diff and with a small adjustment to the
efiboot/bootia32/Makefile below, I was able to rebuild
stand/ and test the thing (with the follow up diff to
the bioscons.c).
As I've mentioned on ICB, Xen exposes this issue, while
Hyper-V doesn't. The change doesn't help or harm Xen, no
change on Hyper-V. But I think it's still useful if this
helps KVM and VMware.
> >
> > > -ml
> > >
> > > > Originally I unconditionally reverted the polling part, which was
> > > > introduced only for Intel Mac Mini. After brief discussion with mikeb@,
> > > > I decided to leave that hack by checking HV bit in CPUID
> > > > (cpu_ecxfeature).
> > > >
> > > > diff --git a/sys/arch/amd64/stand/libsa/bioscons.c
> > > > b/sys/arch/amd64/stand/libsa/bioscons.c
> > > > index bdff9a4..fa42751 100644
> > > > --- a/sys/arch/amd64/stand/libsa/bioscons.c
> > > > +++ b/sys/arch/amd64/stand/libsa/bioscons.c
> > > > @@ -30,6 +30,7 @@
> > > >
> > > > #include
> > > > #include
> > > > +#include
> > > >
> > > > #include
> > > > #include
> > > > @@ -37,7 +38,7 @@
> > > > #include
> > > > #include
> > > >
> > > > -#include
> > > > +#include
> > > >
> > > > #include "biosdev.h"
> > > >
> > > > @@ -82,14 +83,17 @@ pc_getc(dev_t dev)
> > > > return (rv & 0xff);
> > > > }
> > > >
> > > > - /*
> > > > -* Wait for a character to actually become available. Appears
> > > > to
> > > > -* be necessary on (at least) the Intel Mac Mini.
> > > > -*/
> > > > - do {
> > > > - __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > > > - "0" (0x100) : "%ecx", "%edx", "cc" );
> > > > - } while ((rv & 0xff) == 0);
> > > > + if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
> > > > + /*
> > > > + * Wait for a character to actually become available.
> > > > + * Appears to be necessary on (at least) the Intel Mac
> > > > + * Mini.
> > > > +*/
> > > > + do {
> > > > + __asm volatile(DOINT(0x16) "; setnz %b0" : "=a"
> > > > (rv) :
> > > > + "0" (0x100) : "%ecx", "%edx", "cc" );
> > > > + } while ((rv & 0xff) == 0);
> > > > + }
> > > >
> > > > __asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
> > > > "%ecx", "%edx", "cc" );
> > > > diff --git a/sys/arch/i386/stand/libsa/bioscons.c
> > > > b/sys/arch/i386/stand/libsa/bioscons.c
> > > > index 028bef3..b53abcf 100644
> > > > --- a/sys/arch/i386/stand/libsa/bioscons.c
> > > > +++ b/sys/arch/i386/stand/libsa/bioscons.c
> > > > @@ -27,15 +27,20 @@
> > > > */
> > > >
> > > > #include
> > > > +
> > > > #include
> > > > #include
> > > > +#include
> > > > +
> > > > #include
> > > > #include
> > > > #include
> > > > #include
> > > > /* #include */
> > > > #include
> > > > -#include
> > > > +
> > > > +#include
> > > > +
> > > > #include "debug.h"
> > > > #include "biosdev.h"
> > > >
> > > > @@ -80,14 +85,17 @@ pc_getc(dev_t dev)
> > > > return (rv & 0xff);
> > > > }
> > > >
> > > > - /*
> > > > -* Wait for a character to actually become available. Appears
> > > > to
> > > > -* be necessary on (at least) the Intel Mac Mini.
> > > > -*/
> > > > - do {
> > > > - __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > > > - "0" (0x100) : "%ecx", "%edx", "cc" );
> > > > - } while ((rv & 0xff) == 0);
> > > > + if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
> > > > + /*
> > > > + * Wait for a character to actually become available.
> > > > + * Appears to be necessary on (at least) the Intel Mac
> > > > + * Mini.
> > > > +*/
> > > > + do {
> > > > + __asm volatile(DOINT(0x16) "; setnz %b0" : "=a"
> > > > (rv) :
> > > > + "0" (0x100) : "%ecx", "%edx", "cc" );
> > > > + } while ((rv & 0xff) == 0);
> > > > + }
> > > >
> > > > __a
Re: CPUID in amd64/i386 boot code
> Factor out CPUID code in mdrandom(), invoke it once, then save the > result. I'll use it to switch behavior depending on HV or not. > > efiboot is not tested. > > Comments? I don't understand the purpose of the refactoring.
Re: CPUID in amd64/i386 boot code
The process I see here the submission of 2 seperate diffs to the bootblocks, requiring people to take two steps. Then readers of tech@ will realize that the proposition is to test their bootblocks right away, many will balk. The diffs also do not come with a report as to what machines were tested, only a description of the problem it is purposed to solve, that is scary, so even more people will shy from participating. Anyone left? What path do you propose forward to getting this tested? This submission format I see here, is rather unlikely to move forward without a more guidance
Re: CPUID in amd64/i386 boot code
On Wed, Mar 16, 2016 at 11:26:39PM -0700, Mike Larkin wrote:
> On Thu, Mar 17, 2016 at 03:15:07PM +0900, Masao Uebayashi wrote:
> > This is the intended use, avoid busy-polling of BIOS PC console if
> > running on HV. Avoid 100% CPU usage at boot prompt on hypervisors.
> >
>
> Which hypervisors have you tested this on?
VMware ESXi
KVM
QEMU
> -ml
>
> > Originally I unconditionally reverted the polling part, which was
> > introduced only for Intel Mac Mini. After brief discussion with mikeb@,
> > I decided to leave that hack by checking HV bit in CPUID (cpu_ecxfeature).
> >
> > diff --git a/sys/arch/amd64/stand/libsa/bioscons.c
> > b/sys/arch/amd64/stand/libsa/bioscons.c
> > index bdff9a4..fa42751 100644
> > --- a/sys/arch/amd64/stand/libsa/bioscons.c
> > +++ b/sys/arch/amd64/stand/libsa/bioscons.c
> > @@ -30,6 +30,7 @@
> >
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -37,7 +38,7 @@
> > #include
> > #include
> >
> > -#include
> > +#include
> >
> > #include "biosdev.h"
> >
> > @@ -82,14 +83,17 @@ pc_getc(dev_t dev)
> > return (rv & 0xff);
> > }
> >
> > - /*
> > -* Wait for a character to actually become available. Appears to
> > -* be necessary on (at least) the Intel Mac Mini.
> > -*/
> > - do {
> > - __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > - "0" (0x100) : "%ecx", "%edx", "cc" );
> > - } while ((rv & 0xff) == 0);
> > + if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
> > + /*
> > + * Wait for a character to actually become available.
> > + * Appears to be necessary on (at least) the Intel Mac
> > + * Mini.
> > +*/
> > + do {
> > + __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > + "0" (0x100) : "%ecx", "%edx", "cc" );
> > + } while ((rv & 0xff) == 0);
> > + }
> >
> > __asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
> > "%ecx", "%edx", "cc" );
> > diff --git a/sys/arch/i386/stand/libsa/bioscons.c
> > b/sys/arch/i386/stand/libsa/bioscons.c
> > index 028bef3..b53abcf 100644
> > --- a/sys/arch/i386/stand/libsa/bioscons.c
> > +++ b/sys/arch/i386/stand/libsa/bioscons.c
> > @@ -27,15 +27,20 @@
> > */
> >
> > #include
> > +
> > #include
> > #include
> > +#include
> > +
> > #include
> > #include
> > #include
> > #include
> > /* #include */
> > #include
> > -#include
> > +
> > +#include
> > +
> > #include "debug.h"
> > #include "biosdev.h"
> >
> > @@ -80,14 +85,17 @@ pc_getc(dev_t dev)
> > return (rv & 0xff);
> > }
> >
> > - /*
> > -* Wait for a character to actually become available. Appears to
> > -* be necessary on (at least) the Intel Mac Mini.
> > -*/
> > - do {
> > - __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > - "0" (0x100) : "%ecx", "%edx", "cc" );
> > - } while ((rv & 0xff) == 0);
> > + if ((cpu_ecxfeature & CPUIDECX_HV) == 0) {
> > + /*
> > + * Wait for a character to actually become available.
> > + * Appears to be necessary on (at least) the Intel Mac
> > + * Mini.
> > +*/
> > + do {
> > + __asm volatile(DOINT(0x16) "; setnz %b0" : "=a" (rv) :
> > + "0" (0x100) : "%ecx", "%edx", "cc" );
> > + } while ((rv & 0xff) == 0);
> > + }
> >
> > __asm volatile(DOINT(0x16) : "=a" (rv) : "0" (0x000) :
> > "%ecx", "%edx", "cc" );
> >
> > On Thu, Mar 17, 2016 at 02:58:58PM +0900, Masao Uebayashi wrote:
> > > Factor out CPUID code in mdrandom(), invoke it once, then save the
> > > result. I'll use it to switch behavior depending on HV or not.
> > >
> > > efiboot is not tested.
> > >
> > > Comments?
> >
> > > >From 104cb04bbbd0f7e40758938cc3103b2370a2285c Mon Sep 17 00:00:00 2001
> > > From: Masao Uebayashi
> > > Date: Thu, 10 Mar 2016 21:03:07 +0900
> > > Subject: [PATCH 1/3] Factor out CPUID instruction in amd64 boot code
> > >
> > > ---
> > > sys/arch/amd64/stand/boot/Makefile | 1 +
> > > sys/arch/amd64/stand/boot/srt0.S | 1 +
> > > sys/arch/amd64/stand/cdboot/Makefile | 1 +
> > > sys/arch/amd64/stand/cdboot/srt0.S | 2 +
> > > sys/arch/amd64/stand/libsa/cpuid.S | 69
> > >
> > > sys/arch/amd64/stand/libsa/libsa.h | 7
> > > sys/arch/amd64/stand/libsa/random_i386.S | 22 +-
> > > sys/arch/amd64/stand/pxeboot/Makefile| 1 +
> > > sys/arch/amd64/stand/pxeboot/srt0.S | 2 +
> > > 9 files changed, 86 insertions(+), 20 deletions(-)
> > > create mode 100644 sys/arch/amd64/stand/libsa/cpuid.S
> > >
> > > diff --git a/sys/arch/amd64/stand/boot/Makefile
> > > b/sys/arch/amd64/stand/boot/Makefile
> > > index 359ea31..5811646 100644
> > > --- a/sys/arch/amd64/stand/boot/M
Re: CPUID in amd64/i386 boot code
On Thu, Mar 17, 2016 at 16:39 +0100, Mike Belopuhov wrote: > On Wed, Mar 16, 2016 at 23:47 -0700, Mike Larkin wrote: > > On Thu, Mar 17, 2016 at 03:31:45PM +0900, Masao Uebayashi wrote: > > > On Wed, Mar 16, 2016 at 11:26:39PM -0700, Mike Larkin wrote: > > > > On Thu, Mar 17, 2016 at 03:15:07PM +0900, Masao Uebayashi wrote: > > > > > This is the intended use, avoid busy-polling of BIOS PC console if > > > > > running on HV. Avoid 100% CPU usage at boot prompt on hypervisors. > > > > > > > > > > > > > Which hypervisors have you tested this on? > > > > > > VMware ESXi > > > KVM > > > QEMU > > > > While I share some of Theo's concerns about testing this, this particular > > diff seems ok to me. You probably at least need to get someone to test on > > Hyper-V, and run these new bootblocks on a variety of machines (especially > > older non-Intel/AMD i386 machines). > > > > -ml > > > > I've tested the diff and with a small adjustment to the > efiboot/bootia32/Makefile below, I was able to rebuild > stand/ and test the thing (with the follow up diff to > the bioscons.c). > > As I've mentioned on ICB, Xen exposes this issue, while > Hyper-V doesn't. The change doesn't help or harm Xen, no > change on Hyper-V. But I think it's still useful if this > helps KVM and VMware. > Apparently I missed that glass and serial consoles have different implementations. Duh! So with a "glass" console I can see reduction on both Xen and Hyper-V. On Xen CPU load goes from 100% (according to "xl top") to 0, while on Hyper-V: from 8% to 0. Thanks again for the diff.
