Anthony PERARD [Thu, 14 Nov 2024 17:27:42 +0000 (18:27 +0100)]
CI: New stage "containers" to rebuild some containers
Rebuild rolling release containers when XEN_CI_REBUILD_CONTAINERS is
set. This is to be use with a scheduled pipeline.
When $XEN_CI_REBUILD_CONTAINERS is set, only build jobs related to the
containers been rebuild will be executed.
Build jobs that are using one of the containers been rebuild should
wait for the container to be rebuild. If it's a normal pipeline, those
dependency are simply ignored.
Signed-off-by: Anthony PERARD <anthony.perard@vates.tech>
Roger Pau Monné [Thu, 14 Nov 2024 15:13:10 +0000 (16:13 +0100)]
x86/mm: ensure L2 is always freed if empty
The current logic in modify_xen_mappings() allows for fully empty L2 tables to
not be freed and unhooked from the parent L3 if the last L2 slot is not
populated.
Ensure that even when an L2 slot is empty the logic to check whether the whole
L2 can be removed is not skipped.
Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Roger Pau Monné [Thu, 14 Nov 2024 15:12:51 +0000 (16:12 +0100)]
x86/setup: remove bootstrap_map_addr() usage of destroy_xen_mappings()
bootstrap_map_addr() needs to be careful to not remove existing page-table
structures when tearing down mappings, as such pagetable structures might be
needed to fulfill subsequent mappings requests. The comment ahead of the
function already notes that pagetable memory shouldn't be allocated.
Fix this by using map_pages_to_xen(), which does zap the page-table entries,
but does not free page-table structures even when empty.
Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') Signed-off-by: Roger Pau Monné <roger.pau@ctrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Roger Pau Monné [Thu, 14 Nov 2024 15:12:35 +0000 (16:12 +0100)]
x86/mm: skip super-page alignment checks for non-present entries
INVALID_MFN is ~0, so by it having all bits as 1s it doesn't fulfill the
super-page address alignment checks for L3 and L2 entries. Skip the alignment
checks if the new entry is a non-present one.
This fixes a regression introduced by 0b6b51a69f4d, where the switch from 0 to
INVALID_MFN caused all super-pages to be shattered when attempting to remove
mappings by passing INVALID_MFN instead of 0.
Fixes: 0b6b51a69f4d ('xen/mm: Switch map_pages_to_xen to use MFN typesafe') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan Beulich [Thu, 14 Nov 2024 12:03:18 +0000 (13:03 +0100)]
x86emul: avoid double memory read for RORX
Originally only twobyte_table[0x3a] determined what part of generic
operand fetching (near the top of x86_emulate()) comes into play. When
ext0f3a_table[] was added, ->desc was updated to properly describe the
ModR/M byte's function. With that generic source operand fetching came
into play for RORX, rendering the explicit fetching in the respective
case block redundant (and wrong at the very least when MMIO with side
effects is accessed).
While there also make a purely cosmetic / documentary adjustment to
ext0f3a_table[]: RORX really is a 2-operand insn, MOV-like in that it
only writes its destination register.
Fixes: 9f7f5f6bc95b ("x86emul: add tables for 0f38 and 0f3a extension space") Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Make explicit the fallthrough intention by adding the pseudo keyword
where missing and replace fallthrough comments not following the
agreed syntax.
This satisfies the requirements to deviate violations 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: Jan Beulich <jbeulich@suse.com>
x86/emul: auxiliary definition of pseudo keyword fallthrough
The pseudo keyword fallthrough shall be used to make explicit the
fallthrough intention at the end of a case statement (doing this
using comments is deprecated).
A definition of such pseudo keyword is already present in the
Xen build. This auxiliary definition makes it available also for
for test and fuzzing harness without iterfearing with the one
that the Xen build has.
Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> Acked-by: Jan Beulich <jbeulich@suse.com>
Jan Beulich [Thu, 14 Nov 2024 12:00:57 +0000 (13:00 +0100)]
x86emul: ignore VEX.W for BMI{1,2} insns in 32-bit mode
While result values and other status flags are unaffected as long as we
can ignore the case of registers having their upper 32 bits non-zero
outside of 64-bit mode, EFLAGS.SF may obtain a wrong value when we
mistakenly re-execute the original insn with VEX.W set.
Note that guest the memory access, if any, is correctly carried out as
32-bit regardless of VEX.W. The emulator-local memory operand will be
accessed as a 64-bit quantity, but it is pre-initialised to zero so no
internal state can leak.
Fixes: 771daacd197a ("x86emul: support BMI1 insns") Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Jan Beulich [Thu, 14 Nov 2024 12:00:29 +0000 (13:00 +0100)]
x86emul: correct EFLAGS testing for BMI1/BMI2
Apparently I blindly copied the constants from the BEXTR case, where SF
indeed wants leaving out. For BLSI, BLSMSK, BLSR, and BZHI SF is
defined, and hence wants checking. This is noticable in particular for
BLSR, where with the input we use SF will be set in the result (and
hence is being switched to be clear on input).
Convert to using named constants we have available, while omitting DF,
TF, as well as the MBZ bits 3 and 5 from the masking values in the
checks of the produced output. For BZHI also set SF on input, expecting
it to transition to clear.
Fixes: 771daacd197a ("x86emul: support BMI1 insns") Fixes: 8e20924de13d ("x86emul: support BMI2 insns") Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.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.
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>
Andrew Cooper [Thu, 7 Nov 2024 13:45:10 +0000 (13:45 +0000)]
x86/ucode: Rework AMD's microcode_fits()
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>
Andrew Cooper [Wed, 6 Nov 2024 15:22:54 +0000 (15:22 +0000)]
x86/ucode: Fold microcode_update_cpu() and fix error handling
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>
Andrew Cooper [Wed, 6 Nov 2024 14:59:33 +0000 (14:59 +0000)]
x86/ucode: Don't use microcode_update_cpu() in early_microcode_load()
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>
drivers/char: Use sub-page ro API to make just xhci dbc cap RO
Not the whole page, which may contain other registers too. The XHCI
specification describes DbC as designed to be controlled by a different
driver, but does not mandate placing registers on a separate page. In fact
on Tiger Lake and newer (at least), this page do contain other registers
that Linux tries to use. And with share=yes, a domU would use them too.
Without this patch, PV dom0 would fail to initialize the controller,
while HVM would be killed on EPT violation.
With `share=yes`, this patch gives domU more access to the emulator
(although a HVM with any emulated device already has plenty of it). This
configuration is already documented as unsafe with untrusted guests and
not security supported.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
x86/mm: add API for marking only part of a MMIO page read only
In some cases, only few registers on a page needs to be write-protected.
Examples include USB3 console (64 bytes worth of registers) or MSI-X's
PBA table (which doesn't need to span the whole table either), although
in the latter case the spec forbids placing other registers on the same
page. Current API allows only marking whole pages pages read-only,
which sometimes may cover other registers that guest may need to
write into.
Currently, when a guest tries to write to an MMIO page on the
mmio_ro_ranges, it's either immediately crashed on EPT violation - if
that's HVM, or if PV, it gets #PF. In case of Linux PV, if access was
from userspace (like, /dev/mem), it will try to fixup by updating page
tables (that Xen again will force to read-only) and will hit that #PF
again (looping endlessly). Both behaviors are undesirable if guest could
actually be allowed the write.
Introduce an API that allows marking part of a page read-only. Since
sub-page permissions are not a thing in page tables (they are in EPT,
but not granular enough), do this via emulation (or simply page fault
handler for PV) that handles writes that are supposed to be allowed.
The new subpage_mmio_ro_add() takes a start physical address and the
region size in bytes. Both start address and the size need to be 8-byte
aligned, as a practical simplification (allows using smaller bitmask,
and a smaller granularity isn't really necessary right now).
It will internally add relevant pages to mmio_ro_ranges, but if either
start or end address is not page-aligned, it additionally adds that page
to a list for sub-page R/O handling. The list holds a bitmask which
qwords are supposed to be read-only and an address where page is mapped
for write emulation - this mapping is done only on the first access. A
plain list is used instead of more efficient structure, because there
isn't supposed to be many pages needing this precise r/o control.
The mechanism this API is plugged in is slightly different for PV and
HVM. For both paths, it's plugged into mmio_ro_emulated_write(). For PV,
it's already called for #PF on read-only MMIO page. For HVM however, EPT
violation on p2m_mmio_direct page results in a direct domain_crash() for
non hardware domains. To reach mmio_ro_emulated_write(), change how
write violations for p2m_mmio_direct are handled - specifically, check
if they relate to such partially protected page via
subpage_mmio_write_accept() and if so, call hvm_emulate_one_mmio() for
them too. This decodes what guest is trying write and finally calls
mmio_ro_emulated_write(). The EPT write violation is detected as
npfec.write_access and npfec.present both being true (similar to other
places), which may cover some other (future?) cases - if that happens,
emulator might get involved unnecessarily, but since it's limited to
pages marked with subpage_mmio_ro_add() only, the impact is minimal.
Both of those paths need an MFN to which guest tried to write (to check
which part of the page is supposed to be read-only, and where
the page is mapped for writes). This information currently isn't
available directly in mmio_ro_emulated_write(), but in both cases it is
already resolved somewhere higher in the call tree. Pass it down to
mmio_ro_emulated_write() via new mmio_ro_emulate_ctxt.mfn field.
This may give a bit more access to the instruction emulator to HVM
guests (the change in hvm_hap_nested_page_fault()), but only for pages
explicitly marked with subpage_mmio_ro_add() - so, if the guest has a
passed through a device partially used by Xen.
As of the next patch, it applies only configuration explicitly
documented as not security supported.
The subpage_mmio_ro_add() function cannot be called with overlapping
ranges, and on pages already added to mmio_ro_ranges separately.
Successful calls would result in correct handling, but error paths may
result in incorrect state (like pages removed from mmio_ro_ranges too
early). Debug build has asserts for relevant cases.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan Beulich [Tue, 12 Nov 2024 12:33:38 +0000 (13:33 +0100)]
mm: adjust _xvrealloc() declaration
... to match its definition parameter-name-wise, to please Misra C:2012
Rule 8.3.
Fixes: 9102fcd9579f ("mm: introduce xvmalloc() et al and use for grant table allocations") Reported-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Andrew Cooper [Fri, 8 Nov 2024 17:34:32 +0000 (17:34 +0000)]
x86/mach-apic: Move generic_*_probe() declarations into genapic.h
... 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>
Andrew Cooper [Fri, 8 Nov 2024 17:54:22 +0000 (17:54 +0000)]
VT-d: Drop includes of mach_apic.h
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>
Roger Pau Monné [Mon, 11 Nov 2024 12:19:45 +0000 (13:19 +0100)]
xen/x86: prevent addition of .note.gnu.property if livepatch is enabled
GNU assembly that supports such feature will unconditionally add a
.note.gnu.property section to object files. The content of that section can
change depending on the generated instructions. The current logic in
livepatch-build-tools doesn't know how to deal with such section changing
as a result of applying a patch and rebuilding.
Since .note.gnu.property is not consumed by the Xen build, suppress its
addition when livepatch support is enabled.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Andrew Cooper [Fri, 8 Nov 2024 14:08:33 +0000 (14:08 +0000)]
x86/boot: Fix bootinfo.h to be standalone
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: 74af2d98276d ("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>
Daniel P. Smith [Sat, 2 Nov 2024 17:25:46 +0000 (13:25 -0400)]
x86/boot: add cmdline_pa to struct boot_module
Add an address field, cmdline_pa, to struct boot_module to hold the address of
the string field from struct mod.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Daniel P. Smith [Sat, 2 Nov 2024 17:25:45 +0000 (13:25 -0400)]
x86/boot: move kextra into boot info
... so it can be removed as a distinct parameter to create_dom0().
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Daniel P. Smith [Sat, 2 Nov 2024 17:25:44 +0000 (13:25 -0400)]
x86/boot: move headroom to boot modules
The purpose of struct boot_module is to encapsulate the state of boot module as
it is processed by Xen. Locating boot module state struct boot_module reduces
the number of global variables as well as the number of state variables that
must be passed around. It also lays the groundwork for hyperlaunch mult-domain
construction, where multiple instances of state variables like headroom will be
needed.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> Reviewed-by: Jason Andryuk <jason.andryuk@amd.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Xen tracks the total span of physical memory space of each NUMA node,
but the PFN span includes large MMIO holes in e.g. the first NUMA node.
Thus, the span is not the same as the amount of usable RAM of a node.
Xen does not need it, but NUMA node memory amount can be helpful for
management tools and HW information tools like hwloc/lstopo with its
Xen backend for Dom0: https://github.com/xenserver-next/hwloc/
Introduce node_present_pages to node_data[]:
On boot, set the count of usable PFNs and update it on memory_add().
Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
x86/xstate: Remove stale assertions in fpu_x{rstor,save}()
After edb48e76458b("x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu"),
v->arch.xsave_area is always present and we can just remove these asserts.
Fixes: edb48e76458b("x86/fpu: Combine fpu_ctxt and xsave_area in arch_vcpu") Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Acked-by: Jan Beulich <jbeulich@suse.com>
Javi Merino [Fri, 18 Oct 2024 09:17:43 +0000 (10:17 +0100)]
CI: Refresh the Debian 12 x86_32 container
Rework the container to be non-root, use heredocs for readability, and
use apt-get --no-install-recommends to keep the size down. Rename the
job to x86_32, to be consistent with XEN_TARGET_ARCH and the
naming scheme of all the other CI jobs:
${VERSION}-${ARCH}-${BUILD_NAME}
Remove build dependencies for building QEMU. The absence of ninja/meson means
that the container hasn't been able to build QEMU in years.
Remove build dependencies for the documentation as we don't have to
build it for every single arch.
This reduces the size of the container from 2.22GB to 1.32Gb.
Signed-off-by: Javi Merino <javi.merino@cloud.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Javi Merino [Mon, 14 Oct 2024 16:53:31 +0000 (17:53 +0100)]
CI: Refresh the Debian 12 x86_64 container
Rework the container to use heredocs for readability, and use
apt-get --no-install-recommends to keep the size down.
This reduces the size of the (uncompressed) container from 3.44GB to
1.97GB.
The container is left running the builds and tests as root. A
subsequent patch will make the necessary changes to the test scripts
to allow test execution as a non-root user.
Signed-off-by: Javi Merino <javi.merino@cloud.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Javi Merino [Mon, 4 Nov 2024 15:58:14 +0000 (15:58 +0000)]
CI: Don't use -y with apt-get update
apt-get update refreshes the package lists. -y doesn't do anything
here. It is needed for "apt-get install" or "apt-get upgrade" but not
for apt-get update. Drop it.
Signed-off-by: Javi Merino <javi.merino@cloud.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Daniel P. Smith [Sat, 2 Nov 2024 17:25:42 +0000 (13:25 -0400)]
x86/boot: introduce boot module flags
The existing startup code employs various ad-hoc state tracking about certain
boot module types by each area of the code. A boot module flags bitfield is
added to enable tracking these different states. The first state to be
transition by this commit is module relocation.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Daniel P. Smith [Sat, 2 Nov 2024 17:25:41 +0000 (13:25 -0400)]
x86/boot: eliminate module_map
With all boot modules now labeled by type, it is no longer necessary to
track whether a boot module was identified via the module_map bitmap.
Introduce a set of helpers to search the list of boot modules based on type and
the reference type, pointer or array index, desired. Then drop all uses of
setting a bit in module_map and replace its use for looping with the helpers.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com>
Daniel P. Smith [Sat, 2 Nov 2024 17:25:40 +0000 (13:25 -0400)]
x86/boot: introduce boot module types
This commit introduces module types for the types of boot modules that may be
passed to Xen. These include xen, kernel, ramdisk, microcode, and xsm policy.
This reduces the need for hard coded order assumptions and global variables to
be used by consumers of boot modules, such as domain construction.
Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Andrew Cooper [Fri, 25 Oct 2024 19:35:47 +0000 (20:35 +0100)]
x86/ucode: Fold early_update_cache() into its single caller
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>
Andrew Cooper [Fri, 25 Oct 2024 18:49:11 +0000 (19:49 +0100)]
x86/ucode: Drop ucode_mod and ucode_blob
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>
Andrew Cooper [Fri, 25 Oct 2024 19:02:10 +0000 (20:02 +0100)]
x86/ucode: Use bootstrap_unmap() in early_microcode_load()
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>
Andrew Cooper [Fri, 25 Oct 2024 17:08:34 +0000 (18:08 +0100)]
x86/ucode: Enforce invariant about module selection
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: 5ed12565aa32 ("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>
Frediano Ziglio [Tue, 5 Nov 2024 14:13:43 +0000 (14:13 +0000)]
x86/boot: Explicitly list .{sym,shstr,str}tab in build32.lds.S
Currently, building with LLVM's LLD fails:
ld -melf_i386_fbsd --orphan-handling=error -N -T ...
ld: error: <internal>:(.symtab) is being placed in '.symtab'
ld: error: <internal>:(.shstrtab) is being placed in '.shstrtab'
ld: error: <internal>:(.strtab) is being placed in '.strtab'
gmake[11]: *** [arch/x86/boot/Makefile:69: arch/x86/boot/built-in-32.base.bin] Error 1
This is a consequence of --orphan-handling, and it appears that Binutils
doesn't diagnose some orphaned sections even explicitly asked to do so.
List the sections explicitly.
Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot') Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Frediano Ziglio [Wed, 6 Nov 2024 10:22:47 +0000 (10:22 +0000)]
x86/boot: Uses nm command instead of map file to get symbols
combine_two_binaries.py only understands GNU LD's format, and does
not work with LLVM's LLD.
Use nm command instead to get list of symbols; specifically
BSD format as it does not truncate symbols names like sysv one.
Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot') Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Frediano Ziglio [Wed, 6 Nov 2024 10:07:48 +0000 (10:07 +0000)]
x86/boot: Fix intermediate file names to generate 32 bit code
The "base" and "offset" definition were inverted, "base" file should be the
files without offsets applied while "offset" should have the offsets applied.
Also update an old usage of "final" to "apply offset" to make more clear and
consistent (in former commit messages the "final" term was used instead of
"offset").
Fixes: aa9045e77130 ('x86/boot: Rework how 32bit C is linked/included for early boot') Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Update the apt package index before installing, which fixes things.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> Acked-by: Stefano Stabellini <sstabellini@kernel.org>
Andrew Cooper [Fri, 25 Oct 2024 18:20:41 +0000 (19:20 +0100)]
x86/ucode: Move the CPIO path string into microcode_ops
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>
Andrew Cooper [Fri, 25 Oct 2024 16:50:37 +0000 (17:50 +0100)]
x86/ucode: Fold microcode_grab_module() into its single caller
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>
Andrew Cooper [Fri, 25 Oct 2024 16:24:35 +0000 (17:24 +0100)]
x86/ucode: Fold early_microcode_update_cpu() into its single caller
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>
Andrew Cooper [Fri, 25 Oct 2024 16:39:06 +0000 (17:39 +0100)]
x86/ucode: Break early_microcode_load() out of early_microcode_init()
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>
Andrew Cooper [Sat, 26 Oct 2024 17:05:39 +0000 (18:05 +0100)]
x86/ucode: Turn microcode_init_cache() into a presmp_initcall
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>
Roger Pau Monne [Mon, 28 Oct 2024 11:48:31 +0000 (12:48 +0100)]
tools/libxl: remove usage of VLA arrays
Clang 19 complains with the following error when building libxl:
libxl_utils.c:48:15: error: variable length array folded to constant array as an extension [-Werror,-Wgnu-folding-constant]
48 | char path[strlen("/local/domain") + 12];
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
Replace the usage of strlen() with sizeof, which allows the literal
string length to be known at build time. Note sizeof accounts for the
NUL terminator while strlen() didn't, hence subtract 1 from the total
size calculation.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Anthony PERARD <anthony.perard@vates.tech>
Roger Pau Monne [Thu, 31 Oct 2024 08:57:13 +0000 (09:57 +0100)]
x86/io-apic: fix directed EOI when using AMD-Vi interrupt remapping
When using AMD-Vi interrupt remapping the vector field in the IO-APIC RTE is
repurposed to contain part of the offset into the remapping table. Previous to 2ca9fbd739b8 Xen had logic so that the offset into the interrupt remapping
table would match the vector. Such logic was mandatory for end of interrupt to
work, since the vector field (even when not containing a vector) is used by the
IO-APIC to find for which pin the EOI must be performed.
A simple solution wold be to read the IO-APIC RTE each time an EOI is to be
performed, so the raw value of the vector field can be obtained. However
that's likely to perform poorly. Instead introduce a cache to store the
EOI handles when using interrupt remapping, so that the IO-APIC driver can
translate pins into EOI handles without having to read the IO-APIC RTE entry.
Note that to simplify the logic such cache is used unconditionally when
interrupt remapping is enabled, even if strictly it would only be required
for AMD-Vi.
Reported-by: Willi Junga <xenproject@ymy.be> Suggested-by: David Woodhouse <dwmw@amazon.co.uk> Fixes: 2ca9fbd739b8 ('AMD IOMMU: allocate IRTE entries instead of using a static mapping') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Tested-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Wei Chen [Mon, 28 Oct 2024 12:45:44 +0000 (12:45 +0000)]
xen/arm: mpu: Define Xen start address for MPU systems
On Armv8-A, Xen has a fixed virtual start address (link address too) for all
Armv8-A platforms. In an MMU based system, Xen can map its loaded address to
this virtual start address. So, on Armv8-A platforms, the Xen start address does
not need to be configurable. But on Armv8-R platforms, there is no MMU to map
loaded address to a fixed virtual address and different platforms will have very
different address space layout. So Xen cannot use a fixed physical address on
MPU based system and need to have it configurable.
So, we introduce a Kconfig option for users to set the start address. The start
address needs to be aligned to 4KB. We have a check for this alignment.
MPU allows us to define regions which are 64 bits aligned. This restriction
comes from the bitfields of PRBAR, PRLAR (the lower 6 bits are 0 extended to
provide the base and limit address of a region). This means that the start
address of Xen needs to be at least 64 bits aligned (as it will correspond to
the start address of memory protection region).
As for now Xen on MPU tries to use the same memory alignment restrictions as it
has been for MMU. We have added a build assertion to ensure that the page size
is 4KB. Unlike MMU where the starting virtual address is 2MB, Xen on MPU needs
the start address to be 4KB (ie page size) aligned.
In case if the user forgets to set the start address, then 0xffffffff is used
as default. This is to trigger the error (on alignment check) and thereby prompt
user to set the start address.
Also updated config.h so that it includes mpu/layout.h when CONFIG_MPU is
defined.
xen/arm: mpu: Introduce choice between MMU and MPU
There are features in the forthcoming patches which are dependent on
MPU. For eg fixed start address.
Also, some of the Xen features (eg STATIC_MEMORY) will be selected
by the MPU configuration.
Thus, this patch introduces a choice between MMU and MPU for the type
of memory management system. By default, MMU is selected.
MPU is now gated by UNSUPPORTED.
Update SUPPORT.md to state that the support for MPU is experimental.
Also updated CHANGELOG.md as well.
xen/arm: Skip initializing the BSS section when it is empty
If the BSS section is empty, then the function should return.
If one does not check whether the BSS section is empty or not, then there is a
risk of writing 0s outside of BSS section (which may contain critical data).
Recent patches to xen-devel indicate active interest in Argo within the Xen
community, and as the feature has been documented and in use by
OpenXT for multiple years, update the Xen SUPPORT.md statement of status.
This has also been discussed at several XenSummits, with general agreement to
the status upgrade.
Signed-off-by: Christopher Clark <christopher.w.clark@gmail.com> Acked-by: Julien Grall <jgrall@amazon.com> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Roger Pau Monné [Thu, 31 Oct 2024 11:43:10 +0000 (12:43 +0100)]
x86/mm: ensure L2 is always freed if empty
The current logic in modify_xen_mappings() allows for fully empty L2 tables to
not be freed and unhooked from the parent L3 if the last L2 slot is not
populated.
Ensure that even when an L2 slot is empty the logic to check whether the whole
L2 can be removed is not skipped.
Fixes: 4376c05c3113 ('x86-64: use 1GB pages in 1:1 mapping if available') Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Dom0 normally informs Xen of PCI device removal via
PHYSDEVOP_pci_device_remove, e.g. in response to SR-IOV disable or
hot-unplug. We might find ourselves with stale pdevs if a buggy dom0
fails to report removal via PHYSDEVOP_pci_device_remove. In this case,
attempts to access the config space of the stale pdevs would be invalid
and return all 1s.
Some possible conditions leading to this are:
1. Dom0 disables SR-IOV without reporting VF removal to Xen.
The Linux SR-IOV subsystem normally reports VF removal when a PF driver
disables SR-IOV. In case of a buggy dom0 SR-IOV subsystem, SR-IOV could
become disabled with stale dangling VF pdevs in both dom0 Linux and Xen.
2. Dom0 reporting PF removal without reporting VF removal.
During SR-IOV PF removal (hot-unplug), a buggy PF driver may fail to
disable SR-IOV, thus failing to remove the VFs, leaving stale dangling
VFs behind in both Xen and Linux. At least Linux warns in this case:
[ 100.000000] 0000:01:00.0: driver left SR-IOV enabled after remove
In either case, Xen is left with stale VF pdevs, risking invalid PCI
config space accesses.
When Xen is built with CONFIG_DEBUG=y, the following Xen crashes were
observed when dom0 attempted to access the config space of a stale VF:
(XEN) Assertion 'pos' failed at arch/x86/msi.c:1274
(XEN) ----[ Xen-4.20-unstable x86_64 debug=y Tainted: C ]----
...
(XEN) Xen call trace:
(XEN) [<ffff82d040346834>] R pci_msi_conf_write_intercept+0xa2/0x1de
(XEN) [<ffff82d04035d6b4>] F pci_conf_write_intercept+0x68/0x78
(XEN) [<ffff82d0403264e5>] F arch/x86/pv/emul-priv-op.c#pci_cfg_ok+0xa0/0x114
(XEN) [<ffff82d04032660e>] F arch/x86/pv/emul-priv-op.c#guest_io_write+0xb5/0x1c8
(XEN) [<ffff82d0403267bb>] F arch/x86/pv/emul-priv-op.c#write_io+0x9a/0xe0
(XEN) [<ffff82d04037c77a>] F x86_emulate+0x100e5/0x25f1e
(XEN) [<ffff82d0403941a8>] F x86_emulate_wrapper+0x29/0x64
(XEN) [<ffff82d04032802b>] F pv_emulate_privileged_op+0x12e/0x217
(XEN) [<ffff82d040369f12>] F do_general_protection+0xc2/0x1b8
(XEN) [<ffff82d040201aa7>] F x86_64/entry.S#handle_exception_saved+0x2b/0x8c
(XEN) Assertion 'pos' failed at arch/x86/msi.c:1246
(XEN) ----[ Xen-4.20-unstable x86_64 debug=y Tainted: C ]----
...
(XEN) Xen call trace:
(XEN) [<ffff82d040346b0a>] R pci_reset_msix_state+0x47/0x50
(XEN) [<ffff82d040287eec>] F pdev_msix_assign+0x19/0x35
(XEN) [<ffff82d040286184>] F drivers/passthrough/pci.c#assign_device+0x181/0x471
(XEN) [<ffff82d040287c36>] F iommu_do_pci_domctl+0x248/0x2ec
(XEN) [<ffff82d040284e1f>] F iommu_do_domctl+0x26/0x44
(XEN) [<ffff82d0402483b8>] F do_domctl+0x8c1/0x1660
(XEN) [<ffff82d04032977e>] F pv_hypercall+0x5ce/0x6af
(XEN) [<ffff82d0402012d3>] F lstar_enter+0x143/0x150
These ASSERTs triggered because the MSI-X capability position can't be
found for a stale pdev.
Latch the capability positions of MSI and MSI-X during device init, and
replace instances of pci_find_cap_offset(..., PCI_CAP_ID_MSI{,X}) with
the stored value. Introduce one additional ASSERT, while the two
existing ASSERTs in question continue to work as intended, even with a
stale pdev.
Fixes: 484d7c852e4f ("x86/MSI-X: track host and guest mask-all requests separately") Fixes: 575e18d54d19 ("pci: clear {host/guest}_maskall field on assign") Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Jan Beulich [Thu, 31 Oct 2024 11:41:18 +0000 (12:41 +0100)]
byteorder: replace __u64
In {big,little}_endian.h the changes are entirely mechanical, except for
dealing with casting away of const from pointers-to-const on lines
touched anyway.
In swab.h the casting of constants is done away with as well - I simply
don't see what the respective comment is concerned about in our
environment (sizeof(int) >= 4, sizeof(long) >= {4,8} depending on
architecture, sizeof(long long) >= 8). The comment is certainly relevant
in more general cases. Excess parentheses are dropped as well,
___swab64()'s local variable is renamed, and __arch__swab64()'s is
dropped as being redundant with ___swab64()'s.
Excessive casts compared to ___{,constant_}swab{16,32}() are also
dropped. Much like excessive ones in __fswab64().
Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Jan Beulich [Thu, 31 Oct 2024 11:39:33 +0000 (12:39 +0100)]
byteorder: replace __u32
In {big,little}_endian.h the changes are entirely mechanical, except for
dealing with casting away of const from pointers-to-const on lines
touched anyway.
In swab.h the casting of constants is done away with as well - I simply
don't see what the respective comment is concerned about in our
environment (sizeof(int) >= 4, sizeof(long) >= {4,8} depending on
architecture, sizeof(long long) >= 8). The comment is certainly relevant
in more general cases. Excess parentheses are dropped as well,
___swab32()'s local variable is renamed, and __arch__swab32()'s is
dropped as being redundant with ___swab32()'s.
The masking operation is also dropped from __fswab64().
Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Jan Beulich [Thu, 31 Oct 2024 11:39:23 +0000 (12:39 +0100)]
byteorder: replace __u16
In {big,little}_endian.h the changes are entirely mechanical, except for
dealing with casting away of const from pointers-to-const on lines
touched anyway.
In swab.h the casting of constants is done away with as well - I simply
don't see what the respective comment is concerned about in our
environment (sizeof(int) >= 4, sizeof(long) >= {4,8} depending on
architecture, sizeof(long long) >= 8). The comment is certainly relevant
in more general cases. Excess parentheses are dropped as well,
___swab16()'s local variable is renamed, and __arch__swab16()'s is
dropped as being redundant with ___swab16()'s.
With that no uses of the type remain, so it moves to linux-compat.h.
Signed-off-by: Jan Beulich <jbeulich@suse.com> Reviewed-by: Frediano Ziglio <frediano.ziglio@cloud.com>
Andrew Cooper [Wed, 30 Oct 2024 19:03:42 +0000 (19:03 +0000)]
scripts: Fix git-checkout.sh to work with branches other than master
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>
Andrew Cooper [Tue, 29 Oct 2024 17:21:08 +0000 (17:21 +0000)]
x86/cpu-policy: Extend the guest max policy max leaf/subleaves
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>
Victor Lira [Mon, 28 Oct 2024 23:55:35 +0000 (16:55 -0700)]
automation: add x86_64 test (linux argo)
Add x86_64 hardware test that creates a Xen Argo communication
connection between two PVH domains. In the test, dom0 creates a domU and
listens for messages sent by the domU through Argo.
To accomplish this, build Xen with CONFIG_ARGO=y and create a CI test job.
Update the xilinx x86_64 test script to support the new test, and add
"sync_console" to command line to avoid an issue with console messages
being lost.
Frediano Ziglio [Tue, 29 Oct 2024 10:29:39 +0000 (10:29 +0000)]
x86/boot: Reuse code to relocate trampoline
Move code from efi-boot.h to a separate, new, reloc-trampoline.c file.
Reuse this new code, compiling it for 32bit as well, to replace assembly
code in head.S doing the same thing.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
Frediano Ziglio [Tue, 29 Oct 2024 10:29:38 +0000 (10:29 +0000)]
x86/boot: Rework how 32bit C is linked/included for early boot
Right now, the two functions which were really too complicated to write
in asm are compiled as 32bit PIC, linked to a blob and included
directly, using global asm() to arrange for them to have function semantics.
This is limiting and fragile; the use of data relocations will compile
fine but malfunction when used, creating hard-to-debug bugs.
Furthermore, we would like to increase the amount of C, to
deduplicate/unify Xen's boot logic, as well as making it easier to
follow. Therefore, rework how the 32bit objects are included.
Link all 32bit objects together first. This allows for sharing of logic
between translation units. Use differential linking and explicit
imports/exports to confirm that we only have the expected relocations,
and write the object back out as an assembly file so it can be linked
again as if it were 64bit, to integrate with the rest of Xen.
This allows for the use of external references (e.g. access to global
variables) with reasonable assurance of doing so safely.
No functional change.
Signed-off-by: Frediano Ziglio <frediano.ziglio@cloud.com> Reviewed-by: Anthony PERARD <anthony.perard@vates.tech> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>