There is a corner case in the VMRUN instruction where its INTR_SHADOW state
leaks into guest state if a VMExit occurs before the VMRUN is complete. An
example of this could be taking #NPF due to event injection.
Xen can safely execute STI anywhere between CLGI and VMRUN, as CLGI blocks
external interrupts too. However, an exception (while fatal) will appear to
be in an irqs-on region (as GIF isn't considered), so position the STI after
the speculation actions but prior to the GPR pops.
Link: https://lore.kernel.org/all/CADH9ctBs1YPmE4aCfGPNBwA10cA8RuAk2gO7542DjMZgs4uzJQ@mail.gmail.com/
Fixes: 66b245d9ea ("SVM: limit GIF=0 region")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
The panic() function uses a static buffer to format its arguments into, simply
to emit the result via printk("%s", buf). This buffer is not large enough for
some existing users in Xen. e.g.:
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) Invalid device tree blob at physical address 0x46a00000.
(XEN) The DTB must be 8-byte aligned and must not exceed 2 MB in size.
(XEN)
(XEN) Plea****************************************
The remainder of this particular message is 'e check your bootloader.', but
has been inherited by RISC-V from ARM.
It is also pointless double buffering. Implement vprintk() beside printk(),
and use it directly rather than rendering into a local buffer, removing it as
one source of message limitation.
This marginally simplifies panic(), and drops a global used-once buffer.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
While fixing some common/arch boundaries for UBSAN support on other
architectures, the following debugging patch:
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index c1f2d1b89d43..58d1d048d339 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -504,6 +504,8 @@ void asmlinkage __init start_xen(unsigned long fdt_paddr)
system_state = SYS_STATE_active;
+ dump_execution_state();
+
for_each_domain( d )
domain_unpause_by_systemcontroller(d);
failed with:
(XEN) *** Serial input to DOM0 (type 'CTRL-a' three times to switch input)
(XEN) CPU0: Unexpected Trap: Undefined Instruction
(XEN) ----[ Xen-4.20-rc arm32 debug=n Not tainted ]----
(XEN) CPU: 0
<snip>
(XEN)
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Undefined Instruction
(XEN) ****************************************
This is because the condition for init text is wrong. While there's nothing
interesting from that point onwards in start_xen(), it's also wrong for
livepatches too.
Use is_active_kernel_text() which is the correct test for this purpose, and is
aware of init and livepatch regions as well as their lifetimes.
Fixes: 3e802c6ca1 ("xen/arm: Correctly support WARN_ON")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
RISC-V has less complicated headers, so update ubsan.c to pull in everything
it needs. Provide dump_execution_state(), and update the printk() message to
make it more obvious that it's an outstanding task.
As with commit 8ef2ac727e ("automation: enable UBSAN for debug tests"),
enable UBSAN in RISC-V testing too.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
JAL has a maximium displacement of 2M. To branch further, it needs pairing
with an AUIPC instruction. CALL is a pseudoinstruction which allows the
linker to pick the appropriate sequence when relaxations are enabled.
This avoids a build failure of the form:
prelink.o: in function `start':
xen/xen/arch/riscv/riscv64/head.S:28:(.text.header+0x2c):
relocation truncated to fit: R_RISCV_JAL against symbol `calc_phys_offset' defined in .init.text section in prelink.o
make[3]: *** [arch/riscv/Makefile:18: xen-syms] Error 1
when Xen gets large enough, e.g. with CONFIG_UBSAN enabled.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Logic using performance counters needs to look at
MSR_MISC_ENABLE.PERF_AVAILABLE before touching any other resources.
When virtualised under ESX, Xen dies with a #GP fault trying to read
MSR_CORE_PERF_GLOBAL_CTRL.
Factor this logic out into a separate function (it's already too squashed to
the RHS), and insert a check of MSR_MISC_ENABLE.PERF_AVAILABLE.
This also avoids setting X86_FEATURE_ARCH_PERFMON if MSR_MISC_ENABLE says that
PERF is unavailable, although oprofile (the only consumer of this flag)
cross-checks too.
Fixes: 6bdb965178 ("x86/intel: ensure Global Performance Counter Control is setup correctly")
Reported-by: Jonathan Katz <jonathan.katz@aptar.com>
Link: https://xcp-ng.org/forum/topic/10286/nesting-xcp-ng-on-esx-8
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Tested-by: Jonathan Katz <jonathan.katz@aptar.com>
public/arch-ppc.h contains two adjacent #ifndef __ASSEMBLY__ blocks.
With these merged, it becomes very obvious that there's a duplicate
definition of xen_ulong_t, which is also noticed by the docs build:
/usr/bin/perl -w /local/xen.git/docs/xen-headers -O html/hypercall/ppc \
-T 'arch-ppc - Xen public headers' \
-X arch-arm -X arch-riscv -X arch-x86_32 -X arch-x86_64 \
-X xen-arm -X xen-riscv -X xen-x86_32 -X xen-x86_64 \
-X arch-x86 \
/local/xen.git/docs/../xen include/public include/xen/errno.h
include/public/memory.h:63: multiple definitions of Typedef xen_ulong_t: include/public/arch-ppc.h:55
include/public/memory.h:63: multiple definitions of Typedef xen_ulong_t: include/public/arch-ppc.h:61
include/public/memory.h:63: multiple definitions of Typedef xen_ulong_t: include/public/arch-ppc.h:61
include/public/memory.h:63: multiple definitions of Typedef xen_ulong_t: include/public/arch-ppc.h:55
Drop the second typedef. Finally, annotate the #endif so it's clear
what it refers to.
Fixes: 08c192cc11 ("xen/ppc: Add public/arch-ppc.h")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Shawn Anastasio <sanastasio@raptorengineering.com>
* Strip trailing whitspace.
* Remove PRIperfc. It has never been used and isn't useful.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
This is mostly for the removal of xen/lib.h and xen/smp.h from perfc.h. All
that is needed is xen/macros.h.
Trim and sort the includes for perfc.c too. There's no need for smp.h,
keyhandler.h or mm.h, but cpumask.h is needed.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
... and hook it up for RISC-V and PPC.
On RISC-V at least, no combination of headers pulls in errno.h, so include it
explicitly.
Guard the hypercalls array declaration based on NR_hypercalls existing. This
is sufficient to get PERF_COUNTERS fully working on RISC-V and PPC, so drop
the randconfig override.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Oleksii Kurohcko <oleksii.kurochko@gmail.com>
These were only ever used by the IA64 port, which was droped in commit
570c311ca2 ("remove ia64") in 2012.
Remove them, and clean up the arm/x86 stub headers.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Clang, of at least verion 17 complains:
arch/x86/pv/hypercall.c:30:10: error: variable 'eax' is used uninitialized
whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
30 | if ( !compat )
| ^~~~~~~
arch/x86/pv/hypercall.c:87:29: note: uninitialized use occurs here
87 | perfc_incra(hypercalls, eax);
| ^~~
This function is forced always_inline to cause compat to be
constant-propagated through, but that is only a heuristic to try and get the
compiler to do what we want, not a gurantee that it does.
Clang doesn't appear to be able to see that the only case where compat is
true (and therefore the if() is false) is when there's an else clause on the
end which sets eax too.
Initialise eax to -1, which ought to be optimised out, but if for whatever
reason it happens not to be, then perfc_incra() will fail it's bounds check
and do nothing.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
AMD have always used the architectural MSRs for LER. As the first processor
to support LER was the K7 (which was 32bit), we can assume it's presence
unconditionally in 64bit mode.
Intel are about to run out of space in Family 6 and start using 19. It is
only the Pentium 4 which uses non-architectural LER MSRs.
percpu_traps_init(), which runs on every CPU, contains a lot of code which
should be init-only, and is the only reason why opt_ler can't be in initdata.
Write a brand new init_ler() which expects all future Intel and AMD CPUs to
continue using the architectural MSRs, and does all setup together. Call it
from trap_init(), and remove the setup logic percpu_traps_init() except for
the single path configuring MSR_IA32_DEBUGCTLMSR.
Leave behind a warning if the user asked for LER and Xen couldn't enable it.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
AMD have updated the SRSO whitepaper[1] with further information. These
features exist on AMD Zen5 CPUs and are necessary for Xen to use.
The two features are in principle unrelated:
* SRSO_U/S_NO is an enumeration saying that SRSO attacks can't cross the
User(CPL3) / Supervisor(CPL<3) boundary. i.e. Xen don't need to use
IBPB-on-entry for PV64. PV32 guests are explicitly unsupported for
speculative issues, and excluded from consideration for simplicity.
* SRSO_MSR_FIX is an enumeration identifying that the BP_SPEC_REDUCE bit is
available in MSR_BP_CFG. When set, SRSO attacks can't cross the host/guest
boundary. i.e. Xen don't need to use IBPB-on-entry for HVM.
Extend ibpb_calculations() to account for these when calculating
opt_ibpb_entry_{pv,hvm} defaults. Add a `bp-spec-reduce=<bool>` option to
control the use of BP_SPEC_REDUCE, with it active by default.
Because MSR_BP_CFG is core-scoped with a race condition updating it, repurpose
amd_check_erratum_1485() into amd_check_bp_cfg() and calculate all updates at
once.
Xen also needs to to advertise SRSO_U/S_NO to guests to allow the guest kernel
to skip SRSO mitigations too:
* This is trivial for HVM guests. It is also is accurate for PV32 guests
too, but we have already excluded them from consideration, and do so again
here to simplify the policy logic.
* As written, SRSO_U/S_NO does not help for the PV64 user->kernel boundary.
However, after discussing with AMD, an implementation detail of having
BP_SPEC_REDUCE active causes the PV64 user->kernel boundary to have the
property described by SRSO_U/S_NO, so we can advertise SRSO_U/S_NO to
guests when the BP_SPEC_REDUCE precondition is met.
Finally, fix a typo in the SRSO_NO's comment.
[1] https://www.amd.com/content/dam/amd/en/documents/corporate/cr/speculative-return-stack-overflow-whitepaper.pdf
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
... which converts binary content to hex automatically.
Update libxl to match. No API/ABI change.
This removes a latent libxl bug for cases when the buildid is longer than 4092
bytes.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
... which uses XENVER_extraversion2.
In order to do this sensibly, use manual hypercall buffer handling. Not only
does this avoid an extra bounce buffer (we need to strip the xen_varbuf_t
header anyway), it's also shorter and easlier to follow.
Update libxl and the ocaml stubs to match. No API/ABI change in either.
With this change made, `xl info` can now correctly access a >15 char
extraversion:
# xl info xen_version
4.18-unstable+REALLY LONG EXTRAVERSION
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
Acked-by: Christian Lindig <christian.lindig@citrix.com>
kexec-tools uses xc_version(), meaning that it is not a private API. As we're
going to extend the functionality substantially, move it to its own file.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Anthony PERARD <anthony.perard@citrix.com>
struct xen_build_id and struct xen_varbuf are identical from an ABI point of
view, so XENVER_build_id can reuse xenver_varbuf_op() rather than having it's
own almost identical copy of the logic.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
In XenServer, we have encountered problems caused by both XENVER_extraversion
and XENVER_commandline having fixed bounds.
More than just the invariant size, the APIs/ABIs also broken by typedef-ing an
array, and using an unqualified 'char' which has implementation-specific
signed-ness.
Provide brand new ops, which are capable of expressing variable length
strings, and mark the older ops as broken.
This fixes all issues around XENVER_extraversion being longer than 15 chars.
Further work beyond just this API is needed to remove other assumptions about
XENVER_commandline being 1023 chars long.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jason Andryuk <jandryuk@gmail.com>
---
Non-technical objections to this patch were raised, and subsequently rejected
by a community wide vote. The results of the vote have not been shared with
the community at the time of committing.
The arch_get_xen_caps() infrastructure is horribly inefficient for something
that is constant after features have been resolved on boot.
Every instance used snprintf() to format constants into a string (which gets
shorter when %d gets resolved!), and which get double buffered on the stack.
Switch to using string literals with the "3.0" inserted - these numbers
haven't changed in 19 years; the Xen 3.0 release was Dec 5th 2005.
Use initcalls to format the data into xen_cap_info, which is deliberately not
of type xen_capabilities_info_t because a 1k array is a silly overhead for
storing a maximum of 77 chars (the x86 version) and isn't liable to need any
more space in the forseeable future. RISC-V and PPC have their stub dropped,
with the expectation that they won't carry this legacy interface forward.
This speeds up the the XENVER_capabilities hypercall, but the purpose of the
change is to allow us to introduce a better XENVER_* API that doesn't force
the use of a 1k buffer on the stack.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
Decompressing the kernel is necessary to inspect the ELF notes, but the
dombuilder will gunzip() secondary modules too. Specifically gunzip(), no
other decompression algorithms.
This may have been necessary in the dim and distant past, but it is broken
today. Linux specifically supports concatenating CPIO fragments of differing
compressions, and any attempt to interpret it with a single algorithm may
corrupt later parts.
This was an unexpected discovery while trying to test Xen's gunzip()
logic (Xen as a PVH guest, with a gzipped XTF kernel as dom0).
Interpreting secondary modules should be left as an exercise to the guest.
This reduces work done in dom0.
This is not expected to cause a practical difference to guests these days.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Credit2 has no buisness including asm/cpufeature.h or asm/processor.h.
This was caused by a bad original abstraction, and an even less wise attempt
to fix the build on my behalf. It is also the sole reason why PPC and RISC-V
need cpufeature.h header.
Worst of all, cpu_data[cpu].x86_num_siblings doesn't even have the same
meaning between vendors on x86 CPUS.
Implement cpu_nr_siblings() locally in credit2.c, leaving behind a TODO. Drop
the stub from each architecture.
Fixes: 8e2aa76dc1 ("xen: credit2: limit the max number of CPUs in a runqueue")
Fixes: ad33a573c0 ("xen/credit2: Fix build following c/s 8e2aa76dc (take 2)")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Shawn Anastasio <sanastasio@raptorengineering.com>
... as a shorthand for setting both suboptions at once. Currently, an admin
needs to pass cet=no-shstk,no-ibt to turn both off, where cet=0 is a better
option.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Updating the Eclair runner has had knock-on effects with previously-clean
rules now flagging violations:
- x86: Rule 1.1, 1940 violations
- ARM64: Rule 1.1, 725 violations, Rule 2.1, 255 violations
Fixes: 631f535a3d ("xen: update ECLAIR service identifiers from MC3R1 to MC3A2.")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
The Linux rethunk and safe-ret speculative safety techniques involve
transforming `ret` to `jmp __x86_return_thunk` at compile time. Placing naked
`ret`s back in executable .text breaks these mitigations.
CET-IBT requires ENDBR instructions, and while we could in principle fix that,
the need to select between ENDBR32 or ENDBR64 means that the contents of the
hypercall page would need to become more mode-specific than it currently
is (HVM hypercall pages are currently 32bit and 64bit compatbile). However,
there's no feasible way to make a hypercall page compatible with fine-grain
CFI schemes such as FineIBT.
OSes which care about either of these things are better off avoiding the
hypercall page.
This is part of XSA-466.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This script is not referenced by the build system, and has escaped all Python
compatibility work; it's still using print statements.
Also, the regex it uses ties it to a 32bit build of Xen, which was dropped in
Xen 4.3, 11 years ago.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
xc_cpuid_apply_policy() provides compatibility for migration of a pre-4.14 VM
where no CPUID data was provided in the stream.
It guesses the various max-leaf limits, based on what was true at the time of
writing, but this was not correctly adapted when speculative security issues
forced the advertisement of new feature bits. Of note are:
* LFENCE-DISPATCH, in leaf 0x80000021.eax
* BHI-CTRL, in leaf 0x7[2].edx
In both cases, a VM booted on a security-patched Xen 4.13, and then migrated
on to any newer version of Xen on the same or compatible hardware would have
these features stripped back because Xen is still editing the cpu-policy for
sanity behind the back of the toolstack.
For VMs using BHI_DIS_S to mitigate Native-BHI, this resulted in a failure to
restore the guests MSR_SPEC_CTRL setting:
(XEN) HVM d7v0 load MSR 0x48 with value 0x401 failed
(XEN) HVM7 restore: failed to load entry 20/0 rc -6
Fixes: e9b4fe2636 ("x86/cpuid: support LFENCE always serialising CPUID bit")
Fixes: f3709b15fc ("x86/cpuid: Infrastructure for cpuid word 7:2.edx")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This seems to have been introduced in commit f8beb54e24 ("Disable PIE/SSP
features when building Xen, if GCC supports them.") in 2004.
However, neither GCC nor Clang appear to have ever supported taking the
negated form of -fstack-protector-all, meaning this been useless since its
introduction.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Sphinx 5.0 and newer objects to language = None. Switch to 'en'.
Also update the copyright year. Use %Y to avoid this problem in the future,
and provide compatibility for versions of Sphinx prior to 8.1 which don't
support the syntax.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Sphinx complains:
docs/fusa/index.rst:6: WARNING: toctree contains reference to nonexisting document 'fusa/reqs'
docs/fusa/reqs/index.rst:6: WARNING: toctree contains reference to nonexisting document 'fusa/reqs/market-reqs'
docs/fusa/reqs/index.rst:6: WARNING: toctree contains reference to nonexisting document 'fusa/reqs/product-reqs'
docs/fusa/reqs/index.rst:6: WARNING: toctree contains reference to nonexisting document 'fusa/reqs/design-reqs/arm64'
docs/fusa/index.rst: WARNING: document isn't included in any toctree
docs/fusa/reqs/design-reqs/arm64/generic-timer.rst: WARNING: document isn't included in any toctree
docs/fusa/reqs/design-reqs/arm64/sbsa-uart.rst: WARNING: document isn't included in any toctree
docs/fusa/reqs/index.rst: WARNING: document isn't included in any toctree
docs/fusa/reqs/market-reqs/reqs.rst: WARNING: document isn't included in any toctree
docs/fusa/reqs/product-reqs/arm64/reqs.rst: WARNING: document isn't included in any toctree
Fix the toctrees.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Ayan Kumar Halder <ayan.kumar.halder@amd.com>
for_each_set_bit()'s use of a double for loop had an accidental bug with a
break in the inner loop leading to an infinite outer loop.
Adjust for_each_set_bit() to avoid this behaviour, and add extend
test_for_each_set_bit() with a test case for this.
Fixes: ed26376f20 ("xen/bitops: Introduce for_each_set_bit()")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
There's no point rescanning if we didn't load something new. Take the
opportunity to make the comment a bit more concise.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Following commit cd7cc5320b ("x86/boot: add start and size fields to struct
boot_module"), bootstrap_map*() works as soon as boot_info is populated.
Resolve the TODO, and move microcode loading to be the eariest action after
establishing a console.
A sample boot now looks like:
(XEN) Xen version 4.20-unstable (andrew@eng.citrite.net) (gcc (Debian 12.2.0-14) 12.2.0) debug=y Tue Nov 19 21:44:46 GMT 2024
(XEN) Latest ChangeSet: Wed Dec 6 21:54:55 2023 git:1ab612848a23
(XEN) build-id: 52fe616d1b3a2a2cb44775815507d02cca73315d
(XEN) CPU Vendor: AMD, Family 25 (0x19), Model 1 (0x1), Stepping 1 (raw 00a00f11)
(XEN) BSP microcode revision: 0x0a001137
(XEN) microcode: CPU0 updated from revision 0xa001137 to 0xa0011d5, date = 2024-02-23
(XEN) Bootloader: GRUB 2.06
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Eclair reports a Misra Rule 8.4 violation; that do_mca() can't see it's
declaration. It turns out that this is a consequence of do_mca() being
PV-only, and the declaration being compiled out in !PV builds.
Therefore, arrange for do_mca() to be compiled out in !PV builds. This in
turn requires a number of static functions to become __maybe_unused.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Eclair complains that neither reloc_trampoline{32,64}() can see their
declarations.
reloc_trampoline32() needs to become asmlinkage, while reloc_trampoline64()
needs declaring properly in a way that both efi-boot.h and reloc-trampoline.c
can see.
Introduce boot-helpers.h for the purpose.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Eclair complains that fw_unreserved_regions() can't see it's declaration.
Include <asm/setup.h> to address this.
This makes Misra Rule 8.4 clean on ARM, so tag it as such.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Eclair reports a violation of MISRA Rule 5.3.
get_stub() has a local ptr variable which genuinely shadows x86_emul_rmw()'s
parameter of the same name. The logic is correct, so the easiest fix is to
rename one of variables.
With this addressed, Rule 5.3 is clean, so mark it as such.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
All uses of MIS_UCODE, have been removed, leaving only a simple ordering
relation, and microcode_match_result being a stale name.
Drop the enum entirely, and use a simple int -1/0/1 scheme like other standard
ordering primitives in C.
Swap the order or parameters to compare_patch(), to reduce cognitive
complexity; all other logic operates the other way around. Rename the hook to
simply compare().
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
microcode_update_cache() now has a single caller, but inlining it shows how
unnecessarily complicated the logic really is.
Outside of error paths, there is always one microcode patch to free. Its
either result of parse_blob(), or it's the old cached value.
In order to fix this, have a local patch pointer (mostly to avoid the
unnecessary verbosity of patch_with_flags.patch), and always free it at the
end. The only error path needing care is the IS_ERR(patch) path, which is
easy enough to handle.
Also, widen the scope of result. We only need to call compare_patch() once,
and the answer is still good later when updating the cache. In order to
update the cache, simply SWAP() the patch and the cache pointers, allowing the
singular xfree() at the end to cover both cases.
This also removes all callers microcode_free_patch() which fixes the need to
cast away const to allow it to compile. This also removed several violations
of MISRA Rule 11.8 which disallows casting away const.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
With the tangle of logic starting to come under control, it is now plain to
see that parse_blob()'s side effect of re-gathering the signature/revision is
pointless.
The signature is invariant for the lifetime of Xen, and the revision is kept
suitably up to date in apply_microcode(). The BSP gathers this in
early_microcode_init(), and the APs and S3 in microcode_update_one().
Therefore, there is no need for parse_blob() to discard a good copy of the
data and re-gather it.
This finally gets us down to a single call per CPU on boot / S3 resume, and no
calls during late-load hypercalls.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Right now, the non-compat declaration and definition of do_multicall()
differing types for the nr_calls parameter.
This is a MISRA rule 8.3 violation, but it's also time-bomb waiting for the
first 128bit architecture (RISC-V looks as if it might get there first).
Worse, the type chosen here has a side effect of truncating the guest
parameter, because Xen still doesn't have a clean hypercall ABI definition.
Switch uniformly to using unsigned long.
This addresses the MISRA violation, and while it is a guest-visible ABI
change, it's only in the corner case where the guest kernel passed a
bogus-but-correct-when-truncated value. I can't find any any users of
mutilcall which pass a bad size to begin with, so this should have no
practical effect on guests.
In fact, this brings the behaviour of multicalls more in line with the header
description of how it behaves.
With this fix, Xen is now fully clean to Rule 8.3, so mark it so.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The logic is far more sane to follow with a total size, and the position of
the end of the heap. Remove or fix the remaining descriptions of how the
trampoline is laid out.
Move the relevant constants into trampoline.h, which requires making the
header safe to include in assembly files.
No functional change. The compiled binary is identical.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This is, to the best of my knowledge, accurate. I am providing no comment on
how sane I believe it to be.
At the time of writing, the sizes of the regions are:
offset size
AP: 0x0000 0x00b0
S3: 0x00b0 0x0229
Boot: 0x02d9 0x1697
Heap: 0x1970 0xe690
Stack: 0xf000 0x1000
and wakeup_stack overlays boot_edd_info.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
By checking that the permanent trampoline fits within 1k (at the time of
writing, it's 0x229 bytes), we can simplify the wakeup_stack handling.
Move the setup into wakeup.S, because it's rather out of place in
trampoline.S, and change it to a local symbol.
Drop wakeup_stack_start and WAKEUP_STACK_MIN entirely.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The work to address Rule 5.3 introduced violations of Rule 8.3 by failing to
rename the parameters in the declaration.
Fixes: b5fd405aa3 ("x86/e820 address violations of MISRA C:2012 Rule 5.3")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
This resolves 4 Misra violations of Rule 8.4 caused by the function
definitions not being able to see their declarations.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
This function is overloaded, creating complexity; 3 of 4 callers already only
want it for it's "applicable to this CPU or not" answer, and handle revision
calculations separately.
Change it to be microcode_fits_cpu(), returning a simple boolean.
Notably, this removes a path where cpu_request_microcode() inspects
currently-loaded microcode revision, just to discard the answer.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This function is overloaded, creating complexity; 3 of 4 callers already only
want it for it's "applicable to this CPU or not" answer, and handle revision
calculations separately.
Change it to be microcode_fits_cpu(), returning a simple boolean. The
checking of the equiv table can be simplified substantially too; A mapping
will only be inserted if it's correct for the CPU, so any nonzero equiv.sig
suffices to know that equiv.id is correct.
Drop compare_header() too, which is simiarly overloaded, and use
compare_revisions() directly.
Notably, this removes a path where cpu_request_microcode() inspects
currently-loaded microcode revision, just to discard the answer.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Fold microcode_update_cpu() into its single remaining caller. Explain why we
bother grabbing the microcode revision even if we can't load microcode.
This avoids a double collect_cpu_info() call on each AP.
Furthermore, delete the -EIO path.
A hard error updating a single CPU's microcode on AP bringup or S3 resume is
definitely bad (needing special investigation), but freeing the cache is about
the worst possible action we can take in response; it prevents subsequent APs
from taking an update they might have accepted.
While we expect a homogeneous system with respect to microcode applicability,
this doesn't mean that all cores behave identically when given the same blob.
e.g. one failure mode seen in practice is a checksum failure caused by a bad
SRAM cell.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
There are two callers of microcode_update_cpu(), and because one passes NULL
and one doesn't, there are effectively two disjoint pieces of logic wrapped in
a single function.
early_microcode_load()'s use skips all the microcode_cache handling, and is
just a simple patch application.
Use ucode_ops.apply_microcode() directly, and remove the non-cache path from
microcode_update_cpu(). This skips a redundant collect_cpu_info()
call (performed in early_microcode_init(), marginally earlier), and avoids
holding microcode_mutex when we're not interacting with microcode_cache at
all.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
As now documented, this variable holds a page aligned value less than 1M.
However, head.S fills it using 4-byte stores, and reloc_trampoline() is
compiled for both 32bit and 64bit, where unsigned long is a different size.
This happens to work because of the range of the value, but switch to uint32_t
to make it explicit.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
bogus_saved_magic() is assembled in a .code64 section but invoked in 32bit
mode. Moving it causes a real encoding difference.
Before:
66 c7 04 25 14 80 0b 00 53 0e movw $0xe53,0xb8014(,%eiz,1)
After:
66 c7 05 14 80 0b 00 53 0e movw $0xe53,0xb8014
The difference happens to be benign, but move the logic back into a .code32
for sanity sake. Annotate it with ELF metadata while doing so.
Fixes: d8c8fef090 ("Provide basic Xen PM infrastructure")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This header is included in exactly one location. Fold it into mpspec.h
With this done, mach-default/ is empty, so remove the include path.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
It has a single function, and a single user. This is unlikely to change
moving forwards so fold it into mpparse.c.
Update it to use an explicit uint16_t cast, rather than assuming the width of
short.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
irq_vectors.h is included by with multiple paths. Move it to be a regular
header instead.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
All useful content has been moved elsewhere.
enable_apic_mode() and multi_timer_check() are empty stubs. Remove their sole
callers and drop them.
apicid_to_node() and bios_cpu_apicid[] are entirely unused.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
It's an unnecessary wrapper, and longer than the operation it wraps.
It's also the only reason that mpparse.c includes mach_apic.h, other than for
transitive dependencies.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
It's an unnecessary wrapper.
It's also the only reason that smpboot.c includes mach_apic.h, other than for
transitive dependencies.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
It's an unnecessary wrapper, and is longer than the operation it wraps.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
... as the implementations are in genapic/probe.c
This covers the only functions that both setup.c and boot.c were including
mach_apic.h for, although setup.c was depending on io_apic.h transitively too.
The happens to address two MISRA Rule 8.4 violations, as probe.c couldn't
previously see the declarations.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
A number of files don't need mach_apic.h at all, or only need transitive
dependences. Drop the includes.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Neither iommu.c nor quirks.c use any functionality. iommu.c only uses it to
transitively include apic.h and io_apic.h, while quirks.c is only depending on
the ACLINUX wrapping of strtoul() which we spell simple_strtoul() everywhere
else in Xen.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Work to rebase the Trenchboot patch series has encountered:
In file included from ./arch/x86/include/asm/tpm.h:4,
from arch/x86/boot/../tpm.c:23:
./arch/x86/include/asm/bootinfo.h:88:35: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'next_boot_module_index'
88 | static inline unsigned int __init next_boot_module_index(
|
Fix this by including the necessary header.
Fixes: 74af2d9827 ("x86/boot: eliminate module_map")
Reported-by: Krystian Hebel <krystian.hebel@3mdeb.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
The data pointer is known good, so the -ENOMEM path is unnecessary. However,
if we find no patch, something's wrong seeing as early_microcode_init()
indicated it was happy.
We are the entity initialising the cache, so we don't need the complexity of
using microcode_update_cache(). Just assert the cache is empty, and populate
it.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Both are used to pass information from early_microcode_load() to
microcode_init_cache(), and both constitute use-after-free's in certain cases.
Later still in microcode_init() is a failed attempt to "free" this
information, long after the damage has been done.
* ucode_mod is a copy of the module_t identified by `ucode=$n`. It is a copy
of the physical pointer from prior to Xen relocating the modules.
microcode_init_cache() might happen to find the data still intact in it's
old location, but it might be an arbitrary part of some other module.
* ucode_blob is a stashed virtual pointer to a bootstrap_map() which becomes
invalid the moment control returns to __start_xen(), where other logic is
free to to make temporary mappings. This was even noticed and
microcode_init_cache() adjusted to "fix" the stashed pointers.
The information which should be passed between these two functions is which
module to look in. Introduce early_mod_idx for this purpose. opt_scan can be
reused to distinguish between CPIO archives and raw containers.
Notably this means microcode_init_cache() doesn't need to scan all modules any
more; we know exactly which one to look in. However, we do re-parse the CPIO
header for simplicity.
Furthermore, microcode_init_cache(), being a presmp_initcall does not need to
use bootstrap_map() to access modules; it can use mfn_to_virt() directly,
which avoids needing to funnel exit paths through bootstrap_unmap().
Fold microcode_scan_module() into what is now it's single caller. It operates
on the function-wide idx/data/size state which allows for a unified "found"
path irrespective of module selection method.
Delete microcode_init() entirely. It was never legitimate logic.
This resolves all issues to do with holding pointers (physical or virtual)
across returning to __start_xen().
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
If bootstrap_map() has provided a mapping, we must free it when done. Failing
to do so may cause a spurious failure for unrelated logic later.
Inserting a bootstrap_unmap() here does not break the use of ucode_{blob,mod}
any more than they already are.
Add a printk noting when we didn't find a microcode patch. It's at debug
level, because this is the expected case on AMD Client systems, and SDPs, but
for people trying to figure out why microcode loading isn't work, it might be
helpful.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
The work to add the `ucode=nmi` cmdline option left a subtle corner case.
Both scan and an explicit index could be chosen, and we could really find both
a CPIO archive and a microcode file.
Worse, because the if/else chains for processing ucode_{blob,mod} are opposite
ways around in early_microcode_load() and microcode_init_cache(), we can
genuinely perform early microcode loading from the CPIO archive, then cache
from the explicit file.
Therefore, enforce that only one selection method can be active.
Rename ucode_{scan,mod_idx} to have an opt_ prefix. This is both for
consistency with the rest of Xen, and to guarantee that we've got all
instances of the variables covered in this change. Explain how they're use.
During cmdline/config parsing, always update both variables in pairs.
In early_microcode_load(), ASSERT() the invariant just in case. Use a local
variable for idx rather than operating on the static; we're the only consume
of the value.
Expand the index selection logic with comments and warnings to the user when
something went wrong.
Fixes: 5ed12565aa ("microcode: rendezvous CPUs in NMI handler and load ucode")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
We've got a perfectly good vendor abstraction already for microcode. No need
for a second ad-hoc one in microcode_scan_module().
This is in preparation to use ucode_ops.cpio_path in multiple places.
These paths are only used during __init, so take the opportunity to move them
into __initconst.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Diff-wise, as early_microcode_update_cpu() is the larger function, this more
closely resembles "merge early_microcode_load() into it's single callee", but
the end result is the same.
At the same time, rename the len variable to size. This is for better
consistency with existing logic, and to reduce churn later.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
microcode_grab_module() and early_microcode_update_cpu() are logically one
task that passes state via static variables.
We intend to delete said static variables; start by breaking these functions
out of early_microcode_init().
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
There's no need for microcode_init_cache() to be called exactly where it is in
__start_xen(). All that matters is it must be after xmalloc() is available
and before APs start up.
As a consequence, microcode_init_cache() runs a little later on boot now.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
A recent cppcheck run was found to fail:
https://gitlab.com/xen-project/people/andyhhp/xen/-/jobs/8237167472
with:
"type mismatch! call is<type>() before get<type>()" && is<std::string>()
make[3]: *** [arch/x86/boot/Makefile:28: arch/x86/boot/reloc-trampoline.32.o] Error 1
This turns out to be a parallel build issue, combined with a recent change to
x86. Notably, we now have a case where we build both:
CC arch/x86/boot/reloc-trampoline.32.o
CC arch/x86/boot/reloc-trampoline.o
from the same original C file, and cppcheck uses the source C file as the key
for generating it's intermediate files.
Switch cppcheck to use the object file as the unique key instead.
Fixes: 45bfff6511 ("xen/misra: xen-analysis.py: fix parallel analysis Cppcheck errors")
Fixes: db8acf31f9 ("x86/boot: Reuse code to relocate trampoline")
Suggested-by: Luca Fancellu <luca.fancellu@arm.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
Tested-by: Luca Fancellu <luca.fancellu@arm.com>
Andrew Cooper