This resolves 11 Coverity issues along the lines of the following:
1600 for ( i = 0; i < NR_RESERVED_GDT_PAGES; i++ )
CID: Operands don't affect result
(CONSTANT_EXPRESSION_RESULT)result_independent_of_operands: ((33U /* 1U |
0x20U */) | (({...}) ? 8388608U /* 1U << 23 */ : 0) | 0x40U | 2U) & 4095
is always 0x63 regardless of the values of its operands. This occurs as
the bitwise second operand of "|".
1601 l1e_write(pl1e + FIRST_RESERVED_GDT_PAGE + i,
1602 l1e_from_pfn(mfn + i, __PAGE_HYPERVISOR_RW));
This is presumably because once preprocessed, the association of joint logic
inside {get,set}_pte_flags() is lost.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
In some directories we use gcc on source files elsewhere, to generate
a .o here in the current directory. Eg in tools/libxl/,
gcc -I -o build.o /path/to/libacpi/build.c
We pass -MMD and -MF options to generate a .d file right here.
In the general case this .c file might need to include things from the
directory here, eg libacpi/build.c eventually #includes various
*libxl*.h. We pass gcc -I. for this, which means things from the cwd
where we invoked gcc, not the directory of the #including file.
When we do this, gcc's -MMD output mentions /path/to/libxl/*libxl*.h,
even though it could refer to simply *libxl*.h. This is presumably
because gcc has noticed that `.' in this context must mean relative to
the invocation cwd, not relative to build.c, and gcc doesn't realise
that references in the .d file are also wrt the invocation cwd.
make distinguishes targets purely textually. It will canonicalise a
target name by removing ./ before comparison (so _libxl_types.h and
./_libxl_types.h are considered the same target) but it won't examine
the filesystem. So _libxl_types.h and
/path/to/tools/libxl/_libxl_types.h are different targets.
And, _libxl_types.h is generated from a pattern rule. This pattern
rule is therefore instatiated twice, and the two instances may be run
concurrently - but use the same tempfiles and can therefore fail.
The thing that is wrong here is gcc's choice to output an absolute
path.
We could work around it by adding a rule to teach make about a
relationship between these `two different files'. But this has to be
done for every autogenerated file and is therefore fragile (leaving a
race bug when we get it wrong).
Ideally we would fix the problem by fixing the .d file as it is
generated. But the .d files are generated by many many rules
mentioning $(CC) and $(CFLAGS). (We might in theory pass a bash
process substitution to -MF, but 1. that's not portable to people who
don't have bash and 2. it hangs, anyway.)
So instead we do this conversion at include time. That is, we tell
make to include not the raw .d files, but the sedded ones.
The sedding removes occurrences of ` $PWD/'. We use the shell
variable PWD because the make variable sometimes refers to the xen
toplevel. If gcc's output format should change, then this sed rune
may not work any more, but that doesn't seem very likely.
The rune is only effective for dependencies on files which are exactly
in the current directory, or a subdirectory of it named simply by its
subdirectory name. If there are autogenerated include files which
exist in a sibling (or worse, somewhere completely else), this
approach will not work, because we'd have to figure out what name this
Makefile usually uses to refer to them. Hopefully such things don't
exist.
The indirect variables DEPS_RM and DEPS_INCLUDE are necessary to
preserve the assumptions made in the various Makefiles. Specifically,
xen/ Makefiles assume that it is ok to say DEPS+=something (where
something is in a subdirectory); tools/ Makefiles all used to include
DEPS themselves (but now they include DEPS_INCLUDE); and many
Makefiles tended to explictly rm DEPS (but now rm DEPS_RM).
In the new scheme of things: DEPS is the files that come out of gcc
(or perhaps an assembler or something) and may be assigned to by
Makefiles. DEPS_INCLUDE is the processed form. And DEPS_RM is both
combined, so that they both get cleaned.
We need to explicitly use $(wildcard ) to do the wildcard expansion on
DEPS a bit earlier. If we didn't, then DEPS_INCLUDE would contain
`.*.d2' which would not exist.
Evaluation order: DEPS_RM and DEPS_INCLUDE are recursively expanded
variables, so that although they are defined early (in Config.mk),
their actual values are computed at the time of use, using the value
of DEPS that is prevailing at that time.
Reported-by: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
DEPS_INCLUDE is currently the same as DEPS, so no functional change.
This patch is the result of this perl rune:
git-grep -l 'include.*DEPS' | xargs perl -i -pe 'next unless m/^-?include/; s/\bDEPS\b/DEPS_INCLUDE/'
I have verified that I haven't missed anything, with this rune:
git-grep '\bDEPS\b'
Reported-by: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
DEPS_RM is currently the same as DEPS, so no functional change.
This patch is the result of two perl runes:
git-grep -l 'rm.*DEPS' | xargs perl -i~ -pe 'next unless m/^\t+rm\b/; s/\bDEPS\b/DEPS_RM/;'
git-grep -l 'RM.*DEPS' | xargs perl -i~ -pe 'next unless m/^\t+\$\(RM\)/; s/\bDEPS\b/DEPS_RM/;'
And editing tools/xenstat/libxenstat/Makefile by hand.
I verified that I didn't miss anything with this rune:
git-grep '\bDEPS\b' | grep -v include |less
Reported-by: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Ian Jackson <Ian.Jackson@eu.citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Instead, preserve PGC_need_scrub bit when setting PGC_state_inuse
state while still under the lock and clear those pages later.
Note that we still need to grub the lock when clearing PGC_need_scrub
bit since count_info might be updated during MCE handling in
mark_page_offline().
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Due to historical reason, type of parameter '*nr' in 'libxl_psr_cat_get_info'
is 'int'. But this is not right. It should be 'unsigned int'. This patch fixes
this and does related changes.
Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
The libxl interfaces and related functions are not necessary to be included by
'LIBXL_HAVE_PSR_CMT' and 'LIBXL_HAVE_PSR_CAT'. So replace them to common x86
macros. Furthermore, only compile 'xl_psr.c' under x86.
Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Suggested-by: Wei Liu <wei.liu2@citrix.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
For XEN_SMEP and XEN_SMAP to not be cleared while bringing up APs we'd
need to clone the respective hack used for CPUID_FAULTING. Introduce an
inverse of setup_clear_cpu_cap() instead, but let clearing of features
overrule forced setting of them.
XEN_SMAP being wrong post-boot is a problem specifically for live
patching, as a live patch may need alternative instruction patching
keyed off of that feature flag.
Reported-by: Sarah Newman <security@prgmr.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
asmlinkage is defined as nothing on all architectures, and not used
consistently anywhere, even in common code. Remove it all.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
The returned value represents now units of bytes instead of longs.
Fixes commit 11d0044a16 ("tools/libxc: Modify bitmap operations to
take void pointers").
Signed-off-by: Olaf Hering <olaf@aepfle.de>
Acked-by: Wei Liu <wei.liu2@citrix.com>
The patch splits the vm_event into three structures:vm_event_share,
vm_event_paging, vm_event_monitor. The allocation for the
structure is moved to vm_event_enable so that it can be
allocated/init when needed and freed in vm_event_disable.
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
While these are latent issues only for now, correct them right away:
- unnamed (in the SDM) EVEX bits need to be set/clear respectively
- EVEX.V' (called RX in our code) needs to uniformly be 1 in non-64-bit
modes,
- EXEX.R' (called R in our code) is uniformly being ignored in
non-64-bit modes.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Recent changes to the SDM (and XED) have made clear that older hardware
raising #UD when the bit is set was really an erratum. Generalize the
so far AMD-only override.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Going though the XED commits from the last couple of months made me
notice that VPINSRD, other than VPEXTRD, does not clear VEX.W for non-
64-bit modes, leading to an insertion of stray 32-bits of zero in case
the original instruction had the bit set.
Also remove a pointless fall-through in VPEXTRW handling, bringing
things in line with VPINSRW.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Grp7 is abnormally complicated to decode, even by x86's standards, with
{s,l}msw being the problematic cases.
Previously, any value which fell through the first switch statement (looking
for instructions with entirely implicit operands) would be interpreted by the
second switch statement (handling instructions with memory operands).
Unimplemented instructions would then hit the #UD case for having a non-memory
operand, rather than taking the cannot_emulate path.
Consolidate the two switch statements into a single one, using ranges to cover
the instructions with memory operands.
Reported-by: Petre Pircalabu <ppircalabu@bitdefender.com>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <JBeulich@suse.com>
This in turn calls for p2m_alloc_ptp() also being passed the numeric
level.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Calculate entry PFN and flags just once. Convert the two successive
main if()-s to and if/else-if chain. Restrict variable scope where
reasonable. Take the opportunity and also make the induction variable
unsigned.
This at once fixes excessive permissions granted in the 2M PTEs
resulting from splitting a 1G one - original permissions should be
inherited instead. This is not a security issue only because all of
this takes no effect anyway, as iommu_hap_pt_share is always false on
AMD systems for all supported branches.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
There is no need for these to be macros, and the result is easier to read.
No functional change, but bloat-o-meter reports the following improvement:
add/remove: 1/0 grow/shrink: 2/3 up/down: 235/-427 (-192)
function old new delta
__get_page_type 5231 5351 +120
adjust_guest_l1e.isra - 96 +96
free_page_type 1540 1559 +19
ptwr_emulated_update 1008 957 -51
create_grant_pv_mapping 1342 1186 -156
mod_l1_entry 1892 1672 -220
adjust_guest_l1e(), now being a compiler-visible single unit, is chosen for
out-of-line'ing from its several callsites. The other helpers remain inline.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
I've recently got sufficiently annoyed by people not applying enough
common sense to get_maintainer.pl output, Cc-ing all REST maintainers
on ARM-only public interface changes.
Sort ARM's xen/ groups of path specifications at the same time.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Provide a unified entry to avoid going through pte look-up, decode and
emulation cycle more than necessary. The path taken is determined by
the faulting address.
Note that the order of checks is changed in the new function, but the
order of the checks is performed shouldn't matter.
The sole caller is changed to use the new function.
No functional change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Rewrite the code so that it has the same structure as
mmio_ro_emualte_ctxt. x86_emulate_ctxt now points to ptwr_emulate_ctxt
via its data pointer.
This patch will help unify mmio_ro and ptwr code paths later.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Xen and qemu having identical #define-s (with different names) is a
strong hint that these should be part of the public interface, at the
same time making obvious that any change to the values in an interface
modification (and hence needs suitable care).
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Today the maximum record lenth in a migration stream is 8MB. This
limits the size of a PV domain to a little bit less than 1TB in the
migration case, as the P2M frame list will exceed 8MB in this case.
Raising the record size limit by a factor of 16 allows for domain
sizes of nearly 16TB to be migrated. This ought to be enough.
Signed-off-by: Juergen Gross <jgross@suse.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
To match our naming convention. Since we released p9 one release ago,
we need to define a macro inside libxl.h to indicate the change.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
There's no reason for the first Mb to be excluded here. Enforce the
restriction right in the top level page table entries.
Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
From the vcpu_guest_context structure. PVHv2 uses it in the same exact
way as HVM guests, and from the hypervisor point of view PVHv2 is not
even a different guest type, so only mention HVM in the public
headers.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
There is no need to scrub pages freed during dom0 construction since
once dom0 is ready the heap will be scrubbed by scrub_heap_pages() anyway,
setting scrub_debug at the end.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
If scrub_debug is off we don't check pages in check_one_page().
Thus there is no reason to ever poison them.
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Rename it to the more appropriate scrub_debug and define as a macro
for !CONFIG_SCRUB_DEBUG. This will allow us to get rid of some
ifdefs (here and in the subsequent patch).
Suggested-by: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
scrub_heap_pages() does early return if boot-time scrubbing is
disabled, neglecting to initialize lowmem VIRQ.
Because setup_low_mem_virq() doesn't logically belong in
scrub_heap_pages() we put them both into the newly added
heap_init_late().
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
We need to choose ACPI tables properly depending on the device
model version we are running. Previously, this decision was
made by BIOS type specific code in hvmloader, e.g. always load
QEMU traditional specific tables if it's ROMBIOS and always
load QEMU Xen specific tables if it's SeaBIOS.
This change saves this behavior (for compatibility) but adds
an additional way (xenstore key) to specify the correct
device model if we happen to run a non-default one. Toolstack
bit makes use of it.
The enforcement of BIOS type depending on QEMU version will
be lifted later when the rest of ROMBIOS compatibility fixes
are in place.
Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
When SR-IOV is enabled, 'Virtual Functions' of a 'Physical Function'
are under the scope of the same VT-d unit as the 'Physical Function'.
A 'Physical Function' can be a 'Traditional Function' or an ARI
'Extended Function'. And furthermore, 'Extended Functions' on an
endpoint are under the scope of the same VT-d unit as the 'Traditional
Functions' on the endpoint. To search VT-d unit for a VF, if its PF
isn't an extended function, the BDF of PF should be used. Otherwise
the BDF of a traditional function in the same device with the PF
should be used.
Current code uses PCI_SLOT() to recognize an ARI 'Extended Funcion'.
But it is conceptually wrong w/o checking whether PF is an extended
function and would lead to match VFs of a RC integrated PF to a wrong
VT-d unit.
This patch overrides VF 'is_extfn' field and uses this field to
indicate whether the PF of this VF is an extended function. The field
helps to use correct BDF to search VT-d unit.
Reported-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Crawford, Eric R <Eric.R.Crawford@intel.com>
In sysctl.c, the return value of 'psr_get_info' has been checked immediately.
So, it is redundant to check the return value again when copy the field to
guest.
Suggested-by: Roger Pau Monné <roger.pau@citrix.com>
Signed-off-by: Yi Sun <yi.y.sun@linux.intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
There is only one function gate_op_read that needs to be modified.
Rename it to read_mem.
Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
The current implementation configures the test environment to always
trap on writes to the CR4 control register, even on ARM. This leads to
issues as calling xc_monitor_write_ctrlreg on ARM with VM_EVENT_X86_CR4
will always fail.
Signed-off-by: Sergej Proskurin <proskurin@sec.in.tum.de>
Acked-by: Razvan Cojocaru <rcojocaru@bitdefender.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
Coverity complains that gl1e.l1 may be used while uninitialised in
map_ldt_shadow_page(). This isn't actually accurate as guest_get_eff_l1e()
will always write to its parameter.
However, having a void function which returns a 64bit value via pointer is
rather silly. Rearrange the functions to return l1_pgentry_t.
No functional change, but hopefully should help Coverity not to come to the
wrong conclusion.
Bloat-o-meter also reports a modest improvement:
add/remove: 0/0 grow/shrink: 0/4 up/down: 0/-71 (-71)
function old new delta
guest_get_eff_l1e 82 75 -7
mmio_ro_do_page_fault 530 514 -16
map_ldt_shadow_page 501 485 -16
ptwr_do_page_fault 615 583 -32
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
On the CPU where a callback is queued, cpu_is_haltable()
returns false (due to rcu_needs_cpu() being itself false).
That means the CPU would spin inside idle_loop(), continuously
calling do_softirq(), and, in there, continuously checking
rcu_pending(), in a tight loop.
Let's instead allow the CPU to really go idle, but make sure,
by arming a timer, that we periodically check whether the
grace period has come to an ended. As the period of the
timer, we pick a value that makes thing look like what
happens in Linux, with the periodic tick (as this code
comes from there).
Note that the timer will *only* be armed on CPUs that are
going idle while having queued RCU callbacks. On CPUs that
don't, there won't be any timer, and their sleep won't be
interrupted (and even for CPUs with callbacks, we only
expect an handful of wakeups at most, but that depends on
the system load, as much as from other things).
Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Wei Liu