Xen uses master for QEMU_UPSTREAM_REVISION, and has done for other trees too
in the path. Apparently we've never specified a different branch, because the
git-clone rune only pulls in the master branch; it does not pull in diverging
branches. Fix this by stating which branch/tag is wanted.
$TAG is really a committish, and git-clone's -b/--branch takes a committish
too.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
We already have one migration case opencoded (feat.max_subleaf). A more
recent discovery is that we advertise x2APIC to guests without ensuring that
we provide max_leaf >= 0xb.
In general, any leaf known to Xen can be safely configured by the toolstack if
it doesn't violate other constraints.
Therefore, introduce guest_common_{max,default}_leaves() to generalise the
special case we currently have for feat.max_subleaf, in preparation to be able
to provide x2APIC topology in leaf 0xb even on older hardware.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
discard_initial_images() only works because init_domheap_pages() with ps==pe
is a no-op.
In dom0_construct(), explaining the significance of setting the initrd length
to 0, and put an explicit check in discard_initial_images().
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
We can't drop them fully yet, but we can limit their scope to almost nothing,
which serves the same purpose.
This removes the ability to accidentally reintroduce buggy uses of
__va(mbi->mods_addr).
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
We're about to introduce alternative mapping functions, and passing NULL was
always a slightly weird way to express unmap. Make an explicit unmap
function, to avoid having two different valid ways of unmapping.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
This is imported from Linux, but the parameter being signed is dubious in the
first place and we're not plausibly going to gain a use for the functionality.
Linux has subsequently made it an optional parameter to avoid forcing callers
to pass a stack variable they don't care about using.
In the unlikely case that we gain a usecase, we can reintroduce it, but in the
meantime simplify the single caller.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The use of physical addresses in __start_xen() has proved to be fertile soure
of bugs.
The MB1/2 path stashes the MBI pointer in multiboot_ptr (a setup.c variable
even), then re-loads it immediately before calling __start_xen(). For this,
we can just drop the function parameter and read multiboot_ptr in the one
place where it's used.
The EFI path also passes this parameter into __start_xen(). Have the EFI path
set up multiboot_ptr too, and move the explanation of phyiscal-mode pointers.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
While both src and dst are similar, src is mapped only accounting for src's
size, while dst is mapped based on the minimum of both. This means that in
some cases, an overly large mapping is requested for src.
Rework the sz calcuation to be symmetric, and leave an explanation of how
logic works.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Given that soffs is the offset into the 2M superpage,
start = (src - soffs) >> PAGE_SIFT
is a complicated expression for the frame address of the containing superpage.
Except, start is converted straight back to a byte address to use, so the
shifting is unnecessary too.
The only thing done with the mapped pointer is to have soffs added back on for
the memmove() call. bootstrap_map_addr() passes through the offset, so we can
pass src directly in and simplify the memmove() call too. For the end mapping
address, this simplifies to just src + sz too.
The same reasoning holds for dst and doffs.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
move_memory() is very complicated, and buggy. In order to fix the latter, we
have to address the former.
Given prior cleanup, bootstrap_map() is now implemented in terms of
bootstrap_map_addr(), meaning that it is counterproductive to plumb the
mapping through module_t.
Delete mod, and introduce two same-sized/named fields. At this point in boot,
neither fields have their named purpose, so indicate the purpose in comments.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
multiboot_fill_boot_info() taking the physical address of the multiboot_info
structure leads to a subtle use-after-free on the PVH path, with rather less
subtle fallout.
The pointers used by __start_xen(), mbi and mod, are either:
- MB: Directmap pointers into the trampoline, or
- PVH: Xen pointers into .initdata, or
- EFI: Directmap pointers into Xen.
Critically, these either remain valid across move_xen() (MB, PVH), or rely on
move_xen() being inhibited (EFI).
The conversion to multiboot_fill_boot_info(), taking only mbi_p, makes the PVH
path use directmap pointers into Xen, as well as move_xen() which invalidates
said pointers.
Switch multiboot_fill_boot_info() to consume the same virtual addresses that
__start_xen() currently uses. This keeps all the pointers valid for the
duration of __start_xen(), for all boot protocols.
It can be safely untangled once multiboot_fill_boot_info() takes a full copy
the multiboot info data, and __start_xen() has been moved over to using the
new boot_info consistently.
Right now, bi->{loader,cmdline,mods} are problematic. Nothing uses
bi->mods[], and nothing uses bi->cmdline after move_xen().
bi->loader is used after move_xen(), although only for cmdline_cook() of
dom0's cmdline, where it happens to be benign because PVH boot skips the
inspection of the bootloader name.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Right now, various blocks rewrite domU_config= as a whole, even though it is
largely the same.
* dom0pvh-hvm does nothing but change the domain type to hvm
* *-pci sets the domain type, clears vif=[], appends earlyprintk=xen to the
cmdline, and adds some PCI config.
Refactor this to be domU_type (defaults to pvh), domU_vif (defaults to
xenbr0), and domU_extra_config (defaults to empty) and use these variables to
build domU_config= once.
Of note, the default domU_config= now sets cmdline=, and extra= is intended
for inclusion via domU_extra_config as necessary.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
* List all the test_variants and summerise what's going on
* Use case rather than an if/else chain for $test_variant
* Adjust indentation inside the case block
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
pvh_init() sets up the mbi pointer, but leaves mbi_p at 0. This isn't
compatbile with multiboot_fill_boot_info() starting from the physical address,
in order to remove the use of the mbi pointer.
Fixes: 038826b61e ("x86/boot: move x86 boot module counting into a new boot_info struct")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Daniel P. Smith <dpsmith@apertussolutions.com>
The logic would be more robust disabling SMAP based on its precense in CR4,
rather than SMAP's accociation with a synthetic feature.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Fedora 29 is long out of date. Move forward 5 years to Fedora 40.
Include all the usual improvements. Rework the container to be non-root, use
heredocs for legibility, and switch to the new naming scheme.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Javi Merino <javi.merino@cloud.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
UBSAN complains:
(XEN) ================================================================================
(XEN) UBSAN: Undefined behaviour in common/spinlock.c:794:10
(XEN) load of address ffff82d040ae24c8 with insufficient space
(XEN) for an object of type 'struct lock_profile *'
(XEN) ----[ Xen-4.20-unstable x86_64 debug=y ubsan=y Tainted: C ]----
This shows up with GCC-14, but not with GCC-12. I have not bisected further.
Either way, the types for __lock_profile_{start,end} are incorrect.
They are an array of struct lock_profile pointers. Correct the extern's
types, and adjust the loop to match.
No practical change.
Reported-by: Andreas Glashauser <ag@andreasglashauser.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
We spend an awful lot of CI time building QEMU, even though most changes don't
touch the subset of tools/libs/ used by QEMU. Some numbers taken at a time
when CI was otherwise quiet:
With Without
Alpine: 13m38s 6m04s
Debian 12: 10m05s 8m10s
OpenSUSE Tumbleweed: 11m40s 7m54s
Ubuntu 24.04: 14m56s 8m06s
which is a >50% improvement in wallclock time in some cases.
The only build we have that needs QEMU is alpine-3.18-gcc-debug. This is the
build deployed and used by the QubesOS ADL-* and Zen3p-* jobs.
Xilinx-x86_64 deploys it too, but is PVH-only and doesn't use QEMU.
QEMU is also built by CirrusCI for FreeBSD (fully Clang/LLVM toolchain).
This should help quite a lot with Gitlab CI capacity.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
These packages have moved out of main to AUR, and are not easily accessible
any more. Drop them, because they're only needed for RomBIOS which is very
legacy these days.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
A recent update to qemu-xen has bumped the build requirements, with Python 3.8
being the new baseline but also needing the 'ensurepip' and 'tomllib/tomli'
packages.
* Ubuntu/Debian package 'ensurepip' separately, but it can be obtained by
installing the python3-venv package.
* 'tomllib' was added to the python standard library in Python 3.11, but
previously it was a separate package named 'tomli'.
In terms of changes required to build QEMU:
* Ubuntu 24.04 (Noble) has Python 3.12 so only needs python3-venv
* Ubuntu 22.04 (Jammy) has Python 3.10 but does have a python3-tomli package
that QEMU is happy with.
* FreeBSD has Python 3.9, but Python 3.11 is available.
In terms of exclusions:
* Ubuntu 20.04 (Focal) has Python 3.8, but lacks any kind of tomli package.
* Fedora 29 (Python 3.7), OpenSUSE Leap 15.6 (Python 3.6), and Ubuntu
18.04/Bionic (Python 3.6) are now too old.
Detecting tomllib/tomli is more than can fit in build's one-liner, so break it
out into a proper script.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
Acked-by: Roger Pau Monné <roger.pau@citrix.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>
Based on a fix from OpenSUSE, but adjusted to be Clang-compatible too. Pass
-Wno-implicit-function-declaration library-wide rather than using local GCC
pragmas.
Fix of copy_past_newline() to avoid triggering -Wstrict-prototypes.
Link: https://build.opensuse.org/request/show/1178775
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
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>
Despite writing this code, it's not the easiest logic to follow.
Shorten the UCODE_EQUIV_TYPE name, and provide more of an explanation of
what's going on.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
LFENCE_DISPATCH used to be a synthetic feature, but was given a real CPUID bit
by AMD. The define wasn't moved when this was changed.
NSCB has always been a real CPUID bit, and was misplaced when introduced in
the synthetic block alongside LFENCE_DISPATCH.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This is mostly Linux style. Make the file self-consistent. Drop trailing
whitespace, and use tabs consistently.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
This used to be true, but was altered by commit 37786b23b0 ("x86/cet: Remove
writeable mapping of the BSPs shadow stack") which moved cpu0_stack into
.init.bss.stack_aligned.
Fixes: 37786b23b0 ("x86/cet: Remove writeable mapping of the BSPs shadow stack")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
All registration is done at boot. Almost...
iommu_dump_page_tables() is registered in iommu_hwdom_init(), which is called
twice when LATE_HWDOM is in use.
register_irq_keyhandler() has an ASSERT() guarding againt multiple
registration attempts, and the absence of bug reports hints at how many
configurations use LATE_HWDOM in practice.
Move the registration into iommu_setup() just after printing the overall
status of the IOMMU. For starters, the hardware domain is specifically
excluded by iommu_dump_page_tables().
ept_dump_p2m_table is registered in setup_ept_dump() which is non-__init, but
whose sole caller, start_vmx(), is __init. Move setup_ept_dump() to match.
With these two tweeks, all keyhandler reigstration is from __init functions,
so register_{,irq_}keyhandler() can move, and key_table[] can become
__ro_after_init.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Roger Pau Monné <roger.pau@citrix.com>
Lots of files were picking these up transitively, including lib.h
However, lib.h needs __read_mostly for printk_once() and this has the side
effect of kicking the transitive can down the road.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
probe.c includes a large number of headers which are unused, and not from
churn so far as I can see in history. Strip back to a reasonable set.
One complication is that genapic.h has to include xen/cpumask.h because
there's no way to forward declare a cpumask_t.
Also strip trailing whitespace while adjusting the file.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
The root .gitignore is quite stale from recent (and less recent) removals, but
also fails to work for the forthcoming dynamic plugin work.
Strip all Ocaml content out of the root .gitignore, and provide a more local
.gitignore's with up-to-date patterns.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@cloud.com>
It has existed in x86 CPUs since 2008, so we're only 16 years late adding
support. With all the other scafolding in place, implement arch_hweightl()
for x86.
The only complication is that the call to arch_generic_hweightl() is behind
the compilers back. Address this by writing it in ASM and ensure that it
preserves all registers.
Copy the code generation from generic_hweightl(). It's not a complicated
algorithm, and is easy to regenerate if needs be, but cover it with the same
unit tests as test_generic_hweightl() just for piece of mind.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
... and drop generic_hweight{32,64}().
This is identical on all architectures except ARM32. Add one extra SELF_TEST
to check that hweight64() works when the input is split in half.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
... and drop generic_hweight32().
As noted previously, the only two users of hweight32() are in __init paths.
The int-optimised form of generic_hweight() is only two instructions shorter
than the long-optimised form, and even then only on architectures which lack
fast multiplication, so there's no point providing an int-optimised form.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Stefano Stabellini <sstabellini@kernel.org>
asm/config.h is included in every translation unit (via xen/config.h), while
only a handful of functions actually interact with the trampoline.
Move the infrastructure into its own header, and take the opportunity to
document everything.
Change trampoline_realmode_entry() and wakeup_start() to be nocall functions,
rather than char arrays. Also switch to fixed width integers which are less
likely to diverge from the asm declaration.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This removes a level of indirection, as well as removing a somewhat misleading
name; the variable is really "S3 video quirks".
More importantly however it makes it very clear that, right now, parsing the
cmdline and quirks depends on having already placed the trampoline; a
dependency which is going to be gnarly to untangle.
That said, fixing the quirk is easy. The Toshiba Satellite 4030CDT has an
Intel Celeron 300Mhz CPU (Pentium 2 era) from 1998 when MMX was the headline
feature, sporting 64M of RAM. Being a 32-bit processor, it hasn't been able
to run Xen for about a decade now, so drop the quirk entirely.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
All of CONFIG_SCHED_*, and CONFIG_HYPFS build fine.
Add a stub for share_xen_page_with_guest(), which is all that is necessary to
make CONFIG_TRACEBUFFER build.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
SBI has an API for shutdown so wire it up. However, the spec does allow the
call not to be implemented, so we have to cope with sbi_shutdown() returning.
There is a reboot-capable SBI extention, but in the short term route
machine_restart() into machine_halt().
Then, use use machine_halt() rather than an infinite loop at the end of
start_xen(). This avoids the Qemu smoke test needing to wait for the full
timeout in order to succeed.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
There are 6 remaining callers in Xen:
* The two hweight32() calls, _domain_struct_bits() and efi_find_gop_mode(),
are __init only.
* The two hweight_long() calls are both in bitmap_weight().
* The two hweight64() calls are hv_vpset_nr_banks() and x86_emulate().
Only bitmap_weight() and possibly hv_vpset_nr_banks() can be considered fast
paths, and they're all of GPR-width form.
Furthermore, the differences between a generic int and generic long form is
only an ADD and SHIFT, and only in !CONFIG_HAS_FAST_MULTIPLY builds.
Therefore, it is definitely not worth having both generic implemenations.
Implement generic_hweightl() based on the current generic_hweight64(),
adjusted to be compatible with ARM32, along with standard SELF_TESTS.
Implement hweightl() with usual constant-folding and arch opt-in support. PPC
is the only architecture that devates from generic, and it simply uses the
builtin.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Using hweight() is an especially expensive way of determining simply if
multiple bits are set in a value. Worse, 4 of the 10 hweight() calls in Xen
are of this form.
Switch to the new multiple_bits_set() helper. This is far more efficient than
the longhand hweight() algorithm and, owing to its simplicity, likely more
efficient than even a dedicated instruction on a superscalar processor.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This will be used to simplify real logic in the following patch. Add compile
and boot time testing as with other bitops.
Because the expression is so simple, implement it as a function-like macro
which is generic on the type of it's argument, rather than having multiple
variants.
Testing function-like macros needs a minor adjustments to the infrastructure
in xen/self-tests.h to avoid bracketing the fn parameter. The utility of this
outweighs the associated risks.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
All of the ffs()/fls() infrastructure is in fact (attr) const, because it
doesn't even read global state. This allows the compiler even more
flexibility to optimise.
No functional change.
Reported-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
There's no need for the name to be so verbose.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
There are no bits set in lr_mask beyond nr_lrs, so when substituting
bitmap_for_each() for for_each_set_bit(), we don't need to worry about the
upper bound.
However, the type of lr_mask does matter, so switch it to be uint64_t * and
move unsigned long * override until the find_next_zero_bit() call.
Move lr_val into a narrower scope and drop used_lr as it's declared by
for_each_set_bit() itself.
Drop the nr_lrs variable and use gic_get_nr_lrs() in the one location its now
used. It hides a triple pointer dereference, and while it may not be needed
in the PRISTINE case, it certainly doesn't need to be live across the rest of
the function.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
The current expression hits UB with 31 LRs (shifting into the sign bit), and
malfunctions with 32 LRs (shifting beyond the range of int). Swapping 1 for
1ULL fixes some of these, but still malfunctions at 64 LRs which is the
architectural limit.
Instead, shift -1ULL right in order to create the mask.
Fixes: 596f885a32 ("xen/arm: set GICH_HCR_UIE if all the LRs are in use")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
Following the improvements to Xen's bitops, fls() does constant propagation in
all cases. Use it, and drop the local opencoded helper.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
... rather than opencoding locally. With this, defs.h is empty so delete it.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This is marginally more efficient, but is mostly to get rid of the use of
stdcall in cmdline.c and reloc.c
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
... rather than opencoding locally.
This involve collecting various macros scattered around Xen (min()/max()
macros from kernel.h, and _p() from lib.h) and moving them into macros.h
In reloc.c, replace ALIGN_UP() with ROUNDUP().
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
... rather than opencoding locally. This also covers NULL and *_MAX.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Most of Xen is build using -nostdinc and a fully specified include path.
However, the makefile line:
$(head-bin-objs): XEN_CFLAGS := $(CFLAGS_x86_32) -fpic
discards XEN_CFLAGS and replaces them with CFLAGS_x86_32.
Reinstate -nostdinc, and copy the -include and all -I arguments from
XEN_CFLAGS.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
The user of cr4_pv32_mask (the cr4_pv32_restore() function) only exists in a
CONFIG_PV32 build, but right now the variable is unconditionally set up.
To start with, move the setup into set_in_cr4() and remove it from it's
somewhat ad-hoc position in __start_xen(). This means the variable will be
set up in two steps for a CONFIG_PV32=y build, but it's cleaner and more
robust logic overall.
With that, there's no good reason for the variable to stay in setup.c. Move
it to x86/pv/domain.c (beside opt_pv32, for want of any better place to live),
and move the declaration to beside set_in_cr4() and mmu_cr4_features which is
a better position than setup.h.
Guard the reference with CONFIG_PV32, and fix up a recent typo in an adjacent
comment while at it.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
for_each_set_bit() allocates its own variable intentionally as loop-scope
only. Unfortunately, this causes the inner 'i' to shadow the outer 'i', and
violates MISRA Rule 5.3.
Drop the outermost 'i' and 'vcpuid' variables, moving them into a more narrow
scope and correcting them to be unsigned which they should have been all
along. Update the printk() formatting of vcpuid to match.
Fixes: 9429f1a6c4 ("ARM/vgic: Use for_each_set_bit() in vgic_to_sgi()")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
The existing expression is just a very complicated way of expressing a loop
over all bits of target->list. Simplify the expression.
While here, fix the two gprintk()'s. Because of a quotes vs line continuation
issue, there's a line of spaces in the middle of the format string.
$ strings xen-syms-arm32 | grep -e VGIC -e GICD_SGIR
<G><1>%pv VGIC: write r=%08x target->list=%hx, wrong CPUTargetList
<G><1>%pv vGICD:unhandled GICD_SGIR write %08x with wrong mode
not to mention trailing whitespace too.
Rewrite them to be more concise and useful. Use 0x prefixes for hex, rather
than being ambiguous, and identify the problem target vCPU / mode, rather than
simply saying something was wrong.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
... which is better optimised for scalar values, rather than using the
arbitrary-sized bitmap helpers.
Additionally, unlike other vgic helpers, this takes both a domain and vcpu
pointer. The domain parameter isn't used, and the callers pass v->domain.
Strip out the domain pointer.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
In all 3 examples, we're iterating over a scaler. They have an upper bound of
63 which is to exclude the top bit; the COMPRESSED bit.
recalculate_xstate() calculates xstates directly and doesn't set the
COMPRESSED bit. Both xstate_{un,}compressed_size() take architectural
register values, neither of which permit the COMPRESSED bit either.
xstate_uncompressed_size() has an ASSERT() covering this properly; add a
equivelent ASSERT() to xstate_compressed_size() too.
This alone produces:
add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-161 (-161)
Function old new delta
compress_xsave_states 66 58 -8
xstate_uncompressed_size 119 71 -48
xstate_compressed_size 124 76 -48
recalculate_xstate 347 290 -57
where xstate_{un,}compressed_size() have practically halved in size despite
being small before.
The change in compress_xsave_states() is unexpected. The function is almost
entirely dead code, and within what remains there's a smaller stack frame. I
suspect it's leftovers that the optimiser couldn't fully discard.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
... which is better optimised for scalar values, rather than using the
arbitrary-sized bitmap helpers.
For ARM32:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-16 (-16)
Function old new delta
vgic_set_irqs_pending 284 268 -16
including removing calls to _find_{first,next}_bit_le(), and two stack-spilled
words too.
For ARM64:
add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-40 (-40)
Function old new delta
vgic_set_irqs_pending 268 228 -40
including removing three calls to find_next_bit().
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Michal Orzel <michal.orzel@amd.com>
The prior version (renamed to bitmap_for_each()) was inefficeint when used
over a scalar, but this is the more common usage even before accounting for
the many opencoded forms.
Introduce a new version which operates on scalars only and does so without
spilling them to memory. This in turn requires the addition of a type-generic
form of ffs().
Add testing for the new construct alongside the ffs/fls testing.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
... and use it in self-tests.h.
This is intended to replace constructs such as __bitop_bad_size(). It
produces a better diagnostic, and is MISRA-friendly.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The current implementation wants to take an in-memory bitmap. However, all
ARM callers and all-but-1 x86 callers spill a scalar to the stack in order to
use the "generic arbitrary bitmap" helpers under the hood.
This works, but is far from ideal.
Rename the construct and move it into bitmap.h, because having an iterator for
an arbitrary bitmap is a useful thing.
This will allow us to re-implement for_each_set_bit() to be more appropriate
for scalar values.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Michal Orzel <michal.orzel@amd.com>
This hides a shell redirection which is quite rude. It also opencodes
$(move-if-changed) without the benefit of short-circuiting dependent logic
when the content hasn't changed.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@cloud.com>
Reviewed-by: Edwin Török <edwin.torok@cloud.com>
These days, `ocamlopt -h` asks you whether you meant --help instead, meaning
that the $(shell ) invocation here isn't going end up containing '-g'.
Make it unconditional, like it is in OCAMLCFLAGS already.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christian Lindig <christian.lindig@cloud.com>
Reviewed-by: Edwin Török <edwin.torok@cloud.com>
Commit 08aacc392d ("x86/emul: Fix misaligned IO breakpoint behaviour in PV
guests") caused a Coverity INTEGER_OVERFLOW complaint based on the reasoning
that width could be 0.
It can't, but digging into the code generation, GCC 8 and later (bisected on
godbolt) choose to emit a CSWITCH lookup table, and because the range (bottom
2 bits clear), it's a 16-entry lookup table.
So Coverity is understandable, given that GCC did emit a (dead) logic path
where width stayed 0.
Rewrite the logic. Introduce x86_bp_width() which compiles to a single basic
block, which replaces the switch() statement. Take the opportunity to also
make start and width be loop-scope variables.
No practical change, but it should compile better and placate Coverity.
Fixes: 08aacc392d ("x86/emul: Fix misaligned IO breakpoint behaviour in PV guests")
Coverity-ID: 1616152
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
All #DB exceptions result in an update of %dr6, but this isn't captured in
Xen's handling, and is buggy just about everywhere.
To begin resolving this issue, add a new pending_dbg field to x86_event
(unioned with cr2 to avoid taking any extra space, adjusting users to avoid
old-GCC bugs with anonymous unions), and introduce pv_inject_DB() to replace
the current callers using pv_inject_hw_exception().
Push the adjustment of v->arch.dr6 into pv_inject_event(), and use the new
x86_merge_dr6() rather than the current incorrect logic.
A key property is that pending_dbg is taken with positive polarity to deal
with RTM/BLD sensibly. Most callers pass in a constant, but callers passing
in a hardware %dr6 value need to XOR the value with X86_DR6_DEFAULT to flip to
positive polarity.
This fixes the behaviour of the breakpoint status bits; that any left pending
are generally discarded when a new #DB is raised. In principle it would fix
RTM/BLD too, except PV guests can't turn these capabilities on to start with.
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>
Pretty much everywhere in Xen the logic to update %dr6 when injecting #DB is
buggy. Introduce a new x86_merge_dr6() helper, and start fixing the mess by
adjusting the dr6 merge in do_debug(). Also correct the comment.
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>
A last minute review request was to dedup the expression calculating the
domain hash bucket.
While the code reads correctly, it is buggy because rcu_assign_pointer() is a
deeply misleading API assigning by name not value, and - contrary to it's name
- does not hide an indirection.
Therefore, rcu_assign_pointer(bucket, d); updates the local bucket variable on
the stack, not domain_hash[], causing all subsequent domid lookups to fail.
Rework the logic to use pd in the same way that domlist_remove() does.
Fixes: 19995bc70c ("xen/domain: Factor domlist_{insert,remove}() out of domain_{create,destroy}()")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
These are opencoded linked list and hashtable manipulations. Factor them out
into static inline helpers, and discuss the consequence for the domain.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
With arch_domain_create() no longer being called with the idle domain, drop
the last remaining logic.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
With x86 implementing arch_init_idle_domain(), there is no longer any need to
call arch_domain_create() with the idle domain.
Have the idle domain exit early with all other system domains. Move the
static-analysis ASSERT() earlier. Then, remove the !is_idle_domain()
protections around the majority of domain_create() and remove one level of
indentation.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The idle domain needs d->arch.ctxt_switch initialised on x86. Implement the
new arch_init_idle_domain() in order to do this.
Intentionally remove cpu_policy's initialisation to ZERO_BLOCK_PTR. It has
never tripped since it's introduction, and is weird to have in isolation
without a similar approach on other pointers.
No practical change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The idle domain causes a large amount of complexity in domain_create() because
of x86's need to initialise d->arch.ctxt_switch in arch_domain_create().
In order to address this, introduce an optional hook to perform extra
initialisation of the idle domain.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The XSM checks for XEN_DOMCTL_createdomain are problematic. There's a split
between xsm_domctl() called early, and flask_domain_create() called quite late
during domain construction.
All XSM implementations except Flask have a simple IS_PRIV check in
xsm_domctl(), and operate as expected when an unprivileged domain tries to
make a hypercall.
Flask however foregoes any action in xsm_domctl() and defers everything,
including the simple "is the caller permitted to create a domain" check, to
flask_domain_create().
As a consequence, when XSM Flask is active, and irrespective of the policy
loaded, all domains irrespective of privilege can:
* Mutate the global 'rover' variable, used to track the next free domid.
Therefore, all domains can cause a domid wraparound, and combined with a
voluntary reboot, choose their own domid.
* Cause a reasonable amount of a domain to be constructed before ultimately
failing for permission reasons, including the use of settings outside of
supported limits.
In order to remediate this, pass the ssidref into xsm_domctl() and at least
check that the calling domain privileged enough to create domains.
Take the opportunity to also fix the sign of the cmd parameter to be unsigned.
This issue has not been assigned an XSA, because Flask is experimental and not
security supported.
Reported-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Right now the apic_mem_*() helpers only compile because sizeof(void *) ==
sizeof(long long). Switch to using fix_to_virt() which is a void *type.
Also adjust the two places where the APIC/IO-APIC virtual address is rendered
in a printk().
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
xeninfo.pl was introduced in commit 1b0a8bb57e ("Added xeninfo.pl, a script
for collecting statistics from Xen hosts using the Xen-API") and has been
touched exactly twice since to remove hardcoded IP addresses and paths.
The configuration files in vnc/* date from when we had a vendered version of
Qemu living in the tree.
These have never (AFAICT) been wired into the `make install` rule.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Anthony PERARD <anthony.perard@vates.tech>
This reverts commit e42d107d0d.
The Gitlab CI {adl,zen3p}-pci-hvm-x86-64-gcc-debug tests says this breaks PCI
Passthrough to HVM guests, with -EOPNOTSUPP.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
HVMLoader is a single freestanding 32bit program with no external
dependencies. Use the fastcall calling convetion (up to 3 parameters in
registers) globally, which is more efficient than passing all parameters on
the stack.
Some bloat-o-meter highlights are:
add/remove: 0/0 grow/shrink: 3/118 up/down: 8/-3004 (-2996)
Function old new delta
...
hvmloader_acpi_build_tables 1125 961 -164
acpi_build_tables 1277 1081 -196
pci_setup 4756 4516 -240
construct_secondary_tables 1689 1447 -242
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This allows for marginally better code generation including the use of BT
rather than SHR/TEST.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
They're all within a 12 bit range of their respective bases, and this prevents
all the x2APIC MSR addresses being calculated in %rcx.
Also swap two u32's for uint32_t.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
XenServer's instance of Coverity complains of OVERFLOW_BEFORE_WIDEN in
mask_and_ack_level_ioapic_irq(), which is ultimately because of v being
unsigned long, and (1U << ...) being 32 bits.
The reasoning isn't correct. (1U << (x & 0x1f)) can't overflow, but the
complaint is really about having to expand the RHS. While this can be fixed
by changing v to be unsigned int, take the opportunity to do better still.
Introduce a apic_tmr_read() helper like we already have for ISR and IRR, and
use it to remove the opencoded logic. Introduce an is_level boolean to
improve the legibility of the surrounding logic.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
It's very rude for a library to play with signals behind the back of the
application, no matter ones views on the default behaviour of SIGPIPE under
POSIX. Even if the application doesn't care about the xenstored socket, it my
care about others.
This logic has existed since xenstore/xenstored was originally added in commit
29c9e570b1 ("Add xenstore daemon and library") in 2005.
It's also unnecessary. Pass MSG_NOSIGNAL when talking to xenstored over a
pipe (to avoid sucumbing to SIGPIPE if xenstored has crashed), and forgo any
playing with the signal disposition.
This has a side benefit of saving 2 syscalls per xenstore request.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
With the input data now conveniently arranged, use writev()/sendmsg() instead
of decomposing it into write() calls.
This causes all requests to be submitted with a single system call, rather
than at least two. While in principle short writes can occur, the chances of
it happening are slim given that most xenbus comms are only a handful of
bytes.
Nevertheless, provide {writev,sendmsg}_exact() wrappers which take care of
resubmitting on EINTR or short write.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Jason Andryuk <jason.andryuk@amd.com>
Andrew Cooper