Assumption stated in the comments as if fdt_for_each_subnode() checks
for parent < 0 is utterly wrong. If parent is < 0, node offset is set to
0 (i.e. the very first node in the tree) and the loop's body is executed.
This incorrect assumption causes overlay_node_count() to also count nodes
that do not have __overlay__ as their subnode. The same story goes for
overlay_get_nodes_info(), where we end up requiring each node directly
under root node to have "target-path" set. DTBOs can specify other nodes
including special ones like __symbols__, __fixups__ that can be left to
reduce the number of steps a user needs to do to when it comes to invalid
phandles.
Fix it by adding checks if overlay < 0 after respective calls to
fdt_subnode_offset().
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Even though in most cases device nodes are not present directly under
the root node, it's a perfectly valid configuration (e.g. Qemu virt
machine dtb). At the moment, we don't handle this scenario which leads
to unconditional addition of extra leading '/' in the node full path.
This makes the attempt to add such device overlay to fail.
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The assumption stated in the comment that the code will never get there
is incorrect. In overlay_get_nodes_info() we manually combine path from
target-path property with the node path by adding '/' as a separator.
This can differ from a path obtained by libfdt due to more advanced
logic used there which can for instance get rid of excessive slashes.
In case of incorrect target-path (e.g. target-path = "//axi"), the
comparison in dt_find_node_by_path_from() can fail triggering the assert
in debug builds.
Fixes: 0c0facdab6 ("xen/arm: Implement device tree node addition functionalities")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Julien Grall <jgrall@amazon.com>
With CONFIG_OVERLAY_DTB=y, unflatten_dt_alloc() is used as part of
unflatten_dt_node() used during runtime. In case of a binary compiled
such as unflatten_dt_alloc() does not get inlined (e.g. using -Og),
attempt to add an overlay to Xen (xl dt-overlay add) results in a crash.
(XEN) Instruction Abort Trap. Syndrome=0x7
(XEN) Walking Hypervisor VA 0xa00002c8cc0 on CPU2 via TTBR 0x0000000040340000
(XEN) 0TH[0x014] = 0x4033ff7f
(XEN) 1ST[0x000] = 0x4033ef7f
(XEN) 2ND[0x001] = 0x4000004033af7f
(XEN) 3RD[0x0c8] = 0x0
(XEN) CPU2: Unexpected Trap: Instruction Abort
(XEN) ----[ Xen-4.20-unstable arm64 debug=y Not tainted ]----
...
(XEN) Xen call trace:
(XEN) [<00000a00002c8cc0>] 00000a00002c8cc0 (PC)
(XEN) [<00000a0000202410>] device-tree.c#unflatten_dt_node+0xd0/0x504 (LR)
(XEN) [<00000a0000204484>] unflatten_device_tree+0x54/0x1a0
(XEN) [<00000a000020800c>] dt-overlay.c#handle_add_overlay_nodes+0x290/0x3d4
(XEN) [<00000a0000208360>] dt_overlay_sysctl+0x8c/0x110
(XEN) [<00000a000027714c>] arch_do_sysctl+0x1c/0x2c
Fixes: 9e9d2c079d ("xen/arm/device: Remove __init from function type")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Attempt to attach an overlay (xl dt-overlay attach) to a domain without
first adding this overlay to Xen (xl dt-overlay add) results in an
overlay track entry being NULL in handle_attach_overlay_nodes(). This
leads to NULL pointer dereference and the following data abort crash:
(XEN) Cannot find any matching tracker with input dtbo. Operation is supported only for prior added dtbo.
(XEN) Data Abort Trap. Syndrome=0x5
(XEN) Walking Hypervisor VA 0x40 on CPU0 via TTBR 0x0000000046948000
(XEN) 0TH[0x000] = 0x46940f7f
(XEN) 1ST[0x000] = 0x0
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.20-unstable arm64 debug=y Not tainted ]----
...
(XEN) Xen call trace:
(XEN) [<00000a0000208b30>] dt_overlay_domctl+0x304/0x370 (PC)
(XEN) [<00000a0000208b30>] dt_overlay_domctl+0x304/0x370 (LR)
(XEN) [<00000a0000274b7c>] arch_do_domctl+0x48/0x328
Fixes: 4c733873b5 ("xen/arm: Add XEN_DOMCTL_dt_overlay and device attachment to domains")
Signed-off-by: Michal Orzel <michal.orzel@amd.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Avoid using BOOTMOD_XEN region for other purposes or boot modules
which could result in memory corruption or undefined behaviour.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Use virt_to_maddr() instead of LINK_TO_LOAD as virt_to_maddr()
covers all the cases where LINK_TO_LOAD() is used.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Implement the virt_to_maddr() function to convert virtual addresses
to machine addresses. The function includes checks for valid address
ranges, specifically the direct mapping region (DIRECTMAP_VIRT_START)
and the Xen's Linkage (XEN_VIRT_START) region. If the virtual address
falls outside of these regions, an assertion will trigger.
To implement this, the phys_offset variable is made accessible
outside of riscv/mm.c.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
It's not unnecessary (as the earlier commit claimed): The integrated
assembler of Clang up to 11 complains about an "invalid operand for
instruction".
Fixes: b42cf31d11 ("x86: use alternative_input() in cache_flush()")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
While doing this, factor out checks common to architectural and hidden
state.
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
... and move the type itself to linux-compat.h. An exception being
arch/arm/arm64/cpufeature.c and arch/arm/include/asm/arm64/cpufeature.h,
which are to use linux-compat.h instead (the former by including the
latter).
While doing so
- correct the type of union uu's uq field in lib/divmod.c,
- switch a few adjacent types as well, for (a little bit of)
consistency.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Acked-by: Julien Grall <jgrall@amazon.com>
Some error paths in the MSR state loading logic don't contain error messages,
which makes debugging them quite hard without adding extra patches to print the
information.
Add two new log messages to the MSR state load path that print information
about the entry that failed to load, for both PV and HVM.
While there also adjust XEN_DOMCTL_set_vcpu_msrs to return -ENXIO in case the
MSR is unhandled or can't be loaded, so it matches the error code used by HVM
MSR loading (and it's less ambiguous than -EINVAL).
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
External interrupts via logical delivery mode in xAPIC do not benefit
from targeting multiple CPUs and instead simply bloat up the vector
space.
However the xAPIC flat driver currently uses logical delivery for
external interrupts.
This patch switches the xAPIC flat driver to use physical destination
mode for external interrupts, instead of logical destination mode.
This patch also applies the following non-functional changes:
- Remove now unused logical flat functions
- Expand GENAPIC_FLAT and GENAPIC_PHYS macros, and delete them.
Resolves: https://gitlab.com/xen-project/xen/-/issues/194
Signed-off-by: Matthew Barnes <matthew.barnes@cloud.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Since the addition of the MSR_AMD64_DR{1-4}_ADDRESS_MASK MSRs to the
msrs_to_send array, the calculations for the maximum number of MSRs that
the hypercall can handle is off by 4.
Remove the addition of 4 to the maximum number of MSRs that
XEN_DOMCTL_{set,get}_vcpu_msrs supports, as those are already part of the
array.
A further adjustment could be to subtract 4 from the maximum size if the DBEXT
CPUID feature is not exposed to the guest, but guest_{rd,wr}msr() will already
perform that check when fetching or loading the MSRs. The maximum array is
used to indicate the caller of the buffer it needs to allocate in the get case,
and as an early input sanitation in the set case, using a buffer size slightly
lager than required is not an issue.
Fixes: 86d47adcd3 ('x86/msr: Handle MSR_AMD64_DR{0-3}_ADDRESS_MASK in the new MSR infrastructure')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Refactor the code to avoid an implicit fallthrough and address
a violation of MISRA C:2012 Rule 16.3: "An unconditional `break'
statement shall terminate every switch-clause".
No functional change.
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Add defensive code after unreachable program points.
This also meets the requirements to deviate violations of MISRA C:2012
Rule 16.3: "An unconditional `break' statement shall terminate every
switch-clause".
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Returning a literal number is a bad idea anyway when all other returns
use IOREQ_STATUS_* values. The function is dead on Arm, and mapping to
X86EMUL_OKAY is surely wrong on x86.
Fixes: f6bf39f84f ("x86/hvm: add support for broadcast of buffered ioreqs...")
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Julien Grall <jgrall@amazon.com>
Strip all related content out of the root .gitignore, and provide a
more local .gitignore's with up-to-date patterns.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
The header already uses standard types for many fields, extend
their usage.
No functional change.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
The current dpci logic relies on a softirq being executed as a side effect of
the cpu_notifier_call_chain() call in the code path that offlines the target
CPU. However the call to cpu_notifier_call_chain() won't trigger any softirq
processing, and even if it did, such processing should be done after all
interrupts have been migrated off the current CPU, otherwise new pending dpci
interrupts could still appear.
Currently the ASSERT() in the cpu callback notifier is fairly easy to trigger
by doing CPU offline from a PVH dom0.
Solve this by instead moving out any dpci interrupts pending processing once
the CPU is dead. This might introduce more latency than attempting to drain
before the CPU is put offline, but it's less complex, and CPU online/offline is
not a common action. Any extra introduced latency should be tolerable.
Fixes: f6dd295381 ('dpci: replace tasklet with softirq')
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Use agreed syntax for pseudo-keyword fallthrough to meet the
requirements to deviate a violation of MISRA C:2012 Rule 16.3:
"An unconditional `break' statement shall terminate every
switch-clause".
No functional change.
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
guest_handle_ok()'s expansion contains a sizeof() involving its
first argument guest_handle_cast().
The expansion of the latter, in turn, contains a variable
initialization.
Since MISRA considers the initialization (even of a local variable)
a side effect, the chain of expansions mentioned above violates
MISRA C:2012 Rule 13.6 (The operand of the `sizeof' operator shall not
contain any expression which has potential side effect).
Refactor the code to address the rule violation.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
guest_handle_ok()'s expansion contains a sizeof() involving its
first argument which is guest_handle_cast().
The expansion of the latter, in turn, contains a variable
initialization.
Since MISRA considers the initialization (even of a local variable)
a side effect, the chain of expansions mentioned above violates
MISRA C:2012 Rule 13.6 (The operand of the `sizeof' operator shall not
contain any expression which has potential side effect).
Refactor the code to address the rule violation.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
The FRED spec architecturalises the Event Type encoding, previously exposed
only in VMCB/VMCS fields.
Introduce the constants in x86-defns.h, making them a bit more concise, and
retire enum x86_event_type.
Take the opportunity to introduce X86_ET_OTHER. It's absence appears to be a
bug in Introspection's Monitor Trap Flag support, when considering VECTORING
events during another VMExit.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
There are only two remaining standalone uses the legacy ALIGN macro.
Drop these by switching the .incbin's over to using FUNC()/END() which has
alignment handled internally. While the incbin's aren't technically one
single function, they behave as if they are.
Finally, expand ALIGN inside the legacy ENTRY() macro in order to remove ALIGN
itself.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
__efi64_mb2_start() makes some bold assumptions about the efi_platform and
skip_realmode booleans. Set them to 1 explicitly, which is more robust.
Make the comment a little more concise.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Centralize per-cpu area management to reduce code duplication and
enhance maintainability across architectures.
The per-cpu area management code, which is largely common among
architectures, is moved to a shared implementation in
xen/common/percpu.c. This change includes:
* Remove percpu.c from the X86 and Arm architectures.
* For x86, define INVALID_PERCPU_AREAS and PARK_OFFLINE_CPUS_VAR.
* Drop the declaration of __per_cpu_offset[] from stubs.c in
PPC and RISC-V to facilitate the build of the common per-cpu code.
No functional changes for x86.
For Arm add support of CPU_RESUME_FAILED, CPU_REMOVE and freeing of
percpu in the case when system_state != SYS_STATE_suspend, however,
there is no change in behavior for Arm at this time.
Move the asm-generic/percpu.h definitions to xen/percpu.h, except for
__per_cpu_start[] and __per_cpu_data_end[], which are moved to
common/percpu.c as they are only used in common/percpu.c.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
... to boot code, limiting their scope and thus allowing to drop
respective #undef-s from the linker script.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Introduce function which allows to map FDT to Xen.
Also, initialization of device_tree_flattened happens using
early_fdt_map().
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Implement map_pages_to_xen() which requires several
functions to manage page tables and entries:
- pt_update()
- pt_mapping_level()
- pt_update_entry()
- pt_next_level()
- pt_check_entry()
To support these operations, add functions for creating,
mapping, and unmapping Xen tables:
- create_table()
- map_table()
- unmap_table()
Introduce PTE_SMALL to indicate that 4KB mapping is needed
and PTE_POPULATE.
In addition introduce flush_tlb_range_va() for TLB flushing across
CPUs after updating the PTE for the requested mapping.
Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
If available, its use is supposed to be cheaper than LFENCE+RDTSC, and
is virtually guaranteed to be cheaper than MFENCE+RDTSC.
Update commentary (and indentation) while there.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Ever since it's introduction in commit 013351bd7a ("Define new event-channel
and physdev hypercalls") in 2006, the public interface was named nr_ports
while the internal field was called iobmp_limit.
Rename the internal field to iobmp_nr to match the public interface, and
clarify that, when nonzero, Xen will read 2 bytes.
There isn't a perfect parallel with a real TSS, but iobmp_nr being 0 is the
paravirt "no IOPB" case, and it is important that no read occurs in this case.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The switch statement in guest_io_okay() is a very expensive way of
pre-initialising x with ~0, and performing a partial read into it.
However, the logic isn't correct either.
In a real TSS, the CPU always reads two bytes (like here), and any TSS limit
violation turns silently into no-access. But, in-limit accesses trigger #PF
as usual. AMD document this property explicitly, and while Intel don't (so
far as I can tell), they do behave consistently with AMD.
Switch from __copy_from_guest_offset() to __copy_from_guest_pv(), like
everything else in this file. This removes code generation setting up
copy_from_user_hvm() (in the likely path even), and safety LFENCEs from
evaluate_nospec().
Change the logic to raise #PF if __copy_from_guest_pv() fails, rather than
disallowing the IO port access. This brings the behaviour better in line with
normal x86.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
In order to fix a bug with guest_io_okay() (subsequent patch), rework
guest_io_okay() to take in an emulation context, and return X86EMUL_* rather
than a boolean.
For the failing case, take the opportunity to inject #GP explicitly, rather
than returning X86EMUL_UNHANDLEABLE. There is a logical difference between
"we know what this is, and it's #GP", vs "we don't know what this is".
There is no change in practice as emulation is the final step on general #GP
resolution, but returning X86EMUL_UNHANDLEABLE would be a latent bug if a
subsequent action were to appear.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
To fold two 32-bit outputs from the asm()-s into a single 64-bit value
the compiler needs to emit a zero-extension insn for the low half. Both
RDMSR and RDTSC clear the upper halves of their output registers anyway,
though. So despite that zero-extending insn (a simple MOV) being cheap,
we can do better: Without one, by declaring the local variables as 64-
bit ones.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
There's no point using alternative_io() when there are no outputs. While
there drop the unnecessary semicolon after "ds".
No functional change.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
The only functions to modify the list, amd_iommu_detect_one_acpi() and
amd_iommu_init_cleanup(), are already init.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Hitting a page fault clobbers %cr2, so if a page fault is handled while
handling a previous page fault then %cr2 will hold the address of the
latter fault rather than the former. In particular, if a debug key
handler happens to trigger during #PF and before %cr2 is read, and that
handler itself encounters a #PF, then %cr2 will be corrupt for the outer #PF
handler.
This patch makes the page fault path delay re-enabling IRQs until %cr2
has been read in order to ensure it stays consistent.
A similar argument holds in additional cases, but they happen to be safe:
* %dr6 inside #DB: Safe because IST exceptions don't re-enable IRQs.
* MSR_XFD_ERR inside #NM: Safe because AMX isn't used in #NM handler.
While in the area, remove redundant q suffix to a movq in entry.S and
the space after the comma.
Fixes: a4cd20a190 ("[XEN] 'd' key dumps both host and guest state.")
Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Taking a fault on a non-byte-granular insn means that the "number of
bytes not handled" return value would need extra care in calculating, if
we want callers to be able to derive e.g. exception context (to be
injected to the guest) - CR2 for #PF in particular - from the value. To
simplify things rather than complicating them, reduce inline assembly to
just byte-granular string insns. On recent CPUs that's also supposed to
be more efficient anyway.
For singular element accessors, however, alignment checks are added,
hence slightly complicating the code. Misaligned (user) buffer accesses
will now be forwarded to copy_{from,to}_guest_ll().
Naturally copy_{from,to}_unsafe_ll() accessors end up being adjusted the
same way, as they're produced by mere re-processing of the same code.
Otoh copy_{from,to}_unsafe() aren't similarly adjusted, but have their
comments made match reality; down the road we may want to change their
return types, e.g. to bool.
Fixes: 76974398a6 ("Added user-memory accessing functionality for x86_64")
Fixes: 7b8c36701d ("Introduce clear_user and clear_guest")
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Michal Orzel
Jan Beulich
Oleksii Kurochko
Frediano Ziglio
Alejandro Vallejo
Roger Pau Monné
Matthew Barnes
Federico Serafini
Andrew Cooper