The current mechanism is geared towards fast virtual address ->
symbol names lookup. This is fine for the normal use cases
(BUG_ON, WARN_ON, etc), but for xSplice - where we need to find
hypervisor symbols - it is slow.
To understand this patch, a description of the existing
method is explained first. For folks familar go to 'NEW CODE:'.
HOW IT WORKS:
The symbol table lookup mechanism uses a simple encoding mechanism
where it extracts the common ascii characters that the symbol's use.
This saves us space. The lookup mechanism is geared towards looking
up symbols based on address. We have one 0..N (where N is
the number of symbols, so 6849 for example) table:
symbols_addresses[0..N]
And an 1-1 (in a loose fashion) of the symbols (encoded) in a
symbols_names stream of size N.
The N is variable (later on that below)
The symbols_names are sorted based on symbols_addresses, which
means that the decoded entries inside symbols_names are not in
ascending or descending order.
There is also the encoding mechanism - the table of 255 entries
called symbols_token_index[]. And the symbols_token_table which
is an stream of ASCIIZ characters, such as (it really
is not a table as the values are variable):
@0 .asciz "credit"
@6 .asciz "mask"
..
@300 .asciz "S"
And the symbols_token_index:
@0 .short 0
@1 .short 7
@2 .short 12
@4 .short 16
...
@84 .short 300
The relationship between them is that the symbols_token_index
gives us the offset to symbols_token_table.
The symbol_names[] array is a stream of encoded values. Each value
follows the same pattern - <len> followed by <encoding values>.
And the another <len> followed by <encoding values>.
Hence to find the right one you need to read <len>, add <len>
(to skip over), read <len>, add <len>, and so on until one
finds the right tuple offset.
The <encoding values> are the indicies into the symbols_token_index.
Meaning if you have:
0x04, 0x54, 0xda, 0xe2, 0x74
[4, 84, 218, 226, 116 in human numbering]
The 0x04 tells us that the symbol is four bytes past this one (so next
symbol offset starts at 5). If we lookup symbols_token_index[84] we get 300.
symbols_token[300] gets us the "S". And so on, the string eventually
end up being decode to be 'S_stext'. The first character is the type,
then optionally follwed by the filename (and # right after filename)
and then lastly the symbol, such as:
tvpmu_intel.c#core2_vpmu_do_interrupt
Keep in mind that there are two fixed sized tables:
symbols_addresses[0..symbols_num_syms], and
symbols_markers[0..symbols_num_syms/255].
The symbols_markers is used to speed searching for the right address.
It gives us the offsets within symbol_names that start at the <len><encoded value>.
The way to find a symbol based on the address is:
1) Figure out the 'tuple offset' from symbols_address[0..symbols_num_syms].
This table is sorted by virtual addresses so finding the value is simple.
2) Get starting offset of symbol_names by retrieving value of
symbol_markers['tuple offset' / 255].
3). Iterate up to 'tuple_offset & 255' in symbols_markers stream starting
at 'offset'.
4). Decode the <len><encoded value>
This however does not work very well if we want to search the other
way - we have the symbol name and want to find the address.
NEW CODE:
To make that work we add one fixed size table called symbols_sorted_offsets which
has two elements: offset in symbol stream, offset in the symbol-address.
This whole array is sorted on the original symbol name during build-time
(in case of collision we also take into account the type).
The values are for example:
symbols_sorted_offsets:
.long 83363, 6302 # [.bss, len=5]
.long 80459, 6084 # [.data, len=5]
..
[The # added for clarity]
Which makes it incredibly easy to get in the symbols_names and also
symbols_addresses (or symbols_offsets)
Searching for symbols is simplified as we can do a binary search
on symbols_sorted_offsets. Since the symbols are sorted it takes on
average 13 calls to symbols_expand_symbol.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
This change demonstrates how to generate an xSplice ELF payload.
The idea here is that we want to patch in the hypervisor
the 'xen_version_extra' function with an function that will
return 'Hello World'. The 'xl info | grep extraversion'
will reflect the new value after the patching.
To generate this ELF payload file we need:
- C code of the new code (xen_hello_world_func.c).
- C code generating the .xsplice.funcs structure
(xen_hello_world.c)
- The address of the old code (xen_extra_version). We
retrieve it by using 'nm --defined' on xen-syms.
- The size of the new and old code for which we use
nm --defined -S on our code and xen-syms respectively.
There are two C files and one header files generated
during build. One could make this one C file if the
size of the newly patched function size was known in
advance (or an random value was choosen).
There is also a strict order of compiling:
1) xen_hello_world_func.c
2) config.h - extract the size of the new function,
the old function and the old function address.
3) xen_hello_world.c - which contains the .xsplice.funcs
structure.
4) Link the object files in an xen_hello_world.xsplice file.
The use-case is simple:
$xen-xsplice load /usr/lib/debug/xen_hello_world.xsplice
$xen-xsplice list
ID | status
----------------------------------------+------------
xen_hello_world APPLIED
$xl info | grep extra
xen_extra : Hello World
$xen-xsplice revert xen_hello_world
Performing revert: completed
$xen-xsplice unload xen_hello_world
Performing unload: completed
$xl info | grep extra
xen_extra : -unstable
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com> [ARM]
Acked-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
For those users who want to use the virtual addresses that
are in the hypervisor's code/data region address space -
these three new functions allow that.
Implementation wise the vmap API keeps track of two virtual
address regions now:
a) VMAP_VIRT_START
b) Any provided virtual address space (need start and end).
The a) one is the default one and the existing behavior
for users of vmalloc, vmap, etc is the same.
If however one wishes to use the b) one only has to use
the vm_init_type to initialize and the vmzalloc_xen to utilize
it (vfree and vunmap are capable of searching both address spaces).
This allows users (such as xSplice) to provide their own
mechanism to change the the page flags, and also use virtual
addresses closer to the hypervisor virtual addresses (at least
on x86) while not having to deal with the allocation of
pages.
For example of users, see patch titled "xsplice: Implement payload
loading", where we parse the payload's ELF relocations - which
is defined to be signed 32-bit (on x86) (max displacement hence
is 2GB virtual space, ARM32 is 128MB). The displacement of the
hypervisor virtual addresses to the vmalloc (on x86)
is more than 32-bits - which means that ELF relocations would
truncate the 34 and 33th bit. Hence this alternate API.
We also add add extra checks in case the b) range has not been
initialized.
Part of this patch also removes 'vm_alloc' and 'vm_free'
decleration as we do not have any users of it.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Suggested-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <julien.grall@arm.com> [ARM]
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
During execution of the hypervisor we have two regions of
executable code - stext -> _etext, and _sinittext -> _einitext.
The later is not needed after bootup.
We also have various built-in macros and functions to search
in between those two swaths depending on the state of the system.
That is either for bug_frames, exceptions (x86) or symbol
names for the instruction.
With xSplice in the picture - we need a mechanism for new payloads
to searched as well for all of this.
Originally we had extra 'if (xsplice)...' but that gets
a bit tiring and does not hook up nicely.
This 'struct virtual_region' and virtual_region_list provide a
mechanism to search for the bug_frames, exception table,
and symbol names entries without having various calls in
other sub-components in the system.
Code which wishes to participate in bug_frames and exception table
entries search has to only use two public APIs:
- register_virtual_region
- unregister_virtual_region
to let the core code know.
If the ->lookup_symbol is not then the default internal symbol lookup
mechanism is used.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com> [ARM]
Acked-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
A simple tool that allows an system admin to perform
basic xsplice operations:
- Upload a xsplice file (with an unique name)
- List all the xsplice payloads loaded.
- Apply, revert, replace, or unload the payload using the
unique name.
- Do all two - upload, and apply the payload in one go (load).
Also will use the name of the file as the <name>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
The underlaying toolstack code to do the basic
operations when using the XEN_XSPLICE_op syscalls:
- upload the payload,
- get status of an payload,
- list all the payloads,
- apply, check, replace, and revert the payload.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
The implementation does not actually do any patching.
It just adds the framework for doing the hypercalls,
keeping track of ELF payloads, and the basic operations:
- query which payloads exist,
- query for specific payloads,
- check*1, apply*1, replace*1, and unload payloads.
*1: Which of course in this patch are nops.
The functionality is disabled on ARM until all arch
components are implemented.
Also by default it is disabled until the implementation
is in place.
We also use recursive spinlocks to so that the find_payload
function does not need to have a 'lock' and 'non-lock' variant.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
A mechanism is required to binarily patch the running hypervisor with new
opcodes that have come about due to primarily security updates.
This document describes the design of the API that would allow us to
upload to the hypervisor binary patches.
This document has been shaped by the input from:
Martin Pohlack <mpohlack@amazon.de>
Jan Beulich <jbeulich@suse.com>
Thank you!
Input-from: Martin Pohlack <mpohlack@amazon.de>
Input-from: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
This reverts commit 2716d87537.
As it was decided that the existing XENVER hypercall - while having
grown organically over the years can still be expanded.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Requested-and-acked-by: Jan Beulich <jbeulich@suse.com>
Drop paranthesis and function pointer on nmi_callback_t typedef.
Make it more inline with how x86 maintainers want function
typedefs to be.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
It takes a bit of thinking to parse the original statement. It looks
like it is missing an ',' - right after the 'braced' to make it
obvious that:
if (X)
return XYZ;
is OK, while multiple conditionals require braces.
Changing the 'apart from' with 'except' makes it more obvious
(I hope).
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
We change the xen_version libxc code to use the new hypercall.
Which of course means every user in the code base has to
be changed over.
It is important to note that the xc_version_op has a different
return semantic than the previous one. It returns negative
values on error (like the old one), but it also returns
an positive value on success (unlike the old one). The positive
value is the number of bytes copied in.
Note that both Ocaml and xenstat use tabs instead of four
spaces so they look quite odd.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Andrew Cooper <andrew.cooper3@citrix.com> [for the Ocaml stubs]
Acked-by: George Dunlap <george.dunlap@eu.citrix.com> [xenctx bits]
Acked-by: Wei Liu <wei.liu2@citrix.com>
This hypercall mirrors the XENVER_ in that it has similar functionality.
However it is designed differently:
- No compat layer. The data structures are the same size on 32
as on 64-bit.
- The hypercall accepts three arguments - the command, pointer to
an buffer, and the length of the buffer.
- Each sub-ops can be "probed" for size by returning the size of
buffer that will be needed - if the buffer is NULL.
- Subops can complete even if the buffer is too small - truncated
data will be filled and hypercall will return -ENOBUFS.
- VERSION_commandline, VERSION_changeset are privileged.
- There is no XENVER_compile_info equivalent.
- The hypercall can return -EPERM and toolstack/OSes are expected
to deal with. However there are three subops: XEN_VERSION_version,
XEN_VERSION_platform_parameters and XEN_VERSION_get_features
that will always return an value as guests cannot survive without them.
While we combine some of the common code between XENVER_ and VERSION_
take the liberty of moving pae_extended_cr3 in x86 area.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov> [XSM bits]
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
* 'pin' of https://github.com/jgross1/xen:
libxl: add force option for xl vcpu-pin
libxl: print message how to recover from xl cpupool-cpu-remove errors
libxc: do some retries in xc_cpupool_removecpu() for EBUSY case
All patches have Acked-by and Reviewed-by tags.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
So that we have a nice mechansim to figure out the upper
bounds of bug.frames and also catch compiler errors in case
one tries to use a higher frame number.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Stefano Stabellini <stefano.stabellini@citrix.com>
Cc: Julien Grall <julien.grall@arm.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
v3: First time included.
v4: Add BUG_FRAME check also in the assembler version of the macro.
v5: Add Acks, make BUILD_BUG_ON checks look correct. Position the
BUGFRAME_NR properly. Reposition the BUGFRAME_NR again.
---
Most of XENVER_* have now an XSM check for their sub-ops.
The subop for XENVER_commandline is now a priviliged operation.
To not break guests we still return an string - but it is
just '<denied>\0'.
The XENVER_[version|platform_parameters|get_features] - will
always return an value to the guest.
The rest: XENVER_[extraversion|capabilities|page_size|
guest_handle|changeset| compile_info] behave as before -
allowed by default for all guests if using the XSM default
policy or with the dummy one. And if the system admin
wants to curtail access to some of them - they can do
that now with a non-default XSM policy.
Also we add a local variable block.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
---
Cc: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
v2: Do XSM check for all the XENVER_ ops.
- Add empty data conditions.
- Return <denied> for priv subops.
- Move extraversion from priv to normal. Drop the XSM check
for the non-priv subops.
v3:
- Add +1 for strlen(xen_deny()) to include NULL. Move changeset,
compile_info to non-priv subops.
- Remove the \0 on xen_deny()
- Add new XSM domain for xenver hypercall. Add all subops to it.
- Remove the extra line, Add Ack from Daniel
v4:
- Rename the XSM from xen_version_op to xsm_xen_version.
Prefix the types with 'xen' to distinguish it from another
hypercall performing similar operation. Removed Ack from Daniel
as it was so large. Add local variable block.
v5:
- Make XENVER_platform_parameters,get_features,version be excluded
from the XSM check per Jans' review. Add BUILD_BUG_CHECK and fix
odd line removals. Remove stray changes and fix spelling.
The commit bcfaea685d
"arm/config: Declare ELFSIZE_64" was not correct.
For 32-bit ARM, ELFCLASS32 (i.e. 32-bit data types) will always
be used so we need to set ELFSIZE to 32.
Reported-by: Julien Grall <julien.grall@arm.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Julien Grall <julien.grall@arm.com>
The vmap_to_page macro (three levels deep!) can come up with
a NULL pointer. Lets add the proper ASSERT to catch this errant
behavior.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
* 'tracing/sched-events-improvements' of git://xenbits.xen.org/people/dariof/xen:
xenalyze: handle Credit2 scheduler events
xenalyze: handle Credit1 scheduler events
xenalyze: handle scheduling events
xentrace: formats: add domain create and destroy events.
xentrace: formats: add events from RTDS scheduler
xentrace: formats: add events from Credit2 scheduler
xentrace: formats: add events from Credit scheduler
xentrace: formats: update format of scheduling events
On the mailing list all patches have:
Acked-by: George Dunlap <george.dunlap@citrix.com>
and have been
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
so in they go.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
We do not compile 32-bit hypervisor anymore so the code for
the ELFCLASS32 is effectively dead.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Otherwise any code that tries to use Elf_* macros would
require us to use Elf64_* types instead of the more
friendly Elf_ one.
This is OK to do since 32-bit ARM uses LPAE mode.
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
c/s 361b4f9f0f
"build: save generated xen .config" forgot to remove
the config file when uninstalling.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
in the keyhandler.h file. Otherwise on ARM builds if we
just use the keyhandler file - the compile will fail.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To hopefully clarify what it meant. Also point out that mechanism
by which the return 1 value is done is via an intimate knowledge of the
hypercall ABI (i.e. which register - eax - is the return value).
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
The change is simple replace of raw strdup with a libxl variant.
The benefit of that is the libxl variant has the extra
behaviour of abort-on-alloc-fail - and will improve error handling.
libxl_version_info is a bit odd - it is a public function and as libxl.h
mentions - the callers of libxl_ public function needs to call the appropiate
_dispose() function.
"However libxl_get_version_info() is special and returns a cached
result from the ctx which cannot and should not be freed (as evidenced
by it returning a const struct). This data is freed in libxl_ctx_free()
by calling libxl_version_info_dispose(). This is why none of the callers
remember to free -- they shouldn't be doing so." (Ian Campbell)
So the patch makes sure to use the NOGC.
Suggested-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
commit 5d3dc86715
"tools: Refactor "xentoollog" into its own library"
with older python versions (2.6.4) will fail to
the build if attempted to be done twice (which
happens due to pygrub dependencies).
make -C python DESTDIR=/tmp
make -C python DESTDIR=/tmp
The second one will fail with:
error: -Wl, -rpath-link=../../tools/libs/toollog: No such file or directory
even though the directory is there (with the libs).
Andrew pointed out that the linker additions should be in
the "extra_link_args" rather than "depends".
And true enough - with that modification it builds.
CC: Ian Campbell <ian.campbell@citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Suggested-by: Andrew Cooper <andrew.cooper3@citirx.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
[ ijc -- typo in commit message ]
Our 'struct domain' has when lock profiling is enabled is bigger than
one page.
We can't use vmap nor vzalloc as both of those stash the
physical address in struct page which makes the assumptions
in 'arch_init_memory' trip over ASSERTs.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
We must ensure that the prod/cons are only read once and that
the compiler won't try to optimize the reads. That is split
the read of these in multiple instructions influencing later
branch code. As such insert barriers when fetching the cons
and prod index.
This is part of XSA155.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Instead of RING_GET_REQUEST. Using a local copy of the
ring (and also with proper memory barriers) will mean
we can do not have to worry about the compiler optimizing
the code and doing a double-fetch in the shared memory space.
This is part of XSA155.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
It looks to be missing in the policy file for the initial
domain. Eventually we may want to extend this access to
non-dom0 domains but for now it certainly dom0-only.
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
And also remove the extra space. 'gmfn' does not exist in this
function anymore.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
policydb.c: In function ‘user_read’:
policydb.c:1443:26: error: ‘buf[2]’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
usrdatum->bounds = le32_to_cpu(buf[2]);
^
cc1: all warnings being treated as errors
Which (as Andrew mentioned) is because GCC cannot assume
that 'p->policyvers' has the same value between checks.
We make it local, optimize the name to 'ver' and the warnings go away.
We also update another call site with this modification to
make it more inline with the rest of the functions.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Instead of the three member uint64_t structure.
The structure is used by the control stack for
XEN_SYSCTL_TMEM_OP_SAVE_GET_NEXT_[PAGE|INV] only so
moving it to the sysctl header.
Also modified tmemc_save_get_next_page to deal with
the new type - and converted some of the on-stack
usage of the array to use an pointer.
Further work will be to make the xen_sysctl_tmem_op have
an union with proper type for the two: ..GET_NEXT_[PAGE|INV]
operations.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Patch "tmem: Make the uint64_t oid[3] a proper structure:
xen_tmem_oid" converted the sysctl API to use an
proper structure. But it did not do it for the tmem hypercall.
This expands that and converts the tmem hypercall. For this
to work we define the struct in tmem.h and include it in
sysctl.h.
This change also included work to make the compat layer
happy. That was to declare the struct xen_tmem_oid to be
checked in xlat.lst - which will construct an typedef
in the compat file with the same type, hence allowing
copying of 'oid' member without type issues. The kicker
is that the compat layer adds the prefix 'xen' and since
our structure already has it - we must not include it.
The layout (and size) of this structure in memory for the
'struct tmem_op' (so guest facing) is the same! Verified
via pahole and with 32/64 bit guests.
--- /tmp/old 2015-08-27 16:34:00.535638730 -0400
+++ /tmp/new 2015-08-27 16:34:10.447705328 -0400
@@ -8,7 +8,7 @@
uint32_t arg1; /* 28 4 */
} creat; /* 24 */
struct {
- uint64_t oid[3]; /* 8 24 */
+ xen_tmem_oid_t oid; /* 8 24 */
uint32_t index; /* 32 4 */
uint32_t tmem_offset; /* 36 4 */
uint32_t pfn_offset; /* 40 4 */
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Jen Beulich <jbeulich@suse.com>
And use it almost everywhere. It is easy to use it for the
sysctl since the hypervisor and toolstack are intertwined.
But for the tmem hypercall we need to be dilligient (as it
is guest facing) so delaying that to another patch:
"tmem: Use 'struct xen_tmem_oid' for every user" to help
with bisection issues.
We also move some of the parameters on functions to be within
the right location.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Jen Beulich <jbeulich@suse.com>
The sysctl is where the tmem control operations are done and the
XSM checks are done via there. The old mechanism (to check
for control tmem op XSM from do_tmem_op) is not needed anymore.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
The operations are to be used by an control domain to set parameters,
list pools, clients, and to be used during migration.
There is no need to have them in the tmem hypercall path.
This patch moves code without adding fixes - and in fact in
some cases makes the parameters soo long that they hurt eyes - but
that is for another patch.
Note that in regards to existing users:
- Only the control domain could call it - which meant that if
a guest called it would get -EPERM, so we are OK there.
In practice no guests called this TMEM_CONTROL command.
- The spec: https://oss.oracle.com/projects/tmem/dist/documentation/api/tmemspec-v001.pdf
mentions: "TBD [Not sure if this is really needed.]"
which is a carte blanche as any to do this!
Note: The XSM check is the same - we just move it from do_tmem_op
to do_sysctl.
We also add an 32-bit pad to make the sysctl structure have the same
exact size under 32 and 64-bit toolstacks and not worry about aligment
issues.
And the XLAT does not need to deal with the buf as it has been
moved to another structure which is 32/64 fixed.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
Acked-by: Jen Beulich <jbeulich@suse.com>
It mentions it but it is never used. The hypercall interface
knows nothing of this sort of thing either. Lets just remove it.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Wei Liu <wei.liu2@citrix.com> [release + toolstack]
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
We are doing another call to set_xen_guest_handle right
after the xc_hypercall_bounce_pre (the correct place to do it).
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Manipulating the obj-> structures requires us to hold the
pool->rwlock lock. Lets make that obvious in this function to
catch any errant users (none found, but we may in future).
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
This is a regression introduced by a36590e1b5
"tmem: reorg the shared pool allocate path".
When we are using shared pools we have an global array
(on which we put the pool), and an array of pools per domain.
We also have an shared list of clients (guests) _except_
for the very first domain that created the shared pool.
To deal with multiple guests using an shared pool we have an
ref count and a linked list. Whenever an new user of
a the shared pool joins we increase the ref count and add to
the linked list. Whenever an user quits the shared pool
we decrement and remove from the linked list.
Unfortunately this ref counting and linked list never
worked properly. There are multiple issues:
1) If we have one shared pool (and only one guest creating it)
- we do not add it to the shared list of clients. Which
means the logic in 'shared_pool_quit' never removed
the pool from global_shared_pools. That meant when the pool
was de-allocated - we still had an pointer to the pool
which would be accessed by tmemc_list_client (xl tmem-list -a)
and hit a NULL page!
2). If we have two shared pools in a domain - it (shared_pool_quit)
would remove the domain from the share_list linked list, decrements
the refcount to zero - and remove the pool from the global shared pool.
When done it would also clear the client->pools[] to NULL for itself.
Which is good. However since there are two shared pools in the domain
the next entry in the client->pools[] would have a stale pointer to
the just de-allocated pool. Accessing it and trying to de-allocate it
would lead to crashes or hypervisor hang - depending on the build.
Fun times!
To trigger this use
http://xenbits.xen.org/gitweb/?p=xentesttools/bootstrap.git;a=blob;f=root_image/drivers/tmem_test/tmem_test.c
This patch fixes it by making the very first domain that created
an shared pool to follow the same logic as every domain that is
joining a shared pool. That is increment the refcount and also
add itself to the shared list of domains using it.
We also remove an ASSERT that incorrectly assumed
that only one shared pool would exist for a domain.
And to mirror the reporting logic in shared_pool_join
we also add a printk to advertise inter-domain shared pool
joining.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Release-acked-by: Wei Liu <wei.liu2@citrix.com>
Specificially we are pulling in the upstream patch (commit
1b56452121672e6408c38ac8926bdd6998a39004)):
[ath9k] Remove confusing logic inversion in an ANI variable
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
For example on Dell machines we see:
(XEN) 00000fed18000-00000fed19fff type=11 attr=8000000000000000
(XEN) Unknown cachability for MFNs 0xfed18-0xfed19
Let's allow them to be mapped as UC.
We also alter the 'efi-rs' to be 'efi=rs' or 'efi=no-rs'.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Instead of just using -c 0x<some hex value> we can
also use: -c <starting cpu>-<end cpu>, -c <cpu1>,<cpu2>, or a
combination of them, or 'all' for all cpus.
This new format can include just singular CPUs: -c <cpu1>,
or ranges without an start or end (and xentrace will figure out
the values), such as: -c -<cpu2> (which will include cpu0, cpu1,
and cpu2) or -c <cpu2>- (which will include cpu2 and up to MAX_CPUS).
That should make it easier to trace the right CPU if
using this along with 'xl vcpu-list'.
The code has been lifted from the Linux kernel, see file
lib/bitmap.c, function __bitmap_parselist.
To make the old behavior and the new function work, we check
to see if the arguments have '0x' in them. If they do
we use the old style parsing (limited to 32 CPUs). If that
does not exist we use the new parsing.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
We export the xc_cpumap_alloc but not the bit operations.
One could include 'xc_bitops.h' but that is naughty - so instead
we just export the proper functions to do it on the xc_cpumap_t
typedef.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
----
v2: Use our own macro to make sure ARM is not affected negatively
v3: Lifted Ian's explanation.
We have a check to warn the user if they are overcommitting.
But the check only checks the hosts CPU amount and does
not take into account the case when the user is trying to fix
the overcommit. That is - they want to limit the amount of
online VCPUs.
This fix allows the user to offline vCPUs without any
warnings when they are running an overcommitted guest.
Also fix the extra space in the message.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
The check is superflous. If the 'max_vcpus' (argument
value) is greater than pCPU and --ignore-host has not
been supplied we would print an warning and return
and not call this code.
If the --ignore-host parameter had been used we would
never end up in this condition and enforce 'max_vcpus'.
The only time it would be invoked is if max_vcpus < host_cpu
in which case it would set max_vcpus to max_vcpus.
In short - it is dead code.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
The function does not return any values at all. Convert the
internal libxl errors (ERROR_FAIL, ..., etc) to 1.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
There is no sense in trying to online (or offline) CPUs when the size of
cpumap is greater than the maximum number of VCPUs the guest can go to.
As such fail the operation if the count of CPUs to online is greater
than what the guest started with. For the offline case we do not
check (as the bits are unset in the cpumap) and let it go through.
We coalesce some of the underlying libxl_set_vcpuonline code
together which was duplicated in QMP and XenStore codepaths.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Said hypercall for large BARs can take quite a while. As such
we can require that the hypercall MUST break up the request
in smaller values.
Another approach is to add preemption to it - whether we do the
preemption using hypercall_create_continuation or returning
EAGAIN to userspace (and have it re-invocate the call) - either
way the issue we cannot easily solve is that in 'map_mmio_regions'
if we encounter an error we MUST call 'unmap_mmio_regions' for the
whole BAR region.
Since the preemption would re-use input fields such as nr_mfns,
first_gfn, first_mfn - we would lose the original values -
and only undo what was done in the current round (i.e. ignoring
anything that was done prior to earlier preemptions).
Unless we re-used the return value as 'EAGAIN|nr_mfns_done<<10' but
that puts a limit (since the return value is a long) on the amount
of nr_mfns that can provided.
This patch sidesteps this problem by:
- Setting an hard limit of nr_mfns having to be 64 or less.
- Toolstack adjusts correspondingly to the nr_mfn limit.
- If the there is an error when adding the toolstack will call the
remove operation to remove the whole region.
The need to break this hypercall down is for large BARs can take
more than the guest (initial domain usually) time-slice. This has
the negative result in that the guest is locked out for a long
duration and is unable to act on any pending events.
We also augment the code to return zero if nr_mfns instead
of trying to the hypercall.
This is XSA-125 / CVE-2015-2752.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
The commit a8f8a590e0
"libxc: Check xc_domain_maximum_gpfn for negative return values"
introduced an regression in tools outside libxc (migrate v2)
which wanted the unfiltered GPFN value. Said commit added
a wrapper which added +1 if there were no errors.
To make it work pre-commit a8f8a59 we add an xc_domain_nr_gpfns
which will add +1 if there are no errors (and change all in-tree
callers to use it). The xc_domain_maximum_gpfn will return the
unfiltered GPFN value.
Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
instead of the -Exx values (which should go in errno).
This patch has HUGE implications. There is a lot of APIs
that are using do_memory_op. Fortunately most of them
check for 'if (do_memory_op(..) < 0)' so will function
properly. However there were some which printed the return
value to the user. They have been fixed in:
libxc: Don't assign return value to errno for E820 get/set xc_ calls.
libxc: Check xc_sharing_* for proper return values.
libxc: If xc_domain_add_to_physmap fails, include errno value
libxc: Check xc_maximum_ram_page for negative return values.
libxc: Check xc_domain_maximum_gpfn for negative return values
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
We should be using the errno that the hypercall left
instead of overwriting it with the return value.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
If there is a negative return value - check for that and
also use errno for the proper error value.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Instead of assuming everything is always OK. As such
we return now the return value (or zero for success).
The max_mfn is now passed in as the parameter.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Instead of assuming everything is always OK. We stash
the gpfns value as an parameter. Since we use it in three
of places we might as well update xc_domain_maximum_gpfn
to do the right thing.
Suggested-by: Ian Campbell <ian.campbell@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
The API returns now negative values on error and stashes
the error in errno. Fix the user of this API.
The 'xc_hypercall_bounce_pre' can fail - and if so it will
stash its errno values - no need to over-write it.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Oddly enough the user of this API did the right thing -
check for return being negative and used 'errno' for the
real error.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
Instead of returning -Exx we now return -1 for error.
We could stash the -Exx values in errno values but why - the
underlaying functions we call all stash the proper errno
value. Hence we just propagate it up wherver it is needed.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
The users of these (qemu) check for a negative value
so we are safe in regards to that. However they
also use the return value to inform the user of the
error.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
We don't need to put fill errno because xc_hypercall_buffer_alloc
fills the errno with the appropriate errno values and we just
need to pass them up the stack.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
After we have done the hypercall - the errno has the failure
code. However our usage of pthread and munmap can trigger them
to manipulate the errno with their failure values. That would
be bad as what we care about is just the hypercall error value.
Another solution to this would be to save the 'errno' from
pthread/munmap/madvise as an extra parameter to be analyzed
later. However the call-sites above us do not care about it.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
There is race when we clear the STATE_SCHED in the softirq
- which allows the 'raise_softirq_for' (on another CPU or
on the one running the softirq) to schedule the dpci.
Specifically this can happen when the other CPU receives
an interrupt, calls 'raise_softirq_for', and puts the dpci
on its per-cpu list (same dpci structure). Note that
this could also happen on the same physical CPU, however
the explanation for simplicity will assume two CPUs actors.
There would be two 'dpci_softirq' running at the same time
(on different CPUs) where on one CPU it would be executing
hvm_dirq_assist (so had cleared STATE_SCHED and set STATE_RUN)
and on the other CPU it is trying to call:
if ( test_and_set_bit(STATE_RUN, &pirq_dpci->state) )
BUG();
Since STATE_RUN is already set it would end badly.
The reason we can get his with this is when an interrupt
affinity is set over multiple CPUs.
Potential solutions:
a) Instead of the BUG() we can put the dpci back on the per-cpu
list to deal with later (when the softirq are activated again).
This putting the 'dpci' back on the per-cpu list is an spin
until the bad condition clears.
b) We could also expand the test-and-set(STATE_SCHED) in raise_softirq_for
to detect for 'STATE_RUN' bit being set and schedule the dpci.
The BUG() check in dpci_softirq would be replace with a spin until
'STATE_RUN' has been cleared. The dpci would still not
be scheduled when STATE_SCHED bit was set.
c) Only schedule the dpci when the state is cleared
(no STATE_SCHED and no STATE_RUN). It would spin if STATE_RUN is set
(as it is in progress and will finish). If the STATE_SCHED is set
(so hasn't run yet) we won't try to spin and just exit.
Down-sides of the solutions:
a). Live-lock of the CPU. We could be finishing an dpci, then adding
the dpci, exiting, and the processing the dpci once more. And so on.
We would eventually stop as the TIMER_SOFTIRQ would be set, which will
cause SCHEDULER_SOFTIRQ to be set as well and we would exit this loop.
Interestingly the old ('tasklet') code used this mechanism.
If the function assigned to the tasklet was running - the softirq
that ran said function (hvm_dirq_assist) would be responsible for
putting the tasklet back on the per-cpu list. This would allow
to have an running tasklet and an 'to-be-scheduled' tasklet
at the same time.
b). is similar to a) - instead of re-entering the dpci_softirq
we are looping in the softirq waiting for the correct condition to
arrive. As it does not allow unwedging ourselves because the other
softirqs are not called - it is less preferable.
c) can cause an dead-lock if the interrupt comes in when we are
processing the dpci in the softirq - iff this happens on the same CPU.
We would be looping in on raise_softirq waiting for STATE_RUN
to be cleared, while the softirq that was to clear it - is preempted
by our interrupt handler.
As such, this patch - which implements a) is the best candidate
for this quagmire.
Reported-and-Tested-by: Sander Eikelenboom <linux@eikelenboom.it>
Reported-and-Tested-by: Malcolm Crossley <malcolm.crossley@citrix.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
By default we will always use EFI reboot mechanism when
running under EFI platforms. However some EFI platforms
are buggy and need to use the ACPI mechanism to
reboot (such as Lenovo ThinkCentre M57). As such
respect the 'reboot=' override and DMI overrides
for EFI platforms.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
- BOOT_INVALID is just zero
- also consider acpi_disabled in BOOT_INVALID resolution
- duplicate BOOT_INVALID resolution in machine_restart()
- don't fall back from BOOT_ACPI to BOOT_EFI (if it was overridden, it
surely was for a reason)
- adjust doc change formatting
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
.. otherwise it has the side effect that: domain_relinquish_resources
will stop and will return to user-space with -EINTR which it is not
equipped to deal with that error code; or vcpu_reset - which will
ignore it and convert the error to -ENOMEM..
The preemption mechanism we have for domain destruction is to return
-EAGAIN (and then user-space calls the hypercall again) and as such we need
to catch the case of:
domain_relinquish_resources
->vcpu_destroy_pagetables
-> put_page_and_type_preemptible
-> __put_page_type
returns -EINTR
and convert it to the proper type. For:
XEN_DOMCTL_setvcpucontext
-> vcpu_reset
-> vcpu_destroy_pagetables
we need to return -ERESTART otherwise we end up returning -ENOMEM.
There are also other callers of vcpu_destroy_pagetables: arch_vcpu_reset
(vcpu_reset) are:
- hvm_s3_suspend (asserts on any return code),
- vlapic_init_sipi_one (asserts on any return code),
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
commit f6dd295381 "dpci: replace tasklet
with softirq" used the 'masked' as an two-bit state mechanism
(STATE_SCHED, STATE_RUN) to communicate between 'raise_softirq_for' and
'dpci_softirq' to determine whether the 'struct hvm_pirq_dpci' can be
re-scheduled.
However it ignored the 'pt_irq_guest_eoi' was not adhering to the proper
dialogue and was not using locked cmpxchg or test_bit operations and
ended setting 'state' set to zero. That meant 'raise_softirq_for' was
free to schedule it while the 'struct hvm_pirq_dpci'' was still on an
per-cpu list causing an list corruption.
The code would trigger the following path causing list corruption:
\-timer_softirq_action
pt_irq_time_out calls pt_pirq_softirq_cancel sets state to 0.
pirq_dpci is still on dpci_list.
\- dpci_sofitrq
while (!list_emptry(&our_list))
list_del, but has not yet done 'entry->next = LIST_POISON1;'
[interrupt happens]
raise_softirq checks state which is zero. Adds pirq_dpci to the dpci_list.
[interrupt is done, back to dpci_softirq]
finishes the entry->next = LIST_POISON1;
.. test STATE_SCHED returns true, so executes the hvm_dirq_assist.
ends the loop, exits.
\- dpci_softirq
while (!list_emtpry)
list_del, but ->next already has LIST_POISON1 and we blow up.
An alternative solution was proposed (adding STATE_ZOMBIE and making
pt_irq_time_out use the cmpxchg protocol on 'state'), which fixed the above
issue but had an fatal bug. It would miss interrupts that are to be scheduled!
This patch brings back the 'masked' boolean which is used as an
communication channel between 'hvm_do_IRQ_dpci', 'hvm_dirq_assist' and
'pt_irq_guest_eoi'. When we have an interrupt we set 'masked'. Anytime
'hvm_dirq_assist' or 'pt_irq_guest_eoi' executes - it clears it.
The 'state' is left as a seperate mechanism to provide an mechanism between
'raise_sofitrq' and 'softirq_dpci' to communicate the state of the
'struct hvm_dirq_pirq'.
However since we have now two seperate machines we have to deal with an
cancellations and outstanding interrupt being serviced: 'pt_irq_destroy_bind'
is called while an 'hvm_dirq_assist' is just about to service.
The 'pt_irq_destroy_bind' takes the lock first and kills the timer - and
the moment it releases the spinlock, 'hvm_dirq_assist' thunders in and calls
'set_timer' hitting an ASSERT.
By clearing the 'masked' in the 'pt_irq_destroy_bind' we take care of that
scenario by inhibiting 'hvm_dirq_assist' to call the 'set_timer'.
In the 'pt_irq_create_bind' - in the error cases we could be seeing
an softirq scheduled right away and being serviced (though stuck at
the spinlock). The 'pt_irq_create_bind' fails in 'pt_pirq_softirq_reset'
to change the 'state' (as the state is in 'STATE_RUN', not 'STATE_SCHED').
'pt_irq_create_bind' continues on with setting '->flag=0' and unlocks the lock.
'hvm_dirq_assist' grabs the lock and continues one. Since 'flag = 0' and
'digl_list' is empty, it thunders through the 'hvm_dirq_assist' not doing
anything until it hits 'set_timer' which is undefined for MSI. Adding
in 'masked=0' for the MSI case fixes that.
The legacy interrupt one does not need it as there is no chance of
do_IRQ being called at that point.
Reported-by: Sander Eikelenboom <linux@eikelenboom.it>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
The existing tasklet mechanism has a single global spinlock that is
taken every-time the global list is touched. And we use this lock quite
a lot - when we call do_tasklet_work which is called via an softirq and
from the idle loop. We take the lock on any operation on the
tasklet_list.
The problem we are facing is that there are quite a lot of tasklets
scheduled. The most common one that is invoked is the one injecting the
VIRQ_TIMER in the guest. Guests are not insane and don't set the
one-shot or periodic clocks to be in sub 1ms intervals (causing said
tasklet to be scheduled for such small intervalls).
The problem appears when PCI passthrough devices are used over many
sockets and we have an mix of heavy-interrupt guests and idle guests.
The idle guests end up seeing 1/10 of its RUNNING timeslice eaten by
the hypervisor (and 40% steal time).
The mechanism by which we inject PCI interrupts is by hvm_do_IRQ_dpci
which schedules the hvm_dirq_assist tasklet every time an interrupt is
received. The callchain is:
_asm_vmexit_handler
-> vmx_vmexit_handler
->vmx_do_extint
-> do_IRQ
-> __do_IRQ_guest
-> hvm_do_IRQ_dpci
tasklet_schedule(&dpci->dirq_tasklet);
[takes lock to put the tasklet on]
[later on the schedule_tail is invoked which is 'vmx_do_resume']
vmx_do_resume
-> vmx_asm_do_vmentry
-> call vmx_intr_assist
-> vmx_process_softirqs
-> do_softirq
[executes the tasklet function, takes the
lock again]
While on other CPUs they might be sitting in a idle loop and invoked to
deliver an VIRQ_TIMER, which also ends up taking the lock twice: first
to schedule the v->arch.hvm_vcpu.assert_evtchn_irq_tasklet (accounted
to the guests' BLOCKED_state); then to execute it - which is accounted
for in the guest's RUNTIME_state.
The end result is that on a 8 socket machine with PCI passthrough,
where four sockets are busy with interrupts, and the other sockets have
idle guests - we end up with the idle guests having around 40% steal
time and 1/10 of its timeslice (3ms out of 30 ms) being tied up taking
the lock. The latency of the PCI interrupts delieved to guest is also
hindered.
With this patch the problem disappears completly. That is removing the
lock for the PCI passthrough use-case (the 'hvm_dirq_assist' case) by
not using tasklets at all.
The patch is simple - instead of scheduling an tasklet we schedule our
own softirq - HVM_DPCI_SOFTIRQ, which will take care of running
'hvm_dirq_assist'. The information we need on each CPU is which
'struct hvm_pirq_dpci' structure the 'hvm_dirq_assist' needs to run on.
That is simple solved by threading the 'struct hvm_pirq_dpci' through a
linked list. The rule of only running one 'hvm_dirq_assist' for only
one 'hvm_pirq_dpci' is also preserved by having 'schedule_dpci_for'
ignore any subsequent calls for an domain which has already been
scheduled.
== Code details ==
Most of the code complexity comes from the '->dom' field in the
'hvm_pirq_dpci' structure. We use it for ref-counting and as such it
MUST be valid as long as STATE_SCHED bit is set. Whoever clears the
STATE_SCHED bit does the ref-counting and can also reset the '->dom'
field.
To compound the complexity, there are multiple points where the
'hvm_pirq_dpci' structure is reset or re-used. Initially (first time
the domain uses the pirq), the 'hvm_pirq_dpci->dom' field is set to
NULL as it is allocated. On subsequent calls in to 'pt_irq_create_bind'
the ->dom is whatever it had last time.
As this is the initial call (which QEMU ends up calling when the guest
writes an vector value in the MSI field) we MUST set the '->dom' to a
the proper structure (otherwise we cannot do proper ref-counting).
The mechanism to tear it down is more complex as there are three ways
it can be executed. To make it simpler everything revolves around
'pt_pirq_softirq_active'. If it returns -EAGAIN that means there is an
outstanding softirq that needs to finish running before we can continue
tearing down. With that in mind:
a) pci_clean_dpci_irq. This gets called when the guest is being
destroyed. We end up calling 'pt_pirq_softirq_active' to see if it
is OK to continue the destruction.
The scenarios in which the 'struct pirq' (and subsequently the
'hvm_pirq_dpci') gets destroyed is when:
- guest did not use the pirq at all after setup.
- guest did use pirq, but decided to mask and left it in that state.
- guest did use pirq, but crashed.
In all of those scenarios we end up calling 'pt_pirq_softirq_active'
to check if the softirq is still active. Read below on the
'pt_pirq_softirq_active' loop.
b) pt_irq_destroy_bind (guest disables the MSI). We double-check that
the softirq has run by piggy-backing on the existing
'pirq_cleanup_check' mechanism which calls 'pt_pirq_cleanup_check'.
We add the extra call to 'pt_pirq_softirq_active' in
'pt_pirq_cleanup_check'.
NOTE: Guests that use event channels unbind first the event channel
from PIRQs, so the 'pt_pirq_cleanup_check' won't be called as 'event'
is set to zero. In that case we either clean it up via the a) or c)
mechanism.
There is an extra scenario regardless of 'event' being set or not:
the guest did 'pt_irq_destroy_bind' while an interrupt was triggered
and softirq was scheduled (but had not been run). It is OK to still
run the softirq as hvm_dirq_assist won't do anything (as the flags
are set to zero). However we will try to deschedule the softirq if
we can (by clearing the STATE_SCHED bit and us doing the
ref-counting).
c) pt_irq_create_bind (not a typo). The scenarios are:
- guest disables the MSI and then enables it (rmmod and modprobe in
a loop). We call 'pt_pirq_reset' which checks to see if the
softirq has been scheduled. Imagine the 'b)' with interrupts in
flight and c) getting called in a loop.
We will spin up on 'pt_pirq_is_active' (at the start of the
'pt_irq_create_bind') with the event_lock spinlock dropped and waiting
(cpu_relax). We cannot call 'process_pending_softirqs' as it might
result in a dead-lock. hvm_dirq_assist will be executed and then the
softirq will clear 'state' which signals that that we can re-use the
'hvm_pirq_dpci' structure. In case this softirq is scheduled on a
remote CPU the softirq will run on it as the semantics behind an
softirq is that it will execute within the guest interruption.
- we hit once the error paths in 'pt_irq_create_bind' while an
interrupt was triggered and softirq was scheduled.
If the softirq is in STATE_RUN that means it is executing and we should
let it continue on. We can clear the '->dom' field as the softirq has
stashed it beforehand. If the softirq is STATE_SCHED and we are
successful in clearing it, we do the ref-counting and clear the '->dom'
field. Otherwise we let the softirq continue on and the '->dom' field
is left intact. The clearing of the '->dom' is left to a), b) or again
c) case.
Note that in both cases the 'flags' variable is cleared so
hvm_dirq_assist won't actually do anything.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
When an interrupt for an PCI (or PCIe) passthrough device is to be sent
to a guest, we find the appropiate 'hvm_dirq_dpci' structure for the
interrupt (PIRQ), set a bit (masked), and schedule an tasklet.
Then the 'hvm_dirq_assist' tasklet gets called with the 'struct domain'
from where it iterates over the the radix-tree of 'hvm_dirq_dpci' (from
zero to the number of PIRQs allocated) which are masked to the guest
and calls each 'hvm_pirq_assist'. If the PIRQ has a bit set (masked) it
figures out how to inject the PIRQ to the guest.
This is inefficient and not fair as:
- We iterate starting at PIRQ 0 and up every time. That means the PCIe
devices that have lower PIRQs get to be called first.
- If we have many PCIe devices passed in with many PIRQs and if most
of the time only the highest numbered PIRQ get an interrupt (as the
initial ones are for control) we end up iterating over many PIRQs.
But we could do beter - the 'hvm_dirq_dpci' has the field for
'struct domain', which we can use instead of having to pass in the
'struct domain'.
As such this patch moves the tasklet to the 'struct hvm_dirq_dpci' and
sets the 'dom' field to the domain. We also double-check that the
'->dom' is not reset before using it.
We have to be careful with this as that means we MUST have 'dom' set
before pirq_guest_bind() is called. As such we add the
'pirq_dpci->dom = d;' to cover for such cases.
The mechanism to tear it down is more complex as there are two ways it
can be executed:
a) pci_clean_dpci_irq. This gets called when the guest is being
destroyed. We end up calling 'tasklet_kill'.
The scenarios in which the 'struct pirq' (and subsequently the
'hvm_pirq_dpci') gets destroyed is when:
- guest did not use the pirq at all after setup.
- guest did use pirq, but decided to mask and left it in that
state.
- guest did use pirq, but crashed.
In all of those scenarios we end up calling 'tasklet_kill' which
will spin on the tasklet if it is running.
b) pt_irq_destroy_bind (guest disables the MSI). We double-check that
the softirq has run by piggy-backing on the existing
'pirq_cleanup_check' mechanism which calls 'pt_pirq_cleanup_check'.
We add the extra call to 'pt_pirq_softirq_active' in
'pt_pirq_cleanup_check'.
NOTE: Guests that use event channels unbind first the event channel
from PIRQs, so the 'pt_pirq_cleanup_check' won't be called as event
is set to zero. In that case we either clean it up via the a)
mechanism. It is OK to re-use the tasklet when 'pt_irq_create_bind'
is called afterwards.
There is an extra scenario regardless of event being set or not:
the guest did 'pt_irq_destroy_bind' while an interrupt was
triggered and tasklet was scheduled (but had not been run). It is
OK to still run the tasklet as hvm_dirq_assist won't do anything
(as the flags are set to zero). As such we can exit out of
hvm_dirq_assist without doing anything.
Suggested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Currently Xen can only read the configuration file if it is in ASCII
format. If it is in CHAR16 or CHAR8 it will choke. One way to verify
this is to use 'file':
xen.cfg: ASCII text
xen-char16.cfg: Little-endian UTF-16 Unicode text, with CRLF, CR line terminators
The latter is no good.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Commit f74035d3b1 "xl: PCI code cleanups"
introduced an regression where it changed the name from:
/sys/bus/pci/drivers/pciback/do_flr
to
libxl_sprintf(ctx, "%s/pciback/do_flr", SYSFS_PCI_DEV);
And SYSFS_PCI_DEV is "/sys/bus/pci/devices", meaning we would
do /sys/bus/pci/devices/pciback/do_flr. The proper define
should have been SYSFS_PCIBACK_DRIVER, which is what this patch does.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
If the machine has been quite busy it ends up with these messages
printed on the hypervisor console:
(XEN) CPU3: Temperature/speed normal
(XEN) CPU1: Temperature/speed normal
(XEN) CPU0: Temperature/speed normal
(XEN) CPU1: Temperature/speed normal
(XEN) CPU0: Temperature/speed normal
(XEN) CPU2: Temperature/speed normal
(XEN) CPU3: Temperature/speed normal
(XEN) CPU0: Temperature/speed normal
(XEN) CPU2: Temperature/speed normal
(XEN) CPU3: Temperature/speed normal
(XEN) CPU1: Temperature/speed normal
(XEN) CPU0: Temperature above threshold
(XEN) CPU0: Running in modulated clock mode
(XEN) CPU1: Temperature/speed normal
(XEN) CPU2: Temperature/speed normal
(XEN) CPU3: Temperature/speed normal
While the state changes are important, the non-altered state
information is not needed. As such add a latch mechanism to only print
the information if it has changed since the last update (and the
hardware doesn't properly suppress redundant notifications).
This was observed on Intel DQ67SW,
BIOS SWQ6710H.86A.0066.2012.1105.1504 11/05/2012
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Christoph Egger <chegger@amazon.de>
Which of course has a different model number and sports two
serial outputs.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Acked-by: Keir Fraser <keir@xen.org>
This covers all of the OXPCIe952 1 Native UART and
OXPCIe200 1 Native UART chipsets.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Instead of having hard-coded values. We only do PCI vendors
as Jan requested and put all PCI device vendors in one
new file.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Sorted them based on their numerical values per Jan's review]
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
This will allow us to re-use the parameters for multiple PCI
devices.
No functional change.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: s/nr/idx/ of the enum, use __initconst and const by Jan's review]
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Because they are PCIe and machine nowadys have those instead of
PCI, and they are inexpensive.
Tested with 1415:c138. Should also work on 0xc11f, 0xc11b models
of that chip.
Also on OXPCIe200 1 Native UART 1415: 0xc40b, 0xc40f, 0xc41b,
0xc41f, 0xc42b, 0xc42f, 0xc43b, 0xc43f, 0xc44b, 0xc44f, 0xc45b
0xc45f, 0xc46b, 0xc46f, 0xc47b, 0xc47f, 0xc48b, 0xc48f, 0xc49b
0xc49f, 0xc4ab, 0xc4af, 0xc4bb, 0xc4bf, 0xc4cb, 0xc4cf
but since I don't have any of those cards this patch does not
enable it.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Init for ARM and add offset to virt addr]
[v2: Remove the offset usage]
Tested-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The io_base by default is set to be 0x3f8 for COM1 and 0x2f8 for COM2
in __setup_xen. Then we call 'ns16550_init' which copies those in
the appropriate uart, which then calls 'ns16550_parse_port_config'
to deal with parameter parsing. If the 'amt' parameter has been
specified we further call 'pci_uart_config code' which scans the PCI bus.
If it does not find the AMT device it would overwrite the io_base with
0x3f8 regardless whether this is COM1 or COM2 - but only if 'amt'
parameter had been specified.
The overwrite is a way to set it back to the failsafe defaults -
except for COM2 it is bogus.
Note again - if an AMT card is found, this over-write will not happen.
This in theory (as I don't have a machine with two COM ports
readily available) means that if the user specified 'com2=9600,8n1,amt'
and the device did not have an AMT serial device, instead of using
0x2f8 for the io_base it ends up using 0x3f8 - and we don't get the
output on COM2. If the user had done 'com2=9600,8n1' we would never
get in this path so this bug would never manifest itself
(because we don't end up scanning for the AMT device).
We also unconditionally reset the IRQ value - so we would never get the
proper interrupt when falling back to the legacy 0x3f8 and 0x2f8 COM ports.
That is OK - as we would end up using the polling mode - while
not the best - it still would work.
Lastly the clock_hz is also set to the default one (UART_CLOCK_HZ,
which is the same for legacy COM1 and COM2 ports)- that is strictly
not a bug, but it is redundant and not needed.
This bug was introduced with the original AMT support and I cannot
recall why it was done that way - it is a bug.
Fix it by saving the original io_base before starting the
scan of the PCI bus. If we don't find an serial PCI device (because
we did not exit out of the loop using return) then
assign the original io_base value back.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
[v1: Also remove the irq override spotted by Jan]
[v2: Add more details to the commit description]
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The "ns16550: Add support for UART present in Broadcom TruManage
capable NetXtreme chips" implies that only devices that are have
an MMIO BAR and are in the quirks table should be processed.
Even the comment at the end says so:
If we have an io_base, then we succeeded in the lookup
But the code was checking for the !io_base - which is to say if
the io_base was 0 then we would skip scanning. But io_base
always has a value - it is set by 'ns16550_init' to a default
value - so it would never hit the 'continue' path.
This means that if we have an communication device followed by
a serial AMT device we would pick the communication device instead
of the AMT device.
See:
00:16.0 Communication controller: Intel Corporation Cougar Point HECI Controller #1 (rev 04)
Subsystem: Intel Corporation Device 2008
Flags: bus master, fast devsel, latency 0, IRQ 11
Memory at fb12a000 (64-bit, non-prefetchable) [size=16]
00:16.3 Serial controller: Intel Corporation Cougar Point KT Controller (rev 04) (prog-if 02 [16550])
Subsystem: Intel Corporation Device 2008
Flags: bus master, 66MHz, fast devsel, latency 0, IRQ 17
I/O ports at f0e0 [size=8]
Memory at fb129000 (32-bit, non-prefetchable) [size=4K]
pci 0000:00:16.0: [8086:1c3a] type 00 class 0x078000
pci 0000:00:16.3: [8086:1c3d] type 00 class 0x070002
And Xen picks 00:16.0 as its console when using 'com1=115200,8n1,amt'.
This patch fixes it and allows us to use AMT again by zeroing
out io_base to zero. If the scan did not work, the io_base is
set back to a default value (the 'pci_uart_config' does that
already at its end).
Tested-by: Aravind Gopalakrishnan <Aravind.Gopalakrishnan@amd.com>
CC: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
CC: Thomas Lendacky <Thomas.Lendacky@amd.com>
CC: Keir Fraser <keir@xen.org>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
The console_io hypercall is provided for PV guests and for HVM
guests it is done via the 0xe9 port. However the PV hypercall
is more efficient as it takes a string rather than one character
per write.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
We do the microcode code update in two steps - the presmp:
'microcode_presmp_init' and when CPUs are brought up: 'microcode_init'.
The earlier performs the microcode update on the BSP - but
unfortunately it does not check whether the update failed. Which means
that we might try later to update a incorrect payload on the rest of
CPUs.
This patch handles this odd situation.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Keir Fraser <keir@xen.org>
