[Xen-devel] [PATCH] sched/credit: avoid priority boost for capped domains when unpark
When unpausing a capped domain, the scheduler currently clears the
CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn, causes the
vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit boost. The
comment around the changed lines already states that clearing the flag should
happen AFTER the unpause. This bug was introduced in commit be650750945
"credit1: Use atomic bit operations for the flags structure".
Original patch author credit: Xi Xiong.
Signed-off-by: Eslam Elnikety
Reviewed-by: Leonard Foerster
Reviewed-by: Petre Eftime
---
xen/common/sched_credit.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 3abe20def8..8eb1aba12a 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1538,7 +1538,7 @@ csched_acct(void* dummy)
svc->pri = CSCHED_PRI_TS_UNDER;
/* Unpark any capped domains whose credits go positive */
-if ( test_and_clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
+if ( test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
{
/*
* It's important to unset the flag AFTER the unpause()
@@ -1547,6 +1547,8 @@ csched_acct(void* dummy)
*/
SCHED_STAT_CRANK(vcpu_unpark);
vcpu_unpause(svc->vcpu);
+/* Now clear the PARKED flag */
+clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags);
}
/* Upper bound on credits means VCPU stops earning */
--
2.15.3.AMZN
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] sched/credit: avoid priority boost for capped domains when unpark
When unpausing a capped domain, the scheduler currently clears the
CSCHED_FLAG_VCPU_PARKED flag before vcpu_wake(). This, in turn, causes the
vcpu_wake to set CSCHED_PRI_TS_BOOST, resulting in an unfair credit boost. The
comment around the changed lines already states that clearing the flag should
happen AFTER the unpause. This bug was introduced in commit be650750945
"credit1: Use atomic bit operations for the flags structure".
Original patch author credit: Xi Xiong while at Amazon.
Signed-off-by: Eslam Elnikety
Reviewed-by: Leonard Foerster
Reviewed-by: Petre Eftime
Acked-by: Dario Faggioli
---
Changes in v2:
- Adjusted commit message, adding Xi's previous affiliation
- Dropped comment: /* Now clear the PARKED flag */
- Added Dario's A-b
---
xen/common/sched_credit.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
index 3abe20def8..7b7facbace 100644
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -1538,7 +1538,7 @@ csched_acct(void* dummy)
svc->pri = CSCHED_PRI_TS_UNDER;
/* Unpark any capped domains whose credits go positive */
-if ( test_and_clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
+if ( test_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags) )
{
/*
* It's important to unset the flag AFTER the unpause()
@@ -1547,6 +1547,7 @@ csched_acct(void* dummy)
*/
SCHED_STAT_CRANK(vcpu_unpark);
vcpu_unpause(svc->vcpu);
+clear_bit(CSCHED_FLAG_VCPU_PARKED, &svc->flags);
}
/* Upper bound on credits means VCPU stops earning */
--
2.15.3.AMZN
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] mm: option to _always_ scrub freed domheap pages
Give the administrator further control on when to scrub domheap pages by adding
an option to always scrub. This is a safety feature that, when enabled,
prevents a (buggy) domain from leaking secrets if it accidentally frees a page
without proper scrubbing.
Signed-off-by: Eslam Elnikety
---
docs/misc/xen-command-line.pandoc | 8
xen/common/page_alloc.c | 11 +--
2 files changed, 17 insertions(+), 2 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index 7dcb22932a..5a92949c5a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -270,6 +270,14 @@ and not running softirqs. Reduce this if softirqs are not
being run frequently
enough. Setting this to a high value may cause boot failure, particularly if
the NMI watchdog is also enabled.
+### scrub_domheap
+> `= `
+
+> Default: `false`
+
+Scrub domains' freed pages. This is a safety net against a (buggy) domain
+accidentally leaking secrets by releasing pages without proper sanitization.
+
### clocksource (x86)
> `= pit | hpet | acpi | tsc`
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index be44158033..678a00ac9b 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -214,6 +214,12 @@ custom_param("bootscrub", parse_bootscrub_param);
static unsigned long __initdata opt_bootscrub_chunk = MB(128);
size_param("bootscrub_chunk", opt_bootscrub_chunk);
+/*
+ * scrub_domheap -> Domheap pages are scrubbed when freed
+ */
+static bool_t opt_scrub_domheap = 0;
+boolean_param("scrub_domheap", opt_scrub_domheap);
+
#ifdef CONFIG_SCRUB_DEBUG
static bool __read_mostly scrub_debug;
#else
@@ -2378,9 +2384,10 @@ void free_domheap_pages(struct page_info *pg, unsigned
int order)
/*
* Normally we expect a domain to clear pages before freeing them,
* if it cares about the secrecy of their contents. However, after
- * a domain has died we assume responsibility for erasure.
+ * a domain has died we assume responsibility for erasure. We do
+ * scrub regardless if option scrub_domheap is set.
*/
-scrub = d->is_dying || scrub_debug;
+scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
}
else
{
--
2.15.3.AMZN
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] libxl: make vkbd tunable for HVM guests
Each HVM guest currently gets a vkbd frontend/backend pair (c/s ebbd2561b4c).
This consumes host resources unnecessarily for guests that have no use for
vkbd. Make this behaviour tunable to allow an administrator to choose. The
commit retains the current behaviour -- HVM guests still get vkdb unless
specified otherwise.
Signed-off-by: Eslam Elnikety
---
tools/libxl/libxl_create.c | 9 ++---
tools/libxl/libxl_types.idl | 1 +
tools/xl/xl_sxp.c | 2 ++
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 89fe80fc9c..b09244058f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -310,6 +310,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->u.hvm.vpt_align, true);
libxl_defbool_setdefault(&b_info->u.hvm.altp2m, false);
libxl_defbool_setdefault(&b_info->u.hvm.usb,false);
+libxl_defbool_setdefault(&b_info->u.hvm.vkb_device, true);
libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci, true);
libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
@@ -1416,9 +1417,11 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
libxl__device_console_add(gc, domid, &console, state, &device);
libxl__device_console_dispose(&console);
-libxl_device_vkb_init(&vkb);
-libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
-libxl_device_vkb_dispose(&vkb);
+if ( libxl_defbool_val(d_config->b_info.u.hvm.vkb_device) ) {
+libxl_device_vkb_init(&vkb);
+libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
+libxl_device_vkb_dispose(&vkb);
+}
dcs->sdss.dm.guest_domid = domid;
if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b685ac47ac..9a0b92f1d4 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -583,6 +583,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
# - "tablet" for absolute mouse,
# - "mouse" for PS/2 protocol relative
mouse
("usbdevice",string),
+ ("vkb_device", libxl_defbool),
("soundhw", string),
("xen_platform_pci", libxl_defbool),
("usbdevice_list", libxl_string_list),
diff --git a/tools/xl/xl_sxp.c b/tools/xl/xl_sxp.c
index 3e6117d0cd..359a001570 100644
--- a/tools/xl/xl_sxp.c
+++ b/tools/xl/xl_sxp.c
@@ -138,6 +138,8 @@ void printf_info_sexp(int domid, libxl_domain_config
*d_config, FILE *fh)
fprintf(fh, "\t\t\t(boot %s)\n", b_info->u.hvm.boot);
fprintf(fh, "\t\t\t(usb %s)\n",
libxl_defbool_to_string(b_info->u.hvm.usb));
fprintf(fh, "\t\t\t(usbdevice %s)\n", b_info->u.hvm.usbdevice);
+fprintf(fh, "\t\t\t(vkb_device %s)\n",
+ libxl_defbool_to_string(b_info->u.hvm.vkb_device));
fprintf(fh, "\t\t)\n");
break;
case LIBXL_DOMAIN_TYPE_PV:
--
2.15.3.AMZN
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] mm: option to _always_ scrub freed domheap pages
Give the administrator further control on when to scrub domheap pages by adding
an option to always scrub. This is a safety feature that, when enabled,
prevents a (buggy) domain from leaking secrets if it accidentally frees a page
without proper scrubbing.
Signed-off-by: Eslam Elnikety
Acked-by: George Dunlap
---
Changes in v2:
- Renamed parameter to scrub-domheap, and now at the right place
- Used "bool __read_mostly", no zero init, and correct comment style
- Added George's A-b
---
docs/misc/xen-command-line.pandoc | 8
xen/common/page_alloc.c | 9 +++--
2 files changed, 15 insertions(+), 2 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index 6db82f302e..771333fc8a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -1779,6 +1779,14 @@ sockets, &c. This will reduce performance somewhat,
particularly on
systems with hyperthreading enabled, but should reduce power by
enabling more sockets and cores to go into deeper sleep states.
+### scrub-domheap
+> `= `
+
+> Default: `false`
+
+Scrub domains' freed pages. This is a safety net against a (buggy) domain
+accidentally leaking secrets by releasing pages without proper sanitization.
+
### serial_tx_buffer
> `= `
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index be44158033..9c12d71fc1 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -214,6 +214,10 @@ custom_param("bootscrub", parse_bootscrub_param);
static unsigned long __initdata opt_bootscrub_chunk = MB(128);
size_param("bootscrub_chunk", opt_bootscrub_chunk);
+ /* scrub-domheap -> Domheap pages are scrubbed when freed */
+static bool __read_mostly opt_scrub_domheap;
+boolean_param("scrub-domheap", opt_scrub_domheap);
+
#ifdef CONFIG_SCRUB_DEBUG
static bool __read_mostly scrub_debug;
#else
@@ -2378,9 +2382,10 @@ void free_domheap_pages(struct page_info *pg, unsigned
int order)
/*
* Normally we expect a domain to clear pages before freeing them,
* if it cares about the secrecy of their contents. However, after
- * a domain has died we assume responsibility for erasure.
+ * a domain has died we assume responsibility for erasure. We do
+ * scrub regardless if option scrub_domheap is set.
*/
-scrub = d->is_dying || scrub_debug;
+scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
}
else
{
--
2.15.3.AMZN
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] libxl: make vkbd tunable for HVM guests
Each HVM guest currently gets a vkbd frontend/backend pair (c/s ebbd2561b4c).
This consumes host resources unnecessarily for guests that have no use for
vkbd. Make this behaviour tunable to allow an administrator to choose. The
commit retains the current behaviour -- HVM guests still get vkdb unless
specified otherwise.
Signed-off-by: Eslam Elnikety
---
Changes in v2:
- Added a missing hunk / setting vkb_device per config
---
tools/libxl/libxl_create.c | 9 ++---
tools/libxl/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 1 +
tools/xl/xl_sxp.c | 2 ++
4 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 89fe80fc9c..03ce166f4f 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -310,6 +310,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->u.hvm.vpt_align, true);
libxl_defbool_setdefault(&b_info->u.hvm.altp2m, false);
libxl_defbool_setdefault(&b_info->u.hvm.usb,false);
+libxl_defbool_setdefault(&b_info->u.hvm.vkb_device, true);
libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci, true);
libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
@@ -1416,9 +1417,11 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
libxl__device_console_add(gc, domid, &console, state, &device);
libxl__device_console_dispose(&console);
-libxl_device_vkb_init(&vkb);
-libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
-libxl_device_vkb_dispose(&vkb);
+if (libxl_defbool_val(d_config->b_info.u.hvm.vkb_device)) {
+libxl_device_vkb_init(&vkb);
+libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
+libxl_device_vkb_dispose(&vkb);
+}
dcs->sdss.dm.guest_domid = domid;
if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b685ac47ac..9a0b92f1d4 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -583,6 +583,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
# - "tablet" for absolute mouse,
# - "mouse" for PS/2 protocol relative
mouse
("usbdevice",string),
+ ("vkb_device", libxl_defbool),
("soundhw", string),
("xen_platform_pci", libxl_defbool),
("usbdevice_list", libxl_string_list),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index 352cd214dd..e105bda2bb 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2652,6 +2652,7 @@ skip_usbdev:
fprintf(stderr,"xl: Unable to parse usbdevice.\n");
exit(-ERROR_FAIL);
}
+xlu_cfg_get_defbool(config, "vkb_device", &b_info->u.hvm.vkb_device,
0);
xlu_cfg_replace_string (config, "soundhw", &b_info->u.hvm.soundhw, 0);
xlu_cfg_get_defbool(config, "xen_platform_pci",
&b_info->u.hvm.xen_platform_pci, 0);
diff --git a/tools/xl/xl_sxp.c b/tools/xl/xl_sxp.c
index 3e6117d0cd..359a001570 100644
--- a/tools/xl/xl_sxp.c
+++ b/tools/xl/xl_sxp.c
@@ -138,6 +138,8 @@ void printf_info_sexp(int domid, libxl_domain_config
*d_config, FILE *fh)
fprintf(fh, "\t\t\t(boot %s)\n", b_info->u.hvm.boot);
fprintf(fh, "\t\t\t(usb %s)\n",
libxl_defbool_to_string(b_info->u.hvm.usb));
fprintf(fh, "\t\t\t(usbdevice %s)\n", b_info->u.hvm.usbdevice);
+fprintf(fh, "\t\t\t(vkb_device %s)\n",
+ libxl_defbool_to_string(b_info->u.hvm.vkb_device));
fprintf(fh, "\t\t)\n");
break;
case LIBXL_DOMAIN_TYPE_PV:
--
2.15.3.AMZN
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3] libxl: make vkbd tunable for HVM guests
Each HVM guest currently gets a vkbd frontend/backend pair (c/s ebbd2561b4c).
This consumes host resources unnecessarily for guests that have no use for
vkbd. Make this behaviour tunable to allow an administrator to choose. The
commit retains the current behaviour -- HVM guests still get vkdb unless
specified otherwise.
Signed-off-by: Eslam Elnikety
---
Changes in v2:
- Added a missing hunk / setting vkb_device per config
Changes in v3:
- Added entries in libxl.h and in documentation
---
docs/man/xl.cfg.pod.5.in| 4
tools/libxl/libxl.h | 9 +
tools/libxl/libxl_create.c | 9 ++---
tools/libxl/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 1 +
tools/xl/xl_sxp.c | 2 ++
6 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 47d88243b1..c3641c7a57 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -2286,6 +2286,10 @@ devices are defined by the device model configuration,
please see the
B manpage for details. The default is not to export any sound
device.
+=item B
+
+Specifies that the HVM guest gets a vkdb. The default is true (1).
+
=item B
Enables or disables an emulated USB bus in the guest.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index a09d069358..6884c1492a 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -616,6 +616,15 @@ typedef struct libxl__ctx libxl_ctx;
*/
#define LIBXL_HAVE_BUILDINFO_VCPU_AFFINITY_ARRAYS 1
+/*
+ * LIBXL_HAVE_BUILDINFO_VKB_DEVICE
+ *
+ * If this is defined, then the libxl_domain_build_info structure will
+ * contain a boolean hvm.vkb_device which instructs libxl whether to include
+ * a vkbd at build time or not.
+ */
+#define LIBXL_HAVE_BUILDINFO_VKB_DEVICE 1
+
/*
* LIBXL_HAVE_BUILDINFO_USBDEVICE_LIST
*
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 5c9dd4cd21..8aecb9cfba 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -308,6 +308,7 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->u.hvm.vpt_align, true);
libxl_defbool_setdefault(&b_info->u.hvm.altp2m, false);
libxl_defbool_setdefault(&b_info->u.hvm.usb,false);
+libxl_defbool_setdefault(&b_info->u.hvm.vkb_device, true);
libxl_defbool_setdefault(&b_info->u.hvm.xen_platform_pci, true);
libxl_defbool_setdefault(&b_info->u.hvm.spice.enable, false);
@@ -1393,9 +1394,11 @@ static void domcreate_launch_dm(libxl__egc *egc,
libxl__multidev *multidev,
libxl__device_console_add(gc, domid, &console, state, &device);
libxl__device_console_dispose(&console);
-libxl_device_vkb_init(&vkb);
-libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
-libxl_device_vkb_dispose(&vkb);
+if (libxl_defbool_val(d_config->b_info.u.hvm.vkb_device)) {
+libxl_device_vkb_init(&vkb);
+libxl__device_add(gc, domid, &libxl__vkb_devtype, &vkb);
+libxl_device_vkb_dispose(&vkb);
+}
dcs->sdss.dm.guest_domid = domid;
if (libxl_defbool_val(d_config->b_info.device_model_stubdomain))
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 01ec1d1afa..1d436aa8bb 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -571,6 +571,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
# - "tablet" for absolute mouse,
# - "mouse" for PS/2 protocol relative
mouse
("usbdevice",string),
+ ("vkb_device", libxl_defbool),
("soundhw", string),
("xen_platform_pci", libxl_defbool),
("usbdevice_list", libxl_string_list),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e6c54483e0..37bc13bd83 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2295,6 +2295,7 @@ skip_usbdev:
fprintf(stderr,"xl: Unable to parse usbdevice.\n");
exit(-ERROR_FAIL);
}
+xlu_cfg_get_defbool(config, "vkb_device", &b_info->u.hvm.vkb_device,
0);
xlu_cfg_replace_string (config, "soundhw", &b_info->u.hvm.soundhw, 0);
xlu_cfg_get_defbool(config, "xen_platform_pci",
&b_info->u.hvm.xen_platform_pci, 0);
diff --git a/tools/xl/xl_sxp.c b/tools/xl/xl_sxp.c
index 3e6117d0cd..359a001570 100644
--- a/to
[Xen-devel] [PATCH] evtchn: make support for different ABIs tunable
Adding support for FIFO event channel ABI was first introduced in Xen 4.4
(see 88910061ec6). Make this support tunable, since the choice of which
event channel ABI has implications for hibernation. Consider resuming a
pre Xen 4.4 hibernated Linux guest. The guest boot kernel defaults to FIFO
ABI, whereas the resume kernel assumes 2L. This, in turn, results in Xen
and the resumed kernel talking past each other (due to different protocols
FIFO vs 2L).
In order to announce to guests that the event channel ABI does not support
FIFO, the hypervisor returns ENOSYS on init_control operation. When this
operation fails, the guest should continue to use the 2L event channel ABI.
For example, in Linux drivers/xen/events/events_base.c:
if (fifo_events)
ret = xen_evtchn_fifo_init();
if (ret < 0)
xen_evtchn_2l_init();
and xen_evtchn_fifo_init fails when EVTCHNOP_init_control fails. This commit
does not change the current default behaviour: announce FIFO event channels
ABI support for guests unless explicitly stated otherwise at domaincreate.
Signed-off-by: Eslam Elnikety
---
docs/man/xl.cfg.5.pod.in| 5 +
tools/libxl/libxl.h | 8
tools/libxl/libxl_create.c | 5 +
tools/libxl/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 2 ++
tools/xl/xl_sxp.c | 2 ++
xen/common/domain.c | 3 +++
xen/common/event_channel.c | 5 +
xen/include/public/domctl.h | 3 +++
xen/include/xen/sched.h | 1 +
10 files changed, 35 insertions(+)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c99d40307e..f204d8b4f0 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1262,6 +1262,11 @@ FIFO-based event channel ABI support up to 131,071 event
channels.
Other guests are limited to 4095 (64-bit x86 and ARM) or 1023 (32-bit
x86).
+=item B
+
+Indicates if support for FIFO event channel ABI is disabled. The default
+is false (0).
+
=item B
Specifies the virtual display devices to be provided to the guest.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 9bacfb97f0..75b2ee3d1b 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -169,6 +169,14 @@
*/
#define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
+/*
+ * LIBXL_HAVE_BUILDINFO_DISABLE_EVTCHN_FIFO indicates that the
+ * libxl_domain_build_info structure contains a boolean
+ * disable_evtchn_fifo which instructs libxl to enable/disable
+ * support for FIFO event channel ABI at create time.
+ */
+#define LIBXL_HAVE_BUILDINFO_DISABLE_EVTCHN_FIFO 1
+
/*
* libxl_domain_build_info has the u.hvm.ms_vm_genid field.
*/
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 03ce166f4f..aa87e45643 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -217,6 +217,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (!b_info->event_channels)
b_info->event_channels = 1023;
+libxl_defbool_setdefault(&b_info->disable_evtchn_fifo, false);
+
libxl__arch_domain_build_info_setdefault(gc, b_info);
libxl_defbool_setdefault(&b_info->dm_restrict, false);
@@ -564,6 +566,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config
*d_config,
libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
}
+if (libxl_defbool_val(b_info->disable_evtchn_fifo))
+create.flags |= XEN_DOMCTL_CDF_disable_fifo;
+
/* Ultimately, handle is an array of 16 uint8_t, same as uuid */
libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b61399ce36..5f30570443 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -521,6 +521,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("iomem",Array(libxl_iomem_range, "num_iomem")),
("claim_mode", libxl_defbool),
("event_channels", uint32),
+("disable_evtchn_fifo", libxl_defbool),
("kernel", string),
("cmdline", string),
("ramdisk", string),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e105bda2bb..bcf16c31d4 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1496,6 +1496,8 @@ void parse_config_data(const char *config_source,
if (!xlu_cfg_get_long(config, "max_event_channels", &l, 0))
b_info->event_channels = l;
+xlu_cfg_get_defbool(config, "disable_evtchn_fifo",
+&b_info->disable_evtchn_fifo, 0);
xlu_cfg_replace_string (config, "kernel", &b_info->kernel, 0);
xlu_cfg_replace_string (config, "ramdisk", &b_info->ramdisk, 0);
xlu_cfg_replace_string (config, "device_tree", &b_info->device_
[Xen-devel] [PATCH v2] evtchn: make support for different ABIs tunable
Support for FIFO event channel ABI was first introduced in Xen 4.4
(see 88910061ec6). Make this support tunable, since the choice of which
event channel ABI has implications for hibernation. Consider resuming a
pre Xen 4.4 hibernated Linux guest. During resume from hibernation, there
are two kernels involved: the "boot" kernel and the "resume" kernel. The
guest boot kernel defaults to FIFO ABI and instructs Xen via an
EVTCHNOP_init_control to switch from 2L to FIFO. On the other hand, the
resume kernel keeps assuming 2L. This, in turn, results in Xen and the
resumed kernel talking past each other (due to different protocols FIFO
vs 2L). A knob to control the support level for event channel ABIs per
guest would allow an administrator to work around this issue. This patch
introduces this knob.
In order to announce to guests that the event channel ABI does not support
FIFO, the hypervisor returns EPERM on init_control operation. When this
operation fails, the guest should continue to use the 2L event channel ABI.
For example, in Linux drivers/xen/events/events_base.c:
if (fifo_events)
ret = xen_evtchn_fifo_init();
if (ret < 0)
xen_evtchn_2l_init();
and xen_evtchn_fifo_init fails when EVTCHNOP_init_control fails. This commit
does not change the current default behaviour: announce FIFO event channels
ABI support for guests unless explicitly stated otherwise at domaincreate.
Signed-off-by: Eslam Elnikety
---
Changes in v2:
- Reworded the commit message
- Return EPERM instead of ENOSYS when forcing 2L ABI
- Use d->options instead of replicating the flag
---
docs/man/xl.cfg.5.pod.in| 5 +
tools/libxl/libxl.h | 8
tools/libxl/libxl_create.c | 5 +
tools/libxl/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 2 ++
tools/xl/xl_sxp.c | 2 ++
xen/common/event_channel.c | 5 +
xen/include/public/domctl.h | 3 +++
8 files changed, 31 insertions(+)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c99d40307e..f204d8b4f0 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1262,6 +1262,11 @@ FIFO-based event channel ABI support up to 131,071 event
channels.
Other guests are limited to 4095 (64-bit x86 and ARM) or 1023 (32-bit
x86).
+=item B
+
+Indicates if support for FIFO event channel ABI is disabled. The default
+is false (0).
+
=item B
Specifies the virtual display devices to be provided to the guest.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 9bacfb97f0..9ee80d74a6 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -169,6 +169,14 @@
*/
#define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
+/*
+ * LIBXL_HAVE_BUILDINFO_DISABLE_EVTCHN_FIFO indicates that the
+ * libxl_domain_build_info structure contains a boolean
+ * disable_evtchn_fifo which instructs libxl to enable/disable
+ * support for FIFO event channel ABI at create time.
+ */
+#define LIBXL_HAVE_BUILDINFO_DISABLE_EVTCHN_FIFO 1
+
/*
* libxl_domain_build_info has the u.hvm.ms_vm_genid field.
*/
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 03ce166f4f..aa87e45643 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -217,6 +217,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (!b_info->event_channels)
b_info->event_channels = 1023;
+libxl_defbool_setdefault(&b_info->disable_evtchn_fifo, false);
+
libxl__arch_domain_build_info_setdefault(gc, b_info);
libxl_defbool_setdefault(&b_info->dm_restrict, false);
@@ -564,6 +566,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config
*d_config,
libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
}
+if (libxl_defbool_val(b_info->disable_evtchn_fifo))
+create.flags |= XEN_DOMCTL_CDF_disable_fifo;
+
/* Ultimately, handle is an array of 16 uint8_t, same as uuid */
libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b61399ce36..5f30570443 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -521,6 +521,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("iomem",Array(libxl_iomem_range, "num_iomem")),
("claim_mode", libxl_defbool),
("event_channels", uint32),
+("disable_evtchn_fifo", libxl_defbool),
("kernel", string),
("cmdline", string),
("ramdisk", string),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e105bda2bb..bcf16c31d4 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -1496,6 +1496,8 @@ void parse_config_data(const char *config_source,
if (!xlu_cfg_get_long(config, &quo
Re: [Xen-devel] [PATCH v2] evtchn: make support for different ABIs tunable
Hi Jan,
On 08.08.19 17:12, Jan Beulich wrote:
On 08.08.2019 16:31, Elnikety, Eslam wrote:
First of all - can you please try to get your reply quoting
improved, such that readers can actually tell your replies
from context? (I didn't check whether perhaps your mail was
a HTML one, and my plain text reading of it discarded the
markings. If so - please don't send HTML mail.)
Oopsy. It was HTML. I will be more diligent going forward :)
On 8. Aug 2019, at 15:27, Jan Beulich
mailto:jbeul...@suse.com>> wrote:
On 07.08.2019 19:42, Eslam Elnikety wrote:
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -1170,6 +1170,11 @@ long do_event_channel_op(int cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
case EVTCHNOP_init_control: {
struct evtchn_init_control init_control;
+
+/* Fail init_control for domains that must use 2l ABI */
+if ( current->domain->options & XEN_DOMCTL_CDF_disable_fifo )
+return -EPERM;
+
if ( copy_from_guest(&init_control, arg, 1) != 0 )
return -EFAULT;
rc = evtchn_fifo_init_control(&init_control);
I think the check would better go into evtchn_fifo_init_control(),
at least as long as the setting really is FIFO-centric. Also the
Not sure. It is the FIFO ABI that defines EVTCHNOP_init_control
(not defined in 2L). Short-circuiting the hypercall at this place
seems more appropriate.
I'd expect any 3rd variant to also have a need for an init-control
operation, and hence at that point this would become a hook like
many of the other ops. And at that point the check would need to
move anyway. Hence it's better to put it in its long term
designated place right away.
Moreover, doing copy_from_guest and calling into
evtchn_fifo_init_control only to return error is not optimal.
True, yet from my pov the more logical alternative code structure
is still preferable.
Fair point. On a second thought, the additional cycles are not a problem
(given that init_control is not on a critical __must be performant__
path). I am now also in favor of moving the check to
event_fifo_init_control.
Irrespective of these remarks, and along the lines of comments on
the v1 thread, introducing wider control that would also allow
disabling 2-level (for HVM guests) would seem better to me. This
would then hopefully be coded up in a way that would make extending
it straightforward, once a 3rd mechanism appears.
Hmmm... we cannot force guests to call init_control (in order to flip from 2L
to FIFO). Quoting from [1] 4.4 “If this call (EVTCHNOP_init_control) fails on
the boot VCPU, the guest should continue to use the 2-level event channel ABI
for all VCPUs.” Support for 2L ABI does not sound like something that can be
optional. Once a 3rd mechanism appears, I imagine adding a corresponding domctl
flag to control such mechanism.
For HVM, event channels as a whole should be optional; we shouldn't
default a possible control for this to off though. For PV, the
2-level interface indeed has to be considered mandatory. Hence
today there are these (theoretically) possible combinations
PV HVM
nothing invalid valid
2-level onlyvalid valid
The patch at hand here is concerned only with the above line. (In fact,
v3 will rename the subject to: "evtchn: introduce a knob to on/off FIFO
ABI per guest".)
I take it that the concern here is that whatever changes the patch
proposes should play nicely with potential future changes introducing
such a generic framework. Any concrete suggestions?
Thanks,
Eslam
FIFO only ??? valid
bothvalid valid
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3] evtchn: Introduce a per-guest knob to control FIFO ABI
Support for FIFO event channel ABI was first introduced in Xen 4.4
(see 88910061ec6). Make this support tunable, since the choice of which
event channel ABI has implications for hibernation. Consider resuming a
pre Xen 4.4 hibernated Linux guest. During resume from hibernation, there
are two kernels involved: the "boot" kernel and the "resume" kernel. The
guest boot kernel may default to FIFO ABI and instruct Xen via an
EVTCHNOP_init_control to switch from 2L to FIFO. On the other hand, the
resume kernel keeps assuming 2L. This, in turn, results in Xen and the
resumed kernel talking past each other (due to different protocols FIFO
vs 2L). A knob to control the support level for event channel ABIs per
guest would allow an administrator to work around this issue. This patch
introduces this knob.
In order to announce to guests that the event channel ABI does not support
FIFO, the hypervisor returns EPERM on init_control operation. When this
operation fails, the guest should continue to use the 2L event channel ABI.
For example, in Linux drivers/xen/events/events_base.c:
if (fifo_events)
ret = xen_evtchn_fifo_init();
if (ret < 0)
xen_evtchn_2l_init();
and xen_evtchn_fifo_init fails when EVTCHNOP_init_control fails. This commit
does not change the current default behaviour: announce FIFO event channels
ABI support for guests unless explicitly stated otherwise at domaincreate.
Signed-off-by: Eslam Elnikety
---
Changes in v2:
- Reworded the commit message
- Return EPERM instead of ENOSYS when forcing 2L ABI
- Use d->options instead of replicating the flag
Changes in v3:
- Reworded the commit subject
- Check at evtchn_fifo_init_control, rather than EVTCHNOP_init_control
---
docs/man/xl.cfg.5.pod.in| 5 +
tools/libxl/libxl.h | 8
tools/libxl/libxl_create.c | 5 +
tools/libxl/libxl_types.idl | 1 +
tools/xl/xl_parse.c | 2 ++
tools/xl/xl_sxp.c | 2 ++
xen/common/event_channel.c | 1 +
xen/common/event_fifo.c | 4
xen/include/public/domctl.h | 3 +++
9 files changed, 31 insertions(+)
diff --git a/docs/man/xl.cfg.5.pod.in b/docs/man/xl.cfg.5.pod.in
index c99d40307e..f204d8b4f0 100644
--- a/docs/man/xl.cfg.5.pod.in
+++ b/docs/man/xl.cfg.5.pod.in
@@ -1262,6 +1262,11 @@ FIFO-based event channel ABI support up to 131,071 event
channels.
Other guests are limited to 4095 (64-bit x86 and ARM) or 1023 (32-bit
x86).
+=item B
+
+Indicates if support for FIFO event channel ABI is disabled. The default
+is false (0).
+
=item B
Specifies the virtual display devices to be provided to the guest.
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 9bacfb97f0..9ee80d74a6 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -169,6 +169,14 @@
*/
#define LIBXL_HAVE_BUILDINFO_EVENT_CHANNELS 1
+/*
+ * LIBXL_HAVE_BUILDINFO_DISABLE_EVTCHN_FIFO indicates that the
+ * libxl_domain_build_info structure contains a boolean
+ * disable_evtchn_fifo which instructs libxl to enable/disable
+ * support for FIFO event channel ABI at create time.
+ */
+#define LIBXL_HAVE_BUILDINFO_DISABLE_EVTCHN_FIFO 1
+
/*
* libxl_domain_build_info has the u.hvm.ms_vm_genid field.
*/
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 03ce166f4f..aa87e45643 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -217,6 +217,8 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
if (!b_info->event_channels)
b_info->event_channels = 1023;
+libxl_defbool_setdefault(&b_info->disable_evtchn_fifo, false);
+
libxl__arch_domain_build_info_setdefault(gc, b_info);
libxl_defbool_setdefault(&b_info->dm_restrict, false);
@@ -564,6 +566,9 @@ int libxl__domain_make(libxl__gc *gc, libxl_domain_config
*d_config,
libxl_defbool_val(info->oos) ? 0 : XEN_DOMCTL_CDF_oos_off;
}
+if (libxl_defbool_val(b_info->disable_evtchn_fifo))
+create.flags |= XEN_DOMCTL_CDF_disable_fifo;
+
/* Ultimately, handle is an array of 16 uint8_t, same as uuid */
libxl_uuid_copy(ctx, (libxl_uuid *)&create.handle, &info->uuid);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index b61399ce36..5f30570443 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -521,6 +521,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("iomem",Array(libxl_iomem_range, "num_iomem")),
("claim_mode", libxl_defbool),
("event_channels", uint32),
+("disable_evtchn_fifo", libxl_defbool),
("kernel", string),
("cmdline", string),
("ramdisk", string),
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index e105bda2
Re: [Xen-devel] [PATCH] evtchn: make support for different ABIs tunable
On 14.08.19 15:02, Andrew Cooper wrote: On 14/08/2019 13:51, George Dunlap wrote: On 8/7/19 5:03 PM, Jan Beulich wrote: Whatever we do in Xen, it'll only allow to work around that issue. An actual fix belongs in the kernel(s). For this reason I suppose what we're talking about here is a feature (from Xen's pov), not a bug fix. And it being a feature, it should preferably be coded in a way that's usable also going forward. FWIW, I agree with what I understand Jan to be saying: 1. It would be good to be able to disable FIFO event channels, but 2. Disabling them in Xen isn't really the right way to fix Amazon's issue. Rather, probably the soft reboot should reset the event channel state. Depends what you mean about "fix the issue". Amazon have real customer VMs in this situation which will break on a Xen old => new upgrade. Modifying Xen is the only rational option. They are also doing this in an upstream compatible manner (which is great). One way or another, Xen needs to gain this ability to work around broken-linux which is already in the field. As for the ideal way to fix this, this bug has existed in Linux longer than soft-reset has been a thing, and frankly, its still a gruesome hack. We need some interfaces which don't suck so terribly. ~Andrew Thanks, Andrew. I second those points. I have just sent v3 of this patch, mostly addressing comments from Jan. Have a look, and let me know if there are further tweaks you would rather see. Thanks. Cheers, Eslam ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
Xen relies on boot modules to perform early microcode updates. This commit adds
another mode, namely "builtin" via the BUILTIN_UCODE config parameter. If set,
the Xen image itself will contain the microcode updates. Upon boot, Xen
inspects its image for microcode blobs and performs the update.
A Xen image with builtin microcode can be explicitly instructed to:
(a) look for microcode elsewhere (e.g., a boot module that contains more
recent microcodes via ucode=scan), or
(b) skip the builtin microcode update (e.g., ucode=no-builtin).
Signed-off-by: Eslam Elnikety
---
docs/misc/builtin-ucode.txt | 60 +++
docs/misc/xen-command-line.pandoc | 5 ++-
xen/arch/x86/Kconfig | 20 +++
xen/arch/x86/Makefile | 1 +
xen/arch/x86/microcode.c | 60 +--
xen/arch/x86/microcode/Makefile | 40 +
xen/arch/x86/xen.lds.S| 12 +++
7 files changed, 194 insertions(+), 4 deletions(-)
create mode 100644 docs/misc/builtin-ucode.txt
create mode 100644 xen/arch/x86/microcode/Makefile
diff --git a/docs/misc/builtin-ucode.txt b/docs/misc/builtin-ucode.txt
new file mode 100644
index 00..43bb60d3eb
--- /dev/null
+++ b/docs/misc/builtin-ucode.txt
@@ -0,0 +1,60 @@
+-
+Builtin Microcode Support for x86 (AMD and INTEL)
+-
+Author:
+ Eslam Elnikety
+Initial version:
+ Dec 2019
+-
+
+About:
+--
+* This documentation describes preparing the builtin microcode blobs to use as
+ builtin microcode update within the Xen image itself.
+
+* Support for builtin microcode is limited to x86.
+
+* Builtin support is available via the configurations BUILTIN_UCODE and
+ BUILTIN_UCODE_DIR. The first enables the support (default is off), and the
+ latter directs the build system to where it can find the microcode directory
+ (default is /lib/firmware).
+
+Microcode Directory:
+
+This directory holds the microcode blobs to be built in the Xen image. There
+are two subdirectories: amd-ucode and intel-ucode for AMD and INTEL,
+respectively.
+
+INTEL microcode blobs typically follow the naming format FF-MM-SS for
+{F}amily-{M}odel-{S}tepping. Alternatively, GenuineIntel.bin bundles a bunch
+of FF-MM-SS blobs into a single binary and the one matching the host CPU gets
+picked when performing the microcode update. For AMD, the canonical name is
+AuthenticAMD.bin. Similarly, such binary can bundle a bunch of microcode blobs
+for different families.
+
+The builtin microcode is generated by concatenating the microcode blobs under
+intel-ucode into GenuineIntel.bin, and those under amd-ucode into
+AuthenticAMD.bin. Those are then copied into the Xen image itself.
+
+Here is an example microcode directory structure, following the convention [1]:
+
+/lib/firmware
+|-- amd-ucode
+...
+| |-- microcode_amd_fam15h.bin
+...
+|-- intel-ucode
+...
+| |-- 06-3a-09
+...
+
+Alternatively, the subdirectories can directly contain GenuineIntel.bin and
+AuthenticAMD.bin (since both are concatenation of the individual microcode
+blobs and the end result is the same).
+
+An empty or non-existant subdirectory (amd-ucode and/or intel-ucode) excludes
+the respective AMD or INTEL microcode from being built in.
+
+Reference(s):
+-
+[1] https://www.kernel.org/doc/Documentation/x86/microcode.txt
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index 891d2d439f..ba25db95da 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2113,7 +2113,7 @@ logic applies:
active by default.
### ucode (x86)
-> `= List of [ | scan=, nmi= ]`
+> `= List of [ | scan= | builtin=, nmi= ]`
Specify how and where to find CPU microcode update blob.
@@ -2128,6 +2128,9 @@ when used with xen.efi (there the concept of modules
doesn't exist, and
the blob gets specified via the `ucode=` config file/section
entry; see [EFI configuration file description](efi.html)).
+'builtin' instructs the hypervisor to use the builtin microcode update. This
+option is available only if option BUILTIN_UCODE is enabled.
+
'scan' instructs the hypervisor to scan the multiboot images for an cpio
image that contains microcode. Depending on the platform the blob with the
microcode in the cpio name space must be:
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 02bb05f42e..14c5992d86 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -218,6 +218,26 @@ config MEM_SHARING
bool "Xen memory sharing support" if EXPERT = "y"
depends on HVM
+config BUILTIN_UCODE
+ def_bool n
+ prompt "Support for Builtin Microcode"
+ ---help---
+ Include the CPU microcode update i
Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
On 09.12.19 16:19, Andrew Cooper wrote:
On 09/12/2019 08:41, Eslam Elnikety wrote:
diff --git a/docs/misc/builtin-ucode.txt b/docs/misc/builtin-ucode.txt
new file mode 100644
index 00..43bb60d3eb
Instead of introducing a new file, please extend
docs/admin-guide/microcode-loading.rst
I have an in-prep docs/hypervisor-guide/microcode-loading.rst as well,
which I'll see about posting. It would be a more appropriate place for
the technical details.
Agreed!
While the existing docs/admin-guide/microcode-loading.rst speaks a
different tone than what I added, that documentation anyway needs to be
updated with builtin if such support were to be added. I will adjust
accordingly. If docs/hypervisor-guide/microcode-loading.rst makes it in
time for v2 of this patch, I will reflect changes there too.
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 6ced293d88..7afbe44286 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -97,6 +97,14 @@ static struct ucode_mod_blob __initdata ucode_blob;
*/
static bool_t __initdata ucode_scan;
+#ifdef CONFIG_BUILTIN_UCODE
+/* builtin is the default when BUILTIN_UCODE is set */
+static bool_t __initdata ucode_builtin = 1;
bool, true.
Will fix in v2.
+
+extern const char __builtin_intel_ucode_start[], __builtin_intel_ucode_end[];
+extern const char __builtin_amd_ucode_start[], __builtin_amd_ucode_end[];
+#endif
+
/* By default, ucode loading is done in NMI handler */
static bool ucode_in_nmi = true;
@@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int idx)
}
/*
- * The format is '[|scan=, nmi=]'. Both options are
- * optional. If the EFI has forced which of the multiboot payloads is to be
- * used, only nmi= is parsed.
+ * The format is '[|scan=|builtin=, nmi=]'. All
+ * options are optional. If the EFI has forced which of the multiboot payloads
+ * is to be used, only nmi= is parsed.
*/
Please delete this, or I'll do a prereq patch to fix it and the command
line docs. (Both are in a poor state.)
Unless you are planning that along your on-going
docs/hypervisor-guide/microcode-loading.rst effort, I can pick up this
clean-up/prereq patch myself. What do you have in mind? (Or point me to
a good example and I will figure things out).
@@ -237,6 +249,48 @@ void __init microcode_grab_module(
scan:
if ( ucode_scan )
microcode_scan_module(module_map, mbi);
+
+#ifdef CONFIG_BUILTIN_UCODE
+/*
+ * Do not use the builtin microcode if:
+ * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin)
+ * (b) a microcode module has been specified or a scan is successful
+ */
+if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size )
+return;
+
+/* Set ucode_start/_end to the proper blob */
+if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+ucode_blob.size = (size_t)(__builtin_amd_ucode_end
+ - __builtin_amd_ucode_start);
No need to cast the result. Also, preferred Xen style would be to have
- on the preceding line.
Will fix in v2.
+else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+ucode_blob.size = (size_t)(__builtin_intel_ucode_end
+ - __builtin_intel_ucode_start);
+else
+return;
+
+if ( !ucode_blob.size )
+{
+printk("No builtin ucode! 'ucode=builtin' is nullified.\n");
+return;
+}
+else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE )
+{
+printk("Builtin microcode payload too big! (%ld, we can do %d)\n",
+ ucode_blob.size, MAX_EARLY_CPIO_MICROCODE);
+ucode_blob.size = 0;
+return;
+}
+
+ucode_blob.data = xmalloc_bytes(ucode_blob.size);
+if ( !ucode_blob.data )
+return;
Any chance we can reuse the "fits" logic to avoid holding every
inapplicable blob in memory as well?
I think this would be a welcomed change. It seems to me that we have two
ways to go about it.
1) We factor the code in the intel-/amd-specific cpu_request_microcode
to extract logic for finding a match into its own new function, expose
that through microcode_ops, and finally do xalloc only for the matching
microcode when early loading is scan or builtin.
2) Cannot we just do away completely with xalloc? I see that each
individual microcode update gets allocated anyway in
microcode_intel.c/get_next_ucode_from_buffer() and in
microcode_amd.c/cpu_request_microcode(). Unless I am missing something,
the xmalloc_bytes for ucode_blob.data is redundant.
Thoughts?
+
+if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+memcpy(ucode_blob.data, __builtin_amd_ucode_start, ucode_blob.size);
+else
+memcpy(ucode_blob.data, __builtin_intel_ucode_start, ucode_blob.size);
+#endif
}
const struct microcode_ops *microcode_ops;
diff
Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
On 10.12.19 10:21, Jan Beulich wrote:
On 09.12.2019 22:49, Eslam Elnikety wrote:
On 09.12.19 16:19, Andrew Cooper wrote:
On 09/12/2019 08:41, Eslam Elnikety wrote:
--- /dev/null
+++ b/xen/arch/x86/microcode/Makefile
@@ -0,0 +1,40 @@
+# Copyright (C) 2019 Amazon.com, Inc. or its affiliates.
+# Author: Eslam Elnikety
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+obj-y += builtin_ucode.o
+
+# Directory holding the microcode updates.
+UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR))
+amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*)
+intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*)
This is a little dangerous. I can see why you want to do it like this,
and I can't provide any obvious suggestions, but if this glob picks up
anything which isn't a microcode file, it will break the logic to search
for the right blob.
We can limit the amd-blobs and intel-blob to binaries following the
naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and
intel, respectively. Yet, this would be imposing an unnecessary
restriction on administrators who may want to be innovative with naming
(or want to use the naming microcode_amd_*.bin or FF-MM-SS instead).
Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and
CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an
administrator can specify exactly the microcodes to include relative to
the CONFIG_BUILTIN_UCODE_DIR. For example:
CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09"
CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
This would make the feature even less generic - I already meant to
I do not follow the point about being less generic. (I hope my example
did not give the false impression that CONFIG_BUILTIN_UCODE_{AMD,INTEL}
allow for only a single microcode blob for a single signature).
ask whether building ucode into binaries is really a useful thing
when we already have more flexible ways. I could see this being
useful if there was no other way to make ucode available at boot
time.
It is useful in addition to the existing ways to do early microcode
updates. First, when operating many hosts, using boot modules (either a
distinct microcode module or within an initrd) becomes involved. For
instance, tools to update boot entries (e.g.,
https://linux.die.net/man/8/grubby) do not support adding arbitrary
(microcode) modules.
Second, there is often need to couple a Xen build with a minimum
microcode patch level. Having the microcode built within the Xen image
itself is a streamlined, natural way of achieving that.
Thanks,
Eslam
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
On 10.12.19 10:37, Jan Beulich wrote:
On 09.12.2019 09:41, Eslam Elnikety wrote:
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2113,7 +2113,7 @@ logic applies:
active by default.
### ucode (x86)
-> `= List of [ | scan=, nmi= ]`
+> `= List of [ | scan= | builtin=, nmi= ]`
Despite my other question regarding the usefulness of this as a
whole a few comments.
Do "scan" and "builtin" really need to exclude each other? I.e.
don't you mean , instead of | ?
The useful case here would be specifying ucode=scan,builtin which would
translate to fallback onto the builtin microcode if a scan fails. In
fact, this is already the case given the implementation in v1. It will
be better to clarify this semantic by allowing scan,builtin.
On that note, I really see the "" and "scan=" to be
equal. Following the logic earlier we should probably also allow
ucode=,builtin. This translates to fallback to builtin if there
are no modules at index .
@@ -2128,6 +2128,9 @@ when used with xen.efi (there the concept of modules
doesn't exist, and
the blob gets specified via the `ucode=` config file/section
entry; see [EFI configuration file description](efi.html)).
+'builtin' instructs the hypervisor to use the builtin microcode update. This
+option is available only if option BUILTIN_UCODE is enabled.
You also want to clarify its default - your reply to Andrew
suggested to me that only the negative form would really be
useful.
Indeed. This in any case will need a revamp to rework the ", instead of
|". Will address in v2.
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -218,6 +218,26 @@ config MEM_SHARING
bool "Xen memory sharing support" if EXPERT = "y"
depends on HVM
+config BUILTIN_UCODE
+ def_bool n
+ prompt "Support for Builtin Microcode"
These two lines should be folded into just
bool "Support for Builtin Microcode"
irrespective of other bad examples you may find in the code base.
The again ...
Will address in v2.
+ ---help---
+ Include the CPU microcode update in the Xen image itself. With this
+ support, Xen can update the CPU microcode upon boot using the builtin
+ microcode, with no need for an additional microcode boot modules.
+
+ If unsure, say N.
+
+config BUILTIN_UCODE_DIR
+ string
+ default "/lib/firmware"
+ depends on BUILTIN_UCODE
... are two separate options needed at all? Can't this latter one
being the empty string just imply the feature to be disabled?
I can go either way here. To me, two options is clearer.
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -7,6 +7,7 @@ subdir-y += mm
subdir-$(CONFIG_XENOPROF) += oprofile
subdir-$(CONFIG_PV) += pv
subdir-y += x86_64
+subdir-$(CONFIG_BUILTIN_UCODE) += microcode
Please respect the (half way?) alphabetical sorting here, unless
adding last is a requirement (in which case a brief comment should
say so, and why).
@@ -130,6 +138,10 @@ static int __init parse_ucode(const char *s)
{
if ( (val = parse_boolean("scan", s, ss)) >= 0 )
ucode_scan = val;
+#ifdef CONFIG_BUILTIN_UCODE
+ else if ( (val = parse_boolean("builtin", s, ss)) >= 0 )
Please watch out for hard tabs where they don't belong.
Good catch! Will fix in v2.
@@ -237,6 +249,48 @@ void __init microcode_grab_module(
scan:
if ( ucode_scan )
microcode_scan_module(module_map, mbi);
+
+#ifdef CONFIG_BUILTIN_UCODE
+/*
+ * Do not use the builtin microcode if:
+ * (a) builtin has been explicitly turned off (e.g., ucode=no-builtin)
+ * (b) a microcode module has been specified or a scan is successful
+ */
+if ( !ucode_builtin || ucode_mod.mod_end || ucode_blob.size )
+return;
+
+/* Set ucode_start/_end to the proper blob */
+if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
+ucode_blob.size = (size_t)(__builtin_amd_ucode_end
+ - __builtin_amd_ucode_start);
+else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+ucode_blob.size = (size_t)(__builtin_intel_ucode_end
+ - __builtin_intel_ucode_start);
+else
+return;
+
+if ( !ucode_blob.size )
+{
+printk("No builtin ucode! 'ucode=builtin' is nullified.\n");
+return;
+}
+else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE )
With the "return" above please omit the "else" here. But why this
restriction, and ...
Will adjust in v2.
+{
+printk("Builtin microcode payload too big! (%ld, we can do %d)\n",
+ ucode_blob.size, MAX_EARLY_CPIO_MICROCODE);
+ucode
Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
On 11.12.19 10:47, Jan Beulich wrote:
On 10.12.2019 23:40, Eslam Elnikety wrote:
On 10.12.19 10:21, Jan Beulich wrote:
On 09.12.2019 22:49, Eslam Elnikety wrote:
On 09.12.19 16:19, Andrew Cooper wrote:
On 09/12/2019 08:41, Eslam Elnikety wrote:
--- /dev/null
+++ b/xen/arch/x86/microcode/Makefile
@@ -0,0 +1,40 @@
+# Copyright (C) 2019 Amazon.com, Inc. or its affiliates.
+# Author: Eslam Elnikety
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+obj-y += builtin_ucode.o
+
+# Directory holding the microcode updates.
+UCODE_DIR=$(patsubst "%",%,$(CONFIG_BUILTIN_UCODE_DIR))
+amd-blobs := $(wildcard $(UCODE_DIR)/amd-ucode/*)
+intel-blobs := $(wildcard $(UCODE_DIR)/intel-ucode/*)
This is a little dangerous. I can see why you want to do it like this,
and I can't provide any obvious suggestions, but if this glob picks up
anything which isn't a microcode file, it will break the logic to search
for the right blob.
We can limit the amd-blobs and intel-blob to binaries following the
naming convention AuthenticAMD.bin and GenuineIntel.bin for amd and
intel, respectively. Yet, this would be imposing an unnecessary
restriction on administrators who may want to be innovative with naming
(or want to use the naming microcode_amd_*.bin or FF-MM-SS instead).
Alternatively, we can introduce CONFIG_BUILTIN_UCODE_INTEL and
CONFIG_BUILTIN_UCODE_AMD. Both default to empty strings. Then, an
administrator can specify exactly the microcodes to include relative to
the CONFIG_BUILTIN_UCODE_DIR. For example:
CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09"
CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
This would make the feature even less generic - I already meant to
I do not follow the point about being less generic. (I hope my example
did not give the false impression that CONFIG_BUILTIN_UCODE_{AMD,INTEL}
allow for only a single microcode blob for a single signature).
Well, the example indeed has given this impression to me. I'm
having a hard time seeing how, beyond very narrow special cases,
either of the examples could be useful to anyone. Yet I think
examples should be generally useful.
Andrew's earlier response hinted at "what can we do to avoid picking
random bits in the builtin microcode?" My response was outlining
alternatives, and the examples there were not meant to be useful beyond
explaining those alternatives.
ask whether building ucode into binaries is really a useful thing
when we already have more flexible ways. I could see this being
useful if there was no other way to make ucode available at boot
time.
It is useful in addition to the existing ways to do early microcode
updates. First, when operating many hosts, using boot modules (either a
distinct microcode module or within an initrd) becomes involved. For
instance, tools to update boot entries (e.g.,
https://linux.die.net/man/8/grubby) do not support adding arbitrary
(microcode) modules.
I.e. you suggest to work around tools shortcomings by extending
Xen? Wouldn't the more appropriate way to deal with this be to
make the tools more capable?
Second, there is often need to couple a Xen build with a minimum
microcode patch level. Having the microcode built within the Xen image
itself is a streamlined, natural way of achieving that.
Okay, I can accept this as a reason, to some degree at least. Yet
as said elsewhere, I don't think you want then to override a
possible "external" ucode module with the builtin blobs. Instead
the newest of everything that's available should then be loaded.
Extending Xen to work around tools shortcomings is absolutely not what I
have in mind. I should have started with the second reason. Read this
as: Xen relies on a minimum microcode feature set, and it makes sense to
couple both in one binary. This coupling just happens to provide an
added benefit in the face of tools shortcoming.
Thanks,
Eslam
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
On 11.12.19 10:54, Jan Beulich wrote: On 11.12.2019 00:18, Eslam Elnikety wrote: On 10.12.19 10:37, Jan Beulich wrote: On 09.12.2019 09:41, Eslam Elnikety wrote: --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2113,7 +2113,7 @@ logic applies: active by default. ### ucode (x86) -> `= List of [ | scan=, nmi= ]` +> `= List of [ | scan= | builtin=, nmi= ]` Despite my other question regarding the usefulness of this as a whole a few comments. Do "scan" and "builtin" really need to exclude each other? I.e. don't you mean , instead of | ? The useful case here would be specifying ucode=scan,builtin which would translate to fallback onto the builtin microcode if a scan fails. In fact, this is already the case given the implementation in v1. It will be better to clarify this semantic by allowing scan,builtin. On that note, I really see the "" and "scan=" to be equal. Following the logic earlier we should probably also allow ucode=,builtin. This translates to fallback to builtin if there are no modules at index . Almost - if the builtin one is newer than the separate blob, then either of the cmdline options you name should still cause the builtin one to be loaded. IOW you want to honor both options, not prefer the earlier over a later one. On the "newest of everything": That's not what I intend to propose. The microcode provided via a scan (or for that matter) will always override the builtin microcode. The common case would be that the microcode provided via a scan (or ) is newer than the builtin. Yet, an administrator will have the option, if needed, to use an older microcode via a scan (or ) to override a newer builtin microcode. -- Eslam ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
On 13.12.19 14:57, Andrew Cooper wrote: On 12/12/2019 22:13, Eslam Elnikety wrote: Second, there is often need to couple a Xen build with a minimum microcode patch level. Having the microcode built within the Xen image itself is a streamlined, natural way of achieving that. Okay, I can accept this as a reason, to some degree at least. Yet as said elsewhere, I don't think you want then to override a possible "external" ucode module with the builtin blobs. Instead the newest of everything that's available should then be loaded. Extending Xen to work around tools shortcomings is absolutely not what I have in mind. I should have started with the second reason. Read this as: Xen relies on a minimum microcode feature set, and it makes sense to couple both in one binary. This coupling just happens to provide an added benefit in the face of tools shortcoming. Do we have anything which strictly relies on a minimum version? I had in mind microcode speculation mitigation features when reasoning with the minimum patch level argument. -- Eslam ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/microcode: Support builtin CPU microcode
On 13.12.19 14:40, Andrew Cooper wrote:
On 09/12/2019 21:49, Eslam Elnikety wrote:
+
+extern const char __builtin_intel_ucode_start[],
__builtin_intel_ucode_end[];
+extern const char __builtin_amd_ucode_start[],
__builtin_amd_ucode_end[];
+#endif
+
/* By default, ucode loading is done in NMI handler */
static bool ucode_in_nmi = true;
@@ -110,9 +118,9 @@ void __init microcode_set_module(unsigned int
idx)
}
/*
- * The format is '[|scan=, nmi=]'. Both
options are
- * optional. If the EFI has forced which of the multiboot payloads
is to be
- * used, only nmi= is parsed.
+ * The format is '[|scan=|builtin=,
nmi=]'. All
+ * options are optional. If the EFI has forced which of the
multiboot payloads
+ * is to be used, only nmi= is parsed.
*/
Please delete this, or I'll do a prereq patch to fix it and the command
line docs. (Both are in a poor state.)
Unless you are planning that along your on-going
docs/hypervisor-guide/microcode-loading.rst effort, I can pick up this
clean-up/prereq patch myself. What do you have in mind? (Or point me
to a good example and I will figure things out).
c/s 3c5552954, 53a84f672, 633a40947 or 3136dee9c are good examples.
ucode= is definitely more complicated to explain because of its implicit
EFI behaviour.
Currently massaging a patch to that effect.
+ else if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL )
+ ucode_blob.size = (size_t)(__builtin_intel_ucode_end
+ - __builtin_intel_ucode_start);
+ else
+ return;
+
+ if ( !ucode_blob.size )
+ {
+ printk("No builtin ucode! 'ucode=builtin' is nullified.\n");
+ return;
+ }
+ else if ( ucode_blob.size > MAX_EARLY_CPIO_MICROCODE )
+ {
+ printk("Builtin microcode payload too big! (%ld, we can do
%d)\n",
+ ucode_blob.size, MAX_EARLY_CPIO_MICROCODE);
+ ucode_blob.size = 0;
+ return;
+ }
+
+ ucode_blob.data = xmalloc_bytes(ucode_blob.size);
+ if ( !ucode_blob.data )
+ return;
Any chance we can reuse the "fits" logic to avoid holding every
inapplicable blob in memory as well?
I think this would be a welcomed change. It seems to me that we have
two ways to go about it.
1) We factor the code in the intel-/amd-specific cpu_request_microcode
to extract logic for finding a match into its own new function, expose
that through microcode_ops, and finally do xalloc only for the
matching microcode when early loading is scan or builtin.
2) Cannot we just do away completely with xalloc? I see that each
individual microcode update gets allocated anyway in
microcode_intel.c/get_next_ucode_from_buffer() and in
microcode_amd.c/cpu_request_microcode(). Unless I am missing
something, the xmalloc_bytes for ucode_blob.data is redundant.
Thoughts?
I'm certain the code is more complicated than it needs to be.
Cleanup/simplification would be very welcome. And if you're up for
that, there is a related area which would be a great improvement.
At the moment, BSP microcode loading is very late because it depends on
this xmalloc() to begin with. However, no memory allocation is needed
to load microcode from a multiboot module or from the initrd, or from
this future builtin location - all loading can be done from a
directmap/bootmap pointer if needs be.
This would allow moving the BSP microcode to much earlier on boot,
probably somewhere between console setup and E820 handling.
One way or another, the microcode cache which persists past boot has to
be xmalloc()'d, because we will free the module/initrd/builtin. It
would however be more friendly to AP's to only give them the single
correct piece of ucode, rather than everything to scan through.
(These behaviours and expectations are going to be a chunk of my
intended second microcode.rst doc, including a "be aware that machines
exist which do $X" section to cover some of the weirder corner cases we
have encountered.)
Avoiding the xmalloc/memcpy on the scan for microcode is one of the
patches that I will share shortly. In particular, the ucode_blob.data
would directly point to the buffer matching the canonical name within
the cpio name space.
We are still a bit away from pushing the BSP microcode update earlier
though. We will need to surgically remove all the unnecessary
xmalloc/memcpy from within microcode_{amd,intel}.c. Also, as you hinted,
the challenging bit is the per-cpu microcode cache.
+
+builtin_ucode.o: Makefile $(amd-blobs) $(intel-blobs)
+ # Create AMD microcode blob if there are AMD updates on the
build system
+ if [ ! -z "$(amd-blobs)" ]; then \
+ cat $(amd-blobs) > $@.bin ; \
+ $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64
--rename-section
.data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin
$@.amd; \
+ rm -f $@.bin; \
+ fi
+ # Create INTEL micro
[Xen-devel] [PATCH v2 3/4] x86/microcode: use const qualifier for microcode buffer
The buffer holding the microcode bits should be marked as const.
Signed-off-by: Eslam Elnikety
---
xen/arch/x86/microcode.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index c878fc71ff..4616fa9d2e 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -86,7 +86,7 @@ static enum {
* memory.
*/
struct ucode_mod_blob {
-void *data;
+const void *data;
size_t size;
};
@@ -744,7 +744,7 @@ int microcode_update_one(bool start_update)
int __init early_microcode_update_cpu(void)
{
int rc = 0;
-void *data = NULL;
+const void *data = NULL;
size_t len;
struct microcode_patch *patch;
--
2.17.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 0/4] x86/microcode: Support builtin CPU microcode
The main goal of this patch series is to add support for builtin microcode. Towards that end, the series starts with a few improvements for the documentation and parsing of the ucode= Xen command line parameter that controls early loading of microcode (Patches 1--3), and follows with the main builtin suppot (Patch 4). Changes in v2: - An earlier version of Patch 4 was submitted in isolation. Refer to the patch itself for details regarding the relevant changes. - Patches 1--3 are additions. Eslam Elnikety (4): x86/microcode: Improve documentation and parsing for ucode= x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data x86/microcode: use const qualifier for microcode buffer x86/microcode: Support builtin CPU microcode docs/admin-guide/microcode-loading.rst | 31 ++ docs/misc/xen-command-line.pandoc | 26 +++-- xen/arch/x86/Kconfig | 30 ++ xen/arch/x86/Makefile | 1 + xen/arch/x86/microcode.c | 139 ++--- xen/arch/x86/microcode/Makefile| 46 xen/arch/x86/xen.lds.S | 12 +++ 7 files changed, 221 insertions(+), 64 deletions(-) create mode 100644 xen/arch/x86/microcode/Makefile -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=
Decouple the microcode referencing mechanism when using GRUB to that
when using EFI. This allows us to avoid the "unspecified effect" of
using ` | scan` along xen.efi. With that, Xen can explicitly
ignore those named options when using EFI. As an added benefit,
we get a straightfoward parsing of the ucode parameter. While at it,
simplify the logic in microcode_grab_module().
Update the command line documentation for consistency. Also, drop the
leading comment for parse_ucode_param. (No practical use for it given
this commit).
Signed-off-by: Eslam Elnikety
---
docs/misc/xen-command-line.pandoc | 18 ---
xen/arch/x86/microcode.c | 51 ++-
2 files changed, 36 insertions(+), 33 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index 7a1be84ca9..40faf3bc3a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2128,7 +2128,13 @@ logic applies:
### ucode (x86)
> `= List of [ | scan=, nmi= ]`
-Specify how and where to find CPU microcode update blob.
+Applicability: x86
+Default: `nmi`
+
+Controls for CPU microcode loading. For early loading, this parameter can
+specify how and where to find the microcode update blob. For late loading,
+this parameter specifies if the update happens within a NMI handler or in
+a stop_machine context.
'integer' specifies the CPU microcode update blob module index. When positive,
this specifies the n-th module (in the GrUB entry, zero based) to be used
@@ -2136,10 +2142,7 @@ for updating CPU micrcode. When negative, counting
starts at the end of
the modules in the GrUB entry (so with the blob commonly being last,
one could specify `ucode=-1`). Note that the value of zero is not valid
here (entry zero, i.e. the first module, is always the Dom0 kernel
-image). Note further that use of this option has an unspecified effect
-when used with xen.efi (there the concept of modules doesn't exist, and
-the blob gets specified via the `ucode=` config file/section
-entry; see [EFI configuration file description](efi.html)).
+image).
'scan' instructs the hypervisor to scan the multiboot images for an cpio
image that contains microcode. Depending on the platform the blob with the
@@ -2151,6 +2154,11 @@ microcode in the cpio name space must be:
stop_machine context. In NMI handler, even NMIs are blocked, which is
considered safer. The default value is `true`.
+Note: When booting via EFI, both options 'integer' and 'scan' are ignored.
+Here, the concept of modules does not exist. The microcode update blob for
+early loading gets specified via the `ucode=` config file/section
+entry; see [EFI configuration file description](efi.html)).
+
### unrestricted_guest (Intel)
> `= `
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 6ced293d88..8b4d87782c 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -60,7 +60,7 @@
static module_t __initdata ucode_mod;
static signed int __initdata ucode_mod_idx;
-static bool_t __initdata ucode_mod_forced;
+static signed int __initdata ucode_mod_efi_idx;
static unsigned int nr_cores;
/*
@@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache;
void __init microcode_set_module(unsigned int idx)
{
-ucode_mod_idx = idx;
-ucode_mod_forced = 1;
+ucode_mod_efi_idx = idx;
}
-/*
- * The format is '[|scan=, nmi=]'. Both options are
- * optional. If the EFI has forced which of the multiboot payloads is to be
- * used, only nmi= is parsed.
- */
-static int __init parse_ucode(const char *s)
+static int __init parse_ucode_param(const char *s)
{
const char *ss;
int val, rc = 0;
@@ -126,18 +120,15 @@ static int __init parse_ucode(const char *s)
if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
ucode_in_nmi = val;
-else if ( !ucode_mod_forced ) /* Not forced by EFI */
+else if ( (val = parse_boolean("scan", s, ss)) >= 0 )
+ucode_scan = val;
+else
{
-if ( (val = parse_boolean("scan", s, ss)) >= 0 )
-ucode_scan = val;
-else
-{
-const char *q;
-
-ucode_mod_idx = simple_strtol(s, &q, 0);
-if ( q != ss )
-rc = -EINVAL;
-}
+const char *q;
+
+ucode_mod_idx = simple_strtol(s, &q, 0);
+if ( q != ss )
+rc = -EINVAL;
}
s = ss + 1;
@@ -145,7 +136,7 @@ static int __init parse_ucode(const char *s)
return rc;
}
-custom_param("ucode", parse_ucode);
+custom_param("ucode", parse_ucode_param);
/*
* 8MB ought to be enough.
@@ -228,14 +219,18 @@ void __init microcode_grab_module(
{
module_t *mod = (module_t *)__va(mbi-&
[Xen-devel] [PATCH v2 2/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data
When using `ucode=scan` and if a matching module is found, the microcode
payload is maintained in an xmalloc()'d region. This is unnecessary since
the bootmap would just do. Remove the xmalloc and xfree on the microcode
module scan path.
This commit also does away with the restriction on the microcode module
size limit. The concern that a large microcode module would consume too
much memory preventing guests launch is misplaced since this is all the
init path. While having such safeguards is valuable, this should apply
across the board for all early/late microcode loading. Having it just on
the `scan` path is confusing.
Looking forward, we are a bit closer (i.e., one xmalloc down) to pulling
the early microcode loading of the BSP a bit earlier in the early boot
process. This commit is the low hanging fruit. There is still a sizable
amount of work to get there as there are still a handful of xmalloc in
microcode_{amd,intel}.c.
First, there are xmallocs on the path of finding a matching microcode
update. Similar to the commit at hand, searching through the microcode
blob can be done on the already present buffer with no need to xmalloc
any further. Even better, do the filtering in microcode.c before
requesting the microcode update on all CPUs. The latter requires careful
restructuring and exposing the arch-specific logic for iterating over
patches and declaring a match.
Second, there are xmallocs for the microcode cache. Here, we would need
to ensure that the cache corresponding to the BSP gets xmalloc()'d and
populated after the fact.
Signed-off-by: Eslam Elnikety
---
xen/arch/x86/microcode.c | 32
1 file changed, 4 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 8b4d87782c..c878fc71ff 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -138,11 +138,6 @@ static int __init parse_ucode_param(const char *s)
}
custom_param("ucode", parse_ucode_param);
-/*
- * 8MB ought to be enough.
- */
-#define MAX_EARLY_CPIO_MICROCODE (8 << 20)
-
void __init microcode_scan_module(
unsigned long *module_map,
const multiboot_info_t *mbi)
@@ -187,31 +182,12 @@ void __init microcode_scan_module(
cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore */);
if ( cd.data )
{
-/*
- * This is an arbitrary check - it would be sad if the blob
- * consumed most of the memory and did not allow guests
- * to launch.
- */
-if ( cd.size > MAX_EARLY_CPIO_MICROCODE )
-{
-printk("Multiboot %d microcode payload too big! (%ld, we
can do %d)\n",
- i, cd.size, MAX_EARLY_CPIO_MICROCODE);
-goto err;
-}
-ucode_blob.size = cd.size;
-ucode_blob.data = xmalloc_bytes(cd.size);
-if ( !ucode_blob.data )
-cd.data = NULL;
-else
-memcpy(ucode_blob.data, cd.data, cd.size);
+ucode_blob.size = cd.size;
+ucode_blob.data = cd.data;
+break;
}
bootstrap_map(NULL);
-if ( cd.data )
-break;
}
-return;
-err:
-bootstrap_map(NULL);
}
void __init microcode_grab_module(
unsigned long *module_map,
@@ -725,7 +701,7 @@ static int __init microcode_init(void)
*/
if ( ucode_blob.size )
{
-xfree(ucode_blob.data);
+bootstrap_map(NULL);
ucode_blob.size = 0;
ucode_blob.data = NULL;
}
--
2.17.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 4/4] x86/microcode: Support builtin CPU microcode
Xen relies on boot modules to perform early microcode updates. This commit adds
another mode, namely "builtin" via the BUILTIN_UCODE config parameter. If set,
the Xen image itself will contain the microcode updates. Upon boot, Xen
inspects its image for microcode blobs and performs the update.
A Xen image with builtin microcode will, by default, attempt the microcode
update. Disabling the builtin microcode update can be done via the Xen command
line parameter 'ucode=no-builtin'. Moreover, the microcode provided via other
options (such as 'ucode=|scan' or 'ucode=' config when
booting via EFI) takes precedence over the builtin one.
Signed-off-by: Eslam Elnikety
---
Changes in v2:
- Allow for ucode=|scan,{no-}builtin and detail the model. Reflect
those changes onto microcode.c and docs/misc/xen-command-line.pandoc
- Add documentation to the existing docs/admin-guide/microcode-loading.rst
- Build on Patches 1--3 to avoid xmalloc/memcpy for the builtin microcode
- Work configuration in order to specify the individual microcode blobs to use
for the builtin microcode, and rework the microcode/Makefile accordingly
---
docs/admin-guide/microcode-loading.rst | 31 +++
docs/misc/xen-command-line.pandoc | 10 -
xen/arch/x86/Kconfig | 30 +++
xen/arch/x86/Makefile | 1 +
xen/arch/x86/microcode.c | 52 ++
xen/arch/x86/microcode/Makefile| 46 +++
xen/arch/x86/xen.lds.S | 12 ++
7 files changed, 180 insertions(+), 2 deletions(-)
create mode 100644 xen/arch/x86/microcode/Makefile
diff --git a/docs/admin-guide/microcode-loading.rst
b/docs/admin-guide/microcode-loading.rst
index e83cadd2c2..989e8d446b 100644
--- a/docs/admin-guide/microcode-loading.rst
+++ b/docs/admin-guide/microcode-loading.rst
@@ -104,6 +104,37 @@ The ``ucode=scan`` command line option will cause Xen to
search through all
modules to find any CPIO archives, and search the archive for the applicable
file. Xen will stop searching at the first match.
+Loading microcode built within the Xen image
+
+
+Xen can bundle microcode updates within its image. This support is conditional
+on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
+useful to ensure that, by default, a minimum microcode patch level will be
+applied to the underlying CPU.
+
+To use microcode updates available on the build system as builtin,
+use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
+and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
+BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
+instance, the configuration below is suitable for a build system which has a
+``/lib/firmware/`` directory which, in turn, includes the individual microcode
+patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
+``intel-ucode/06-2f-02``.
+
+ CONFIG_BUILTIN_UCODE=y
+ CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
+ CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
+ CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
+
+Alternatively, CONFIG_BUILTIN_UCODE_{AMD,INTEL} can directly point to the
+concatenation of the individual microcode blobs. For instance, assuming that
+``amd-ucode/AuthenticAMD.bin`` and ``intel-ucode/GenuineIntel.bin`` hold
+multiple microcode updates for AMD and INTEL, respectively, you may use the
+configuration below.
+
+ CONFIG_BUILTIN_UCODE_AMD="amd-ucode/AuthenticAMD.bin"
+ CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/GenuineIntel.bin"
+
Run time microcode loading
--
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index 40faf3bc3a..9cfc2df05a 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2126,10 +2126,10 @@ logic applies:
active by default.
### ucode (x86)
-> `= List of [ | scan=, nmi= ]`
+> `= List of [ | scan=, builtin=, nmi= ]`
Applicability: x86
-Default: `nmi`
+Default: `nmi` if BUILTIN_UCODE is not enabled, `builtin,nmi` otherwise
Controls for CPU microcode loading. For early loading, this parameter can
specify how and where to find the microcode update blob. For late loading,
@@ -2150,6 +2150,12 @@ microcode in the cpio name space must be:
- on Intel: kernel/x86/microcode/GenuineIntel.bin
- on AMD : kernel/x86/microcode/AuthenticAMD.bin
+'builtin' instructs the hypervisor to use the builtin microcode update. This
+option is available only if option BUILTIN_UCODE is enabled at build. The
+default value is `true`. If a microcode is provided via other options (such
+as 'integer', 'scan', or `ucode=` config when booting via EFI),
+the pro
Re: [Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=
On 18.12.19 12:49, Jan Beulich wrote:
On 18.12.2019 02:32, Eslam Elnikety wrote:
Decouple the microcode referencing mechanism when using GRUB to that
when using EFI. This allows us to avoid the "unspecified effect" of
using ` | scan` along xen.efi.
I guess "unspecified effect" in the doc was pretty pointless - such
options have been ignored before; in fact ...
With that, Xen can explicitly
ignore those named options when using EFI.
... I don't see things becoming any more explicit (not even parsing
the options was quite explicit to me).
I agree that those options have been ignored so far in the case of EFI.
The documentation contradicts that however. The documentation:
* says has unspecified effect.
* does not mention anything about scan being ignored.
With this patch, it is explicit in code and in documentation that both
options are ignored in case of EFI.
As an added benefit,
we get a straightfoward parsing of the ucode parameter.
It's a single if() you eliminate - for me this doesn't make it
meaningfully more straightforward.
As we decouple ucode_mod_idx and ucode_mod_efi_idx, the parsing of the
ucode= just follows the pattern "[ | scan=, nmi=
]" and it does not have to cater for corner cases. In either case, if
you strongly disagree with the wording, I can drop that sentence.
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2128,7 +2128,13 @@ logic applies:
### ucode (x86)
> `= List of [ | scan=, nmi= ]`
-Specify how and where to find CPU microcode update blob.
+Applicability: x86
+Default: `nmi`
+
+Controls for CPU microcode loading. For early loading, this parameter can
+specify how and where to find the microcode update blob. For late loading,
+this parameter specifies if the update happens within a NMI handler or in
+a stop_machine context.
It's always stop_machine context, isn't it? I also don't think this
implementation detail belongs here.
Needs a better wording indeed. Let me know if you have particular
suggestions, and I will incorporate in v3.
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -60,7 +60,7 @@
static module_t __initdata ucode_mod;
static signed int __initdata ucode_mod_idx;
-static bool_t __initdata ucode_mod_forced;
+static signed int __initdata ucode_mod_efi_idx;
I don't see anything negative be put into here - should be unsigned
int then.
Correct! The type just carried over from the coupling with
ucode_mod_idx. Will adjust in v3.
@@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache;
void __init microcode_set_module(unsigned int idx)
{
-ucode_mod_idx = idx;
-ucode_mod_forced = 1;
+ucode_mod_efi_idx = idx;
Is it guaranteed (now and forever) that the index passed in is
non-zero? You changes to microcode_grab_module() imply so, but
just looking at the call site of the function I can't convince
myself this is the case. _If_ it is (thought to be) guaranteed,
then I think you at least want to ASSERT() here, perhaps with
a comment.
For x86, it seems we have that guarantee (given that we must have a
dom0). Right?
}
-/*
- * The format is '[|scan=, nmi=]'. Both options are
- * optional. If the EFI has forced which of the multiboot payloads is to be
- * used, only nmi= is parsed.
- */
-static int __init parse_ucode(const char *s)
+static int __init parse_ucode_param(const char *s)
Any particular reason for the renaming? The function name was quite
fine imo.
To me, "parse_ucode" is a misnomer.
Thanks for your comments, Jan.
-- Eslam
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data
On 18.12.19 13:05, Jan Beulich wrote:
On 18.12.2019 02:32, Eslam Elnikety wrote:
@@ -725,7 +701,7 @@ static int __init microcode_init(void)
*/
if ( ucode_blob.size )
{
-xfree(ucode_blob.data);
+bootstrap_map(NULL);
As much as I like the change, I wholeheartedly disagree with this
aspect of it: You make it largely unpredictable when the boot
mappings will go away - it becomes entirely dependent upon link
order. And of course we really want these mappings to be gone,
the very latest (I think), by the time we start bringing up APs
(but generally the sooner the better). This is (one of?) the main
reason(s) why it hadn't been done this way to begin with. The
alternative is more complicated (set up a proper, long term
mapping), but it's going to be more clean (including the mapping
then also being suitable to post-boot CPU onlining).
This change is an improvement on the current status. We get to avoid
xmalloc/memcpy in the case of a successful ucode=scan. The problematic
aspect you highlight is anyway there regardless of this patch (ref. to
the "else if ( ucode_mod.mod_end )" in microcode_init). The proper, long
term mapping would be a welcome change, but is otherwise independent of
this patch imo.
Thanks,
Eslam
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 4/4] x86/microcode: Support builtin CPU microcode
On 18.12.19 13:42, Jan Beulich wrote:
On 18.12.2019 02:32, Eslam Elnikety wrote:
+
+
+Xen can bundle microcode updates within its image. This support is conditional
+on the build configuration BUILTIN_UCODE being enabled. Builtin microcode is
+useful to ensure that, by default, a minimum microcode patch level will be
+applied to the underlying CPU.
+
+To use microcode updates available on the build system as builtin,
+use BUILTIN_UCODE_DIR to refer to the directory containing the firmware updates
+and specify the individual microcode patches via either BUILTIN_UCODE_AMD or
+BUILTIN_UCODE_INTEL for AMD microcode or INTEL microcode, respectively. For
+instance, the configuration below is suitable for a build system which has a
+``/lib/firmware/`` directory which, in turn, includes the individual microcode
+patches ``amd-ucode/microcode_amd_fam15h.bin``, ``intel-ucode/06-3a-09``, and
+``intel-ucode/06-2f-02``.
+
+ CONFIG_BUILTIN_UCODE=y
+ CONFIG_BUILTIN_UCODE_DIR="/lib/firmware/"
+ CONFIG_BUILTIN_UCODE_AMD="amd-ucode/microcode_amd_fam15h.bin"
+ CONFIG_BUILTIN_UCODE_INTEL="intel-ucode/06-3a-09 intel-ucode/06-2f-02"
Rather than a blank as separator, the more conventional one on
Unix and alike would be : I think. Of course ideally there wouldn't
be any restriction at all on the characters usable here for file
names.
It would be great if there is a particular convention. The blank
separator is aligned with Linux way of doing builtin microcode.
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -218,6 +218,36 @@ config MEM_SHARING
bool "Xen memory sharing support" if EXPERT = "y"
depends on HVM
+config BUILTIN_UCODE
+ bool "Support for Builtin Microcode"
+ ---help---
+ Include the CPU microcode update in the Xen image itself. With this
+ support, Xen can update the CPU microcode upon boot using the builtin
+ microcode, with no need for an additional microcode boot modules.
+
+ If unsure, say N.
I continue to be unconvinced that this separate option is needed.
Albeit compared to the v1 approach I will agree that handling
would become more complicated without.
Any particular preference between the v1 vs v2 approach?
@@ -701,7 +747,13 @@ static int __init microcode_init(void)
*/
if ( ucode_blob.size )
{
+#ifdef CONFIG_BUILTIN_UCODE
+/* No need to destroy module mappings if builtin was used */
+if ( !ucode_builtin )
+bootstrap_map(NULL);
+#else
bootstrap_map(NULL);
+#endif
First of all - is there no ucode unrelated side effect of this
invocation? I.e. can it safely be skipped?
Maybe I am missing something. Are you asking if we can safely skip the
bootstrap_map(NULL)? (Quoting your response on PATCH v2 2/4 "And of
course we really want these mappings to be gone")
If yes, then I think
you want to get away without #ifdef here, by having a suitably
placed
#define ucode_builtin false
somewhere up the file.
Agreed. That will make the code snippet more readable indeed.
--- /dev/null
+++ b/xen/arch/x86/microcode/Makefile
@@ -0,0 +1,46 @@
+# Copyright (C) 2019 Amazon.com, Inc. or its affiliates.
+# Author: Eslam Elnikety
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+
+# Remove quotes and excess spaces from configuration strings
+UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR)))
+UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD)))
+UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL)))
+
+# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs.
+amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD)))
+intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL)))
+
+ifneq ($(amd-blobs),)
+obj-y += ucode_amd.o
+endif
+
+ifneq ($(intel-blobs),)
+obj-y += ucode_intel.o
+endif
+
+ifeq ($(amd-blobs)$(intel-blobs),)
+obj-y += ucode_dummy.o
+endif
+
+ucode_amd.o: Makefile $(amd-blobs)
+ cat $(amd-blobs) > $@.bin
+ $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section
.data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@
+ rm -f $@.bin
+
+ucode_intel.o: Makefile $(intel-blobs)
+ cat $(intel-blobs) > $@.bin
+ $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section
.data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@
+
Re: [Xen-devel] [PATCH 2/2] x86: explicitly disallow guest access to PPIN
Thanks for this series, Jan. On 30.10.19 11:39, Jan Beulich wrote: To fulfill the "protected" in its name, don't let the real hardware values "shine through". Report a control register value expressing this. Signed-off-by: Jan Beulich --- TBD: Do we want to permit Dom0 access? It would be nice to give an administrator a way to get PPIN outside the context of an MCE when needed. --- a/xen/arch/x86/msr.c +++ b/xen/arch/x86/msr.c @@ -135,6 +135,8 @@ int guest_rdmsr(struct vcpu *v, uint32_t case MSR_TSX_FORCE_ABORT: case MSR_AMD64_LWP_CFG: case MSR_AMD64_LWP_CBADDR: +case MSR_PPIN: +case MSR_AMD_PPIN: /* Not offered to guests. */ goto gp_fault; @@ -237,6 +239,18 @@ int guest_rdmsr(struct vcpu *v, uint32_t ARRAY_SIZE(msrs->dr_mask))]; break; +case MSR_PPIN_CTL: +if ( d->arch.cpuid->x86_vendor != X86_VENDOR_INTEL ) +goto gp_fault; +*val = PPIN_LOCKOUT; +break; + +case MSR_AMD_PPIN_CTL: +if ( !cp->extd.amd_ppin ) +goto gp_fault; +*val = PPIN_LOCKOUT; +break; + nit: It is not clear to me why you use "d->arch.cpuid->.." (and not "cp->..") in the first if condition. -- Eslam ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=
Picking this up again after the break. Apologies for the delay.
On 20.12.19 10:53, Jan Beulich wrote:
On 19.12.2019 22:08, Eslam Elnikety wrote:
On 18.12.19 12:49, Jan Beulich wrote:
On 18.12.2019 02:32, Eslam Elnikety wrote:
Decouple the microcode referencing mechanism when using GRUB to that
when using EFI. This allows us to avoid the "unspecified effect" of
using ` | scan` along xen.efi.
I guess "unspecified effect" in the doc was pretty pointless - such
options have been ignored before; in fact ...
With that, Xen can explicitly
ignore those named options when using EFI.
... I don't see things becoming any more explicit (not even parsing
the options was quite explicit to me).
I agree that those options have been ignored so far in the case of EFI.
The documentation contradicts that however. The documentation:
* says has unspecified effect.
* does not mention anything about scan being ignored.
With this patch, it is explicit in code and in documentation that both
options are ignored in case of EFI.
But isn't it rather that ucode=scan could (and hence perhaps should)
also have its value on EFI?
I do not see "ucode=scan" applicable in anyway in the case of EFI. In
EFI, there are not "modules" to scan through, but rather the efi config
points exactly to the microcode blob.
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2128,7 +2128,13 @@ logic applies:
### ucode (x86)
> `= List of [ | scan=, nmi= ]`
-Specify how and where to find CPU microcode update blob.
+Applicability: x86
+Default: `nmi`
+
+Controls for CPU microcode loading. For early loading, this parameter can
+specify how and where to find the microcode update blob. For late loading,
+this parameter specifies if the update happens within a NMI handler or in
+a stop_machine context.
It's always stop_machine context, isn't it? I also don't think this
implementation detail belongs here.
Needs a better wording indeed. Let me know if you have particular
suggestions, and I will incorporate in v3.
Just drop everything from "or" onwards?
Ack
@@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache;
void __init microcode_set_module(unsigned int idx)
{
-ucode_mod_idx = idx;
-ucode_mod_forced = 1;
+ucode_mod_efi_idx = idx;
Is it guaranteed (now and forever) that the index passed in is
non-zero? You changes to microcode_grab_module() imply so, but
just looking at the call site of the function I can't convince
myself this is the case. _If_ it is (thought to be) guaranteed,
then I think you at least want to ASSERT() here, perhaps with
a comment.
For x86, it seems we have that guarantee (given that we must have a
dom0). Right?
For fully bringing up the system - yes. But there's no reason to
have a Dom0 if all you're after is testing early Xen boot. There'll
be a panic() in the case, but there shouldn't be anything breaking
proper behavior prior to this point.
That's a valid point indeed. v3 will handle index being zero.
}
-/*
- * The format is '[|scan=, nmi=]'. Both options are
- * optional. If the EFI has forced which of the multiboot payloads is to be
- * used, only nmi= is parsed.
- */
-static int __init parse_ucode(const char *s)
+static int __init parse_ucode_param(const char *s)
Any particular reason for the renaming? The function name was quite
fine imo.
To me, "parse_ucode" is a misnomer.
Well, parse_"ucode= isn't a valid identifier. parse_ucode is what
results when stripping everything that makes it invalid. I can
see the ambiguity with parsing actual ucode, but the context here
makes it pretty clear what the function is about. Personally I'd
prefer such unnecessary renames to be avoided.
Ack
-- Eslam
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 4/4] x86/microcode: Support builtin CPU microcode
On 20.12.19 11:12, Jan Beulich wrote: On 19.12.2019 23:11, Eslam Elnikety wrote: On 18.12.19 13:42, Jan Beulich wrote: On 18.12.2019 02:32, Eslam Elnikety wrote: --- /dev/null +++ b/xen/arch/x86/microcode/Makefile @@ -0,0 +1,46 @@ +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. +# Author: Eslam Elnikety +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# Remove quotes and excess spaces from configuration strings +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) + +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) + +ifneq ($(amd-blobs),) +obj-y += ucode_amd.o +endif + +ifneq ($(intel-blobs),) +obj-y += ucode_intel.o +endif + +ifeq ($(amd-blobs)$(intel-blobs),) +obj-y += ucode_dummy.o +endif + +ucode_amd.o: Makefile $(amd-blobs) + cat $(amd-blobs) > $@.bin + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@ + rm -f $@.bin + +ucode_intel.o: Makefile $(intel-blobs) + cat $(intel-blobs) > $@.bin + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@ + rm -f $@.bin This can be had with a pattern rule (with the vendor being the stem) and hence without duplication, I think. Also - is simply concatenating the blobs reliable enough? There's no build time diagnostic that the result would actually be understood at runtime. Concatenation is reliable (as long as the individual microcode blobs are not malformed, and in that case the builtin is not making matters worse compared to presenting the malformed update via | scan). A malformed update found the other way is a bug in the tools constructing the respective images. A malformed built-in update is a bug in the Xen build system. The put the question differently: Is it specified somewhere that the blobs all have to have certain properties, which the straight concatenation relies upon? Refer to ( https://www.kernel.org/doc/Documentation/x86/microcode.txt ). AuthenticAMD.bin and GenuineIntel.bin are both concatenations of individual microcode blobs. +ucode_dummy.o: Makefile + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; Since the commit message doesn't explain why this is needed, I have to ask (I guess we somewhere have a dependency on $(obj-y) not being empty). Your guess is correct. All sub-directories of xen/arch/x86 are expected to produce built_in.o. If there are not amd nor intel microcode blobs, there will be no build dependencies and the build fails preparing the built_in.o That's rather poor, but it's of course not your task to get this fixed (it shouldn't be very difficult to create an empty built_in.o for an empty $(obj-y)). _If_ it is needed, I don't see why you need ifeq() around its use. In fact you could have obj-y := ucode-dummy.o right at the top of the file. Furthermore I don't really understand why you need this in the first place. While cat won't do what you want with an empty argument list, can't you simply prepend / append /dev/null? To make sure we are on the same page. You are suggesting using "/dev/null" in case there are no amd/intel ucode to generate the ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as input (complains about empty binary). That's again rather poor, this time of the utility - it should be easy enough to produce an object with an empty .data (or whatever it is) section. As above - I'm fine with you keeping the logic then as is, provided you say in the description why it can't be simplified. Ack. Will justify the logic in comments. -- Eslam Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 4/4] x86/microcode: Support builtin CPU microcode
On 20.12.19 11:34, Jürgen Groß wrote: On 20.12.19 11:12, Jan Beulich wrote: On 19.12.2019 23:11, Eslam Elnikety wrote: On 18.12.19 13:42, Jan Beulich wrote: On 18.12.2019 02:32, Eslam Elnikety wrote: --- /dev/null +++ b/xen/arch/x86/microcode/Makefile @@ -0,0 +1,46 @@ +# Copyright (C) 2019 Amazon.com, Inc. or its affiliates. +# Author: Eslam Elnikety +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 2 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. + +# Remove quotes and excess spaces from configuration strings +UCODE_DIR=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_DIR))) +UCODE_AMD=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_AMD))) +UCODE_INTEL=$(strip $(subst $\",,$(CONFIG_BUILTIN_UCODE_INTEL))) + +# AMD and INTEL microcode blobs. Use 'wildcard' to filter for existing blobs. +amd-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_AMD))) +intel-blobs := $(wildcard $(addprefix $(UCODE_DIR),$(UCODE_INTEL))) + +ifneq ($(amd-blobs),) +obj-y += ucode_amd.o +endif + +ifneq ($(intel-blobs),) +obj-y += ucode_intel.o +endif + +ifeq ($(amd-blobs)$(intel-blobs),) +obj-y += ucode_dummy.o +endif + +ucode_amd.o: Makefile $(amd-blobs) + cat $(amd-blobs) > $@.bin + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_amd_ucode,alloc,load,readonly,data,contents $@.bin $@ + rm -f $@.bin + +ucode_intel.o: Makefile $(intel-blobs) + cat $(intel-blobs) > $@.bin + $(OBJCOPY) -I binary -O elf64-x86-64 -B i386:x86-64 --rename-section .data=.builtin_intel_ucode,alloc,load,readonly,data,contents $@.bin $@ + rm -f $@.bin This can be had with a pattern rule (with the vendor being the stem) and hence without duplication, I think. Also - is simply concatenating the blobs reliable enough? There's no build time diagnostic that the result would actually be understood at runtime. Concatenation is reliable (as long as the individual microcode blobs are not malformed, and in that case the builtin is not making matters worse compared to presenting the malformed update via | scan). A malformed update found the other way is a bug in the tools constructing the respective images. A malformed built-in update is a bug in the Xen build system. The put the question differently: Is it specified somewhere that the blobs all have to have certain properties, which the straight concatenation relies upon? +ucode_dummy.o: Makefile + $(CC) $(CFLAGS) -c -x c /dev/null -o $@; Since the commit message doesn't explain why this is needed, I have to ask (I guess we somewhere have a dependency on $(obj-y) not being empty). Your guess is correct. All sub-directories of xen/arch/x86 are expected to produce built_in.o. If there are not amd nor intel microcode blobs, there will be no build dependencies and the build fails preparing the built_in.o That's rather poor, but it's of course not your task to get this fixed (it shouldn't be very difficult to create an empty built_in.o for an empty $(obj-y)). _If_ it is needed, I don't see why you need ifeq() around its use. In fact you could have obj-y := ucode-dummy.o right at the top of the file. Furthermore I don't really understand why you need this in the first place. While cat won't do what you want with an empty argument list, can't you simply prepend / append /dev/null? To make sure we are on the same page. You are suggesting using "/dev/null" in case there are no amd/intel ucode to generate the ucode_amd/intel.o? If so, objcopy does not allow using /dev/null as input (complains about empty binary). That's again rather poor, this time of the utility - it should be easy enough to produce an object with an empty .data (or whatever it is) section. As above - I'm fine with you keeping the logic then as is, provided you say in the description why it can't be simplified. What about using the attached patch for including the binary files? I wanted to post that for my hypervisor-fs series, but I think it would fit here quite nice. Thanks, Jürgen. That tool is indeed useful on its own right for flask/policies. However, it seems to me that creating a built_in.o right out of the microcode blobs is simpler and keeps the whole business contained within xen/arch/x86/microcode/. -- Eslam Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=
On 20.01.20 09:42, Jan Beulich wrote: On 17.01.2020 20:06, Eslam Elnikety wrote: On 20.12.19 10:53, Jan Beulich wrote: On 19.12.2019 22:08, Eslam Elnikety wrote: On 18.12.19 12:49, Jan Beulich wrote: On 18.12.2019 02:32, Eslam Elnikety wrote: Decouple the microcode referencing mechanism when using GRUB to that when using EFI. This allows us to avoid the "unspecified effect" of using ` | scan` along xen.efi. I guess "unspecified effect" in the doc was pretty pointless - such options have been ignored before; in fact ... With that, Xen can explicitly ignore those named options when using EFI. ... I don't see things becoming any more explicit (not even parsing the options was quite explicit to me). I agree that those options have been ignored so far in the case of EFI. The documentation contradicts that however. The documentation: * says has unspecified effect. * does not mention anything about scan being ignored. With this patch, it is explicit in code and in documentation that both options are ignored in case of EFI. But isn't it rather that ucode=scan could (and hence perhaps should) also have its value on EFI? I do not see "ucode=scan" applicable in anyway in the case of EFI. In EFI, there are not "modules" to scan through, but rather the efi config points exactly to the microcode blob. What would be wrong with the EFI code to also inspect whatever has been specified with ramdisk= if there was no ucode= ? Jan I see, interesting. This sounds like a legitimate use case indeed. I wonder, would I be breaking anything if I simply allow the existing code that iterates over modules to kick in when ucode=scan irrespective of EFI or legacy boot? Also, it seems to me that the ucode= specified by efi.cfg would take precedence over the ucode=scan. Do not you think? Eslam ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=
On 21.01.20 10:27, Jan Beulich wrote: On 21.01.2020 00:50, Eslam Elnikety wrote: On 20.01.20 09:42, Jan Beulich wrote: On 17.01.2020 20:06, Eslam Elnikety wrote: On 20.12.19 10:53, Jan Beulich wrote: On 19.12.2019 22:08, Eslam Elnikety wrote: On 18.12.19 12:49, Jan Beulich wrote: On 18.12.2019 02:32, Eslam Elnikety wrote: Decouple the microcode referencing mechanism when using GRUB to that when using EFI. This allows us to avoid the "unspecified effect" of using ` | scan` along xen.efi. I guess "unspecified effect" in the doc was pretty pointless - such options have been ignored before; in fact ... With that, Xen can explicitly ignore those named options when using EFI. ... I don't see things becoming any more explicit (not even parsing the options was quite explicit to me). I agree that those options have been ignored so far in the case of EFI. The documentation contradicts that however. The documentation: * says has unspecified effect. * does not mention anything about scan being ignored. With this patch, it is explicit in code and in documentation that both options are ignored in case of EFI. But isn't it rather that ucode=scan could (and hence perhaps should) also have its value on EFI? I do not see "ucode=scan" applicable in anyway in the case of EFI. In EFI, there are not "modules" to scan through, but rather the efi config points exactly to the microcode blob. What would be wrong with the EFI code to also inspect whatever has been specified with ramdisk= if there was no ucode= ? I see, interesting. This sounds like a legitimate use case indeed. I wonder, would I be breaking anything if I simply allow the existing code that iterates over modules to kick in when ucode=scan irrespective of EFI or legacy boot? I don't think so, no, but it would need double checking (and mentioning in the description and/or documentation). Also, it seems to me that the ucode= specified by efi.cfg would take precedence over the ucode=scan. Do not you think? I guess we need to settle on what we want to take precedence and then make sure code also behaves this way. But yes, I think ucode= from the .cfg should supersede ucode=scan on the command line. A possibly useful adjustment to this might be to distinguish whether the ucode=scan was in a specific .cfg section while the ucode= was in [global] (i.e. sort of a default), in which case maybe the ucode=scan should win. Thoughts? Jan I think any ucode= in the EFI .cfg ought to supersede the ucode=scan. The semantics are simpler in this case, rather than having to worry about where exactly the ucode= was specified in the EFI .cfg. With that, an administrator would default to using ucode=scan on the commandline to load the ramdisk microcode, and a ucode= in .cfg would be an explicit signal to use different microcode. Eslam ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1 3/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data
When using `ucode=scan` and if a matching module is found, the microcode
payload is maintained in an xmalloc()'d region. This is unnecessary since
the bootmap would just do. Remove the xmalloc and xfree on the microcode
module scan path.
This commit also does away with the restriction on the microcode module
size limit. The concern that a large microcode module would consume too
much memory preventing guests launch is misplaced since this is all the
init path. While having such safeguards is valuable, this should apply
across the board for all early/late microcode loading. Having it just on
the `scan` path is confusing.
Looking forward, we are a bit closer (i.e., one xmalloc down) to pulling
the early microcode loading of the BSP a bit earlier in the early boot
process. This commit is the low hanging fruit. There is still a sizable
amount of work to get there as there are still a handful of xmalloc in
microcode_{amd,intel}.c.
First, there are xmallocs on the path of finding a matching microcode
update. Similar to the commit at hand, searching through the microcode
blob can be done on the already present buffer with no need to xmalloc
any further. Even better, do the filtering in microcode.c before
requesting the microcode update on all CPUs. The latter requires careful
restructuring and exposing the arch-specific logic for iterating over
patches and declaring a match.
Second, there are xmallocs for the microcode cache. Here, we would need
to ensure that the cache corresponding to the BSP gets xmalloc()'d and
populated after the fact.
Signed-off-by: Eslam Elnikety
Acked-by: Jan Beulich
---
xen/arch/x86/microcode.c | 32
1 file changed, 4 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index e1d98fa55e..a662a7f438 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -141,11 +141,6 @@ static int __init parse_ucode(const char *s)
}
custom_param("ucode", parse_ucode);
-/*
- * 8MB ought to be enough.
- */
-#define MAX_EARLY_CPIO_MICROCODE (8 << 20)
-
void __init microcode_scan_module(
unsigned long *module_map,
const multiboot_info_t *mbi)
@@ -190,31 +185,12 @@ void __init microcode_scan_module(
cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore */);
if ( cd.data )
{
-/*
- * This is an arbitrary check - it would be sad if the blob
- * consumed most of the memory and did not allow guests
- * to launch.
- */
-if ( cd.size > MAX_EARLY_CPIO_MICROCODE )
-{
-printk("Multiboot %d microcode payload too big! (%ld, we
can do %d)\n",
- i, cd.size, MAX_EARLY_CPIO_MICROCODE);
-goto err;
-}
-ucode_blob.size = cd.size;
-ucode_blob.data = xmalloc_bytes(cd.size);
-if ( !ucode_blob.data )
-cd.data = NULL;
-else
-memcpy(ucode_blob.data, cd.data, cd.size);
+ucode_blob.size = cd.size;
+ucode_blob.data = cd.data;
+break;
}
bootstrap_map(NULL);
-if ( cd.data )
-break;
}
-return;
-err:
-bootstrap_map(NULL);
}
void __init microcode_grab_module(
unsigned long *module_map,
@@ -734,7 +710,7 @@ static int __init microcode_init(void)
*/
if ( ucode_blob.size )
{
-xfree(ucode_blob.data);
+bootstrap_map(NULL);
ucode_blob.size = 0;
ucode_blob.data = NULL;
}
--
2.17.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1 2/4] x86/microcode: Improve parsing for ucode=
Decouple the microcode indexing mechanism when using GRUB to that
when using EFI. This allows us to avoid the "unspecified effect" of
using `` when booting via EFI. With that, Xen can explicitly
ignore that option when using EFI. This is the only functinal change
this commit introduces. Update the command line documentation for
consistency. As an added benefit, the 'parse_ucode' logic becomes
independent of GRUB vs. EFI.
While at it, drop the leading comment for parse_ucode. No practical
use for it given this commit.
Signed-off-by: Eslam Elnikety
---
docs/misc/xen-command-line.pandoc | 6 ++---
xen/arch/x86/microcode.c | 38 +--
2 files changed, 24 insertions(+), 20 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc
b/docs/misc/xen-command-line.pandoc
index ebec6d387e..821b9281a1 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2147,9 +2147,9 @@ for updating CPU micrcode. When negative, counting starts
at the end of
the modules in the GrUB entry (so with the blob commonly being last,
one could specify `ucode=-1`). Note that the value of zero is not valid
here (entry zero, i.e. the first module, is always the Dom0 kernel
-image). Note further that use of this option has an unspecified effect
-when used with xen.efi (there the concept of modules doesn't exist, and
-the blob gets specified via the `ucode=` config file/section
+image). This option should be used only with legacy boot, as it is explicitly
+ignored in EFI boot. When booting via EFI, the microcode update blob for
+early loading can be specified via the `ucode=` config file/section
entry; see [EFI configuration file description](efi.html)).
'scan' instructs the hypervisor to scan the multiboot images for an cpio
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 6ced293d88..e1d98fa55e 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -35,6 +35,7 @@
#include
#include
#include
+#include
#include
#include
@@ -61,6 +62,7 @@
static module_t __initdata ucode_mod;
static signed int __initdata ucode_mod_idx;
static bool_t __initdata ucode_mod_forced;
+static unsigned int __initdata ucode_mod_efi_idx;
static unsigned int nr_cores;
/*
@@ -105,15 +107,10 @@ static struct microcode_patch *microcode_cache;
void __init microcode_set_module(unsigned int idx)
{
-ucode_mod_idx = idx;
+ucode_mod_efi_idx = idx;
ucode_mod_forced = 1;
}
-/*
- * The format is '[|scan=, nmi=]'. Both options are
- * optional. If the EFI has forced which of the multiboot payloads is to be
- * used, only nmi= is parsed.
- */
static int __init parse_ucode(const char *s)
{
const char *ss;
@@ -126,18 +123,15 @@ static int __init parse_ucode(const char *s)
if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
ucode_in_nmi = val;
-else if ( !ucode_mod_forced ) /* Not forced by EFI */
+else if ( (val = parse_boolean("scan", s, ss)) >= 0 )
+ucode_scan = val;
+else
{
-if ( (val = parse_boolean("scan", s, ss)) >= 0 )
-ucode_scan = val;
-else
-{
-const char *q;
-
-ucode_mod_idx = simple_strtol(s, &q, 0);
-if ( q != ss )
-rc = -EINVAL;
-}
+const char *q;
+
+ucode_mod_idx = simple_strtol(s, &q, 0);
+if ( q != ss )
+rc = -EINVAL;
}
s = ss + 1;
@@ -228,6 +222,16 @@ void __init microcode_grab_module(
{
module_t *mod = (module_t *)__va(mbi->mods_addr);
+if ( efi_enabled(EFI_BOOT) )
+{
+if ( ucode_mod_forced ) /* Microcode specified by EFI */
+{
+ucode_mod = mod[ucode_mod_efi_idx];
+return;
+}
+goto scan;
+}
+
if ( ucode_mod_idx < 0 )
ucode_mod_idx += mbi->mods_count;
if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count ||
--
2.17.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1 0/4] x86/microcode: Improve documentation and code
This patch series introduces improvements to the existing documentation and code of x86/microcode. Patches 1 and 2 improve the documentation and parsing for `ucode=`. Patches 3 and 4 introduce nits/improvements to the microcode early loading code. Some (variant of the) patches have been sent earlier under "Support builtin CPU microcode" as those patches were motivated by discussions following the initial submission of the builtin microcode. On a second thought, such improvements should have gone independently. So here it goes. (Those improvements will be dropped from the builtin microcode series as I submit its v3). Changes since submitted under [v2] x86/microcode: Support builtin CPU microcode - Patch 1: New / explicitly document the current behaviour of ucode=scan with EFI - Patch 2: Fix index data type, drop unwelcomed function rename - Patch 3 and 4: Added Acked-by, otherwise as before Eslam Elnikety (4): x86/microcode: Improve documentation for ucode= x86/microcode: Improve parsing for ucode= x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data x86/microcode: use const qualifier for microcode buffer docs/misc/xen-command-line.pandoc | 14 -- xen/arch/x86/microcode.c | 74 +++ 2 files changed, 37 insertions(+), 51 deletions(-) -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1 1/4] x86/microcode: Improve documentation for ucode=
Specify applicability and the default value. Also state that, in case of EFI, the microcode update blob specified in the EFI cfg takes precedence over `ucode=scan`, if the latter is specified on Xen commend line. No functional changes. Signed-off-by: Eslam Elnikety --- docs/misc/xen-command-line.pandoc | 8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index 981a5e2381..ebec6d387e 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2134,7 +2134,12 @@ logic applies: ### ucode (x86) > `= List of [ | scan=, nmi= ]` -Specify how and where to find CPU microcode update blob. +Applicability: x86 +Default: `nmi` + +Controls for CPU microcode loading. For early loading, this parameter can +specify how and where to find the microcode update blob. For late loading, +this parameter specifies if the update happens within a NMI handler. 'integer' specifies the CPU microcode update blob module index. When positive, this specifies the n-th module (in the GrUB entry, zero based) to be used @@ -2152,6 +2157,7 @@ image that contains microcode. Depending on the platform the blob with the microcode in the cpio name space must be: - on Intel: kernel/x86/microcode/GenuineIntel.bin - on AMD : kernel/x86/microcode/AuthenticAMD.bin +If EFI boot, the `ucode=` config takes precendence over `scan`. 'nmi' determines late loading is performed in NMI handler or just in stop_machine context. In NMI handler, even NMIs are blocked, which is -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v1 4/4] x86/microcode: use const qualifier for microcode buffer
The buffer holding the microcode bits should be marked as const.
Signed-off-by: Eslam Elnikety
Acked-by: Jan Beulich
---
xen/arch/x86/microcode.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index a662a7f438..0639551173 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -88,7 +88,7 @@ static enum {
* memory.
*/
struct ucode_mod_blob {
-void *data;
+const void *data;
size_t size;
};
@@ -753,7 +753,7 @@ int microcode_update_one(bool start_update)
int __init early_microcode_update_cpu(void)
{
int rc = 0;
-void *data = NULL;
+const void *data = NULL;
size_t len;
struct microcode_patch *patch;
--
2.17.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 1/4] x86/microcode: Improve documentation and parsing for ucode=
On 21.01.20 21:51, Eslam Elnikety wrote: On 21.01.20 10:27, Jan Beulich wrote: On 21.01.2020 00:50, Eslam Elnikety wrote: On 20.01.20 09:42, Jan Beulich wrote: On 17.01.2020 20:06, Eslam Elnikety wrote: On 20.12.19 10:53, Jan Beulich wrote: On 19.12.2019 22:08, Eslam Elnikety wrote: On 18.12.19 12:49, Jan Beulich wrote: On 18.12.2019 02:32, Eslam Elnikety wrote: Decouple the microcode referencing mechanism when using GRUB to that when using EFI. This allows us to avoid the "unspecified effect" of using ` | scan` along xen.efi. I guess "unspecified effect" in the doc was pretty pointless - such options have been ignored before; in fact ... With that, Xen can explicitly ignore those named options when using EFI. ... I don't see things becoming any more explicit (not even parsing the options was quite explicit to me). I agree that those options have been ignored so far in the case of EFI. The documentation contradicts that however. The documentation: * says has unspecified effect. * does not mention anything about scan being ignored. With this patch, it is explicit in code and in documentation that both options are ignored in case of EFI. But isn't it rather that ucode=scan could (and hence perhaps should) also have its value on EFI? I do not see "ucode=scan" applicable in anyway in the case of EFI. In EFI, there are not "modules" to scan through, but rather the efi config points exactly to the microcode blob. What would be wrong with the EFI code to also inspect whatever has been specified with ramdisk= if there was no ucode= ? I see, interesting. This sounds like a legitimate use case indeed. I wonder, would I be breaking anything if I simply allow the existing code that iterates over modules to kick in when ucode=scan irrespective of EFI or legacy boot? I don't think so, no, but it would need double checking (and mentioning in the description and/or documentation). Also, it seems to me that the ucode= specified by efi.cfg would take precedence over the ucode=scan. Do not you think? I guess we need to settle on what we want to take precedence and then make sure code also behaves this way. But yes, I think ucode= from the .cfg should supersede ucode=scan on the command line. A possibly useful adjustment to this might be to distinguish whether the ucode=scan was in a specific .cfg section while the ucode= was in [global] (i.e. sort of a default), in which case maybe the ucode=scan should win. Thoughts? Jan I think any ucode= in the EFI .cfg ought to supersede the ucode=scan. The semantics are simpler in this case, rather than having to worry about where exactly the ucode= was specified in the EFI .cfg. With that, an administrator would default to using ucode=scan on the commandline to load the ramdisk microcode, and a ucode= in .cfg would be an explicit signal to use different microcode. Eslam So that happens to be the existing behaviour already :) I was under the impression that ucode=scan was simply ignored under EFI. That's not the case. It is only ignored if ucode= is specified in the EFI config. In other words, what we had just discussed above is already the case. This clearly needs spelling out in the documentation, which is the first patch in the "x86/microcode: Improve documentation and code" series I have sent just now. Cheers, Eslam ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v1 2/4] x86/microcode: Improve parsing for ucode=
Thanks for getting the other patches in the series onto master, Jan. This is the only patch out of this series that did not make it through, so I keeping my comments here. On 23.01.20 11:26, Jan Beulich wrote: On 22.01.2020 23:30, Eslam Elnikety wrote: Decouple the microcode indexing mechanism when using GRUB to that when using EFI. This allows us to avoid the "unspecified effect" of using `` when booting via EFI. Personally I'd prefer us to continue call this unspecified. It simply shouldn't be used. People shouldn't rely on it getting ignored. (Iirc, despite this being v1, you had previously submitted a patch to this effect, with me replaying about the same.) With that, Xen can explicitly ignore that option when using EFI. Don't we do so already, by way of the "if ( !ucode_mod_forced )" you delete? Not quite. If cfg.efi does not specify "ucode=", then a `ucode=` from the commandline gets parsed, resulting in the "unspecified" behaviour. Here, the ucode_mod_idx will hold the which will be used as index into the modules prepared in whatever order the efi/boot.c defines. In any case, let me know (and others too) if, later on, you may want to favor the explicitly ignore (opposed to unspecified) semantic and I will be happy to send another revision of this patch while addressing your comment below. --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2147,9 +2147,9 @@ for updating CPU micrcode. When negative, counting starts at the end of the modules in the GrUB entry (so with the blob commonly being last, one could specify `ucode=-1`). Note that the value of zero is not valid here (entry zero, i.e. the first module, is always the Dom0 kernel -image). Note further that use of this option has an unspecified effect -when used with xen.efi (there the concept of modules doesn't exist, and -the blob gets specified via the `ucode=` config file/section +image). This option should be used only with legacy boot, as it is explicitly +ignored in EFI boot. When booting via EFI, the microcode update blob for +early loading can be specified via the `ucode=` config file/section entry; see [EFI configuration file description](efi.html)). Just like in patch 1, please distinguish "EFI boot" in general from "use of xen.efi" (relevant here of course only if indeed we went with this patch). Jan You have mentioned this very same remark on the other patch too. My understanding is that "EFI boot" may be ambiguous between (a) we are actually booting from xen.efi or (b) a system with EFI support (and the latter may still be falling onto grub for booting). Let me know if that's not actually what your remark is about. Cheers, Eslam ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
