Re: [XEN PATCH 03/12] x86/vm_event: address violation of MISRA C Rule 16.3
On Tue, Sep 10, 2024 at 6:09 AM Federico Serafini wrote: > > Address a violation of MISRA C:2012 Rule 16.3: > "An unconditional `break' statement shall terminate every > switch-clause". > > No functional change. > > Signed-off-by: Federico Serafini Acked-by: Tamas K Lengyel
Re: [XEN PATCH 05/12] x86/monitor: address violation of MISRA C Rule 16.3
On Tue, Sep 10, 2024 at 6:09 AM Federico Serafini wrote: > > Address a violation of MISRA C:2012 Rule 16.3: > "An unconditional `break' statement shall terminate every > switch-clause". > > No functional change. > > Signed-off-by: Federico Serafini Acked-by: Tamas K Lengyel
[PATCH] oss-fuzz: Fix coverage runtime error
The oss-fuzz infrastructure collects runtime coverage information for debugging and fuzzing evaluation. Currently it appears broken due to missing C files. This is because the fuzzer's Makefile only symlinks the C files from various locations in the Xen source tree into the build folder. These symlinks however are gone as oss-fuzz uses separate docker containers for the build and for the run. Update the oss-fuzz build script to copy the required C files into the build folder to fix this oss-fuzz specific issue. Signed-off-by: Tamas K Lengyel --- tools/fuzz/oss-fuzz/build.sh | 4 1 file changed, 4 insertions(+) diff --git a/tools/fuzz/oss-fuzz/build.sh b/tools/fuzz/oss-fuzz/build.sh index 08eeb66e4c..002d86c44f 100644 --- a/tools/fuzz/oss-fuzz/build.sh +++ b/tools/fuzz/oss-fuzz/build.sh @@ -9,3 +9,7 @@ cd xen make clang=y -C tools/include make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness cp tools/fuzz/x86_instruction_emulator/libfuzzer-harness $OUT/x86_instruction_emulator + +# Runtime coverage collection requires access to source files and symlinks don't work +cp xen/lib/x86/*.c tools/fuzz/x86_instruction_emulator +cp tools/tests/x86_emulator/*.c tools/fuzz/x86_instruction_emulator -- 2.34.1
Re: [XEN PATCH v5 02/13] x86/monitor: guard altp2m usage
On Tue, Jul 30, 2024 at 6:18 AM Sergiy Kibrik wrote: > > Explicitly check whether altp2m is on for domain when getting altp2m index. > If explicit call to altp2m_active() always returns false, DCE will remove > call to altp2m_vcpu_idx(). > > p2m_get_mem_access() expects 0 as altp2m_idx parameter when altp2m not active > or not supported, so 0 is a fallback value then. > > The puspose of that is later to be able to disable altp2m support and "purpose" misspelled here > exclude its code from the build completely, when not supported by target > platform (as of now it's supported for VT-d only). Probably wanted to say "VT-x only" here > > Also all other calls to altp2m_vcpu_idx() are guarded by altp2m_active(), so > this change puts usage of this routine in line with the rest of code. > > Signed-off-by: Sergiy Kibrik With the commit message fixed (probably could be done when applied): Acked-by: Tamas K Lengyel
Re: [PATCH v3 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Mon, Jul 22, 2024 at 9:57 AM Jan Beulich wrote: > > On 22.07.2024 15:51, Tamas K Lengyel wrote: > > On Mon, Jul 22, 2024 at 8:24 AM Jan Beulich wrote: > >> > >> On 22.07.2024 13:27, Tamas K Lengyel wrote: > >>> This target enables integration into oss-fuzz. Changing invalid input > >>> return > >>> to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding the > >>> missing __wrap_vsnprintf wrapper which is required for successful oss-fuzz > >>> build. > >>> > >>> Signed-off-by: Tamas K Lengyel > >>> --- > >>> v3: don't include libfuzzer-harness in target 'all' as it requires > >>> specific cc > >> > >> With this, how is it going to be built at all? Only by invoking the special > >> target "manually" as it seems? Which sets this up for easy bit-rotting. I > >> wonder what others think here ... > > > > Yes, by calling make with the specific target. It's not going to > > bitrot because oss-fuzz will pick up any regression on a daily basis > > to this target. I assume you would be interested in receiving the > > fuzzing reports so it would show as a build failure in case something > > broke it. > > Please forgive my lack of knowledge here, but which part of whose > infrastructure would pick up stuff in a daily basis, and what fuzzing > reports (I've never seen any, daily or not) are you talking about? > For now it feels to me as if you're talking of what's possible down > the road, not what's going to happen from the moment this patch was > committed in a 2nd try. If so, the gap between both points in time > may be significant, and hence my bit-rotting concern would still > apply. Once these two patches are merged to Xen I'll send my PR to oss-fuzz to have it pull Xen daily and build this fuzzer and run it on their infrastructure. It usually takes them less than 24 hours to respond to PRs, I have added multiple projects there already so the "lag" you worry about it's not something to worry about. Having these two patches upstream in Xen is not required by the way, I could just send these to be upstream to oss-fuzz and have them apply them after it pulling the xen git repo but it gives more flexibility to you guys to add additional fuzzers in the future more easily if these are in your repository because you don't even have to touch oss-fuzz afterwards. If you need to learn more about what oss-fuzz is and how it operates they have a quite nice documentation. Tamas
Re: [PATCH v3 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Mon, Jul 22, 2024 at 8:24 AM Jan Beulich wrote: > > On 22.07.2024 13:27, Tamas K Lengyel wrote: > > This target enables integration into oss-fuzz. Changing invalid input return > > to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding the > > missing __wrap_vsnprintf wrapper which is required for successful oss-fuzz > > build. > > > > Signed-off-by: Tamas K Lengyel > > --- > > v3: don't include libfuzzer-harness in target 'all' as it requires specific > > cc > > With this, how is it going to be built at all? Only by invoking the special > target "manually" as it seems? Which sets this up for easy bit-rotting. I > wonder what others think here ... Yes, by calling make with the specific target. It's not going to bitrot because oss-fuzz will pick up any regression on a daily basis to this target. I assume you would be interested in receiving the fuzzing reports so it would show as a build failure in case something broke it. Tamas
Re: [PATCH v3 2/2] Add tools/fuzz/oss-fuzz/build.sh
On Mon, Jul 22, 2024 at 8:20 AM Jan Beulich wrote: > > On 22.07.2024 13:27, Tamas K Lengyel wrote: > > --- /dev/null > > +++ b/LICENSES/Apache-2.0 > > @@ -0,0 +1,202 @@ > > + > > + Apache License > > + Version 2.0, January 2004 > > +http://www.apache.org/licenses/ > > Better https:// (also near the end) nowadays? This is a copy of https://www.apache.org/licenses/LICENSE-2.0.txt and I'm not going to start modifying it even for such trivial reasons. > Other files in this directory also have at least one Valid-License-Identifier: > line and at least one SPDX-URL: one. I didn't notice but should be trivial to add. Tamas
Re: [PATCH v2 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Mon, Jul 22, 2024 at 7:34 AM Jan Beulich wrote: > > On 22.07.2024 13:29, Tamas K Lengyel wrote: > > On Mon, Jul 22, 2024 at 7:08 AM Jan Beulich wrote: > >> > >> On 22.07.2024 13:03, Tamas K Lengyel wrote: > >>> On Mon, Jul 22, 2024 at 5:20 AM Jan Beulich wrote: > >>>> > >>>> On 26.06.2024 00:47, Tamas K Lengyel wrote: > >>>>> This target enables integration into oss-fuzz. Changing invalid input > >>>>> return > >>>>> to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding > >>>>> the > >>>>> missing __wrap_vsnprintf wrapper which is required for successful > >>>>> oss-fuzz > >>>>> build. > >>>>> > >>>>> Signed-off-by: Tamas K Lengyel > >>>> > >>>> I've reverted this right away, because of ... > >>>> > >>>>> @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o > >>>>> afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) > >>>>> cpuid.o wrappers.o > >>>>> $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix > >>>>> -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > >>>>> > >>>>> +libfuzzer-harness: $(OBJS) cpuid.o wrappers.o > >>>>> + $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer > >>>>> $(addprefix -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > >>>> > >>>> ... this causing > >>>> > >>>> gcc: error: unrecognized argument to '-fsanitize=' option: 'fuzzer' > >>>> make[6]: *** [Makefile:62: libfuzzer-harness] Error 1 > >>>> > >>>> with apparently a fair set of gcc-s used by distro-s we use for CI. > >>> > >>> Well let me see if I can hack the Makefile to only build this with clang.. > >> > >> Oh, and - please don't special case Clang. Instead please check for option > >> availability (e.g. using cc-option), such that for possible future gcc, > >> when support there may have been added, we'd then build it there as well. > > > > I decided to just not include the libfuzzer harness in the default 'all' > > target. > > Hmm. I'll look (and comment) there, but I'm not sure that's a route we want to > take. Goals generally ought to work or be unavailable, I'm inclined to say. That Makefile already has targets that are not part of all so I don't think it's a big deal. Tamas
Re: [PATCH v2 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Mon, Jul 22, 2024 at 7:08 AM Jan Beulich wrote: > > On 22.07.2024 13:03, Tamas K Lengyel wrote: > > On Mon, Jul 22, 2024 at 5:20 AM Jan Beulich wrote: > >> > >> On 26.06.2024 00:47, Tamas K Lengyel wrote: > >>> This target enables integration into oss-fuzz. Changing invalid input > >>> return > >>> to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding the > >>> missing __wrap_vsnprintf wrapper which is required for successful oss-fuzz > >>> build. > >>> > >>> Signed-off-by: Tamas K Lengyel > >> > >> I've reverted this right away, because of ... > >> > >>> @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o > >>> afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) > >>> cpuid.o wrappers.o > >>> $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix > >>> -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > >>> > >>> +libfuzzer-harness: $(OBJS) cpuid.o wrappers.o > >>> + $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $(addprefix > >>> -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > >> > >> ... this causing > >> > >> gcc: error: unrecognized argument to '-fsanitize=' option: 'fuzzer' > >> make[6]: *** [Makefile:62: libfuzzer-harness] Error 1 > >> > >> with apparently a fair set of gcc-s used by distro-s we use for CI. > > > > Well let me see if I can hack the Makefile to only build this with clang.. > > Oh, and - please don't special case Clang. Instead please check for option > availability (e.g. using cc-option), such that for possible future gcc, > when support there may have been added, we'd then build it there as well. I decided to just not include the libfuzzer harness in the default 'all' target. Tamas
[PATCH v3 2/2] Add tools/fuzz/oss-fuzz/build.sh
The build integration script for oss-fuzz targets. Future fuzzing targets can be added to this script and those targets will be automatically picked up by oss-fuzz without having to open separate PRs on the oss-fuzz repo. Signed-off-by: Tamas K Lengyel --- v3: Add Apache-2.0 to LICENSES and use only SPDX on script --- LICENSES/Apache-2.0 | 202 +++ tools/fuzz/oss-fuzz/build.sh | 11 ++ 2 files changed, 213 insertions(+) create mode 100644 LICENSES/Apache-2.0 create mode 100755 tools/fuzz/oss-fuzz/build.sh diff --git a/LICENSES/Apache-2.0 b/LICENSES/Apache-2.0 new file mode 100644 index 00..d645695673 --- /dev/null +++ b/LICENSES/Apache-2.0 @@ -0,0 +1,202 @@ + + Apache License + Version 2.0, January 2004 +http://www.apache.org/licenses/ + + TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION + + 1. Definitions. + + "License" shall mean the terms and conditions for use, reproduction, + and distribution as defined by Sections 1 through 9 of this document. + + "Licensor" shall mean the copyright owner or entity authorized by + the copyright owner that is granting the License. + + "Legal Entity" shall mean the union of the acting entity and all + other entities that control, are controlled by, or are under common + control with that entity. For the purposes of this definition, + "control" means (i) the power, direct or indirect, to cause the + direction or management of such entity, whether by contract or + otherwise, or (ii) ownership of fifty percent (50%) or more of the + outstanding shares, or (iii) beneficial ownership of such entity. + + "You" (or "Your") shall mean an individual or Legal Entity + exercising permissions granted by this License. + + "Source" form shall mean the preferred form for making modifications, + including but not limited to software source code, documentation + source, and configuration files. + + "Object" form shall mean any form resulting from mechanical + transformation or translation of a Source form, including but + not limited to compiled object code, generated documentation, + and conversions to other media types. + + "Work" shall mean the work of authorship, whether in Source or + Object form, made available under the License, as indicated by a + copyright notice that is included in or attached to the work + (an example is provided in the Appendix below). + + "Derivative Works" shall mean any work, whether in Source or Object + form, that is based on (or derived from) the Work and for which the + editorial revisions, annotations, elaborations, or other modifications + represent, as a whole, an original work of authorship. For the purposes + of this License, Derivative Works shall not include works that remain + separable from, or merely link (or bind by name) to the interfaces of, + the Work and Derivative Works thereof. + + "Contribution" shall mean any work of authorship, including + the original version of the Work and any modifications or additions + to that Work or Derivative Works thereof, that is intentionally + submitted to Licensor for inclusion in the Work by the copyright owner + or by an individual or Legal Entity authorized to submit on behalf of + the copyright owner. For the purposes of this definition, "submitted" + means any form of electronic, verbal, or written communication sent + to the Licensor or its representatives, including but not limited to + communication on electronic mailing lists, source code control systems, + and issue tracking systems that are managed by, or on behalf of, the + Licensor for the purpose of discussing and improving the Work, but + excluding communication that is conspicuously marked or otherwise + designated in writing by the copyright owner as "Not a Contribution." + + "Contributor" shall mean Licensor and any individual or Legal Entity + on behalf of whom a Contribution has been received by Licensor and + subsequently incorporated within the Work. + + 2. Grant of Copyright License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + copyright license to reproduce, prepare Derivative Works of, + publicly display, publicly perform, sublicense, and distribute the + Work and such Derivative Works in Source or Object form. + + 3. Grant of Patent License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual,
[PATCH v3 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
This target enables integration into oss-fuzz. Changing invalid input return
to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding the
missing __wrap_vsnprintf wrapper which is required for successful oss-fuzz
build.
Signed-off-by: Tamas K Lengyel
---
v3: don't include libfuzzer-harness in target 'all' as it requires specific cc
---
tools/fuzz/x86_instruction_emulator/Makefile| 6 +-
tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 6 ++
tools/tests/x86_emulator/wrappers.c | 11 +++
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/tools/fuzz/x86_instruction_emulator/Makefile
b/tools/fuzz/x86_instruction_emulator/Makefile
index 1e4c6b37f5..459743f4d9 100644
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o
afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) cpuid.o
wrappers.o
$(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix
-Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@
+libfuzzer-harness: $(OBJS) cpuid.o wrappers.o
+ $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $(addprefix
-Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@
+
# Common targets
.PHONY: all
all: x86-insn-fuzz-all
@@ -67,7 +70,8 @@ distclean: clean
.PHONY: clean
clean:
- rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno
*.gcov
+ rm -f *.a *.o $(DEPS_RM) *.gcda *.gcno *.gcov
+ rm -f afl-harness afl-harness-cov libfuzzer-harness
rm -rf x86_emulate x86-emulate.c x86-emulate.h wrappers.c cpuid.c
.PHONY: install
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index eeeb6931f4..2ba9ca9e0b 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -906,14 +906,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t
size)
if ( size <= DATA_OFFSET )
{
-printf("Input too small\n");
-return 1;
+return -1;
}
if ( size > FUZZ_CORPUS_SIZE )
{
-printf("Input too large\n");
-return 1;
+return -1;
}
memcpy(&input, data_p, size);
diff --git a/tools/tests/x86_emulator/wrappers.c
b/tools/tests/x86_emulator/wrappers.c
index 3829a6f416..8f3bd1656f 100644
--- a/tools/tests/x86_emulator/wrappers.c
+++ b/tools/tests/x86_emulator/wrappers.c
@@ -91,6 +91,17 @@ int __wrap_snprintf(char *buf, size_t n, const char *fmt,
...)
return rc;
}
+int __wrap_vsnprintf(char *buf, size_t n, const char *fmt, va_list varg)
+{
+int rc;
+
+emul_save_fpu_state();
+rc = __real_vsnprintf(buf, n, fmt, varg);
+emul_restore_fpu_state();
+
+return rc;
+}
+
char *__wrap_strstr(const char *s1, const char *s2)
{
char *s;
--
2.34.1
Re: [PATCH v2 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Mon, Jul 22, 2024 at 5:20 AM Jan Beulich wrote: > > On 26.06.2024 00:47, Tamas K Lengyel wrote: > > This target enables integration into oss-fuzz. Changing invalid input return > > to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding the > > missing __wrap_vsnprintf wrapper which is required for successful oss-fuzz > > build. > > > > Signed-off-by: Tamas K Lengyel > > I've reverted this right away, because of ... > > > @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o > > afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) cpuid.o > > wrappers.o > > $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix > > -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > > > > +libfuzzer-harness: $(OBJS) cpuid.o wrappers.o > > + $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $(addprefix > > -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > > ... this causing > > gcc: error: unrecognized argument to '-fsanitize=' option: 'fuzzer' > make[6]: *** [Makefile:62: libfuzzer-harness] Error 1 > > with apparently a fair set of gcc-s used by distro-s we use for CI. Well let me see if I can hack the Makefile to only build this with clang.. Tamas
Re: [PATCH v2 2/2] Add scripts/oss-fuzz/build.sh
On Thu, Jul 18, 2024 at 1:36 PM Alejandro Vallejo wrote: > > On Tue Jun 25, 2024 at 11:47 PM BST, Tamas K Lengyel wrote: > > The build integration script for oss-fuzz targets. Future fuzzing targets > > can > > be added to this script and those targets will be automatically picked up by > > oss-fuzz without having to open separate PRs on the oss-fuzz repo. > > > > Signed-off-by: Tamas K Lengyel > > --- > > scripts/oss-fuzz/build.sh | 23 +++ > > 1 file changed, 23 insertions(+) > > create mode 100755 scripts/oss-fuzz/build.sh > > > > diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh > > new file mode 100755 > > index 00..2cfd72adf1 > > --- /dev/null > > +++ b/scripts/oss-fuzz/build.sh > > @@ -0,0 +1,23 @@ > > +#!/bin/bash -eu > > The shebang probably wants to be "/usr/bin/env bash" to account for systems > that don't have bash specifically there. > > With that "-eu" would need to move down a line to be "set -eu" Thanks but this script is specifically made for just one environment and does not need to account for other systems. Cheers, Tamas
Re: [PATCH v2 2/2] Add scripts/oss-fuzz/build.sh
On Thu, Jul 18, 2024 at 9:03 AM Jan Beulich wrote: > > On 18.07.2024 14:54, Tamas K Lengyel wrote: > > On Thu, Jul 18, 2024 at 7:17 AM Jan Beulich wrote: > >> On 26.06.2024 00:47, Tamas K Lengyel wrote: > >>> +cd xen > >> > >> This looks to suggest that the expectation is for the script to be invoked > >> from the root of a xen.git clone. Imo something like > >> > >> cd $(dirname $0)/../../xen > >> > >> would be more flexible. > > > > No, it will be invoked after a git clone is made, so you have to enter > > the xen folder that was just cloned. > > Yet the suggested alternative would still work then, wouldn't it? And > it would permit easier use of the script from outside of that very > specific environment, e.g. when wanting to re-invoke it without > running a full cloning process every time. This script is specifically made for that one environment and is not intended to be invoked from anywhere else. I don't think we need to complicate this needlessly. > > >>> +make clang=y -C tools/include > >>> +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness > >> > >> In how far is it a requirement to have "clang=y" here? Wasn't this question > >> even asked before? I'm not even sure whether mid- or long-term we mean to > >> retain that functionality. Overrides of tool chain (components) may better > >> be done using CC= and friends. Plus perhaps by whoever is invoking this > >> script? > > > > It is an absolute requirement to use clang=y here as oss-fuzz uses a > > specific clang as compiler for C/C++ projects. The CC environment > > variables are already set by the oss-fuzz docker environment but it's > > insufficient for a successful clang build. Without clang=y the > > following error is encountered: > > > > gcc: error: unrecognized debug output level 'line-tables-only' > > gcc: error: unrecognized argument to '-fsanitize=' option: 'fuzzer-no-link' > > Could you add a sentence to this effect to the description? Sure. Thanks, Tamas
Re: [PATCH v2 2/2] Add scripts/oss-fuzz/build.sh
On Thu, Jul 18, 2024 at 7:17 AM Jan Beulich wrote: > > On 26.06.2024 00:47, Tamas K Lengyel wrote: > > --- /dev/null > > +++ b/scripts/oss-fuzz/build.sh > > @@ -0,0 +1,23 @@ > > +#!/bin/bash -eu > > +# SPDX-License-Identifier: Apache-2.0 > > Hmm. Aiui this line is supposed to make unnecessary ... > > > +# Copyright 2024 Google LLC > > +# > > +# Licensed under the Apache License, Version 2.0 (the "License"); > > +# you may not use this file except in compliance with the License. > > +# You may obtain a copy of the License at > > +# > > +# http://www.apache.org/licenses/LICENSE-2.0 > > +# > > +# Unless required by applicable law or agreed to in writing, software > > +# distributed under the License is distributed on an "AS IS" BASIS, > > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > +# See the License for the specific language governing permissions and > > +# limitations under the License. > > ... all of this text, provided an entry is first put in ./LICENSES/. > > > + > > + > > +cd xen > > This looks to suggest that the expectation is for the script to be invoked > from the root of a xen.git clone. Imo something like > > cd $(dirname $0)/../../xen > > would be more flexible. No, it will be invoked after a git clone is made, so you have to enter the xen folder that was just cloned. > > > +./configure --disable-stubdom --disable-pvshim --disable-docs --disable-xen > > Going forward we mean to no longer bundle e.g. qemu in release tarballs, > yet I wonder whether passing a couple of --with-system-...= here wouldn't > be better nevertheless. It largely doesn't matter as long as the configure script completes successfully since we aren't going to compile QEMU. But sure, I can add it. > > > +make clang=y -C tools/include > > +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness > > In how far is it a requirement to have "clang=y" here? Wasn't this question > even asked before? I'm not even sure whether mid- or long-term we mean to > retain that functionality. Overrides of tool chain (components) may better > be done using CC= and friends. Plus perhaps by whoever is invoking this > script? It is an absolute requirement to use clang=y here as oss-fuzz uses a specific clang as compiler for C/C++ projects. The CC environment variables are already set by the oss-fuzz docker environment but it's insufficient for a successful clang build. Without clang=y the following error is encountered: gcc: error: unrecognized debug output level 'line-tables-only' gcc: error: unrecognized argument to '-fsanitize=' option: 'fuzzer-no-link' Tamas
Re: [PATCH v2 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Thu, Jul 18, 2024 at 7:03 AM Jan Beulich wrote: > > On 26.06.2024 00:47, Tamas K Lengyel wrote: > > This target enables integration into oss-fuzz. Changing invalid input return > > to -1 as values other then 0/-1 are reserved by libfuzzer. > > And existing behavior (for afl) is unaffected because there we (wrongly) > ignore the return value altogether. > > > @@ -67,7 +70,8 @@ distclean: clean > > > > .PHONY: clean > > clean: > > - rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno > > *.gcov > > + rm -f *.a *.o $(DEPS_RM) *.gcda *.gcno *.gcov \ > > +afl-harness afl-harness-cov libfuzzer-harness > > rm -rf x86_emulate x86-emulate.c x86-emulate.h wrappers.c cpuid.c > > This is what I said for v1: > > "I'm inclined to suggest it's time to split this line (e.g. keeping all the > wildcard patterns together and moving the rest to a new rm invocation)." > > Could this really be misunderstood to mean anything other than > > clean: > rm -f *.a *.o $(DEPS_RM) *.gcda *.gcno *.gcov > rm -f afl-harness afl-harness-cov libfuzzer-harness > rm -rf x86_emulate x86-emulate.c x86-emulate.h wrappers.c cpuid.c > > ? Evidently, yes. > With that > Acked-by: Jan Beulich > and I'm kind of okay making that adjustment myself while committing. Thanks! That is appreciated. Tamas
Re: [PATCH v2 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Tue, Jul 9, 2024 at 12:12 PM Jan Beulich wrote: > > On 09.07.2024 17:37, Tamas K Lengyel wrote: > > On Tue, Jun 25, 2024 at 6:47 PM Tamas K Lengyel wrote: > >> > >> This target enables integration into oss-fuzz. Changing invalid input > >> return > >> to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding the > >> missing __wrap_vsnprintf wrapper which is required for successful oss-fuzz > >> build. > >> > >> Signed-off-by: Tamas K Lengyel > > > > Patch ping. > > It's on my list of things to look at, yet even if fully ack-ed it couldn't > go in right now anyway. Thanks, just wanted to make sure it's not lost. Tamas
Re: [PATCH v2 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Tue, Jun 25, 2024 at 6:47 PM Tamas K Lengyel wrote: > > This target enables integration into oss-fuzz. Changing invalid input return > to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding the > missing __wrap_vsnprintf wrapper which is required for successful oss-fuzz > build. > > Signed-off-by: Tamas K Lengyel Patch ping.
Re: [PATCH 2/2] Add scripts/oss-fuzz/build.sh
On Wed, Jun 26, 2024 at 8:41 AM Julien Grall wrote: > > Hi Tamas, > > On 24/06/2024 23:18, Tamas K Lengyel wrote: > > On Mon, Jun 24, 2024 at 5:58 PM Julien Grall wrote: > >> > >> Hi, > >> > >> On 21/06/2024 20:14, Tamas K Lengyel wrote: > >>> The build integration script for oss-fuzz targets. > >> > >> Do you have any details how this is meant and/or will be used? > > > > https://google.github.io/oss-fuzz/getting-started/new-project-guide/#buildsh > > > >> > >> I also couldn't find a cover letter. For series with more than one > >> patch, it is recommended to have one as it help threading and could also > >> give some insight on what you are aiming to do. > >> > >>> > >>> Signed-off-by: Tamas K Lengyel > >>> --- > >>>scripts/oss-fuzz/build.sh | 22 ++ > >>>1 file changed, 22 insertions(+) > >>>create mode 100755 scripts/oss-fuzz/build.sh > >>> > >>> diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh > >>> new file mode 100755 > >>> index 00..48528bbfc2 > >>> --- /dev/null > >>> +++ b/scripts/oss-fuzz/build.sh > >> > >> Depending on the answer above, we may want to consider to create the > >> directory oss-fuzz under automation or maybe tools/fuzz/. > > > > I'm fine with moving it wherever. > > What about tools/fuzz then? This is where are all the tooling for the > fuzzing. > > > > >> > >>> @@ -0,0 +1,22 @@ > >>> +#!/bin/bash -eu > >>> +# Copyright 2024 Google LLC > >> > >> I am a bit confused with this copyright. Is this script taken from > >> somewhere? > > > > Yes, I took an existing build.sh from oss-fuzz, > > It is unclear to me what is left from that "existing" build.sh. At least > everything below seems to be Xen specific. > > Anyway, if you want to give the copyright to Google then fair enough, > but I think you want to use an Origin tag (or similar) to indicate the > original copy. > > > it is recommended to > > have the more complex part of build.sh as part of the upstream > > repository so that additional targets/fixes can be merged there > > instead of opening PRs on oss-fuzz directly. With this setup the > > build.sh I merge to oss-fuzz will just just this build.sh in the Xen > > repository. See > > https://github.com/tklengyel/oss-fuzz/commit/552317ae9d24ef1c00d87595516cc364bc33b662. > > > >> > >>> +# > >>> +# Licensed under the Apache License, Version 2.0 (the "License"); > >>> +# you may not use this file except in compliance with the License. > >>> +# You may obtain a copy of the License at > >>> +# > >>> +# http://www.apache.org/licenses/LICENSE-2.0 > >>> +# > >>> +# Unless required by applicable law or agreed to in writing, software > >>> +# distributed under the License is distributed on an "AS IS" BASIS, > >>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > >>> implied. > >>> +# See the License for the specific language governing permissions and > >>> +# limitations under the License. > >>> +# > >>> + > >>> + > >>> +cd xen > >>> +./configure clang=y --disable-stubdom --disable-pvshim --disable-docs > >>> --disable-xen > >> > >> Looking at the help from ./configure, 'clang=y' is not mentioned and it > >> doesn't make any difference in the config.log. Can you clarify why this > >> was added? > > > > Just throwing stuff at the wall till I was able to get a clang build. > > If it's indeed not needed I can remove it. > > > >> > >>> +make clang=y -C tools/include > >>> +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness > >>> +cp tools/fuzz/x86_instruction_emulator/libfuzzer-harness > >>> $OUT/x86_instruction_emulator > >> > >> Who will be defining $OUT? > > > > oss-fuzz > > Ok. Can you add a link to the documentation in build.sh? This would be > helpful for the future reader to understand what's $OUT really mean. Sure, it turns out there is already a README.oss-fuzz in tools/fuzz that points to the oss-fuzz so I don't think there is anything else needed here, we can just move the build script there. Tamas
[PATCH v2 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
This target enables integration into oss-fuzz. Changing invalid input return
to -1 as values other then 0/-1 are reserved by libfuzzer. Also adding the
missing __wrap_vsnprintf wrapper which is required for successful oss-fuzz
build.
Signed-off-by: Tamas K Lengyel
---
tools/fuzz/x86_instruction_emulator/Makefile| 11 +--
tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 6 ++
tools/tests/x86_emulator/wrappers.c | 11 +++
3 files changed, 22 insertions(+), 6 deletions(-)
diff --git a/tools/fuzz/x86_instruction_emulator/Makefile
b/tools/fuzz/x86_instruction_emulator/Makefile
index 1e4c6b37f5..7b6655805f 100644
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -3,7 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
.PHONY: x86-insn-fuzz-all
ifeq ($(CONFIG_X86_64),y)
-x86-insn-fuzz-all: x86-insn-fuzzer.a fuzz-emul.o afl
+x86-insn-fuzz-all: x86-insn-fuzzer.a fuzz-emul.o afl libfuzzer
else
x86-insn-fuzz-all:
endif
@@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o
afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) cpuid.o
wrappers.o
$(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix
-Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@
+libfuzzer-harness: $(OBJS) cpuid.o wrappers.o
+ $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $(addprefix
-Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@
+
# Common targets
.PHONY: all
all: x86-insn-fuzz-all
@@ -67,7 +70,8 @@ distclean: clean
.PHONY: clean
clean:
- rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno
*.gcov
+ rm -f *.a *.o $(DEPS_RM) *.gcda *.gcno *.gcov \
+afl-harness afl-harness-cov libfuzzer-harness
rm -rf x86_emulate x86-emulate.c x86-emulate.h wrappers.c cpuid.c
.PHONY: install
@@ -81,4 +85,7 @@ afl: afl-harness
.PHONY: afl-cov
afl-cov: afl-harness-cov
+.PHONY: libfuzzer
+libfuzzer: libfuzzer-harness
+
-include $(DEPS_INCLUDE)
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index eeeb6931f4..2ba9ca9e0b 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -906,14 +906,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t
size)
if ( size <= DATA_OFFSET )
{
-printf("Input too small\n");
-return 1;
+return -1;
}
if ( size > FUZZ_CORPUS_SIZE )
{
-printf("Input too large\n");
-return 1;
+return -1;
}
memcpy(&input, data_p, size);
diff --git a/tools/tests/x86_emulator/wrappers.c
b/tools/tests/x86_emulator/wrappers.c
index 3829a6f416..8f3bd1656f 100644
--- a/tools/tests/x86_emulator/wrappers.c
+++ b/tools/tests/x86_emulator/wrappers.c
@@ -91,6 +91,17 @@ int __wrap_snprintf(char *buf, size_t n, const char *fmt,
...)
return rc;
}
+int __wrap_vsnprintf(char *buf, size_t n, const char *fmt, va_list varg)
+{
+int rc;
+
+emul_save_fpu_state();
+rc = __real_vsnprintf(buf, n, fmt, varg);
+emul_restore_fpu_state();
+
+return rc;
+}
+
char *__wrap_strstr(const char *s1, const char *s2)
{
char *s;
--
2.34.1
[PATCH v2 2/2] Add scripts/oss-fuzz/build.sh
The build integration script for oss-fuzz targets. Future fuzzing targets can be added to this script and those targets will be automatically picked up by oss-fuzz without having to open separate PRs on the oss-fuzz repo. Signed-off-by: Tamas K Lengyel --- scripts/oss-fuzz/build.sh | 23 +++ 1 file changed, 23 insertions(+) create mode 100755 scripts/oss-fuzz/build.sh diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh new file mode 100755 index 00..2cfd72adf1 --- /dev/null +++ b/scripts/oss-fuzz/build.sh @@ -0,0 +1,23 @@ +#!/bin/bash -eu +# SPDX-License-Identifier: Apache-2.0 +# Copyright 2024 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + + +cd xen +./configure --disable-stubdom --disable-pvshim --disable-docs --disable-xen +make clang=y -C tools/include +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness +cp tools/fuzz/x86_instruction_emulator/libfuzzer-harness $OUT/x86_instruction_emulator -- 2.34.1
Re: [PATCH 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Tue, Jun 25, 2024 at 10:56 AM Jan Beulich wrote: > > On 25.06.2024 15:40, Tamas K Lengyel wrote: > > On Tue, Jun 25, 2024 at 9:15 AM Jan Beulich wrote: > >> > >> On 25.06.2024 14:40, Tamas K Lengyel wrote: > >>> On Tue, Jun 25, 2024 at 7:52 AM Jan Beulich wrote: > >>>> > >>>> On 25.06.2024 13:12, Tamas K Lengyel wrote: > >>>>> On Tue, Jun 25, 2024 at 2:00 AM Jan Beulich wrote: > >>>>>> > >>>>>> On 24.06.2024 23:23, Tamas K Lengyel wrote: > >>>>>>> On Mon, Jun 24, 2024 at 11:55 AM Jan Beulich > >>>>>>> wrote: > >>>>>>>> > >>>>>>>> On 21.06.2024 21:14, Tamas K Lengyel wrote: > >>>>>>>>> @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o > >>>>>>>>> wrappers.o > >>>>>>>>> afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) > >>>>>>>>> cpuid.o wrappers.o > >>>>>>>>> $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix > >>>>>>>>> -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > >>>>>>>>> > >>>>>>>>> +libfuzzer-harness: $(OBJS) cpuid.o > >>>>>>>>> + $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $^ -o > >>>>>>>>> $@ > >>>>>>>> > >>>>>>>> What is LIB_FUZZING_ENGINE? I don't think we have any use of that in > >>>>>>>> the > >>>>>>>> tree anywhere. > >>>>>>> > >>>>>>> It's used by oss-fuzz, otherwise it's not doing anything. > >>>>>>> > >>>>>>>> > >>>>>>>> I'm further surprised you get away here without wrappers.o. > >>>>>>> > >>>>>>> Wrappers.o was actually breaking the build for oss-fuzz at the linking > >>>>>>> stage. It works just fine without it. > >>>>>> > >>>>>> I'm worried here, to be honest. The wrappers serve a pretty important > >>>>>> role, and I'm having a hard time seeing why they shouldn't be needed > >>>>>> here when they're needed both for the test and afl harnesses. Could > >>>>>> you add some more detail on the build issues you encountered? > >>>>> > >>>>> With wrappers.o included doing the build in the oss-fuzz docker > >>>>> (ubuntu 20.04 base) fails with: > >>>>> > >>>>> ... > >>>>> clang -O1 -fno-omit-frame-pointer -gline-tables-only > >>>>> -Wno-error=enum-constexpr-conversion > >>>>> -Wno-error=incompatible-function-pointer-types > >>>>> -Wno-error=int-conversion -Wno-error=deprecated-declarations > >>>>> -Wno-error=implicit-function-declaration -Wno-error=implicit-int > >>>>> -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address > >>>>> -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -m64 > >>>>> -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes > >>>>> -Wno-unused-but-set-variable -Wno-unused-local-typedefs -g3 -Werror > >>>>> -Og -fno-omit-frame-pointer > >>>>> -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MP > >>>>> -MF .libfuzzer-harness.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE > >>>>> -I/src/xen/tools/fuzz/x86_instruction_emulator/../../../tools/include > >>>>> -D__XEN_TOOLS__ -iquote . -fsanitize=fuzzer -fsanitize=fuzzer > >>>>> -Wl,--wrap=fwrite -Wl,--wrap=memcmp -Wl,--wrap=memcpy > >>>>> -Wl,--wrap=memset -Wl,--wrap=printf -Wl,--wrap=putchar -Wl,--wrap=puts > >>>>> -Wl,--wrap=snprintf -Wl,--wrap=strstr -Wl,--wrap=vprintf > >>>>> -Wl,--wrap=vsnprintf fuzz-emul.o x86-emulate.o x86_emulate/0f01.o > >>>>> x86_emulate/0fae.o x86_emulate/0fc7.o x86_emulate/decode.o > >>>>> x86_emulate/fpu.o cpuid.o wrappers.o -o libfuzzer-harness > >>>>> /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: > >>>>> 0x25 > >>>>> /usr/local/lib/clang/18/lib/x86_64-unknown-linux-gnu/libclang_rt.fuzzer.a(fuzzer.o): >
Re: [PATCH 2/2] Add scripts/oss-fuzz/build.sh
On Tue, Jun 25, 2024 at 9:18 AM Jan Beulich wrote: > > On 25.06.2024 14:39, Tamas K Lengyel wrote: > > On Tue, Jun 25, 2024 at 7:40 AM Jan Beulich wrote: > >> > >> On 25.06.2024 13:15, Tamas K Lengyel wrote: > >>> On Tue, Jun 25, 2024 at 5:17 AM Jan Beulich wrote: > >>>> > >>>> On 21.06.2024 21:14, Tamas K Lengyel wrote: > >>>>> --- /dev/null > >>>>> +++ b/scripts/oss-fuzz/build.sh > >>>>> @@ -0,0 +1,22 @@ > >>>>> +#!/bin/bash -eu > >>>>> +# Copyright 2024 Google LLC > >>>>> +# > >>>>> +# Licensed under the Apache License, Version 2.0 (the "License"); > >>>>> +# you may not use this file except in compliance with the License. > >>>>> +# You may obtain a copy of the License at > >>>>> +# > >>>>> +# http://www.apache.org/licenses/LICENSE-2.0 > >>>>> +# > >>>>> +# Unless required by applicable law or agreed to in writing, software > >>>>> +# distributed under the License is distributed on an "AS IS" BASIS, > >>>>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > >>>>> implied. > >>>>> +# See the License for the specific language governing permissions and > >>>>> +# limitations under the License. > >>>>> +# > >>>>> + > >>>> > >>>> I'm a little concerned here, but maybe I shouldn't be. According to what > >>>> I'm reading, the Apache 2.0 license is at least not entirely compatible > >>>> with GPLv2. While apparently the issue is solely with linking in Apache- > >>>> licensed code, I wonder whether us not having a respective file under > >>>> ./LICENSES/ (and no pre-cooked SPDX identifier to use) actually has a > >>>> reason possibly excluding the use of such code in the project. > >>>> > >>>>> +cd xen > >>>>> +./configure clang=y --disable-stubdom --disable-pvshim --disable-docs > >>>>> --disable-xen > >>>>> +make clang=y -C tools/include > >>>>> +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness > >>>>> +cp tools/fuzz/x86_instruction_emulator/libfuzzer-harness > >>>>> $OUT/x86_instruction_emulator > >>>> > >>>> In addition to what Julien said, I further think that filename / > >>>> directory > >>>> name are too generic for a file with this pretty specific contents. > >>> > >>> I don't really get your concern here? > >> > >> The thing that is built is specifically a x86 emulator piece of fuzzing > >> binary. Neither the directory name nor the file name contain either x86 > >> or (at least) emul. > > > > Because this build script is not necessarily restricted to build only > > this one harness in the future. Right now that's the only one that has > > a suitable libfuzzer harness, but the reason this build script is here > > is to be easily able to add additional fuzzing binaries without the > > need to open PRs on the oss-fuzz repo, which as I understand no one > > was willing to do in the xen community due to the CLA. Now that the > > integration is going to be in oss-fuzz, the only thing you have to do > > in the future is add more stuff to this script to get fuzzed. Anything > > that's compiled with libfuzzer and copied to $OUT will be picked up by > > oss-fuzz automatically. Makes sense? > > It does, yes. Yet nothing like that was said in the description. How > should anyone have known there are future possibilities with this script? Apologies, to me "The build integration script for oss-fuzz targets." was sufficiently descriptive but it may require some familiarity with oss-fuzz to get. I can certainly add the above text to the commit message if that helps. Tamas
Re: [PATCH 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Tue, Jun 25, 2024 at 9:15 AM Jan Beulich wrote: > > On 25.06.2024 14:40, Tamas K Lengyel wrote: > > On Tue, Jun 25, 2024 at 7:52 AM Jan Beulich wrote: > >> > >> On 25.06.2024 13:12, Tamas K Lengyel wrote: > >>> On Tue, Jun 25, 2024 at 2:00 AM Jan Beulich wrote: > >>>> > >>>> On 24.06.2024 23:23, Tamas K Lengyel wrote: > >>>>> On Mon, Jun 24, 2024 at 11:55 AM Jan Beulich wrote: > >>>>>> > >>>>>> On 21.06.2024 21:14, Tamas K Lengyel wrote: > >>>>>>> @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o > >>>>>>> wrappers.o > >>>>>>> afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) > >>>>>>> cpuid.o wrappers.o > >>>>>>> $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix > >>>>>>> -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > >>>>>>> > >>>>>>> +libfuzzer-harness: $(OBJS) cpuid.o > >>>>>>> + $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $^ -o $@ > >>>>>> > >>>>>> What is LIB_FUZZING_ENGINE? I don't think we have any use of that in > >>>>>> the > >>>>>> tree anywhere. > >>>>> > >>>>> It's used by oss-fuzz, otherwise it's not doing anything. > >>>>> > >>>>>> > >>>>>> I'm further surprised you get away here without wrappers.o. > >>>>> > >>>>> Wrappers.o was actually breaking the build for oss-fuzz at the linking > >>>>> stage. It works just fine without it. > >>>> > >>>> I'm worried here, to be honest. The wrappers serve a pretty important > >>>> role, and I'm having a hard time seeing why they shouldn't be needed > >>>> here when they're needed both for the test and afl harnesses. Could > >>>> you add some more detail on the build issues you encountered? > >>> > >>> With wrappers.o included doing the build in the oss-fuzz docker > >>> (ubuntu 20.04 base) fails with: > >>> > >>> ... > >>> clang -O1 -fno-omit-frame-pointer -gline-tables-only > >>> -Wno-error=enum-constexpr-conversion > >>> -Wno-error=incompatible-function-pointer-types > >>> -Wno-error=int-conversion -Wno-error=deprecated-declarations > >>> -Wno-error=implicit-function-declaration -Wno-error=implicit-int > >>> -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address > >>> -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -m64 > >>> -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes > >>> -Wno-unused-but-set-variable -Wno-unused-local-typedefs -g3 -Werror > >>> -Og -fno-omit-frame-pointer > >>> -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MP > >>> -MF .libfuzzer-harness.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE > >>> -I/src/xen/tools/fuzz/x86_instruction_emulator/../../../tools/include > >>> -D__XEN_TOOLS__ -iquote . -fsanitize=fuzzer -fsanitize=fuzzer > >>> -Wl,--wrap=fwrite -Wl,--wrap=memcmp -Wl,--wrap=memcpy > >>> -Wl,--wrap=memset -Wl,--wrap=printf -Wl,--wrap=putchar -Wl,--wrap=puts > >>> -Wl,--wrap=snprintf -Wl,--wrap=strstr -Wl,--wrap=vprintf > >>> -Wl,--wrap=vsnprintf fuzz-emul.o x86-emulate.o x86_emulate/0f01.o > >>> x86_emulate/0fae.o x86_emulate/0fc7.o x86_emulate/decode.o > >>> x86_emulate/fpu.o cpuid.o wrappers.o -o libfuzzer-harness > >>> /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: > >>> 0x25 > >>> /usr/local/lib/clang/18/lib/x86_64-unknown-linux-gnu/libclang_rt.fuzzer.a(fuzzer.o): > >>> in function `std::__Fuzzer::__libcpp_snprintf_l(char*, unsigned long, > >>> __locale_struct*, char const*, ...)': > >>> cxa_noexception.cpp:(.text._ZNSt8__Fuzzer19__libcpp_snprintf_lEPcmP15__locale_structPKcz[_ZNSt8__Fuzzer19__libcpp_snprintf_lEPcmP15__locale_structPKcz]+0x9a): > >>> undefined reference to `__wrap_vsnprintf' > >>> clang: error: linker command failed with exit code 1 (use -v to see > >>> invocation) > >>> make: *** [Makefile:62: libfuzzer-harness] Error 1 > >>> rm x86-emulate.c wrappers.c cpuid.c > >>> make: Leaving directory '/src/xen/tools/fuzz/x86_instruction_emulat
Re: [PATCH 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Tue, Jun 25, 2024 at 7:52 AM Jan Beulich wrote: > > On 25.06.2024 13:12, Tamas K Lengyel wrote: > > On Tue, Jun 25, 2024 at 2:00 AM Jan Beulich wrote: > >> > >> On 24.06.2024 23:23, Tamas K Lengyel wrote: > >>> On Mon, Jun 24, 2024 at 11:55 AM Jan Beulich wrote: > >>>> > >>>> On 21.06.2024 21:14, Tamas K Lengyel wrote: > >>>>> @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o > >>>>> afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) > >>>>> cpuid.o wrappers.o > >>>>> $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix > >>>>> -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > >>>>> > >>>>> +libfuzzer-harness: $(OBJS) cpuid.o > >>>>> + $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $^ -o $@ > >>>> > >>>> What is LIB_FUZZING_ENGINE? I don't think we have any use of that in the > >>>> tree anywhere. > >>> > >>> It's used by oss-fuzz, otherwise it's not doing anything. > >>> > >>>> > >>>> I'm further surprised you get away here without wrappers.o. > >>> > >>> Wrappers.o was actually breaking the build for oss-fuzz at the linking > >>> stage. It works just fine without it. > >> > >> I'm worried here, to be honest. The wrappers serve a pretty important > >> role, and I'm having a hard time seeing why they shouldn't be needed > >> here when they're needed both for the test and afl harnesses. Could > >> you add some more detail on the build issues you encountered? > > > > With wrappers.o included doing the build in the oss-fuzz docker > > (ubuntu 20.04 base) fails with: > > > > ... > > clang -O1 -fno-omit-frame-pointer -gline-tables-only > > -Wno-error=enum-constexpr-conversion > > -Wno-error=incompatible-function-pointer-types > > -Wno-error=int-conversion -Wno-error=deprecated-declarations > > -Wno-error=implicit-function-declaration -Wno-error=implicit-int > > -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address > > -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -m64 > > -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes > > -Wno-unused-but-set-variable -Wno-unused-local-typedefs -g3 -Werror > > -Og -fno-omit-frame-pointer > > -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MP > > -MF .libfuzzer-harness.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE > > -I/src/xen/tools/fuzz/x86_instruction_emulator/../../../tools/include > > -D__XEN_TOOLS__ -iquote . -fsanitize=fuzzer -fsanitize=fuzzer > > -Wl,--wrap=fwrite -Wl,--wrap=memcmp -Wl,--wrap=memcpy > > -Wl,--wrap=memset -Wl,--wrap=printf -Wl,--wrap=putchar -Wl,--wrap=puts > > -Wl,--wrap=snprintf -Wl,--wrap=strstr -Wl,--wrap=vprintf > > -Wl,--wrap=vsnprintf fuzz-emul.o x86-emulate.o x86_emulate/0f01.o > > x86_emulate/0fae.o x86_emulate/0fc7.o x86_emulate/decode.o > > x86_emulate/fpu.o cpuid.o wrappers.o -o libfuzzer-harness > > /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25 > > /usr/local/lib/clang/18/lib/x86_64-unknown-linux-gnu/libclang_rt.fuzzer.a(fuzzer.o): > > in function `std::__Fuzzer::__libcpp_snprintf_l(char*, unsigned long, > > __locale_struct*, char const*, ...)': > > cxa_noexception.cpp:(.text._ZNSt8__Fuzzer19__libcpp_snprintf_lEPcmP15__locale_structPKcz[_ZNSt8__Fuzzer19__libcpp_snprintf_lEPcmP15__locale_structPKcz]+0x9a): > > undefined reference to `__wrap_vsnprintf' > > clang: error: linker command failed with exit code 1 (use -v to see > > invocation) > > make: *** [Makefile:62: libfuzzer-harness] Error 1 > > rm x86-emulate.c wrappers.c cpuid.c > > make: Leaving directory '/src/xen/tools/fuzz/x86_instruction_emulator' > > ERROR:__main__:Building fuzzers failed. > > Hmm, yes, means we'll need an actual vsnprintf() wrapper, not just a > declaration thereof. I don't really get what this wrapper accomplishes and as I said, fuzzing works with oss-fuzz just fine without it. Tamas
Re: [PATCH 2/2] Add scripts/oss-fuzz/build.sh
On Tue, Jun 25, 2024 at 7:40 AM Jan Beulich wrote: > > On 25.06.2024 13:15, Tamas K Lengyel wrote: > > On Tue, Jun 25, 2024 at 5:17 AM Jan Beulich wrote: > >> > >> On 21.06.2024 21:14, Tamas K Lengyel wrote: > >>> --- /dev/null > >>> +++ b/scripts/oss-fuzz/build.sh > >>> @@ -0,0 +1,22 @@ > >>> +#!/bin/bash -eu > >>> +# Copyright 2024 Google LLC > >>> +# > >>> +# Licensed under the Apache License, Version 2.0 (the "License"); > >>> +# you may not use this file except in compliance with the License. > >>> +# You may obtain a copy of the License at > >>> +# > >>> +# http://www.apache.org/licenses/LICENSE-2.0 > >>> +# > >>> +# Unless required by applicable law or agreed to in writing, software > >>> +# distributed under the License is distributed on an "AS IS" BASIS, > >>> +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > >>> implied. > >>> +# See the License for the specific language governing permissions and > >>> +# limitations under the License. > >>> +# > >>> + > >> > >> I'm a little concerned here, but maybe I shouldn't be. According to what > >> I'm reading, the Apache 2.0 license is at least not entirely compatible > >> with GPLv2. While apparently the issue is solely with linking in Apache- > >> licensed code, I wonder whether us not having a respective file under > >> ./LICENSES/ (and no pre-cooked SPDX identifier to use) actually has a > >> reason possibly excluding the use of such code in the project. > >> > >>> +cd xen > >>> +./configure clang=y --disable-stubdom --disable-pvshim --disable-docs > >>> --disable-xen > >>> +make clang=y -C tools/include > >>> +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness > >>> +cp tools/fuzz/x86_instruction_emulator/libfuzzer-harness > >>> $OUT/x86_instruction_emulator > >> > >> In addition to what Julien said, I further think that filename / directory > >> name are too generic for a file with this pretty specific contents. > > > > I don't really get your concern here? > > The thing that is built is specifically a x86 emulator piece of fuzzing > binary. Neither the directory name nor the file name contain either x86 > or (at least) emul. Because this build script is not necessarily restricted to build only this one harness in the future. Right now that's the only one that has a suitable libfuzzer harness, but the reason this build script is here is to be easily able to add additional fuzzing binaries without the need to open PRs on the oss-fuzz repo, which as I understand no one was willing to do in the xen community due to the CLA. Now that the integration is going to be in oss-fuzz, the only thing you have to do in the future is add more stuff to this script to get fuzzed. Anything that's compiled with libfuzzer and copied to $OUT will be picked up by oss-fuzz automatically. Makes sense? Tamas
Re: [PATCH 2/2] Add scripts/oss-fuzz/build.sh
On Tue, Jun 25, 2024 at 5:17 AM Jan Beulich wrote: > > On 21.06.2024 21:14, Tamas K Lengyel wrote: > > --- /dev/null > > +++ b/scripts/oss-fuzz/build.sh > > @@ -0,0 +1,22 @@ > > +#!/bin/bash -eu > > +# Copyright 2024 Google LLC > > +# > > +# Licensed under the Apache License, Version 2.0 (the "License"); > > +# you may not use this file except in compliance with the License. > > +# You may obtain a copy of the License at > > +# > > +# http://www.apache.org/licenses/LICENSE-2.0 > > +# > > +# Unless required by applicable law or agreed to in writing, software > > +# distributed under the License is distributed on an "AS IS" BASIS, > > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > +# See the License for the specific language governing permissions and > > +# limitations under the License. > > +# > > + > > I'm a little concerned here, but maybe I shouldn't be. According to what > I'm reading, the Apache 2.0 license is at least not entirely compatible > with GPLv2. While apparently the issue is solely with linking in Apache- > licensed code, I wonder whether us not having a respective file under > ./LICENSES/ (and no pre-cooked SPDX identifier to use) actually has a > reason possibly excluding the use of such code in the project. The script is standalone in a clearly separate folder, not linking with anything else in the project so there is no license mixing. Adding an SPDX tag to the file would be fine. Tamas
Re: [PATCH 2/2] Add scripts/oss-fuzz/build.sh
On Tue, Jun 25, 2024 at 5:17 AM Jan Beulich wrote: > > On 21.06.2024 21:14, Tamas K Lengyel wrote: > > --- /dev/null > > +++ b/scripts/oss-fuzz/build.sh > > @@ -0,0 +1,22 @@ > > +#!/bin/bash -eu > > +# Copyright 2024 Google LLC > > +# > > +# Licensed under the Apache License, Version 2.0 (the "License"); > > +# you may not use this file except in compliance with the License. > > +# You may obtain a copy of the License at > > +# > > +# http://www.apache.org/licenses/LICENSE-2.0 > > +# > > +# Unless required by applicable law or agreed to in writing, software > > +# distributed under the License is distributed on an "AS IS" BASIS, > > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > +# See the License for the specific language governing permissions and > > +# limitations under the License. > > +# > > + > > I'm a little concerned here, but maybe I shouldn't be. According to what > I'm reading, the Apache 2.0 license is at least not entirely compatible > with GPLv2. While apparently the issue is solely with linking in Apache- > licensed code, I wonder whether us not having a respective file under > ./LICENSES/ (and no pre-cooked SPDX identifier to use) actually has a > reason possibly excluding the use of such code in the project. > > > +cd xen > > +./configure clang=y --disable-stubdom --disable-pvshim --disable-docs > > --disable-xen > > +make clang=y -C tools/include > > +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness > > +cp tools/fuzz/x86_instruction_emulator/libfuzzer-harness > > $OUT/x86_instruction_emulator > > In addition to what Julien said, I further think that filename / directory > name are too generic for a file with this pretty specific contents. I don't really get your concern here? Tamas
Re: [PATCH 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Tue, Jun 25, 2024 at 2:00 AM Jan Beulich wrote: > > On 24.06.2024 23:23, Tamas K Lengyel wrote: > > On Mon, Jun 24, 2024 at 11:55 AM Jan Beulich wrote: > >> > >> On 21.06.2024 21:14, Tamas K Lengyel wrote: > >>> @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o > >>> afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) > >>> cpuid.o wrappers.o > >>> $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix > >>> -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@ > >>> > >>> +libfuzzer-harness: $(OBJS) cpuid.o > >>> + $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $^ -o $@ > >> > >> What is LIB_FUZZING_ENGINE? I don't think we have any use of that in the > >> tree anywhere. > > > > It's used by oss-fuzz, otherwise it's not doing anything. > > > >> > >> I'm further surprised you get away here without wrappers.o. > > > > Wrappers.o was actually breaking the build for oss-fuzz at the linking > > stage. It works just fine without it. > > I'm worried here, to be honest. The wrappers serve a pretty important > role, and I'm having a hard time seeing why they shouldn't be needed > here when they're needed both for the test and afl harnesses. Could > you add some more detail on the build issues you encountered? With wrappers.o included doing the build in the oss-fuzz docker (ubuntu 20.04 base) fails with: ... clang -O1 -fno-omit-frame-pointer -gline-tables-only -Wno-error=enum-constexpr-conversion -Wno-error=incompatible-function-pointer-types -Wno-error=int-conversion -Wno-error=deprecated-declarations -Wno-error=implicit-function-declaration -Wno-error=implicit-int -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION -fsanitize=address -fsanitize-address-use-after-scope -fsanitize=fuzzer-no-link -m64 -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wno-unused-but-set-variable -Wno-unused-local-typedefs -g3 -Werror -Og -fno-omit-frame-pointer -D__XEN_INTERFACE_VERSION__=__XEN_LATEST_INTERFACE_VERSION__ -MMD -MP -MF .libfuzzer-harness.d -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -I/src/xen/tools/fuzz/x86_instruction_emulator/../../../tools/include -D__XEN_TOOLS__ -iquote . -fsanitize=fuzzer -fsanitize=fuzzer -Wl,--wrap=fwrite -Wl,--wrap=memcmp -Wl,--wrap=memcpy -Wl,--wrap=memset -Wl,--wrap=printf -Wl,--wrap=putchar -Wl,--wrap=puts -Wl,--wrap=snprintf -Wl,--wrap=strstr -Wl,--wrap=vprintf -Wl,--wrap=vsnprintf fuzz-emul.o x86-emulate.o x86_emulate/0f01.o x86_emulate/0fae.o x86_emulate/0fc7.o x86_emulate/decode.o x86_emulate/fpu.o cpuid.o wrappers.o -o libfuzzer-harness /usr/bin/ld: /usr/bin/ld: DWARF error: invalid or unhandled FORM value: 0x25 /usr/local/lib/clang/18/lib/x86_64-unknown-linux-gnu/libclang_rt.fuzzer.a(fuzzer.o): in function `std::__Fuzzer::__libcpp_snprintf_l(char*, unsigned long, __locale_struct*, char const*, ...)': cxa_noexception.cpp:(.text._ZNSt8__Fuzzer19__libcpp_snprintf_lEPcmP15__locale_structPKcz[_ZNSt8__Fuzzer19__libcpp_snprintf_lEPcmP15__locale_structPKcz]+0x9a): undefined reference to `__wrap_vsnprintf' clang: error: linker command failed with exit code 1 (use -v to see invocation) make: *** [Makefile:62: libfuzzer-harness] Error 1 rm x86-emulate.c wrappers.c cpuid.c make: Leaving directory '/src/xen/tools/fuzz/x86_instruction_emulator' ERROR:__main__:Building fuzzers failed.
Re: [PATCH 2/2] Add scripts/oss-fuzz/build.sh
On Mon, Jun 24, 2024 at 5:58 PM Julien Grall wrote: > > Hi, > > On 21/06/2024 20:14, Tamas K Lengyel wrote: > > The build integration script for oss-fuzz targets. > > Do you have any details how this is meant and/or will be used? https://google.github.io/oss-fuzz/getting-started/new-project-guide/#buildsh > > I also couldn't find a cover letter. For series with more than one > patch, it is recommended to have one as it help threading and could also > give some insight on what you are aiming to do. > > > > > Signed-off-by: Tamas K Lengyel > > --- > > scripts/oss-fuzz/build.sh | 22 ++ > > 1 file changed, 22 insertions(+) > > create mode 100755 scripts/oss-fuzz/build.sh > > > > diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh > > new file mode 100755 > > index 00..48528bbfc2 > > --- /dev/null > > +++ b/scripts/oss-fuzz/build.sh > > Depending on the answer above, we may want to consider to create the > directory oss-fuzz under automation or maybe tools/fuzz/. I'm fine with moving it wherever. > > > @@ -0,0 +1,22 @@ > > +#!/bin/bash -eu > > +# Copyright 2024 Google LLC > > I am a bit confused with this copyright. Is this script taken from > somewhere? Yes, I took an existing build.sh from oss-fuzz, it is recommended to have the more complex part of build.sh as part of the upstream repository so that additional targets/fixes can be merged there instead of opening PRs on oss-fuzz directly. With this setup the build.sh I merge to oss-fuzz will just just this build.sh in the Xen repository. See https://github.com/tklengyel/oss-fuzz/commit/552317ae9d24ef1c00d87595516cc364bc33b662. > > > +# > > +# Licensed under the Apache License, Version 2.0 (the "License"); > > +# you may not use this file except in compliance with the License. > > +# You may obtain a copy of the License at > > +# > > +# http://www.apache.org/licenses/LICENSE-2.0 > > +# > > +# Unless required by applicable law or agreed to in writing, software > > +# distributed under the License is distributed on an "AS IS" BASIS, > > +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. > > +# See the License for the specific language governing permissions and > > +# limitations under the License. > > +# > > + > > + > > +cd xen > > +./configure clang=y --disable-stubdom --disable-pvshim --disable-docs > > --disable-xen > > Looking at the help from ./configure, 'clang=y' is not mentioned and it > doesn't make any difference in the config.log. Can you clarify why this > was added? Just throwing stuff at the wall till I was able to get a clang build. If it's indeed not needed I can remove it. > > > +make clang=y -C tools/include > > +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness > > +cp tools/fuzz/x86_instruction_emulator/libfuzzer-harness > > $OUT/x86_instruction_emulator > > Who will be defining $OUT? oss-fuzz Tamas
Re: [PATCH 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
On Mon, Jun 24, 2024 at 11:55 AM Jan Beulich wrote:
>
> On 21.06.2024 21:14, Tamas K Lengyel wrote:
> > @@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o
> > afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) cpuid.o
> > wrappers.o
> > $(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix
> > -Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@
> >
> > +libfuzzer-harness: $(OBJS) cpuid.o
> > + $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $^ -o $@
>
> What is LIB_FUZZING_ENGINE? I don't think we have any use of that in the
> tree anywhere.
It's used by oss-fuzz, otherwise it's not doing anything.
>
> I'm further surprised you get away here without wrappers.o.
Wrappers.o was actually breaking the build for oss-fuzz at the linking
stage. It works just fine without it.
>
> Finally, despite its base name the lack of an extension suggest to me
> this isn't actually a library. Can you help me bring both aspects together?
Libfuzzer is the the name of the fuzzing engine, like afl:
https://llvm.org/docs/LibFuzzer.html
>
> > @@ -67,7 +70,7 @@ distclean: clean
> >
> > .PHONY: clean
> > clean:
> > - rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno
> > *.gcov
> > + rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno
> > *.gcov libfuzzer-harness
>
> I'm inclined to suggest it's time to split this line (e.g. keeping all the
> wildcard patterns together and moving the rest to a new rm invocation).
Sure.
>
> > --- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> > +++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
> > @@ -906,14 +906,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p,
> > size_t size)
> >
> > if ( size <= DATA_OFFSET )
> > {
> > -printf("Input too small\n");
> > -return 1;
> > +return -1;
> > }
> >
> > if ( size > FUZZ_CORPUS_SIZE )
> > {
> > -printf("Input too large\n");
> > -return 1;
> > +return -1;
> > }
> >
> > memcpy(&input, data_p, size);
>
> This part of the change clearly needs explaining in the description.
> It's not even clear to me in how far this is related to the purpose
> of the patch here (iow it may want to be a separate change, depending
> on why the change is needed).
The printf simply produces a ton of unnecessary output while the
fuzzer is running, slowing it down. It's also not useful at all, even
for debugging. Switching the return -1 is necessary because beside
0/-1 values are reserved by libfuzzer for "future use". No issue about
putting this info into the commit message.
Tamas
[PATCH 1/2] Add libfuzzer target to fuzz/x86_instruction_emulator
This target enables integration into oss-fuzz.
Signed-off-by: Tamas K Lengyel
---
tools/fuzz/x86_instruction_emulator/Makefile| 10 --
tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 6 ++
2 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/tools/fuzz/x86_instruction_emulator/Makefile
b/tools/fuzz/x86_instruction_emulator/Makefile
index 1e4c6b37f5..de5f1e7e30 100644
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -3,7 +3,7 @@ include $(XEN_ROOT)/tools/Rules.mk
.PHONY: x86-insn-fuzz-all
ifeq ($(CONFIG_X86_64),y)
-x86-insn-fuzz-all: x86-insn-fuzzer.a fuzz-emul.o afl
+x86-insn-fuzz-all: x86-insn-fuzzer.a fuzz-emul.o afl libfuzzer
else
x86-insn-fuzz-all:
endif
@@ -58,6 +58,9 @@ afl-harness: afl-harness.o $(OBJS) cpuid.o wrappers.o
afl-harness-cov: afl-harness-cov.o $(patsubst %.o,%-cov.o,$(OBJS)) cpuid.o
wrappers.o
$(CC) $(CFLAGS) $(GCOV_FLAGS) $(addprefix
-Wl$(comma)--wrap=,$(WRAPPED)) $^ -o $@
+libfuzzer-harness: $(OBJS) cpuid.o
+ $(CC) $(CFLAGS) $(LIB_FUZZING_ENGINE) -fsanitize=fuzzer $^ -o $@
+
# Common targets
.PHONY: all
all: x86-insn-fuzz-all
@@ -67,7 +70,7 @@ distclean: clean
.PHONY: clean
clean:
- rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno
*.gcov
+ rm -f *.a *.o $(DEPS_RM) afl-harness afl-harness-cov *.gcda *.gcno
*.gcov libfuzzer-harness
rm -rf x86_emulate x86-emulate.c x86-emulate.h wrappers.c cpuid.c
.PHONY: install
@@ -81,4 +84,7 @@ afl: afl-harness
.PHONY: afl-cov
afl-cov: afl-harness-cov
+.PHONY: libfuzzer
+libfuzzer: libfuzzer-harness
+
-include $(DEPS_INCLUDE)
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index eeeb6931f4..2ba9ca9e0b 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -906,14 +906,12 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t
size)
if ( size <= DATA_OFFSET )
{
-printf("Input too small\n");
-return 1;
+return -1;
}
if ( size > FUZZ_CORPUS_SIZE )
{
-printf("Input too large\n");
-return 1;
+return -1;
}
memcpy(&input, data_p, size);
--
2.34.1
[PATCH 2/2] Add scripts/oss-fuzz/build.sh
The build integration script for oss-fuzz targets. Signed-off-by: Tamas K Lengyel --- scripts/oss-fuzz/build.sh | 22 ++ 1 file changed, 22 insertions(+) create mode 100755 scripts/oss-fuzz/build.sh diff --git a/scripts/oss-fuzz/build.sh b/scripts/oss-fuzz/build.sh new file mode 100755 index 00..48528bbfc2 --- /dev/null +++ b/scripts/oss-fuzz/build.sh @@ -0,0 +1,22 @@ +#!/bin/bash -eu +# Copyright 2024 Google LLC +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + + +cd xen +./configure clang=y --disable-stubdom --disable-pvshim --disable-docs --disable-xen +make clang=y -C tools/include +make clang=y -C tools/fuzz/x86_instruction_emulator libfuzzer-harness +cp tools/fuzz/x86_instruction_emulator/libfuzzer-harness $OUT/x86_instruction_emulator -- 2.34.1
Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
On Fri, May 10, 2024 at 8:32 AM Alessandro Zucchelli wrote: > > In order to comply to MISRA C:2012 Rule 8.4 for ARM the following > changes are done: > revert preprocessor conditional changes to xen/mem_access.h which > had it build unconditionally, add conditional build for xen/mem_access.c > as well and provide stubs in asm/mem_access.h for the users of this > header. > > Signed-off-by: Alessandro Zucchelli Acked-by: Tamas K Lengyel
Re: [PATCH for-4.19? v3 4/6] x86: Make the maximum number of altp2m views configurable
> -ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx); > +ap2m = d->arch.altp2m_p2m[altp2m_idx]; Why is it no longer necessary to use array_access_nospec? Tamas
Re: [PATCH v10 11/14] xen/riscv: introduce vm_event_*() functions
On Fri, May 17, 2024 at 9:55 AM Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko Acked-by: Tamas K Lengyel
Re: [PATCH v10 07/14] xen/riscv: introduce monitor.h
On Fri, May 17, 2024 at 9:55 AM Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko Acked-by: Tamas K Lengyel
Re: [XEN PATCH v2 05/15] x86: introduce CONFIG_ALTP2M Kconfig option
> Currently altp2m support provided for VT-d only, so option is dependant on > VMX. No clue what is meant by "support provided for VT-d only". Altp2m has nothing to do with VT-d. It would be more accurate to say it's only implemented for Intel EPT. Tamas
Re: [XEN PATCH] xen/mem_access: address violations of MISRA C: 2012 Rule 8.4
On Wed, May 8, 2024 at 12:26 PM Julien Grall wrote: > > Hi Tamas, > > On 08/05/2024 17:01, Tamas K Lengyel wrote: > > On Wed, May 8, 2024 at 10:02 AM Alessandro Zucchelli > > wrote: > >> > >> On 2024-05-03 11:32, Julien Grall wrote: > >>> Hi, > >>> > >>> On 03/05/2024 08:09, Alessandro Zucchelli wrote: > >>>> On 2024-04-29 17:58, Jan Beulich wrote: > >>>>> On 29.04.2024 17:45, Alessandro Zucchelli wrote: > >>>>>> Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), > >>>>>> allowing asm/mem_access.h to be included in all ARM build > >>>>>> configurations. > >>>>>> This is to address the violation of MISRA C: 2012 Rule 8.4 which > >>>>>> states: > >>>>>> "A compatible declaration shall be visible when an object or > >>>>>> function > >>>>>> with external linkage is defined". Functions p2m_mem_access_check > >>>>>> and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not > >>>>>> defined in ARM builds don't have visible declarations in the file > >>>>>> containing their definitions. > >>>>>> > >>>>>> Signed-off-by: Alessandro Zucchelli > >>>>>> > >>>>>> --- > >>>>>> xen/include/xen/mem_access.h | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/xen/include/xen/mem_access.h > >>>>>> b/xen/include/xen/mem_access.h > >>>>>> index 87d93b31f6..ec0630677d 100644 > >>>>>> --- a/xen/include/xen/mem_access.h > >>>>>> +++ b/xen/include/xen/mem_access.h > >>>>>> @@ -33,7 +33,7 @@ > >>>>>>*/ > >>>>>> struct vm_event_st; > >>>>>> > >>>>>> -#ifdef CONFIG_MEM_ACCESS > >>>>>> +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) > >>>>>> #include > >>>>>> #endif > >>>>> > >>>>> This doesn't look quite right. If Arm supports mem-access, why would > >>>>> it > >>>>> not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, > >>>>> then > >>>>> those would better move here, thus eliminating the need for a > >>>>> per-arch > >>>>> stub header (see what was e.g. done for numa.h). This way RISC-V and > >>>>> PPC > >>>>> (and whatever is to come) would then be taken care of as well. > >>>>> > >>>> ARM does support mem-access, so I don't think this is akin to the > >>>> changes done to handle numa.h. > >>>> ARM also allows users to set MEM_ACCESS=n (e.g. > >>>> xen/arch/arm/configs/tiny64_defconfig) and builds just fine; however, > >>>> the implementation file mem_access.c is compiled unconditionally in > >>>> ARM's makefile, hence why the violation was spotted. > >>>> This is a bit unusual, so I was also hoping to get some feedback from > >>>> mem-access maintainers as to why this discrepancy from x86 exists. I > >>>> probably should have also included some ARM maintainers as well, so > >>>> I'm going to loop them in now. > >>>> > >>>> An alternative option I think is to make the compilation of arm's > >>>> mem_access.c conditional on CONFIG_MEM_ACCESS (as for x86/mm and > >>>> common). > >>> > >>> I can't think of a reason to have mem_access.c unconditional compiled > >>> in. So I think it should be conditional like on x86. > >> > >> Hi, > >> attempting to build ARM with a configuration where MEM_ACCESS=n and > >> mem_access.c is conditioned on CONFIG_MEM_ACCESS results in a fail as > >> there are other pieces of code that call some mem_access.c functions > >> (p2m_mem_access_check_and_get_page, p2m_mem_access_check). > >> In a Matrix chat Julien was suggesting adding stubs for the functions > >> for this use case. > > > > Perhaps just wrap the callers into #ifdef CONFIG_MEM_ACCESS blocks? > > In Xen, we tend prefer stubs over #ifdef-ing code blocks. I would rather > use this approach here too. I was looking at arch/x86/hvm/hvm.c for examples on how MEM_PAGING and MEM_SHARING calls are handled and those were ifdef'd. I have no preference for one vs the other, both work. Tamas
Re: [XEN PATCH] xen/mem_access: address violations of MISRA C: 2012 Rule 8.4
On Wed, May 8, 2024 at 10:02 AM Alessandro Zucchelli wrote: > > On 2024-05-03 11:32, Julien Grall wrote: > > Hi, > > > > On 03/05/2024 08:09, Alessandro Zucchelli wrote: > >> On 2024-04-29 17:58, Jan Beulich wrote: > >>> On 29.04.2024 17:45, Alessandro Zucchelli wrote: > Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), > allowing asm/mem_access.h to be included in all ARM build > configurations. > This is to address the violation of MISRA C: 2012 Rule 8.4 which > states: > "A compatible declaration shall be visible when an object or > function > with external linkage is defined". Functions p2m_mem_access_check > and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not > defined in ARM builds don't have visible declarations in the file > containing their definitions. > > Signed-off-by: Alessandro Zucchelli > > --- > xen/include/xen/mem_access.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/include/xen/mem_access.h > b/xen/include/xen/mem_access.h > index 87d93b31f6..ec0630677d 100644 > --- a/xen/include/xen/mem_access.h > +++ b/xen/include/xen/mem_access.h > @@ -33,7 +33,7 @@ > */ > struct vm_event_st; > > -#ifdef CONFIG_MEM_ACCESS > +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) > #include > #endif > >>> > >>> This doesn't look quite right. If Arm supports mem-access, why would > >>> it > >>> not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, > >>> then > >>> those would better move here, thus eliminating the need for a > >>> per-arch > >>> stub header (see what was e.g. done for numa.h). This way RISC-V and > >>> PPC > >>> (and whatever is to come) would then be taken care of as well. > >>> > >> ARM does support mem-access, so I don't think this is akin to the > >> changes done to handle numa.h. > >> ARM also allows users to set MEM_ACCESS=n (e.g. > >> xen/arch/arm/configs/tiny64_defconfig) and builds just fine; however, > >> the implementation file mem_access.c is compiled unconditionally in > >> ARM's makefile, hence why the violation was spotted. > >> This is a bit unusual, so I was also hoping to get some feedback from > >> mem-access maintainers as to why this discrepancy from x86 exists. I > >> probably should have also included some ARM maintainers as well, so > >> I'm going to loop them in now. > >> > >> An alternative option I think is to make the compilation of arm's > >> mem_access.c conditional on CONFIG_MEM_ACCESS (as for x86/mm and > >> common). > > > > I can't think of a reason to have mem_access.c unconditional compiled > > in. So I think it should be conditional like on x86. > > Hi, > attempting to build ARM with a configuration where MEM_ACCESS=n and > mem_access.c is conditioned on CONFIG_MEM_ACCESS results in a fail as > there are other pieces of code that call some mem_access.c functions > (p2m_mem_access_check_and_get_page, p2m_mem_access_check). > In a Matrix chat Julien was suggesting adding stubs for the functions > for this use case. Perhaps just wrap the callers into #ifdef CONFIG_MEM_ACCESS blocks? Tamas
Re: [PATCH] x86/monitor: allow fast-singlestepping without enabling singlestep monitor
On Sun, Apr 14, 2024 at 2:21 PM Petr Beneš wrote: > > From: Petr Beneš > > Reorder the condition checks within the HVM_MONITOR_SINGLESTEP_BREAKPOINT > case to enable fast singlestepping independently of the singlestep monitor > being enabled. Previously, fast singlestepping required the singlestep > monitor to be explicitly enabled through xc_monitor_singlestep, even though > it operates entirely within Xen and does not generate external events. > > Signed-off-by: Petr Beneš Acked-by: Tamas K Lengyel
Re: [XEN PATCH v1 06/15] x86/p2m: guard altp2m code with CONFIG_VMX option
On Tue, Apr 16, 2024 at 3:29 AM Andrew Cooper wrote: > > On 16/04/2024 7:31 am, Sergiy Kibrik wrote: > > Instead of using generic CONFIG_HVM option switch to a bit more specific > > CONFIG_VMX option for altp2m support, as it depends on VMX. Also guard > > altp2m routines, so that it can be disabled completely in the build. > > > > Signed-off-by: Sergiy Kibrik > > Altp2m is not VMX-specific. It's just no-one has wired it up on AMD, or > accepted the long-outstanding ARM patchset where it was made to work. > > If you want to compile it, you probably want CONFIG_ALTP2M. > > However, it's not even x86 specific. See the uses in common/monitor.c As Andrew said, it is not VMX specific so shouldn't be tied to that. Adding a CONFIG_ALTP2M would be OK. Tamas
Re: [XEN PATCH 6/7] xen/vm-event: address a violation of MISRA C:2012 Rule 16.3
On Tue, Apr 2, 2024 at 3:24 AM Federico Serafini
wrote:
>
> Add break statement to address a violation of MISRA C:2012 Rule 16.3
> ("An unconditional `break' statement shall terminate every
> switch-clause ").
>
> No functional change.
>
> Signed-off-by: Federico Serafini
Acked-by: Tamas K Lengyel
Re: [PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop
On Wed, Feb 28, 2024 at 8:28 AM Roger Pau Monné wrote:
>
> On Wed, Feb 28, 2024 at 12:18:31PM +0100, Jan Beulich wrote:
> > On 28.02.2024 11:53, Roger Pau Monné wrote:
> > > On Fri, Feb 23, 2024 at 08:43:24AM +0100, Jan Beulich wrote:
> > >> On 22.02.2024 19:03, Tamas K Lengyel wrote:
> > >>> On Thu, Feb 22, 2024 at 5:06 AM Jan Beulich wrote:
> > >>>> On 22.02.2024 10:05, Roger Pau Monne wrote:
> > >>>>> The usage of a cmpxchg loop in get_next_handle() is unnecessary, as
> > >>>>> the same
> > >>>>> can be achieved with an atomic increment, which is both simpler to
> > >>>>> read, and
> > >>>>> avoid any need for a loop.
> > >>>>>
> > >>>>> The cmpxchg usage is likely a remnant of 32bit support, which didn't
> > >>>>> have an
> > >>>>> instruction to do an atomic 64bit add, and instead a cmpxchg had to
> > >>>>> be used.
> > >>>>>
> > >>>>> Suggested-by: Andrew Cooper
> > >>>>> Signed-of-by: Roger Pau Monné
> > >>>>
> > >>>> Reviewed-by: Jan Beulich
> > >>>> albeit ...
> > >>>>
> > >>>>> --- a/xen/arch/x86/mm/mem_sharing.c
> > >>>>> +++ b/xen/arch/x86/mm/mem_sharing.c
> > >>>>> @@ -179,13 +179,7 @@ static void mem_sharing_page_unlock(struct
> > >>>>> page_info *pg)
> > >>>>>
> > >>>>> static shr_handle_t get_next_handle(void)
> > >>>>> {
> > >>>>> -/* Get the next handle get_page style */
> > >>>>> -uint64_t x, y = next_handle;
> > >>>>> -do {
> > >>>>> -x = y;
> > >>>>> -}
> > >>>>> -while ( (y = cmpxchg(&next_handle, x, x + 1)) != x );
> > >>>>> -return x + 1;
> > >>>>> +return arch_fetch_and_add(&next_handle, 1) + 1;
> > >>>>> }
> > >>>>
> > >>>> ... the adding of 1 here is a little odd when taken together with
> > >>>> next_handle's initializer. Tamas, you've not written that code, but do
> > >>>> you have any thoughts towards the possible removal of either the
> > >>>> initializer or the adding here? Plus that variable of course could
> > >>>> very well do with moving into this function.
> > >>>
> > >>> I have to say I find the existing logic here hard to parse but by the
> > >>> looks I don't think we need the + 1 once we switch to
> > >>> arch_fetch_and_add. Also could go without initializing next_handle to
> > >>> 1. Moving it into the function would not really accomplish anything
> > >>> other than style AFAICT?
> > >>
> > >> Well, limiting scope of things can be viewed as purely style, but I
> > >> think it's more than that: It makes intentions more clear and reduces
> > >> the chance of abuse (deliberate or unintentional).
> > >
> > > I'm afraid that whatever is the outcome here, I will defer it to a
> > > further commit, since the purpose here is to be a non-functional
> > > change.
> >
> > That's fine with me, but an ack from Tamas is still pending, unless I
> > missed something somewhere.
>
> No, just wanted to clarify that I wasn't expecting to do further
> changes here, FTAOD. Not sure if Tamas was expecting me to further
> adjust the code.
Acked-by: Tamas K Lengyel
Re: [PATCH 1/2] x86/memsharing: use an atomic add instead of a cmpxchg loop
On Thu, Feb 22, 2024 at 5:06 AM Jan Beulich wrote:
>
> On 22.02.2024 10:05, Roger Pau Monne wrote:
> > The usage of a cmpxchg loop in get_next_handle() is unnecessary, as the same
> > can be achieved with an atomic increment, which is both simpler to read, and
> > avoid any need for a loop.
> >
> > The cmpxchg usage is likely a remnant of 32bit support, which didn't have an
> > instruction to do an atomic 64bit add, and instead a cmpxchg had to be used.
> >
> > Suggested-by: Andrew Cooper
> > Signed-of-by: Roger Pau Monné
>
> Reviewed-by: Jan Beulich
> albeit ...
>
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -179,13 +179,7 @@ static void mem_sharing_page_unlock(struct page_info
> > *pg)
> >
> > static shr_handle_t get_next_handle(void)
> > {
> > -/* Get the next handle get_page style */
> > -uint64_t x, y = next_handle;
> > -do {
> > -x = y;
> > -}
> > -while ( (y = cmpxchg(&next_handle, x, x + 1)) != x );
> > -return x + 1;
> > +return arch_fetch_and_add(&next_handle, 1) + 1;
> > }
>
> ... the adding of 1 here is a little odd when taken together with
> next_handle's initializer. Tamas, you've not written that code, but do
> you have any thoughts towards the possible removal of either the
> initializer or the adding here? Plus that variable of course could
> very well do with moving into this function.
I have to say I find the existing logic here hard to parse but by the
looks I don't think we need the + 1 once we switch to
arch_fetch_and_add. Also could go without initializing next_handle to
1. Moving it into the function would not really accomplish anything
other than style AFAICT?
Tamas
Re: [PATCH] x86/hvm: Fix fast singlestep state persistence
On Thu, Feb 8, 2024 at 4:20 PM Petr Beneš wrote: > > From: Petr Beneš > > This patch addresses an issue where the fast singlestep setting would persist > despite xc_domain_debug_control being called with > XEN_DOMCTL_DEBUG_OP_SINGLE_STEP_OFF. > Specifically, if fast singlestep was enabled in a VMI session and that session > stopped before the MTF trap occurred, the fast singlestep setting remained > active even though MTF itself was disabled. This led to a situation where, > upon > starting a new VMI session, the first event to trigger an EPT violation would > cause the corresponding EPT event callback to be skipped due to the lingering > fast singlestep setting. > > The fix ensures that the fast singlestep setting is properly reset when > disabling single step debugging operations. > > Signed-off-by: Petr Beneš Thanks, this has been a known bug that awaited a fix for a long time. Reviewed-by: Tamas K Lengyel
Re: [PATCH] x86/altp2m: p2m_altp2m_get_or_propagate() should honor ap2m->default_access
On Thu, Feb 8, 2024 at 9:00 AM Jan Beulich wrote: > > On 08.02.2024 14:45, Tamas K Lengyel wrote: > > On Thu, Feb 8, 2024 at 2:46 AM Jan Beulich wrote: > >> > >> On 08.02.2024 05:32, George Dunlap wrote: > >>> Er, ok, just one more comment: this could allow an altp2m to have more > >>> permissions than the host; for example, the host p2m entry could be > >>> p2m_access_r, but if the altp2m's default_access were p2m_access_rw, > >>> it would override that. Is that the behavior we want? Or do we want > >>> to do some sort of intersection of permissions? > >>> > >>> If the former, I'd propose the comment be adjusted thus: > > > > No intersection of permissions please, that needlessly complicates > > things and makes it hard to reason about the state of a view where > > default permissions are used. No need to force a specific type of > > usecase here where the hostp2m's permissions are special just cause we > > say so. No, the permissions in the hostp2m should not have more weight > > then the specifically requested default permission. > > > >>> > >>> * If the entry is invalid, and the host entry was valid, propagate > >>> * the host's entry to the altp2m, retaining page order but using the > >>> * altp2m's default_access, and indicate that the caller should re-try > >>> * the faulting instruction. > >> > >> I find it highly questionable that such blind overriding should be taking > >> place. > > > > It's not blind overriding, it's the requested default permission set > > for a view where no entry was present before. It is the expected > > behavior. It would be way harder to design applications with this > > feature if it was special cased and it would take different > > permissions based on what permission is set in another view. > > But the default can be only one specific value: How do you make sure that > R/O, R/X, and R/W mappings all retain their respective restrictions in the > alternative view? To not over-restrict permissions, the default would then > need to be RWX, yet then all mappings will have full permission. What am I > missing? Why do you assume you need to retain the permissions that were set in the hostp2m across all altp2ms? It is entirely reasonable to set permissions for a couple entries manually in an altp2m and set the default to _n, which may be totally different then what the hostp2m has. When you get the event for entries you didn't manually specify you can decide which view to switch to, which may be the hostp2m, but may be some other view. If you wanted to have a view with a default permission that inherits permissions that are always at least as restrictive as the hostp2m, you could do that, but the way to do that would be to introduce a special mem_access permission to signify that that's the expected behavior (similar to permissions like n2rwx). I don't think anyone is asking for that. Tamas
Re: [PATCH] x86/altp2m: p2m_altp2m_get_or_propagate() should honor ap2m->default_access
On Thu, Feb 8, 2024 at 2:46 AM Jan Beulich wrote: > > On 08.02.2024 05:32, George Dunlap wrote: > > Er, ok, just one more comment: this could allow an altp2m to have more > > permissions than the host; for example, the host p2m entry could be > > p2m_access_r, but if the altp2m's default_access were p2m_access_rw, > > it would override that. Is that the behavior we want? Or do we want > > to do some sort of intersection of permissions? > > > > If the former, I'd propose the comment be adjusted thus: No intersection of permissions please, that needlessly complicates things and makes it hard to reason about the state of a view where default permissions are used. No need to force a specific type of usecase here where the hostp2m's permissions are special just cause we say so. No, the permissions in the hostp2m should not have more weight then the specifically requested default permission. > > > > * If the entry is invalid, and the host entry was valid, propagate > > * the host's entry to the altp2m, retaining page order but using the > > * altp2m's default_access, and indicate that the caller should re-try > > * the faulting instruction. > > I find it highly questionable that such blind overriding should be taking > place. It's not blind overriding, it's the requested default permission set for a view where no entry was present before. It is the expected behavior. It would be way harder to design applications with this feature if it was special cased and it would take different permissions based on what permission is set in another view. Tamas
Re: [PATCH] x86/altp2m: p2m_altp2m_get_or_propagate() should honor ap2m->default_access
On Wed, Feb 7, 2024 at 5:21 PM Andrew Cooper wrote: > > On 07/02/2024 1:18 am, George Dunlap wrote: > > On Tue, Feb 6, 2024 at 6:08 PM Petr Beneš wrote: > >> From: Petr Beneš > >> > >> This patch addresses a behavior discrepancy in the handling of altp2m > >> views, > >> where upon the creation and subsequent EPT violation, the page access > >> permissions were incorrectly inherited from the hostp2m instead of > >> respecting > >> the altp2m default_access. > >> > >> Previously, when a new altp2m view was established with restrictive > >> default_access permissions and activated via xc_altp2m_switch_to_view(), > >> it failed to trigger an event on the first access violation. This behavior > >> diverged from the intended mechanism, where the altp2m's default_access > >> should dictate the initial permissions, ensuring proper event triggering on > >> access violations. > >> > >> The correction involves modifying the handling mechanism to respect the > >> altp2m view's default_access upon its activation, eliminating the need for > >> setting memory access permissions for the entire altp2m range (e.g. within > >> xen-access.c). This change not only aligns the behavior with the expected > >> access control logic but also results in a significant performance > >> improvement > >> by reducing the overhead associated with setting memory access permissions > >> across the altp2m range. > >> > >> Signed-off-by: Petr Beneš > > Thanks Petr, this looks like a great change. > > > > Two things: > > > > - Probably worth adjusting the comment at the top of > > p2m_altp2m_get_or_propagate to mention that you use the altp2m > > default_access when propagating from the host p2m > > > > - This represents a change in behavior, so probably at least worth a > > mention in CHANGELOG.md? > > This is a bugfix to an tech preview feature. It's not remotely close to > CHANGELOG material. > > Tangent. SUPPORT.md says tech preview on ARM, despite there being no > support in the slightest. Patches were posted and never taken. > > > Tamas, I guess this is OK from an interface compatibility point of > > view? In theory it should always have been behaving this way. > > Given the already-provided Ack, I expect that has been considered and > deemed ok. Correct, this is just a bugfix. Tamas
Re: [PATCH] x86/altp2m: p2m_altp2m_get_or_propagate() should honor ap2m->default_access
On Tue, Feb 6, 2024 at 5:08 AM Petr Beneš wrote: > > From: Petr Beneš > > This patch addresses a behavior discrepancy in the handling of altp2m views, > where upon the creation and subsequent EPT violation, the page access > permissions were incorrectly inherited from the hostp2m instead of respecting > the altp2m default_access. > > Previously, when a new altp2m view was established with restrictive > default_access permissions and activated via xc_altp2m_switch_to_view(), > it failed to trigger an event on the first access violation. This behavior > diverged from the intended mechanism, where the altp2m's default_access > should dictate the initial permissions, ensuring proper event triggering on > access violations. > > The correction involves modifying the handling mechanism to respect the > altp2m view's default_access upon its activation, eliminating the need for > setting memory access permissions for the entire altp2m range (e.g. within > xen-access.c). This change not only aligns the behavior with the expected > access control logic but also results in a significant performance improvement > by reducing the overhead associated with setting memory access permissions > across the altp2m range. > > Signed-off-by: Petr Beneš Acked-by: Tamas K Lengyel
Re: [PATCH v7 4/7] xen/asm-generic: ifdef inclusion of
On Fri, Jan 26, 2024 at 10:42 AM Oleksii Kurochko wrote: > > ifdefing inclusion of in > allows to avoid generation of empty header > for the case when !CONFIG_MEM_ACCESS. > > For Arm it was explicitly added inclusion of for p2m.c > and traps.c because they require some functions from which > aren't available in case of !CONFIG_MEM_ACCESS. > > Suggested-by: Jan Beulich > Signed-off-by: Oleksii Kurochko Acked-by: Tamas K Lengyel
Re: [PATCH v7 3/7] xen/asm-generic: introduce stub header monitor.h
On Fri, Jan 26, 2024 at 10:42 AM Oleksii Kurochko wrote: > > The header is shared between several archs so it is > moved to asm-generic. > > Switch partly Arm and PPC to asm-generic/monitor.h and only > arch_monitor_get_capabilities() left in arch-specific/monitor.h. > > Signed-off-by: Oleksii Kurochko > Acked-by: Jan Beulich Acked-by: Tamas K Lengyel
Re: [PATCH v2 3/3] x86/vmx: Disallow the use of inactivity states
On Thu, Jan 11, 2024 at 6:13 PM Andrew Cooper wrote:
>
> Right now, vvmx will blindly copy L12's ACTIVITY_STATE into the L02 VMCS and
> enter the vCPU. Luckily for us, nested-virt is explicitly unsupported for
> security bugs.
>
> The inactivity states are HLT, SHUTDOWN and WAIT-FOR-SIPI, and as noted by the
> SDM in Vol3 27.7 "Special Features of VM Entry":
>
> If VM entry ends with the logical processor in an inactive activity state,
> the VM entry generates any special bus cycle that is normally generated when
> that activity state is entered from the active state.
>
> Also,
>
> Some activity states unconditionally block certain events.
>
> I.e. A VMEntry with ACTIVITY=SHUTDOWN will initiate a platform reset, while a
> VMEntry with ACTIVITY=WAIT-FOR-SIPI will really block everything other than
> SIPIs.
>
> Both of these activity states are for the TXT ACM to use, not for regular
> hypervisors, and Xen doesn't support dropping the HLT intercept either.
>
> There are two paths in Xen which operate on ACTIVITY_STATE.
>
> 1) The vmx_{get,set}_nonreg_state() helpers for VM-Fork.
>
>As regular VMs can't use any inactivity states, this is just duplicating
>the 0 from construct_vmcs(). Retain the ability to query activity_state,
>but crash the domain on any attempt to set an inactivity state.
>
> 2) Nested virt, because of ACTIVITY_STATE in vmcs_gstate_field[].
>
>Explicitly hide the inactivity states in the guest's view of MSR_VMX_MISC,
>and remove ACTIVITY_STATE from vmcs_gstate_field[].
>
>In virtual_vmentry(), we should trigger a VMEntry failure for the use of
>any inactivity states, but there's no support for that in the code at all
>so leave a TODO for when we finally start working on nested-virt in
>earnest.
>
> Reported-by: Reima Ishii
> Signed-off-by: Andrew Cooper
Reviewed-by: Tamas K Lengyel
Re: [PATCH] x86/mwait-idle: fix ubsan warning
On Fri, Jan 5, 2024 at 2:34 AM Jan Beulich wrote:
>
> On 04.01.2024 18:13, Tamas K Lengyel wrote:
> > Fix warning:
> > (XEN) UBSAN: Undefined behaviour in arch/x86/cpu/mwait-idle.c:1300:44
> > (XEN) left shift of 15 by 28 places cannot be represented in type 'int'
> >
> > Signed-off-by: Tamas K Lengyel
> > Fixes: 5a211704e88 ("mwait-idle: prevent SKL-H boot failure when C8+C9+C10
> > enabled")
>
> No matter that I appreciate the change, I think this wants fixing by a
> patch to the (Linux) original, which we'd then import (like we do for
> other changes, including the one referenced by the Fixes: tag).
Feel free to submit it to other projects if the same issue applies to
them. I only ran into this with Xen and can only test it with Xen.
Tamas
Re: [PATCH] xen: Use -Wuninitialized and -Winit-self
> > Getting ~0 back is strictly less bad than getting stack rubble because > > at least it's obviously wrong. > > But then why not change things so there's no issue anymore? Plus I'm not > sure how / whether "obviously wrong" would manifest. I expect it would > be an entirely unobvious boot hang, or other misbehavior. +1 for changing these APIs to make it clear when an error happened instead of returning magic value. Otherwise yea clearly should not use init-to-self anywhere just to silence other warnings.. Tamas
[PATCH] x86/mwait-idle: fix ubsan warning
Fix warning:
(XEN) UBSAN: Undefined behaviour in arch/x86/cpu/mwait-idle.c:1300:44
(XEN) left shift of 15 by 28 places cannot be represented in type 'int'
Signed-off-by: Tamas K Lengyel
Fixes: 5a211704e88 ("mwait-idle: prevent SKL-H boot failure when C8+C9+C10
enabled")
---
xen/arch/x86/include/asm/mwait.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/include/asm/mwait.h b/xen/arch/x86/include/asm/mwait.h
index f377d9f..9298f98 100644
--- a/xen/arch/x86/include/asm/mwait.h
+++ b/xen/arch/x86/include/asm/mwait.h
@@ -4,7 +4,7 @@
#include
#define MWAIT_SUBSTATE_MASK0xf
-#define MWAIT_CSTATE_MASK 0xf
+#define MWAIT_CSTATE_MASK 0xfU
#define MWAIT_SUBSTATE_SIZE4
#define CPUID_MWAIT_LEAF 5
--
2.34.1
Re: [XEN PATCH 4/7] xen/arm: mem_access: address violations of MISRA C:2012 Rule 16.3
On Wed, Dec 20, 2023 at 6:53 AM Julien Grall wrote:
>
> Hi Federico,
>
> On 20/12/2023 11:03, Federico Serafini wrote:
> > Refactor of the code to have a break statement at the end of the
> > switch-clause. This addresses violations of Rule 16.3
> > ("An unconditional `break' statement shall terminate every
> > switch-clause").
> > No functional change.
> >
> > Signed-off-by: Federico Serafini
> > ---
> > xen/arch/arm/mem_access.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> > index 31db846354..fbcb5471f7 100644
> > --- a/xen/arch/arm/mem_access.c
> > +++ b/xen/arch/arm/mem_access.c
> > @@ -168,10 +168,10 @@ p2m_mem_access_check_and_get_page(vaddr_t gva,
> > unsigned long flag,
> >* If this was a read then it was because of mem_access, but if
> > it was
> >* a write then the original get_page_from_gva fault was correct.
> >*/
> > -if ( flag == GV2M_READ )
> > -break;
> > -else
> > +if ( flag != GV2M_READ )
> > goto err;
> > +
> > +break;
>
> On both hunks, I find the original version better as it is easier to
> match with the comment. I also understand that it wouldn't be great to
> add a deviation for this construct. So maybe we should tweak a bit the
> comment?
Simplifying the if-else to a single if is fine by me. Adjusting the
comment to reflect the new logic would help though, so +1 to Julien's
comment.
Thanks,
Tamas
Re: INFORMAL VOTE REQUIRED - DOCUMENTATION WORDING
Hi all, I think this form is bad and is not helpful. We ought to be able to recommend an alternative term beside "broken" and "deprecated". I would not use the term broken in this context but that also doesn't mean we shouldn't use it in any context. But also in this context deprecated is not the right term to use either since deprecated would require us to actually make the old hypercalls stop working altogether at some future point, which we won't ever do AFAIU. My vote would be to use the term superseded in this context, which I can't express clearly in the form so I'm not going to cast a vote. Tamas On Thu, Nov 30, 2023 at 5:28 PM Stefano Stabellini wrote: > > Hi all, > > This vote is in the context of this thread: > https://marc.info/?l=xen-devel&m=169213351810075 > > > On Thu, 30 Nov 2023, Kelly Choi wrote: > > Hi all, > > There have been a few discussions about how we use documentation wording > > within the community. Whilst there are differences in opinions and > > perceptions of the definition, it would be helpful to see a wider consensus > > of how we feel. > > > > Discussion: Should we use the term 'broken' in our documentation, or do you > > think an alternative wording would be better? If you agree or > > disagree, please vote as this will impact future discussions. > > > > I have purposely made the vote between two options to help us move in a > > forward direction. > > > > PLEASE VOTE HERE. Deadline 15th December 2023. > > Your name will be required but will be private. If you answer anonymously, > > your vote will not count. This is to ensure it is fair and each > > person gets one vote. > > > > As an open-source project, we need to come to a common ground, which > > sometimes means we may not personally agree. To make this fair, please > > note the final results will be used to determine our future actions within > > the community. > > > > If the majority votes for/against, we will respect the majority and > > implement this accordingly. > > > > Many thanks, > > Kelly Choi > > > > Xen Project Community Manager > > XenServer, Cloud Software Group > > > >
Re: [PATCH] x86/mem_sharing: Release domain if we are not able to enable memory sharing
On Wed, Nov 22, 2023 at 2:42 PM Andrew Cooper wrote:
>
> On 22/11/2023 4:39 pm, Frediano Ziglio wrote:
> > In case it's not possible to enable memory sharing (mem_sharing_control
> > fails) we just return the error code without releasing the domain
> > acquired some lines above by rcu_lock_live_remote_domain_by_id.
>
> Fixes: 72f8d45d69b8 ("x86/mem_sharing: enable mem_sharing on first memop")
>
> > Signed-off-by: Frediano Ziglio
>
> Reviewed-by: Andrew Cooper
Acked-by: Tamas K Lengyel
Re: [PATCH] x86/mem_sharing: Fix typo in comment
On Wed, Nov 22, 2023 at 11:27 AM Andrew Cooper wrote: > > On 22/11/2023 4:26 pm, Frediano Ziglio wrote: > > ambigious -> ambiguous > > > > Signed-off-by: Frediano Ziglio > > Acked-by: Andrew Cooper Not sure if it's still needed but either case: Acked-by: Tamas K Lengyel
Re: [PATCH 2/2] x86/vmx: Disallow the use of inactivity states
On Wed, Nov 1, 2023 at 3:21 PM Andrew Cooper wrote:
>
> Right now, vvmx will blindly copy L12's ACTIVITY_STATE into the L02 VMCS and
> enter the vCPU. Luckily for us, nested-virt is explicitly unsupported for
> security bugs.
>
> The inactivity states are HLT, SHUTDOWN and WAIT-FOR-SIPI, and as noted by the
> SDM in Vol3 27.7 "Special Features of VM Entry":
>
> If VM entry ends with the logical processor in an inactive activity state,
> the VM entry generates any special bus cycle that is normally generated when
> that activity state is entered from the active state.
>
> Also,
>
> Some activity states unconditionally block certain events.
>
> I.e. A VMEntry with ACTIVITY=SHUTDOWN will initiate a platform reset, while a
> VMEntry with ACTIVITY=WAIT-FOR-SIPI will really block everything other than
> SIPIs.
>
> Both of these activity states are for the TXT ACM to use, not for regular
> hypervisors, and Xen doesn't support dropping the HLT intercept either.
>
> There are two paths in Xen which operate on ACTIVITY_STATE.
>
> 1) The vmx_{get,set}_nonreg_state() helpers for VM-Fork.
>
>As regular VMs can't use any inactivity states, this is just duplicating
>the 0 from construct_vmcs(). Drop the field, leaving a comment as to why
>it is skipped.
I would like to keep the vmx_get_nonreg_state() function being able to
gather this field as it might be an interesting piece of data we want
to keep an eye on during fuzzing. I would prefer just sanitizing the
value in the set() function with perhaps a gdprintk message that it
happened?
Tamas
Re: [PATCH for-4.18] x86/mem_sharing: add missing m2p entry when mapping shared_info page
On Fri, Oct 20, 2023 at 6:57 AM Andrew Cooper wrote:
>
> On 20/10/2023 2:14 am, Henry Wang wrote:
> > Hi all,
> >
> >> On Oct 20, 2023, at 07:48, Andrew Cooper wrote:
> >>
> >> On 18/10/2023 9:02 am, Tamas K Lengyel wrote:
> >>> When mapping in the shared_info page to a fork the m2p entry wasn't set
> >>> resulting in the shared_info being reset even when the fork reset was
> >>> called
> >>> with only reset_state and not reset_memory. This results in an extra
> >>> unnecessary TLB flush.
> >>>
> >>> Fixes: 1aac775 ("mem_sharing: map shared_info page to same gfn during
> >>> fork")
> >>> Signed-off-by: Tamas K Lengyel
> >>> ---
> >>> xen/arch/x86/mm/mem_sharing.c | 2 ++
> >>> 1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> >>> index 94b6b782ef..142258f16a 100644
> >>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>> @@ -1847,6 +1847,8 @@ static int copy_special_pages(struct domain *cd,
> >>> struct domain *d)
> >>> p2m_ram_rw, p2m->default_access, -1);
> >>> if ( rc )
> >>> return rc;
> >>> +
> >>> +set_gpfn_from_mfn(mfn_x(new_mfn), gfn_x(old_gfn));
> >>> }
> >>> }
> >>>
> >> Acked-by: Andrew Cooper
> >>
> >> CC Henry. This needs a view about a release ack.
> >>
> >> Cons: it's been broken since Xen 4.14 and we're very deep into the 4.18
> >> code freeze.
> >>
> >> Pros: it's a bug and would clearly qualify for backport, and is in a
> >> niche feature so isn't plausibly going to adversely affect other users.
> > Given the fact that it will be backported anyway, I had the same discussion
> > with Juergen
> > in his thread [1]. So if we can bear the risk of delaying merging this
> > patch for a week,
> > would it be ok to wait for the release and backport this patch to the
> > stable tree? Thanks!
> >
> > [1]
> > https://lore.kernel.org/xen-devel/83e6dcf6-926c-43a6-94d2-eb3b2c444...@arm.com/
> >
> > Kind regards,
> > Henry
>
> That's fine. I'll pull this into my next branch for when the 4.19 tree
> opens.
Thanks, I agree that backporting is perfectly fine for this one.
Tamas
[PATCH for-4.18] x86/mem_sharing: add missing m2p entry when mapping shared_info page
When mapping in the shared_info page to a fork the m2p entry wasn't set
resulting in the shared_info being reset even when the fork reset was called
with only reset_state and not reset_memory. This results in an extra
unnecessary TLB flush.
Fixes: 1aac775 ("mem_sharing: map shared_info page to same gfn during fork")
Signed-off-by: Tamas K Lengyel
---
xen/arch/x86/mm/mem_sharing.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 94b6b782ef..142258f16a 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1847,6 +1847,8 @@ static int copy_special_pages(struct domain *cd, struct
domain *d)
p2m_ram_rw, p2m->default_access, -1);
if ( rc )
return rc;
+
+set_gpfn_from_mfn(mfn_x(new_mfn), gfn_x(old_gfn));
}
}
--
2.34.1
Re: [XEN PATCH][for-next v2 5/7] x86/vm_event: add missing include for hvm_vm_event_do_resume
On Mon, Oct 9, 2023 at 2:55 AM Nicola Vetrini
wrote:
>
> The missing header makes the declaration visible when the function
> is defined, thereby fixing a violation of MISRA C:2012 Rule 8.4.
>
> Fixes: 1366a0e76db6 ("x86/vm_event: add hvm/vm_event.{h,c}")
> Signed-off-by: Nicola Vetrini
Acked-by: Tamas K Lengyel
Re: [XEN PATCH][for-next v2 7/7] x86/mem_access: make function static
On Mon, Oct 9, 2023 at 2:55 AM Nicola Vetrini wrote: > > The function is used only within this file, and therefore can be static. > > No functional change. > > Signed-off-by: Nicola Vetrini Acked-by: Tamas K Lengyel
Re: [PATCH v5 01/10] mem_sharing/fork: do not attempt to populate vcpu_info page
On Mon, Oct 2, 2023 at 11:12 AM Roger Pau Monne wrote:
>
> Instead let map_vcpu_info() and it's call to get_page_from_gfn()
> populate the page in the child as needed. Also remove the bogus
> copy_domain_page(): should be placed before the call to map_vcpu_info(),
> as the later can update the contents of the vcpu_info page.
>
> Note that this eliminates a bug in copy_vcpu_settings(): The function did
> allocate a new page regardless of the GFN already having a mapping, thus in
> particular breaking the case of two vCPU-s having their info areas on the same
> page.
>
> Fixes: 41548c5472a3 ('mem_sharing: VM forking')
> Signed-off-by: Roger Pau Monné
Re-sending due to mailserver issues:
Acked-by: Tamas K Lengyel
Re: [PATCH v6 06/10] x86/mem-sharing: copy GADDR based shared guest areas
On Wed, Oct 4, 2023 at 9:54 AM Roger Pau Monne wrote: > > From: Jan Beulich > > In preparation of the introduction of new vCPU operations allowing to > register the respective areas (one of the two is x86-specific) by > guest-physical address, add the necessary fork handling (with the > backing function yet to be filled in). > > Signed-off-by: Jan Beulich > Signed-off-by: Roger Pau Monné Acked-by: Tamas K Lengyel
Re: [PATCH v5 06/10] x86/mem-sharing: copy GADDR based shared guest areas
On Tue, Oct 3, 2023 at 11:07 AM Julien Grall wrote:
>
> Hi Roger,
>
> On 03/10/2023 15:29, Roger Pau Monné wrote:
> > On Tue, Oct 03, 2023 at 09:53:11AM -0400, Tamas K Lengyel wrote:
>
> Tamas, somehow your e-mails don't show up in my inbox (even if I am
> CCed) or even on lore.kernel.org/xen-devel. It is not even in my SPAM
> folder.
Thanks, I've switched mailservers, hopefully that resolves the issue.
>
> >> On Mon, Oct 2, 2023 at 11:13 AM Roger Pau Monne
> >> wrote:
> >>>
> >>> From: Jan Beulich
> >>>
> >>> In preparation of the introduction of new vCPU operations allowing to
> >>> register the respective areas (one of the two is x86-specific) by
> >>> guest-physical address, add the necessary fork handling (with the
> >>> backing function yet to be filled in).
> >>>
> >>> Signed-off-by: Jan Beulich
> >>> Signed-off-by: Roger Pau Monné
> >>> ---
> >>> Changes since v4:
> >>> - Rely on map_guest_area() to populate the child p2m if necessary.
> >>> ---
> >>> xen/arch/x86/mm/mem_sharing.c | 31 +++
> >>> xen/common/domain.c | 7 +++
> >>> 2 files changed, 38 insertions(+)
> >>>
> >>> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> >>> index 5f8f1fb4d871..99cf001fd70f 100644
> >>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>> @@ -1641,6 +1641,24 @@ static void copy_vcpu_nonreg_state(struct vcpu
> >>> *d_vcpu, struct vcpu *cd_vcpu)
> >>> hvm_set_nonreg_state(cd_vcpu, &nrs);
> >>> }
> >>>
> >>> +static int copy_guest_area(struct guest_area *cd_area,
> >>> + const struct guest_area *d_area,
> >>> + struct vcpu *cd_vcpu,
> >>> + const struct domain *d)
> >>> +{
> >>> +unsigned int offset;
> >>> +
> >>> +/* Check if no area to map, or already mapped. */
> >>> +if ( !d_area->pg || cd_area->pg )
> >>> +return 0;
> >>> +
> >>> +offset = PAGE_OFFSET(d_area->map);
> >>> +return map_guest_area(cd_vcpu, gfn_to_gaddr(
> >>> + mfn_to_gfn(d,
> >>> page_to_mfn(d_area->pg))) +
> >>> + offset,
> >>> + PAGE_SIZE - offset, cd_area, NULL);
> >>> +}
> >>> +
> >>> static int copy_vpmu(struct vcpu *d_vcpu, struct vcpu *cd_vcpu)
> >>> {
> >>> struct vpmu_struct *d_vpmu = vcpu_vpmu(d_vcpu);
> >>> @@ -1709,6 +1727,16 @@ static int copy_vcpu_settings(struct domain *cd,
> >>> const struct domain *d)
> >>> return ret;
> >>> }
> >>>
> >>> +/* Same for the (physically registered) runstate and time info
> >>> areas. */
> >>> +ret = copy_guest_area(&cd_vcpu->runstate_guest_area,
> >>> + &d_vcpu->runstate_guest_area, cd_vcpu, d);
> >>> +if ( ret )
> >>> +return ret;
> >>> +ret = copy_guest_area(&cd_vcpu->arch.time_guest_area,
> >>> + &d_vcpu->arch.time_guest_area, cd_vcpu, d);
> >>> +if ( ret )
> >>> +return ret;
> >>> +
> >>> ret = copy_vpmu(d_vcpu, cd_vcpu);
> >>> if ( ret )
> >>> return ret;
> >>> @@ -1950,7 +1978,10 @@ int mem_sharing_fork_reset(struct domain *d, bool
> >>> reset_state,
> >>>
> >>>state:
> >>> if ( reset_state )
> >>> +{
> >>> rc = copy_settings(d, pd);
> >>> +/* TBD: What to do here with -ERESTART? */
> >>
> >> There is no situation where we get an -ERESTART here currently. Is
> >> map_guest_area expected to run into situations where it fails with
> >> that rc?
> >
> > Yes, there's a spin_trylock() call that will result in
> > map_guest_area() returning -ERESTART.
> >
> >> If yes we might need a lock in place so we can block until it
> >> can succeed.
> >
> > I'm not sure whether returning -ERESTART can actually happen in
> > map_guest_area() for the fork case: the child domain is still paused
> > at this point, so there can't be concurrent guest hypercalls that
> > would also cause the domain hypercall_deadlock_mutex to be acquired.
Perhaps turning it into an ASSERT(rc != -ERESTART) is the way to go at
this point. If we run into any cases where it trips we can reason it
out.
> hypercall_deadlock_mutex is also acquired by domctls. So, I believe,
> -ERESTART could be returned if the toolstack is also issuing domclt
> right at the same time as forking.
That's not a concern in this path, only toolstack can start the reset
so we can assume it can coordinate not to have another toolstack
messing with the fork at the same time.
Thanks,
Tamas
Re: [PATCH v1 44/57] xen/riscv: introduce asm/vm_event.h
On Wed, Aug 16, 2023 at 12:30 PM Oleksii Kurochko wrote: > > Signed-off-by: Oleksii Kurochko > --- > xen/arch/riscv/include/asm/vm_event.h | 52 +++ > 1 file changed, 52 insertions(+) > create mode 100644 xen/arch/riscv/include/asm/vm_event.h I don't think we ought to replicate this header for every arch when clearly the functions in them are only relevant for specific architectures. Would make more sense to me to just conditionalize the caller side to the specific archs where these functions are actually defined, which would largely just be CONFIG_X86. Tamas
Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas
On Thu, May 4, 2023 at 3:44 AM Jan Beulich wrote:
>
> On 03.05.2023 19:14, Tamas K Lengyel wrote:
> >> @@ -1974,7 +2046,10 @@ int mem_sharing_fork_reset(struct domain
> >>
> >> state:
> >> if ( reset_state )
> >> +{
> >> rc = copy_settings(d, pd);
> >> +/* TBD: What to do here with -ERESTART? */
> >
> > Ideally we could avoid hitting code-paths that are restartable during
fork
> > reset since it gets called from vm_event replies that have no concept of
> > handling errors. If we start having errors like this we would just have
to
> > drop the vm_event reply optimization and issue a standalone fork reset
> > hypercall every time which isn't a big deal, it's just slower.
>
> I'm afraid I don't follow: We are in the process of fork-reset here. How
> would issuing "a standalone fork reset hypercall every time" make this
> any different? The possible need for a continuation here comes from a
> failed spin_trylock() in map_guest_area(). That won't change the next
> time round.
Why not? Who is holding the lock and why wouldn't it ever relinquish it? If
that's really true then there is a larger issue then just not being able to
report the error back to the user on vm_event_resume path and we need to
devise a way of being able to copy this from the parent bypassing this
lock. The parent is paused and its state should not be changing while forks
are active, so if the lock was turned into an rwlock of some sort so we can
acquire the read-lock would perhaps be a possible way out of this.
>
> But perhaps I should say that till now I didn't even pay much attention
> to the 2nd use of the function by vm_event_resume(); I was mainly
> focused on the one from XENMEM_sharing_op_fork_reset, where no
> continuation handling exists. Yet perhaps your comment is mainly
> related to that use?
>
> I actually notice that the comment ahead of the function already has a
> continuation related TODO, just that there thought is only of larger
> memory footprint.
With XENMEM_sharing_op_fork_reset the caller actually receives the error
code and can decide what to do next. With vm_event_resume there is no path
currently to notify the agent of an error. We could generate another
vm_event to send such an error, but the expectation with fork_reset is that
it will always work because the parent is paused, so not having that path
for an error to get back to the agent isn't a big deal.
Now, if it becomes the case that due to this locking we can get an error
even while the parent is paused, that will render the vm_event_resume path
unreliably so we would just switch to using XENMEM_sharing_op_fork_reset so
that at least it can re-try in case of an issue. Of course, only if a
reissue of the hypercall has any reasonable chance of succeeding.
>
> > My
> > preference would actually be that after the initial forking is
performed a
> > local copy of the parent's state is maintained for the fork to reset to
so
> > there would be no case of hitting an error like this. It would also
allow
> > us in the future to unpause the parent for example..
>
> Oh, I guess I didn't realize the parent was kept paused. Such state
> copying / caching may then indeed be a possibility, but that's nothing
> I can see myself deal with, even less so in the context of this series.
> I need a solution to the problem at hand within the scope of what is
> there right now (or based on what could be provided e.g. by you within
> the foreseeable future). Bubbling up the need for continuation from the
> XENMEM_sharing_op_fork_reset path is the most I could see me handle
> myself ... For vm_event_resume() bubbling state up the domctl path
> _may_ also be doable, but mem_sharing_notification() and friends don't
> even check the function's return value.
Sure, I wasn't expecting that work to be done as part of this series, just
as something I would like to get to in the future.
Tamas
Re: [PATCH v3 4/8] x86/mem-sharing: copy GADDR based shared guest areas
> @@ -1974,7 +2046,10 @@ int mem_sharing_fork_reset(struct domain
>
> state:
> if ( reset_state )
> +{
> rc = copy_settings(d, pd);
> +/* TBD: What to do here with -ERESTART? */
Ideally we could avoid hitting code-paths that are restartable during fork
reset since it gets called from vm_event replies that have no concept of
handling errors. If we start having errors like this we would just have to
drop the vm_event reply optimization and issue a standalone fork reset
hypercall every time which isn't a big deal, it's just slower. My
preference would actually be that after the initial forking is performed a
local copy of the parent's state is maintained for the fork to reset to so
there would be no case of hitting an error like this. It would also allow
us in the future to unpause the parent for example..
Tamas
Re: Best way to use altp2m to support VMFUNC EPT-switching?
On Wed, Mar 29, 2023 at 10:29 PM Johnson, Ethan wrote: > > On 2023-03-16 02:14:18 +, Andrew Cooper wrote: > > Ok, so there is a lot here. Apologies in advance for the overly long > > answer. > > > > First, while altp2m was developed in parallel with EPTP-switching, we > > took care to split the vendor neutral parts from the vendor specific > > bits. So while we do have VMFUNC support, that's considered "just" a > > hardware optimisation to speed up the HVMOP_altp2m_switch_p2m hypercall. > > > > But before you start, it is important to understand your security > > boundaries. You've found external mode, and this is all about > > controlling which aspects of altp2m the guest can invoke itself, and > > modes other than external let the guest issue HVMOP_altp2m ops itself. > > > > If you permit the guest to change views itself, either with VMFUNC, or > > HVMOP_altp2m_switch_p2m, you have to realise that these are just > > "regular" CPL0 actions, and can be invoked by any kernel code, not just > > your driver. i.e. the union of all primary and alternative views is one > > single security domain. > > > > For some usecases this is fine, but yours doesn't look like it fits in > > this category. In particular, no amount of protection on the trampoline > > pages stops someone writing a VMFUNC instruction elsewhere in kernel > > space and executing it. > > > > (I have seen plenty of research papers try to construct a security > > boundary around VMFUNC. I have yet see one that does so robustly, but I > > do enjoy being surprised on occasion...) > > > > The first production use this technology I'm aware of was Bitdefender's > > HVMI, where the guest had no control at all, and was subject to the > > permission restrictions imposed on it by the agent in dom0. The agent > > trapped everything it considered sensitive, including writes to > > sensitive areas of memory using reduced EPT permissions, and either > > permitted execution to continue, or took other preventative action. > > > > This highlights another key point. Some entity in the system needs to > > deal with faults that occur when the guest accidentally (or otherwise) > > violates the reduced EPT permissions. #VE is, again, an optimisation to > > let violations be handled in guest context, rather than taking a VMExit, > > but even with #VE the complicated corner cases are left to the external > > agent. > > > > With HVMI, #VE (but not VMFUNC IIRC) did get used as an optimisation to > > mitigate the perf hit from Window's Meltdown mitigation electing to use > > LOCK'd BTS/BTC operations on pagetables (which were write protected > > behind the scenes), but I'm reliably informed that the hoops required to > > jump through to make that work, and in particular avoid the notice of > > PatchGuard, were substantial. > > > > Perhaps a more accessible example is > > https://github.com/intel/kernel-fuzzer-for-xen-project and the > > underlying libvmi. There is also a very basic example in > > tools/misc/xen-access.c in the Xen tree. > > > > For your question specifically about mapping other frames, we do have > > hypercalls to map other frames (its necessary for e.g. mapping BARs of > > passed-through PCI devices), but for obvious reasons, it's restricted to > > control software (Qemu) in dom0. I suspect we don't actually have a > > hypercall to map MMIO into an alternative view, but it shouldn't be hard > > to add (if you still decide you want it by the end of this email). > > > > > > But on to the specifics of mapping the xAPIC page. Sorry, but > > irrespective of altp2m, that is a non-starter, for reasons that date > > back to ~1997 or thereabouts. > > > > It's worth saying that AMD can fully virtualise IPI delivery from one > > vCPU to another without either taking a VMExit in the common case, since > > Zen1 (IIRC). Intel has a similar capability since Sapphire Rapids > > (IIRC). Xen doesn't support either yet, because there are only so many > > hours in the day... > > > > It is technically possible to map the xAPIC window into a guest, and > > such a guest could interact the real interrupt controller. But now > > you've got the problem that two bits of software (Xen, and your magic > > piece of guest kernel) are trying to driver the same single interrupt > > controller. > > > > Even if you were to say that the guest would only use ICR to send > > interrupts, that still doesn't work. In xAPIC, ICR is formed of two > > half registers, as it dates from the days of 32bit processors, with a > > large stride between the two half registers. > > > > Therefore, it is a minimum of two separate instructions (set destination > > in ICR_HI, set type/mode/etc in ICR_LO) to send an interrupt. > > > > A common bug in kernels is to try and send IPIs when interrupts are > > enabled, or in NMI context, both of which could interrupt an IPI > > sequence. This results in a sequence of writes (from the LAPIC's point > > of view) of ICR_HI, ICR_HI, ICR_LO, ICR_LO, whic
Re: [XEN PATCH v5] x86/monitor: Add new monitor event to catch I/O instructions
On Tue, Mar 28, 2023 at 4:59 AM Jan Beulich wrote: > > On 21.03.2023 14:58, Dmitry Isaykin wrote: > > Adds monitor support for I/O instructions. > > > > Signed-off-by: Dmitry Isaykin > > Signed-off-by: Anton Belousov > > Acked-by: Jan Beulich Acked-by: Tamas K Lengyel
Re: [XEN PATCH v1 1/1] x86/domctl: add gva_to_gfn command
On Tue, Mar 21, 2023 at 3:49 AM Ковалёв Сергей wrote: > > > > 21.03.2023 2:34, Tamas K Lengyel пишет: > > > > > > On Mon, Mar 20, 2023 at 3:23 PM Ковалёв Сергей > <mailto:va...@list.ru>> wrote: > > > > > > > > > > > > 21.03.2023 1:51, Tamas K Lengyel wrote: > > > > > > > > > > > > On Mon, Mar 20, 2023 at 12:32 PM Ковалёв Сергей > <mailto:va...@list.ru> > > > > <mailto:va...@list.ru <mailto:va...@list.ru>>> wrote: > > > > > > > > > > gva_to_gfn command used for fast address translation in LibVMI > > project. > > > > > With such a command it is possible to perform address translation in > > > > > single call instead of series of queries to get every page table. > > > > > > > > You have a couple assumptions here: > > > > - Xen will always have a direct map of the entire guest memory - > > there > > > > are already plans to move away from that. Without that this approach > > > > won't have any advantage over doing the same mapping by LibVMI > > > > > > Thanks! I didn't know about the plan. Though I use this patch > > > back ported into 4.16. > > > > > > > - LibVMI has to map every page for each page table for every lookup - > > > > you have to do that only for the first, afterwards the pages on which > > > > the pagetable is are kept in a cache and subsequent lookups would be > > > > actually faster then having to do this domctl since you can keep being > > > > in the same process instead of having to jump to Xen. > > > > > > Yes. I know about the page cache. But I have faced with several issues > > > with cache like this one https://github.com/libvmi/libvmi/pull/1058 > > <https://github.com/libvmi/libvmi/pull/1058> . > > > So I had to disable the cache. > > > > The issue you linked to is an issue with a stale v2p cache, which is a > > virtual TLB. The cache I talked about is the page cache, which is just > > maintaining a list of the pages that were accessed by LibVMI for future > > accesses. You can have one and not the other (ie. ./configure > > --disable-address-cache --enable-page-cache). > > > > Tamas > > Thanks. I know about the page cache. Though I'm not familiar with > it close enough. > > As far as I understand at the time the page cache implementation in > LibVMI looks like this: > 1. Call sequence: vmi_read > vmi_read_page > driver_read_page > > xen_read_page > memory_cache_insert ..> get_memory_data > > xen_get_memory > xen_get_memory_pfn > xc_map_foreign_range > 2. This is perfectly valid while guest OS keeps page there. And > physical pages are always there. > 3. To renew cache the "age_limit" counter is used. > 4. In Xen driver implementation in LibVMI the "age_limit" is > disabled. > 5. Also it is possible to invalidate cache with "xen_write" or > "vmi_pagecache_flush". But it is not used. > 6. Other way to avoid too big cache is cache size limit. So on > every insert half of the cache is dropped on size overflow. > > So the only thing we should know is valid mapping of guest > virtual address to guest physical address. > > And the slow paths are: > 1. A first traversal of new page table set. E.g. for the new process. > 2. Or new subset of page tables for known process. > 3. Subsequent page access after cache clean on size overflow. > > Am I right? > > The main idea behind the patch: > 1. For the very first time it would be done faster with hypercall. > 2. For subsequent calls v2p translation cache could be used (used in > my current work in LibVMI). > 3. To avoid errors with stale cache v2p cache could be invalidated > on every event (VMI_FLUSH_RATE = 1). If you set a flush rate to 1 then you would effectively run without any caching between events. It will still be costly. Yes, you save some performance on having to map the pages because Xen already has them but overall this is still a subpar solution. IMHO you are not addressing the real issue, which is just the lack of hooks into the OS that would tell you when the v2p cache actually needs to be invalidated. The OS does TLB maintenance already when it updates its pagetables. If you wrote logic to hook into that, you wouldn't have to disable the caches or run with flush rate = 1. On the DRAKVUF side this has been a TODO for a long time https://github.com/tklengyel/drakvuf/blob/df2d274dfe349bbdacdb121229707f6c91449b38/src/libdrakvuf/private.h#L140. If you had those hooks into the TLB maintenance logic you could just use the existing page-cache and be done with it. Yes, the very first access may still be slower than the hypercall but I doubt it would be noticeable in the long run. Tamas
Re: [XEN PATCH v1 1/1] x86/domctl: add gva_to_gfn command
On Mon, Mar 20, 2023 at 3:34 PM Andrew Cooper wrote: > > On 20/03/2023 7:22 pm, Ковалёв Сергей wrote: > > > > 21.03.2023 1:51, Tamas K Lengyel wrote: > >> On Mon, Mar 20, 2023 at 12:32 PM Ковалёв Сергей >> <mailto:va...@list.ru>> wrote: > >> > > >> > gva_to_gfn command used for fast address translation in LibVMI > >> project. > >> > With such a command it is possible to perform address translation in > >> > single call instead of series of queries to get every page table. > >> > >> You have a couple assumptions here: > >> - Xen will always have a direct map of the entire guest memory - > >> there are already plans to move away from that. Without that this > >> approach won't have any advantage over doing the same mapping by LibVMI > > > > Thanks! I didn't know about the plan. > > To be clear, "not mapping the guest by default" is for speculative > safety/hardening reasons. > > Xen will always need to be capable of mapping arbitrary parts of the > guest, even if only transiently, so there's no relevant interaction with > this new proposed hypercall. > > > The truth is that Xen will always be able to do a single pagewalk faster > than libvmi can do it (via mappings, or otherwise), but if libvmi does > properly maintain a cache of mappings then it will be faster that > repeated hypercalls into Xen. Where the split lies depends heavily on > the libvmi workload. > > I don't see a problem in principle with a hypercall like this - it is > "just" a performance optimisation over capabilities that libvmi already > has - but the version presented here is overly simplistic. For debugging purposes sure it would be fine to have this hypercall but I wouldn't set it as the default for LibVMI. Oftentimes the lookup needs to be more nuanced then what Xen understands about paging. For example, on Windows guests you can have transition pages that don't have the present bit set yet are perfectly valid for introspection purposes ( https://citeseerx.ist.psu.edu/document?repid=rep1&type=pdf&doi=3311ed0c63d4ca707c49256655e401f37f25ec50). Xen would need to be enlightened about this type of OS-specific tidbits for which I think LibVMI is a much better place to keep the logic for. Tamas
Re: [XEN PATCH v1 1/1] x86/domctl: add gva_to_gfn command
On Mon, Mar 20, 2023 at 3:23 PM Ковалёв Сергей wrote: > > > > 21.03.2023 1:51, Tamas K Lengyel wrote: > > > > > > On Mon, Mar 20, 2023 at 12:32 PM Ковалёв Сергей > <mailto:va...@list.ru>> wrote: > > > > > > gva_to_gfn command used for fast address translation in LibVMI project. > > > With such a command it is possible to perform address translation in > > > single call instead of series of queries to get every page table. > > > > You have a couple assumptions here: > > - Xen will always have a direct map of the entire guest memory - there > > are already plans to move away from that. Without that this approach > > won't have any advantage over doing the same mapping by LibVMI > > Thanks! I didn't know about the plan. Though I use this patch > back ported into 4.16. > > > - LibVMI has to map every page for each page table for every lookup - > > you have to do that only for the first, afterwards the pages on which > > the pagetable is are kept in a cache and subsequent lookups would be > > actually faster then having to do this domctl since you can keep being > > in the same process instead of having to jump to Xen. > > Yes. I know about the page cache. But I have faced with several issues > with cache like this one https://github.com/libvmi/libvmi/pull/1058 . > So I had to disable the cache. The issue you linked to is an issue with a stale v2p cache, which is a virtual TLB. The cache I talked about is the page cache, which is just maintaining a list of the pages that were accessed by LibVMI for future accesses. You can have one and not the other (ie. ./configure --disable-address-cache --enable-page-cache). Tamas
Re: [XEN PATCH v1 1/1] x86/domctl: add gva_to_gfn command
On Mon, Mar 20, 2023 at 12:32 PM Ковалёв Сергей wrote: > > gva_to_gfn command used for fast address translation in LibVMI project. > With such a command it is possible to perform address translation in > single call instead of series of queries to get every page table. You have a couple assumptions here: - Xen will always have a direct map of the entire guest memory - there are already plans to move away from that. Without that this approach won't have any advantage over doing the same mapping by LibVMI - LibVMI has to map every page for each page table for every lookup - you have to do that only for the first, afterwards the pages on which the pagetable is are kept in a cache and subsequent lookups would be actually faster then having to do this domctl since you can keep being in the same process instead of having to jump to Xen. With these perspectives in mind I don't think this would be a useful addition. Please prove me wrong with performance numbers and a specific use-case that warrants adding this and how you plan to introduce it into LibVMI without causing performance regression to all other use-cases. Tamas
Re: [XEN PATCH v3] x86/monitor: Add new monitor event to catch I/O instructions
> Are you actually sure you want to tie the vm-event interface to the ioreq > one (this is also a question to you, Tamas)? It would look slightly better > to me if this was a simple boolean named after its purpose (e.g. "write" > or "out" when it's meant to be set for OUT / OUTS and clear for IN / INS). A boolean would be preferred indeed. Tamas
Re: [XEN PATCH v2] x86/monitor: Add new monitor event to catch I/O instructions
On Wed, Mar 15, 2023 at 2:55 PM Dmitry Isaykin wrote: > > Adds monitor support for I/O instructions. > > Signed-off-by: Dmitry Isaykin > Signed-off-by: Anton Belousov Acked-by: Tamas K Lengyel
Re: [XEN PATCH] x86/monitor: Add new monitor event to catch I/O instructions
On Fri, Mar 10, 2023 at 10:57 PM Dmitry Isaykin
wrote:
>
> Adds monitor support for I/O instructions.
>
> Signed-off-by: Dmitry Isaykin
> Signed-off-by: Anton Belousov
> ---
> tools/include/xenctrl.h| 1 +
> tools/libs/ctrl/xc_monitor.c | 13 +
> xen/arch/x86/hvm/hvm.c | 5 +
> xen/arch/x86/hvm/monitor.c | 21 +
> xen/arch/x86/hvm/vmx/vmx.c | 2 ++
> xen/arch/x86/include/asm/domain.h | 1 +
> xen/arch/x86/include/asm/hvm/monitor.h | 3 +++
> xen/arch/x86/include/asm/hvm/support.h | 3 +++
> xen/arch/x86/include/asm/monitor.h | 3 ++-
> xen/arch/x86/monitor.c | 13 +
> xen/include/public/domctl.h| 1 +
> xen/include/public/vm_event.h | 10 ++
> 12 files changed, 75 insertions(+), 1 deletion(-)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 23037874d3..05967ecc92 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -2102,6 +2102,7 @@ int xc_monitor_emul_unimplemented(xc_interface
*xch, uint32_t domain_id,
>bool enable);
> int xc_monitor_vmexit(xc_interface *xch, uint32_t domain_id, bool enable,
>bool sync);
> +int xc_monitor_io(xc_interface *xch, uint32_t domain_id, bool enable);
> /**
> * This function enables / disables emulation for each REP for a
> * REP-compatible instruction.
> diff --git a/tools/libs/ctrl/xc_monitor.c b/tools/libs/ctrl/xc_monitor.c
> index c5fa62ff30..3cb96f444f 100644
> --- a/tools/libs/ctrl/xc_monitor.c
> +++ b/tools/libs/ctrl/xc_monitor.c
> @@ -261,6 +261,19 @@ int xc_monitor_vmexit(xc_interface *xch, uint32_t
domain_id, bool enable,
> return do_domctl(xch, &domctl);
> }
>
> +int xc_monitor_io(xc_interface *xch, uint32_t domain_id, bool enable)
> +{
> +DECLARE_DOMCTL;
> +
> +domctl.cmd = XEN_DOMCTL_monitor_op;
> +domctl.domain = domain_id;
> +domctl.u.monitor_op.op = enable ? XEN_DOMCTL_MONITOR_OP_ENABLE
> +: XEN_DOMCTL_MONITOR_OP_DISABLE;
> +domctl.u.monitor_op.event = XEN_DOMCTL_MONITOR_EVENT_IO;
> +
> +return do_domctl(xch, &domctl);
> +}
> +
> /*
> * Local variables:
> * mode: C
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 0c81e2afc7..72c9f65626 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -3484,6 +3484,11 @@ int hvm_vmexit_cpuid(struct cpu_user_regs *regs,
unsigned int inst_len)
> return hvm_monitor_cpuid(inst_len, leaf, subleaf);
> }
>
> +void hvm_io_instruction_intercept(uint16_t port, int dir, unsigned int
bytes, unsigned int string_ins)
> +{
> +hvm_monitor_io_instruction(port, dir, bytes, string_ins);
> +}
> +
> void hvm_rdtsc_intercept(struct cpu_user_regs *regs)
> {
> msr_split(regs, hvm_get_guest_tsc(current));
> diff --git a/xen/arch/x86/hvm/monitor.c b/xen/arch/x86/hvm/monitor.c
> index a11cd76f4d..f8b11d1de9 100644
> --- a/xen/arch/x86/hvm/monitor.c
> +++ b/xen/arch/x86/hvm/monitor.c
> @@ -233,6 +233,27 @@ int hvm_monitor_cpuid(unsigned long insn_length,
unsigned int leaf,
> return monitor_traps(curr, 1, &req);
> }
>
> +void hvm_monitor_io_instruction(uint16_t port, int dir,
> +unsigned int bytes, unsigned int
string_ins)
> +{
> +struct vcpu *curr = current;
> +struct arch_domain *ad = &curr->domain->arch;
> +vm_event_request_t req = {};
> +
> +if ( !ad->monitor.io_enabled )
> +return;
> +
> +req.reason = VM_EVENT_REASON_IO_INSTRUCTION;
> +req.u.io_instruction.data_size = bytes;
> +req.u.io_instruction.port = port;
> +req.u.io_instruction.dir = dir;
> +req.u.io_instruction.string_ins = string_ins;
> +
> +set_npt_base(curr, &req);
> +
> +monitor_traps(curr, true, &req);
> +}
> +
> void hvm_monitor_interrupt(unsigned int vector, unsigned int type,
> unsigned int err, uint64_t cr2)
> {
> diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> index 278b829f73..a64c5078c5 100644
> --- a/xen/arch/x86/hvm/vmx/vmx.c
> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> @@ -4579,6 +4579,8 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> uint16_t port = (exit_qualification >> 16) & 0x;
> int bytes = (exit_qualification & 0x07) + 1;
> int dir = (exit_qualification & 0x08) ? IOREQ_READ :
IOREQ_WRITE;
> +int str_ins = (exit_qualification & 0x10) ? 1 : 0;
You are already in a branch here where str_ins is checked and known to be 1.
> +hvm_io_instruction_intercept(port, dir, bytes, str_ins);
IMHO you should have this intercept be called outside the if-else. The
function already kind-of indicates str_ins is an input yet right now only
called when it's 1.
The rest of the plumbing in the patch LGTM.
Thanks,
Tamas
Re: [PATCH v2] x86: Perform mem_sharing teardown before paging teardown
On Wed, Feb 22, 2023 at 5:48 AM Jan Beulich wrote:
>
> On 21.02.2023 16:59, Tamas K Lengyel wrote:
> > On Tue, Feb 21, 2023 at 8:54 AM Jan Beulich wrote:
> >>
> >> On 15.02.2023 18:07, Tamas K Lengyel wrote:
> >>> An assert failure has been observed in p2m_teardown when performing vm
> >>> forking and then destroying the forked VM (p2m-basic.c:173). The
assert
> >>> checks whether the domain's shared pages counter is 0. According to
the
> >>> patch that originally added the assert (7bedbbb5c31) the p2m_teardown
> >>> should only happen after mem_sharing already relinquished all shared
> > pages.
> >>>
> >>> In this patch we flip the order in which relinquish ops are called to
> > avoid
> >>> tripping the assert.
> >>>
> >>> Signed-off-by: Tamas K Lengyel
> >>> Fixes: e7aa55c0aab3 ("x86/p2m: free the paging memory pool
> > preemptively")
> >>
> >> Especially considering the backporting requirement I'm disappointed to
> >> find that you haven't extended the description to explain why this
> >> heavier code churn is to be preferred over an adjustment to the
offending
> >> assertion. As said in reply to v1 - I'm willing to accept arguments
> >> towards this being better, but I need to know those arguments.
> >
> > The assert change as you proposed is hard to understand and will be thus
> > harder to maintain. Conceptually sharing being torn down makes sense to
> > happen before paging is torn down.
>
> This is (perhaps) the crucial thing to say. Whereas ...
>
> > Considering that's how it has been
> > before the unfortunate regression I'm fixing here I don't think more
> > justification is needed.
>
> ... I disagree here - that's _not_ how it has been before:
paging_teardown()
> has always been called first. The regression was with how much of teardown
> is performed from there vs at the final stage of domain cleanup.
>
> I'd be fine adding the "Conceptually ..." sentence to the description when
> committing, of course provided you agree.
Perfectly fine by me.
Thanks,
Tamas
Re: [PATCH v2] x86: Perform mem_sharing teardown before paging teardown
On Tue, Feb 21, 2023 at 8:54 AM Jan Beulich wrote:
>
> On 15.02.2023 18:07, Tamas K Lengyel wrote:
> > An assert failure has been observed in p2m_teardown when performing vm
> > forking and then destroying the forked VM (p2m-basic.c:173). The assert
> > checks whether the domain's shared pages counter is 0. According to the
> > patch that originally added the assert (7bedbbb5c31) the p2m_teardown
> > should only happen after mem_sharing already relinquished all shared
pages.
> >
> > In this patch we flip the order in which relinquish ops are called to
avoid
> > tripping the assert.
> >
> > Signed-off-by: Tamas K Lengyel
> > Fixes: e7aa55c0aab3 ("x86/p2m: free the paging memory pool
preemptively")
>
> Especially considering the backporting requirement I'm disappointed to
> find that you haven't extended the description to explain why this
> heavier code churn is to be preferred over an adjustment to the offending
> assertion. As said in reply to v1 - I'm willing to accept arguments
> towards this being better, but I need to know those arguments.
The assert change as you proposed is hard to understand and will be thus
harder to maintain. Conceptually sharing being torn down makes sense to
happen before paging is torn down. Considering that's how it has been
before the unfortunate regression I'm fixing here I don't think more
justification is needed. The code-churn is minimal anyway.
Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 10:24 AM Jan Beulich wrote:
>
> On 16.02.2023 16:10, Tamas K Lengyel wrote:
> > On Thu, Feb 16, 2023 at 9:27 AM Jan Beulich wrote:
> >>
> >> On 16.02.2023 13:22, Tamas K Lengyel wrote:
> >>> On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote:
> >>>>
> >>>> On 15.02.2023 17:29, Tamas K Lengyel wrote:
> >>>>> On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich
wrote:
> >>>>>> Did you consider the alternative of adjusting the ASSERT() in
> > question
> >>>>>> instead? It could reasonably become
> >>>>>>
> >>>>>> #ifdef CONFIG_MEM_SHARING
> >>>>>> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root ||
> >>>>> !atomic_read(&d->shr_pages));
> >>>>>> #endif
> >>>>>>
> >>>>>> now, I think. That would be less intrusive a change (helpful for
> >>>>>> backporting), but there may be other (so far unnamed) benefits of
> >>> pulling
> >>>>>> ahead the shared memory teardown.
> >>>>>
> >>>>> I have a hard time understanding this proposed ASSERT.
> >>>>
> >>>> It accounts for the various ways p2m_teardown() can (now) be called,
> >>>> limiting the actual check for no remaining shared pages to the last
> >>>> of all these invocations (on the host p2m with remove_root=true).
> >>>>
> >>>> Maybe
> >>>>
> >>>> /* Limit the check to the final invocation. */
> >>>> if ( p2m_is_hostp2m(p2m) && remove_root )
> >>>> ASSERT(!atomic_read(&d->shr_pages));
> >>>>
> >>>> would make this easier to follow? Another option might be to move
> >>>> the assertion to paging_final_teardown(), ahead of that specific call
> >>>> to p2m_teardown().
> >>>
> >>> AFAICT d->shr_pages would still be more then 0 when this is called
> > before
> >>> sharing is torn down so the rearrangement is necessary even if we do
> > this
> >>> assert only on the final invocation. I did a printk in place of this
> > assert
> >>> without the rearrangement and afaict it was always != 0.
> >>
> >> Was your printk() in an if() as above? paging_final_teardown() runs
really
> >> late during cleanup (when we're about to free struct domain), so I
would
> >> be somewhat concerned if by that time the count was still non-zero.
> >
> > Just replaced the assert with a printk. Without calling relinquish
shared
> > pages I don't find it odd that the count is non-zero, it only gets
> > decremented when you do call relinquish. Once the order is corrected it
is
> > zero.
>
> I may be getting you wrong, but it feels like you're still talking about
> early invocations of p2m_teardown() (from underneath domain_kill()) when
> I'm talking about the final one. No matter what ordering inside
> domain_relinquish_resources() (called from domain_kill()), the freeing
> will have happened by the time that process completes. Which is before
> the (typically last) last domain ref would be put (near the end of
> domain_kill()), and hence before the domain freeing process might be
> invoked (which is where paging_final_teardown() gets involved; see
> {,arch_}domain_destroy()).
I don't recall seeing a count with 0 before I reordered things but the
output on the serial may also have been truncated due to it printing a ton
of lines very quickly, so it could be the last one was zero just didn't
make it to my screen.
Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 9:27 AM Jan Beulich wrote: > > On 16.02.2023 13:22, Tamas K Lengyel wrote: > > On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote: > >> > >> On 15.02.2023 17:29, Tamas K Lengyel wrote: > >>> On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: > >>>> Did you consider the alternative of adjusting the ASSERT() in question > >>>> instead? It could reasonably become > >>>> > >>>> #ifdef CONFIG_MEM_SHARING > >>>> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || > >>> !atomic_read(&d->shr_pages)); > >>>> #endif > >>>> > >>>> now, I think. That would be less intrusive a change (helpful for > >>>> backporting), but there may be other (so far unnamed) benefits of > > pulling > >>>> ahead the shared memory teardown. > >>> > >>> I have a hard time understanding this proposed ASSERT. > >> > >> It accounts for the various ways p2m_teardown() can (now) be called, > >> limiting the actual check for no remaining shared pages to the last > >> of all these invocations (on the host p2m with remove_root=true). > >> > >> Maybe > >> > >> /* Limit the check to the final invocation. */ > >> if ( p2m_is_hostp2m(p2m) && remove_root ) > >> ASSERT(!atomic_read(&d->shr_pages)); > >> > >> would make this easier to follow? Another option might be to move > >> the assertion to paging_final_teardown(), ahead of that specific call > >> to p2m_teardown(). > > > > AFAICT d->shr_pages would still be more then 0 when this is called before > > sharing is torn down so the rearrangement is necessary even if we do this > > assert only on the final invocation. I did a printk in place of this assert > > without the rearrangement and afaict it was always != 0. > > Was your printk() in an if() as above? paging_final_teardown() runs really > late during cleanup (when we're about to free struct domain), so I would > be somewhat concerned if by that time the count was still non-zero. Just replaced the assert with a printk. Without calling relinquish shared pages I don't find it odd that the count is non-zero, it only gets decremented when you do call relinquish. Once the order is corrected it is zero. Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Thu, Feb 16, 2023 at 3:31 AM Jan Beulich wrote: > > On 15.02.2023 17:29, Tamas K Lengyel wrote: > > On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote: > >> Did you consider the alternative of adjusting the ASSERT() in question > >> instead? It could reasonably become > >> > >> #ifdef CONFIG_MEM_SHARING > >> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root || > > !atomic_read(&d->shr_pages)); > >> #endif > >> > >> now, I think. That would be less intrusive a change (helpful for > >> backporting), but there may be other (so far unnamed) benefits of pulling > >> ahead the shared memory teardown. > > > > I have a hard time understanding this proposed ASSERT. > > It accounts for the various ways p2m_teardown() can (now) be called, > limiting the actual check for no remaining shared pages to the last > of all these invocations (on the host p2m with remove_root=true). > > Maybe > > /* Limit the check to the final invocation. */ > if ( p2m_is_hostp2m(p2m) && remove_root ) > ASSERT(!atomic_read(&d->shr_pages)); > > would make this easier to follow? Another option might be to move > the assertion to paging_final_teardown(), ahead of that specific call > to p2m_teardown(). AFAICT d->shr_pages would still be more then 0 when this is called before sharing is torn down so the rearrangement is necessary even if we do this assert only on the final invocation. I did a printk in place of this assert without the rearrangement and afaict it was always != 0. Tamas
[PATCH v2] x86: Perform mem_sharing teardown before paging teardown
An assert failure has been observed in p2m_teardown when performing vm
forking and then destroying the forked VM (p2m-basic.c:173). The assert
checks whether the domain's shared pages counter is 0. According to the
patch that originally added the assert (7bedbbb5c31) the p2m_teardown
should only happen after mem_sharing already relinquished all shared pages.
In this patch we flip the order in which relinquish ops are called to avoid
tripping the assert.
Signed-off-by: Tamas K Lengyel
Fixes: e7aa55c0aab3 ("x86/p2m: free the paging memory pool preemptively")
---
v2: comsetic fixes
---
xen/arch/x86/domain.c | 56 ++-
1 file changed, 29 insertions(+), 27 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index db3ebf062d..a42f03e8e5 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2289,9 +2289,9 @@ int domain_relinquish_resources(struct domain *d)
enum {
PROG_iommu_pagetables = 1,
+PROG_shared,
PROG_paging,
PROG_vcpu_pagetables,
-PROG_shared,
PROG_xen,
PROG_l4,
PROG_l3,
@@ -2310,6 +2310,34 @@ int domain_relinquish_resources(struct domain *d)
if ( ret )
return ret;
+#ifdef CONFIG_MEM_SHARING
+PROGRESS(shared):
+
+if ( is_hvm_domain(d) )
+{
+/*
+ * If the domain has shared pages, relinquish them allowing
+ * for preemption.
+ */
+ret = relinquish_shared_pages(d);
+if ( ret )
+return ret;
+
+/*
+ * If the domain is forked, decrement the parent's pause count
+ * and release the domain.
+ */
+if ( mem_sharing_is_fork(d) )
+{
+struct domain *parent = d->parent;
+
+d->parent = NULL;
+domain_unpause(parent);
+put_domain(parent);
+}
+}
+#endif
+
PROGRESS(paging):
/* Tear down paging-assistance stuff. */
@@ -2350,32 +2378,6 @@ int domain_relinquish_resources(struct domain *d)
d->arch.auto_unmask = 0;
}
-#ifdef CONFIG_MEM_SHARING
-PROGRESS(shared):
-
-if ( is_hvm_domain(d) )
-{
-/* If the domain has shared pages, relinquish them allowing
- * for preemption. */
-ret = relinquish_shared_pages(d);
-if ( ret )
-return ret;
-
-/*
- * If the domain is forked, decrement the parent's pause count
- * and release the domain.
- */
-if ( mem_sharing_is_fork(d) )
-{
-struct domain *parent = d->parent;
-
-d->parent = NULL;
-domain_unpause(parent);
-put_domain(parent);
-}
-}
-#endif
-
spin_lock(&d->page_alloc_lock);
page_list_splice(&d->arch.relmem_list, &d->page_list);
INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
--
2.34.1
Re: [PATCH 2/2] x86/mem_sharing: Add extra variable to track fork progress
On Wed, Feb 15, 2023 at 6:03 AM Jan Beulich wrote: > > On 14.02.2023 06:07, Tamas K Lengyel wrote: > > When VM forking is initiated a VM is not supposed to try to perform mem_sharing > > yet as the fork process hasn't completed all required steps. However, the vCPU > > bring-up paths trigger guest memory accesses that can lead to such premature > > sharing ops. However, the gating check to see whether a VM is a fork is set > > already (ie. the domain has a parent). > > I find this confusing, and I had to read the patch to understand what's > meant. Since there are a total of three VMs involved here (the one > driving the fork, the one being forked, and the new clone), "a VM" is > insufficient. The sentence further reads as if that VM is performing > sharing operations on itself, which according to my understanding is > impossible. > > Furthermore "the vCPU bringup paths" is also not specific enough imo. > The forked VM as well as the new clone are both paused if I'm not > mistaken, so neither can be in the process of bringing up any vCPU-s. > I assume you refer to bring_up_vcpus(), but I'm afraid I view this > function as misnamed. vCPU-s are being _created_ there, not brought up. > Furthermore there are no guest memory accesses from underneath > bring_up_vcpus(), so with those accesses you may be referring to > copy_settings() instead? Which in turn - I'm sorry for my ignorance - > raises the question why (implicit) hypervisor side accesses to guest > memory might trigger sharing: So far I was working from the assumption > that it's only control tool requests which do. Otoh you talk of > "sharing ops", which suggests it may indeed be requests coming from > the control tool. Yet that's also what invoked the request to fork, > so shouldn't it coordinate with itself and avoid issuing sharing ops > prematurely? I went back to double-check and here is the memory access during vcpu_create: (XEN) Xen call trace: (XEN)[] R mem_sharing.c#mem_sharing_gfn_alloc+0x5c/0x5e (XEN)[] F mem_sharing.c#add_to_physmap+0x175/0x233 (XEN)[] F mem_sharing_fork_page+0x4ee/0x51e (XEN)[] F p2m_get_gfn_type_access+0x119/0x1a7 (XEN)[] F hap.c#hap_update_paging_modes+0xbe/0x2ee (XEN)[] F vmx_create_vmcs+0x981/0xb71 (XEN)[] F vmx.c#vmx_vcpu_initialise+0x64/0x1a0 (XEN)[] F hvm_vcpu_initialise+0x97/0x19e (XEN)[] F arch_vcpu_create+0xf3/0x1db (XEN)[] F vcpu_create+0x211/0x35d (XEN)[] F mem_sharing_memop+0x16a9/0x1869 (XEN)[] F subarch_memory_op+0x298/0x2c4 (XEN)[] F arch_memory_op+0xa9f/0xaa4 (XEN)[] F do_memory_op+0x2150/0x2297 (XEN)[] F pv_do_multicall_call+0x22c/0x4c7 (XEN)[] F arch_do_multicall_call+0x23/0x2c (XEN)[] F do_multicall+0xd3/0x417 (XEN)[] F pv_hypercall+0xea/0x3a0 (XEN)[] F lstar_enter+0x122/0x130 Any time any page is requested in a fork that's not present it gets populated from the parent (if the parent has it). What I was worried about is nominate_page being called on the fork but I was mistaken, it was called on the parent before add_to_physmap is called on the fork - which is fine and the expected behavior. We can drop this patch, sorry for the noise. Tamas
Re: [PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
On Wed, Feb 15, 2023 at 5:27 AM Jan Beulich wrote:
>
> On 14.02.2023 06:07, Tamas K Lengyel wrote:
> > An assert failure has been observed at p2m-basic.c:173 when performing
vm
>
> Please can you at least also name the function here? The line number is
> going to change, and when coming back to this change later, it'll be
> more troublesome to figure out which precise assertion was meant. Yes,
> ...
>
> > forking and then destroying the forked VM. The assert checks whether the
> > domain's shared pages counter is 0.
>
> ... you verbally describe it here, but still.
Sure.
>
> > According to the patch that originally
> > added the assert (7bedbbb5c31) the p2m_teardown should only happen after
> > mem_sharing already relinquished all shared pages.
> >
> > In this patch we flip the order in which relinquish ops are called to
avoid
> > tripping the assert.
> >
> > Signed-off-by: Tamas K Lengyel
> > ---
> > Note: it is unclear why this assert hasn't tripped in the past. It
hasn't
> > been observed with 4.17-rc1 but it is in RELEASE-4.17.0
>
> That's almost certainly a result of e7aa55c0aab3 ("x86/p2m: free the
paging
> memory pool preemptively"), which added calls to p2m_teardown() to
> hap_teardown(). If you agree, this wants recording in a Fixes: tag, as
> - being an XSA fix - that change has been backported to everywhere, and
> hence the issue now also exists everywhere.
Ough.. In that case we'll need this patch backported too. Will add the
Fixes: tag.
>
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -2310,6 +2310,32 @@ int domain_relinquish_resources(struct domain *d)
> > if ( ret )
> > return ret;
> >
> > +#ifdef CONFIG_MEM_SHARING
> > +PROGRESS(shared):
>
> If we go with the re-ordering as you suggest, then please also move the
> enumerator near the top of the switch() body.
Sure.
>
> Did you consider the alternative of adjusting the ASSERT() in question
> instead? It could reasonably become
>
> #ifdef CONFIG_MEM_SHARING
> ASSERT(!p2m_is_hostp2m(p2m) || !remove_root ||
!atomic_read(&d->shr_pages));
> #endif
>
> now, I think. That would be less intrusive a change (helpful for
> backporting), but there may be other (so far unnamed) benefits of pulling
> ahead the shared memory teardown.
I have a hard time understanding this proposed ASSERT.
>
> > +if ( is_hvm_domain(d) )
> > +{
> > +/* If the domain has shared pages, relinquish them allowing
> > + * for preemption. */
>
> Similarly, if the code is to be moved, please correct style here at this
> occasion.
Sure.
>
> > +ret = relinquish_shared_pages(d);
> > +if ( ret )
> > +return ret;
>
> While I can easily agree with the movement ahead of this being okay, ...
>
> > +/*
> > + * If the domain is forked, decrement the parent's pause
count
> > + * and release the domain.
> > + */
> > +if ( mem_sharing_is_fork(d) )
> > +{
> > +struct domain *parent = d->parent;
> > +
> > +d->parent = NULL;
> > +domain_unpause(parent);
> > +put_domain(parent);
> > +}
>
> ... I can only trust you on being sure that moving ahead of this is
> okay, too.
That's fine, we are in the teardown of the fork so it doesn't matter where
you are releasing the parent as long as its after the fork is unlinked from
it.
Thanks,
Tamas
[PATCH 2/2] x86/mem_sharing: Add extra variable to track fork progress
When VM forking is initiated a VM is not supposed to try to perform mem_sharing
yet as the fork process hasn't completed all required steps. However, the vCPU
bring-up paths trigger guest memory accesses that can lead to such premature
sharing ops. However, the gating check to see whether a VM is a fork is set
already (ie. the domain has a parent).
In this patch we introduce a separate variable to store the information that
forking has been initiated so that the non-restartable part of the forking op
is not repeated and we avoid the premature sharing ops from triggering.
Signed-off-by: Tamas K Lengyel
---
xen/arch/x86/include/asm/hvm/domain.h | 8
xen/arch/x86/mm/mem_sharing.c | 10 +++---
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/include/asm/hvm/domain.h
b/xen/arch/x86/include/asm/hvm/domain.h
index 698455444e..76a08b55f9 100644
--- a/xen/arch/x86/include/asm/hvm/domain.h
+++ b/xen/arch/x86/include/asm/hvm/domain.h
@@ -33,6 +33,14 @@ struct mem_sharing_domain
{
bool enabled, block_interrupts;
+/*
+ * We need to avoid trying to nominate pages for forking until the
+ * fork operation is completely finished. As parts of fork creation
+ * is restartable we mark here if the process started to skip the
+ * non-restartable portion.
+ */
+bool fork_started;
+
/*
* When releasing shared gfn's in a preemptible manner, recall where
* to resume the search.
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index 649d93dc54..b55c5bccdd 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1888,11 +1888,12 @@ static int copy_settings(struct domain *cd, struct
domain *d)
static int fork(struct domain *cd, struct domain *d)
{
int rc = -EBUSY;
+struct mem_sharing_domain *msd = &cd->arch.hvm.mem_sharing;
if ( !cd->controller_pause_count )
return rc;
-if ( !cd->parent )
+if ( !msd->fork_started )
{
if ( !get_domain(d) )
{
@@ -1905,7 +1906,7 @@ static int fork(struct domain *cd, struct domain *d)
*cd->arch.cpuid = *d->arch.cpuid;
*cd->arch.msr = *d->arch.msr;
cd->vmtrace_size = d->vmtrace_size;
-cd->parent = d;
+msd->fork_started = 1;
}
/* This is preemptible so it's the first to get done */
@@ -1918,8 +1919,11 @@ static int fork(struct domain *cd, struct domain *d)
rc = copy_settings(cd, d);
done:
-if ( rc && rc != -ERESTART )
+if ( !rc )
+cd->parent = d;
+else if ( rc != -ERESTART )
{
+msd->fork_started = 0;
cd->parent = NULL;
domain_unpause(d);
put_domain(d);
--
2.34.1
[PATCH 1/2] x86: Perform mem_sharing teardown before paging teardown
An assert failure has been observed at p2m-basic.c:173 when performing vm
forking and then destroying the forked VM. The assert checks whether the
domain's shared pages counter is 0. According to the patch that originally
added the assert (7bedbbb5c31) the p2m_teardown should only happen after
mem_sharing already relinquished all shared pages.
In this patch we flip the order in which relinquish ops are called to avoid
tripping the assert.
Signed-off-by: Tamas K Lengyel
---
Note: it is unclear why this assert hasn't tripped in the past. It hasn't
been observed with 4.17-rc1 but it is in RELEASE-4.17.0
---
xen/arch/x86/domain.c | 52 +--
1 file changed, 26 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index db3ebf062d..453ec52b6a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2310,6 +2310,32 @@ int domain_relinquish_resources(struct domain *d)
if ( ret )
return ret;
+#ifdef CONFIG_MEM_SHARING
+PROGRESS(shared):
+
+if ( is_hvm_domain(d) )
+{
+/* If the domain has shared pages, relinquish them allowing
+ * for preemption. */
+ret = relinquish_shared_pages(d);
+if ( ret )
+return ret;
+
+/*
+ * If the domain is forked, decrement the parent's pause count
+ * and release the domain.
+ */
+if ( mem_sharing_is_fork(d) )
+{
+struct domain *parent = d->parent;
+
+d->parent = NULL;
+domain_unpause(parent);
+put_domain(parent);
+}
+}
+#endif
+
PROGRESS(paging):
/* Tear down paging-assistance stuff. */
@@ -2350,32 +2376,6 @@ int domain_relinquish_resources(struct domain *d)
d->arch.auto_unmask = 0;
}
-#ifdef CONFIG_MEM_SHARING
-PROGRESS(shared):
-
-if ( is_hvm_domain(d) )
-{
-/* If the domain has shared pages, relinquish them allowing
- * for preemption. */
-ret = relinquish_shared_pages(d);
-if ( ret )
-return ret;
-
-/*
- * If the domain is forked, decrement the parent's pause count
- * and release the domain.
- */
-if ( mem_sharing_is_fork(d) )
-{
-struct domain *parent = d->parent;
-
-d->parent = NULL;
-domain_unpause(parent);
-put_domain(parent);
-}
-}
-#endif
-
spin_lock(&d->page_alloc_lock);
page_list_splice(&d->arch.relmem_list, &d->page_list);
INIT_PAGE_LIST_HEAD(&d->arch.relmem_list);
--
2.34.1
Re: [PATCH v2 4/8] x86/mem-sharing: copy GADDR based shared guest areas
On Thu, Jan 26, 2023 at 11:48 AM Jan Beulich wrote:
>
> On 26.01.2023 16:41, Tamas K Lengyel wrote:
> > On Thu, Jan 26, 2023 at 3:14 AM Jan Beulich wrote:
> >>
> >> On 25.01.2023 16:34, Tamas K Lengyel wrote:
> >>> On Tue, Jan 24, 2023 at 6:19 AM Jan Beulich wrote:
> >>>>
> >>>> On 23.01.2023 19:32, Tamas K Lengyel wrote:
> >>>>> On Mon, Jan 23, 2023 at 11:24 AM Jan Beulich
> > wrote:
> >>>>>> On 23.01.2023 17:09, Tamas K Lengyel wrote:
> >>>>>>> On Mon, Jan 23, 2023 at 9:55 AM Jan Beulich
> > wrote:
> >>>>>>>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>>>>>>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>>>>>>> @@ -1653,6 +1653,65 @@ static void copy_vcpu_nonreg_state(struc
> >>>>>>>> hvm_set_nonreg_state(cd_vcpu, &nrs);
> >>>>>>>> }
> >>>>>>>>
> >>>>>>>> +static int copy_guest_area(struct guest_area *cd_area,
> >>>>>>>> + const struct guest_area *d_area,
> >>>>>>>> + struct vcpu *cd_vcpu,
> >>>>>>>> + const struct domain *d)
> >>>>>>>> +{
> >>>>>>>> +mfn_t d_mfn, cd_mfn;
> >>>>>>>> +
> >>>>>>>> +if ( !d_area->pg )
> >>>>>>>> +return 0;
> >>>>>>>> +
> >>>>>>>> +d_mfn = page_to_mfn(d_area->pg);
> >>>>>>>> +
> >>>>>>>> +/* Allocate & map a page for the area if it hasn't been
> > already.
> >>>>> */
> >>>>>>>> +if ( !cd_area->pg )
> >>>>>>>> +{
> >>>>>>>> +gfn_t gfn = mfn_to_gfn(d, d_mfn);
> >>>>>>>> +struct p2m_domain *p2m =
p2m_get_hostp2m(cd_vcpu->domain);
> >>>>>>>> +p2m_type_t p2mt;
> >>>>>>>> +p2m_access_t p2ma;
> >>>>>>>> +unsigned int offset;
> >>>>>>>> +int ret;
> >>>>>>>> +
> >>>>>>>> +cd_mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL,
> >>>>> NULL);
> >>>>>>>> +if ( mfn_eq(cd_mfn, INVALID_MFN) )
> >>>>>>>> +{
> >>>>>>>> +struct page_info *pg =
> >>> alloc_domheap_page(cd_vcpu->domain,
> >>>>>>> 0);
> >>>>>>>> +
> >>>>>>>> +if ( !pg )
> >>>>>>>> +return -ENOMEM;
> >>>>>>>> +
> >>>>>>>> +cd_mfn = page_to_mfn(pg);
> >>>>>>>> +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> >>>>>>>> +
> >>>>>>>> +ret = p2m->set_entry(p2m, gfn, cd_mfn,
PAGE_ORDER_4K,
> >>>>>>> p2m_ram_rw,
> >>>>>>>> + p2m->default_access, -1);
> >>>>>>>> +if ( ret )
> >>>>>>>> +return ret;
> >>>>>>>> +}
> >>>>>>>> +else if ( p2mt != p2m_ram_rw )
> >>>>>>>> +return -EBUSY;
> >>>>>>>> +
> >>>>>>>> +/*
> >>>>>>>> + * Simply specify the entire range up to the end of the
> >>> page.
> >>>>>>> All the
> >>>>>>>> + * function uses it for is a check for not crossing page
> >>>>>>> boundaries.
> >>>>>>>> + */
> >>>>>>>> +offset = PAGE_OFFSET(d_area->map);
> >>>>>>>> +ret = map_guest_area(cd_vcpu, gfn_to_gaddr(gfn) +
offset,
> >>>>>>>> + PAGE_SIZE - offset, cd_area, NULL);
> >>>>>>>> +if ( ret )
> >>>>>>>> +return ret;
> >>>>>>>> +}
> &
Re: [PATCH v2 4/8] x86/mem-sharing: copy GADDR based shared guest areas
On Thu, Jan 26, 2023 at 3:14 AM Jan Beulich wrote:
>
> On 25.01.2023 16:34, Tamas K Lengyel wrote:
> > On Tue, Jan 24, 2023 at 6:19 AM Jan Beulich wrote:
> >>
> >> On 23.01.2023 19:32, Tamas K Lengyel wrote:
> >>> On Mon, Jan 23, 2023 at 11:24 AM Jan Beulich
wrote:
> >>>> On 23.01.2023 17:09, Tamas K Lengyel wrote:
> >>>>> On Mon, Jan 23, 2023 at 9:55 AM Jan Beulich
wrote:
> >>>>>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>>>>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>>>>> @@ -1653,6 +1653,65 @@ static void copy_vcpu_nonreg_state(struc
> >>>>>> hvm_set_nonreg_state(cd_vcpu, &nrs);
> >>>>>> }
> >>>>>>
> >>>>>> +static int copy_guest_area(struct guest_area *cd_area,
> >>>>>> + const struct guest_area *d_area,
> >>>>>> + struct vcpu *cd_vcpu,
> >>>>>> + const struct domain *d)
> >>>>>> +{
> >>>>>> +mfn_t d_mfn, cd_mfn;
> >>>>>> +
> >>>>>> +if ( !d_area->pg )
> >>>>>> +return 0;
> >>>>>> +
> >>>>>> +d_mfn = page_to_mfn(d_area->pg);
> >>>>>> +
> >>>>>> +/* Allocate & map a page for the area if it hasn't been
already.
> >>> */
> >>>>>> +if ( !cd_area->pg )
> >>>>>> +{
> >>>>>> +gfn_t gfn = mfn_to_gfn(d, d_mfn);
> >>>>>> +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
> >>>>>> +p2m_type_t p2mt;
> >>>>>> +p2m_access_t p2ma;
> >>>>>> +unsigned int offset;
> >>>>>> +int ret;
> >>>>>> +
> >>>>>> +cd_mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL,
> >>> NULL);
> >>>>>> +if ( mfn_eq(cd_mfn, INVALID_MFN) )
> >>>>>> +{
> >>>>>> +struct page_info *pg =
> > alloc_domheap_page(cd_vcpu->domain,
> >>>>> 0);
> >>>>>> +
> >>>>>> +if ( !pg )
> >>>>>> +return -ENOMEM;
> >>>>>> +
> >>>>>> +cd_mfn = page_to_mfn(pg);
> >>>>>> +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> >>>>>> +
> >>>>>> +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K,
> >>>>> p2m_ram_rw,
> >>>>>> + p2m->default_access, -1);
> >>>>>> +if ( ret )
> >>>>>> +return ret;
> >>>>>> +}
> >>>>>> +else if ( p2mt != p2m_ram_rw )
> >>>>>> +return -EBUSY;
> >>>>>> +
> >>>>>> +/*
> >>>>>> + * Simply specify the entire range up to the end of the
> > page.
> >>>>> All the
> >>>>>> + * function uses it for is a check for not crossing page
> >>>>> boundaries.
> >>>>>> + */
> >>>>>> +offset = PAGE_OFFSET(d_area->map);
> >>>>>> +ret = map_guest_area(cd_vcpu, gfn_to_gaddr(gfn) + offset,
> >>>>>> + PAGE_SIZE - offset, cd_area, NULL);
> >>>>>> +if ( ret )
> >>>>>> +return ret;
> >>>>>> +}
> >>>>>> +else
> >>>>>> +cd_mfn = page_to_mfn(cd_area->pg);
> >>>>>
> >>>>> Everything to this point seems to be non mem-sharing/forking
related.
> >>> Could
> >>>>> these live somewhere else? There must be some other place where
> >>> allocating
> >>>>> these areas happens already for non-fork VMs so it would make sense
to
> >>> just
> >>>>> refactor that code to be callable from here.
> >>>>
> >>>> It is the "copy" aspect with makes this mem-sharing (or really fork)
> >>>> specific. Plus in the end this is no different from what you have
> >>>> there right now for copying the vCPU info area. In the final patch
> >>>> that other code gets removed by re-using the code here.
> >>>
> >>> Yes, the copy part is fork-specific. Arguably if there was a way to do
> > the
> >>> allocation of the page for vcpu_info I would prefer that being
> > elsewhere,
> >>> but while the only requirement is allocate-page and copy from parent
> > I'm OK
> >>> with that logic being in here because it's really straight forward.
But
> > now
> >>> you also do extra sanity checks here which are harder to comprehend in
> > this
> >>> context alone.
> >>
> >> What sanity checks are you talking about (also below, where you claim
> >> map_guest_area() would be used only to sanity check)?
> >
> > Did I misread your comment above "All the function uses it for is a
check
> > for not crossing page boundaries"? That sounds to me like a simple
sanity
> > check, unclear why it matters though and why only for forks.
>
> The comment is about the function's use of the range it is being passed.
> It doesn't say in any way that the function is doing only sanity checking.
> If the comment wording is ambiguous or unclear, I'm happy to take
> improvement suggestions.
Yes, please do, it definitely was confusing while reviewing the patch.
Thanks,
Tamas
Re: [PATCH v2 4/8] x86/mem-sharing: copy GADDR based shared guest areas
On Tue, Jan 24, 2023 at 6:19 AM Jan Beulich wrote:
>
> On 23.01.2023 19:32, Tamas K Lengyel wrote:
> > On Mon, Jan 23, 2023 at 11:24 AM Jan Beulich wrote:
> >> On 23.01.2023 17:09, Tamas K Lengyel wrote:
> >>> On Mon, Jan 23, 2023 at 9:55 AM Jan Beulich wrote:
> >>>> --- a/xen/arch/x86/mm/mem_sharing.c
> >>>> +++ b/xen/arch/x86/mm/mem_sharing.c
> >>>> @@ -1653,6 +1653,65 @@ static void copy_vcpu_nonreg_state(struc
> >>>> hvm_set_nonreg_state(cd_vcpu, &nrs);
> >>>> }
> >>>>
> >>>> +static int copy_guest_area(struct guest_area *cd_area,
> >>>> + const struct guest_area *d_area,
> >>>> + struct vcpu *cd_vcpu,
> >>>> + const struct domain *d)
> >>>> +{
> >>>> +mfn_t d_mfn, cd_mfn;
> >>>> +
> >>>> +if ( !d_area->pg )
> >>>> +return 0;
> >>>> +
> >>>> +d_mfn = page_to_mfn(d_area->pg);
> >>>> +
> >>>> +/* Allocate & map a page for the area if it hasn't been already.
> > */
> >>>> +if ( !cd_area->pg )
> >>>> +{
> >>>> +gfn_t gfn = mfn_to_gfn(d, d_mfn);
> >>>> +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
> >>>> +p2m_type_t p2mt;
> >>>> +p2m_access_t p2ma;
> >>>> +unsigned int offset;
> >>>> +int ret;
> >>>> +
> >>>> +cd_mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL,
> > NULL);
> >>>> +if ( mfn_eq(cd_mfn, INVALID_MFN) )
> >>>> +{
> >>>> +struct page_info *pg =
alloc_domheap_page(cd_vcpu->domain,
> >>> 0);
> >>>> +
> >>>> +if ( !pg )
> >>>> +return -ENOMEM;
> >>>> +
> >>>> +cd_mfn = page_to_mfn(pg);
> >>>> +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> >>>> +
> >>>> +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K,
> >>> p2m_ram_rw,
> >>>> + p2m->default_access, -1);
> >>>> +if ( ret )
> >>>> +return ret;
> >>>> +}
> >>>> +else if ( p2mt != p2m_ram_rw )
> >>>> +return -EBUSY;
> >>>> +
> >>>> +/*
> >>>> + * Simply specify the entire range up to the end of the
page.
> >>> All the
> >>>> + * function uses it for is a check for not crossing page
> >>> boundaries.
> >>>> + */
> >>>> +offset = PAGE_OFFSET(d_area->map);
> >>>> +ret = map_guest_area(cd_vcpu, gfn_to_gaddr(gfn) + offset,
> >>>> + PAGE_SIZE - offset, cd_area, NULL);
> >>>> +if ( ret )
> >>>> +return ret;
> >>>> +}
> >>>> +else
> >>>> +cd_mfn = page_to_mfn(cd_area->pg);
> >>>
> >>> Everything to this point seems to be non mem-sharing/forking related.
> > Could
> >>> these live somewhere else? There must be some other place where
> > allocating
> >>> these areas happens already for non-fork VMs so it would make sense to
> > just
> >>> refactor that code to be callable from here.
> >>
> >> It is the "copy" aspect with makes this mem-sharing (or really fork)
> >> specific. Plus in the end this is no different from what you have
> >> there right now for copying the vCPU info area. In the final patch
> >> that other code gets removed by re-using the code here.
> >
> > Yes, the copy part is fork-specific. Arguably if there was a way to do
the
> > allocation of the page for vcpu_info I would prefer that being
elsewhere,
> > but while the only requirement is allocate-page and copy from parent
I'm OK
> > with that logic being in here because it's really straight forward. But
now
> > you also do extra sanity checks here which are harder to comprehend in
this
> > context alone.
>
> What sanity checks are you talking about (also below, where you claim
> map_guest_area() would be used o
Re: [PATCH v2 4/8] x86/mem-sharing: copy GADDR based shared guest areas
On Mon, Jan 23, 2023 at 11:24 AM Jan Beulich wrote:
>
> On 23.01.2023 17:09, Tamas K Lengyel wrote:
> > On Mon, Jan 23, 2023 at 9:55 AM Jan Beulich wrote:
> >> --- a/xen/arch/x86/mm/mem_sharing.c
> >> +++ b/xen/arch/x86/mm/mem_sharing.c
> >> @@ -1653,6 +1653,65 @@ static void copy_vcpu_nonreg_state(struc
> >> hvm_set_nonreg_state(cd_vcpu, &nrs);
> >> }
> >>
> >> +static int copy_guest_area(struct guest_area *cd_area,
> >> + const struct guest_area *d_area,
> >> + struct vcpu *cd_vcpu,
> >> + const struct domain *d)
> >> +{
> >> +mfn_t d_mfn, cd_mfn;
> >> +
> >> +if ( !d_area->pg )
> >> +return 0;
> >> +
> >> +d_mfn = page_to_mfn(d_area->pg);
> >> +
> >> +/* Allocate & map a page for the area if it hasn't been already.
*/
> >> +if ( !cd_area->pg )
> >> +{
> >> +gfn_t gfn = mfn_to_gfn(d, d_mfn);
> >> +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
> >> +p2m_type_t p2mt;
> >> +p2m_access_t p2ma;
> >> +unsigned int offset;
> >> +int ret;
> >> +
> >> +cd_mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL,
NULL);
> >> +if ( mfn_eq(cd_mfn, INVALID_MFN) )
> >> +{
> >> +struct page_info *pg = alloc_domheap_page(cd_vcpu->domain,
> > 0);
> >> +
> >> +if ( !pg )
> >> +return -ENOMEM;
> >> +
> >> +cd_mfn = page_to_mfn(pg);
> >> +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> >> +
> >> +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K,
> > p2m_ram_rw,
> >> + p2m->default_access, -1);
> >> +if ( ret )
> >> +return ret;
> >> +}
> >> +else if ( p2mt != p2m_ram_rw )
> >> +return -EBUSY;
> >> +
> >> +/*
> >> + * Simply specify the entire range up to the end of the page.
> > All the
> >> + * function uses it for is a check for not crossing page
> > boundaries.
> >> + */
> >> +offset = PAGE_OFFSET(d_area->map);
> >> +ret = map_guest_area(cd_vcpu, gfn_to_gaddr(gfn) + offset,
> >> + PAGE_SIZE - offset, cd_area, NULL);
> >> +if ( ret )
> >> +return ret;
> >> +}
> >> +else
> >> +cd_mfn = page_to_mfn(cd_area->pg);
> >
> > Everything to this point seems to be non mem-sharing/forking related.
Could
> > these live somewhere else? There must be some other place where
allocating
> > these areas happens already for non-fork VMs so it would make sense to
just
> > refactor that code to be callable from here.
>
> It is the "copy" aspect with makes this mem-sharing (or really fork)
> specific. Plus in the end this is no different from what you have
> there right now for copying the vCPU info area. In the final patch
> that other code gets removed by re-using the code here.
Yes, the copy part is fork-specific. Arguably if there was a way to do the
allocation of the page for vcpu_info I would prefer that being elsewhere,
but while the only requirement is allocate-page and copy from parent I'm OK
with that logic being in here because it's really straight forward. But now
you also do extra sanity checks here which are harder to comprehend in this
context alone. What if extra sanity checks will be needed in the future? Or
the sanity checks in the future diverge from where this happens for normal
VMs because someone overlooks this needing to be synched here too?
> I also haven't been able to spot anything that could be factored
> out (and one might expect that if there was something, then the vCPU
> info area copying should also already have used it). map_guest_area()
> is all that is used for other purposes as well.
Well, there must be a location where all this happens for normal VMs as
well, no? Why not factor that code so that it can be called from here, so
that we don't have to track sanity check requirements in two different
locations? Or for normal VMs that sanity checking bit isn't required? If
so, why?
> >> +
> >> +copy_domain_page(cd_mfn, d_mfn);
> >> +
> >> +return 0;
> >> +}
> >&
Re: [PATCH v2 4/8] x86/mem-sharing: copy GADDR based shared guest areas
On Mon, Jan 23, 2023 at 9:55 AM Jan Beulich wrote:
>
> In preparation of the introduction of new vCPU operations allowing to
> register the respective areas (one of the two is x86-specific) by
> guest-physical address, add the necessary fork handling (with the
> backing function yet to be filled in).
>
> Signed-off-by: Jan Beulich
>
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1653,6 +1653,65 @@ static void copy_vcpu_nonreg_state(struc
> hvm_set_nonreg_state(cd_vcpu, &nrs);
> }
>
> +static int copy_guest_area(struct guest_area *cd_area,
> + const struct guest_area *d_area,
> + struct vcpu *cd_vcpu,
> + const struct domain *d)
> +{
> +mfn_t d_mfn, cd_mfn;
> +
> +if ( !d_area->pg )
> +return 0;
> +
> +d_mfn = page_to_mfn(d_area->pg);
> +
> +/* Allocate & map a page for the area if it hasn't been already. */
> +if ( !cd_area->pg )
> +{
> +gfn_t gfn = mfn_to_gfn(d, d_mfn);
> +struct p2m_domain *p2m = p2m_get_hostp2m(cd_vcpu->domain);
> +p2m_type_t p2mt;
> +p2m_access_t p2ma;
> +unsigned int offset;
> +int ret;
> +
> +cd_mfn = p2m->get_entry(p2m, gfn, &p2mt, &p2ma, 0, NULL, NULL);
> +if ( mfn_eq(cd_mfn, INVALID_MFN) )
> +{
> +struct page_info *pg = alloc_domheap_page(cd_vcpu->domain,
0);
> +
> +if ( !pg )
> +return -ENOMEM;
> +
> +cd_mfn = page_to_mfn(pg);
> +set_gpfn_from_mfn(mfn_x(cd_mfn), gfn_x(gfn));
> +
> +ret = p2m->set_entry(p2m, gfn, cd_mfn, PAGE_ORDER_4K,
p2m_ram_rw,
> + p2m->default_access, -1);
> +if ( ret )
> +return ret;
> +}
> +else if ( p2mt != p2m_ram_rw )
> +return -EBUSY;
> +
> +/*
> + * Simply specify the entire range up to the end of the page.
All the
> + * function uses it for is a check for not crossing page
boundaries.
> + */
> +offset = PAGE_OFFSET(d_area->map);
> +ret = map_guest_area(cd_vcpu, gfn_to_gaddr(gfn) + offset,
> + PAGE_SIZE - offset, cd_area, NULL);
> +if ( ret )
> +return ret;
> +}
> +else
> +cd_mfn = page_to_mfn(cd_area->pg);
Everything to this point seems to be non mem-sharing/forking related. Could
these live somewhere else? There must be some other place where allocating
these areas happens already for non-fork VMs so it would make sense to just
refactor that code to be callable from here.
> +
> +copy_domain_page(cd_mfn, d_mfn);
> +
> +return 0;
> +}
> +
> static int copy_vpmu(struct vcpu *d_vcpu, struct vcpu *cd_vcpu)
> {
> struct vpmu_struct *d_vpmu = vcpu_vpmu(d_vcpu);
> @@ -1745,6 +1804,16 @@ static int copy_vcpu_settings(struct dom
> copy_domain_page(new_vcpu_info_mfn, vcpu_info_mfn);
> }
>
> +/* Same for the (physically registered) runstate and time info
areas. */
> +ret = copy_guest_area(&cd_vcpu->runstate_guest_area,
> + &d_vcpu->runstate_guest_area, cd_vcpu, d);
> +if ( ret )
> +return ret;
> +ret = copy_guest_area(&cd_vcpu->arch.time_guest_area,
> + &d_vcpu->arch.time_guest_area, cd_vcpu, d);
> +if ( ret )
> +return ret;
> +
> ret = copy_vpmu(d_vcpu, cd_vcpu);
> if ( ret )
> return ret;
> @@ -1987,7 +2056,10 @@ int mem_sharing_fork_reset(struct domain
>
> state:
> if ( reset_state )
> +{
> rc = copy_settings(d, pd);
> +/* TBD: What to do here with -ERESTART? */
Where does ERESTART coming from?
Re: [PATCH] xen/x86: Make XEN_DOMCTL_get_vcpu_msrs more configurable
On Wed, Oct 26, 2022 at 8:22 AM Jan Beulich wrote:
>
> On 26.10.2022 13:58, Tamas K Lengyel wrote:
> > On Wed, Oct 26, 2022, 7:48 AM Jan Beulich wrote:
> >
> >> On 26.10.2022 13:34, Tamas K Lengyel wrote:
> >>> On Wed, Oct 26, 2022, 7:06 AM Andrew Cooper
> >>> wrote:
> >>>
> >>>> On 24/10/2022 17:58, Tamas K Lengyel wrote:
> >>>>> Currently the XEN_DOMCTL_get_vcpu_msrs is only capable of gathering
a
> >>>> handful
> >>>>> of predetermined vcpu MSRs. In our use-case gathering the vPMU MSRs
by
> >> an
> >>>>> external privileged tool is necessary, thus we extend the domctl to
> >>>> allow for
> >>>>> querying for any guest MSRs. To remain compatible with the existing
> >>>> setup if
> >>>>> no specific MSR is requested via the domctl the default list is
> >> returned.
> >>>>>
> >>>>> Signed-off-by: Tamas K Lengyel
> >>>>
> >>>> Naming aside, XEN_DOMCTL_{get,set}_vcpu_msrs is supposed to be "get
me
> >>>> all MSRs needed to migrate a vCPU". (I do intend to retire the
> >>>> hypercall as part of fixing the Xen side of migration, but that's
ages
> >>>> away)
> >>>>
> >>>> It seems like what you want is something more like
> >>>> XEN_DOMCTL_{rd,wr}msr_list (convenient timing, given the recent ISE
> >>>> update). I think those would be better as a separate pair of
> >>>> hypercalls, rather than trying to repurpose an existing hypercall.
> >>>>
> >>>>
> >>>> As for actually getting the values, please fix up guest_{rd,wr}msr()
to
> >>>> access the perf MSRs safely. I know the vpmu MSR handling is in a
> >>>> tragic state, but this new get_msr subop is making the problem even
more
> >>>> tangled.
> >>>>
> >>>
> >>> Adding a separate hypercall is fine.
> >>
> >> Which will then also avoid altering the behavior of the existing
hypercall:
> >> You can't just assume e.g. input fields to be zeroed (or otherwise
> >> suitably set) by existing callers, when those were previously output
only.
> >>
> >
> > I did add a memset to zero it on the single caller I could find.
>
> Some may deem this sufficient on the assumption that all users should
> go through the libxenguest function. But then at the minimum you want
> to update documentation in the public header. Yet then this wasn't
> the only issue I spotted (hence the use of "e.g.") - you also alter
> documented behavior as to the returned number of MSRs when the input
> value was too small, if I'm not mistaken.
No, there shouldn't have been any alteration of the previous behavior other
than assuming the buffer sent by the toolstack is zero initialized when the
default list is requested. Either way, adding the separate hypercall is
fine by me.
Tamas
Re: [PATCH] xen/x86: Make XEN_DOMCTL_get_vcpu_msrs more configurable
On Wed, Oct 26, 2022, 7:48 AM Jan Beulich wrote:
> On 26.10.2022 13:34, Tamas K Lengyel wrote:
> > On Wed, Oct 26, 2022, 7:06 AM Andrew Cooper
> > wrote:
> >
> >> On 24/10/2022 17:58, Tamas K Lengyel wrote:
> >>> Currently the XEN_DOMCTL_get_vcpu_msrs is only capable of gathering a
> >> handful
> >>> of predetermined vcpu MSRs. In our use-case gathering the vPMU MSRs by
> an
> >>> external privileged tool is necessary, thus we extend the domctl to
> >> allow for
> >>> querying for any guest MSRs. To remain compatible with the existing
> >> setup if
> >>> no specific MSR is requested via the domctl the default list is
> returned.
> >>>
> >>> Signed-off-by: Tamas K Lengyel
> >>
> >> Naming aside, XEN_DOMCTL_{get,set}_vcpu_msrs is supposed to be "get me
> >> all MSRs needed to migrate a vCPU". (I do intend to retire the
> >> hypercall as part of fixing the Xen side of migration, but that's ages
> >> away)
> >>
> >> It seems like what you want is something more like
> >> XEN_DOMCTL_{rd,wr}msr_list (convenient timing, given the recent ISE
> >> update). I think those would be better as a separate pair of
> >> hypercalls, rather than trying to repurpose an existing hypercall.
> >>
> >>
> >> As for actually getting the values, please fix up guest_{rd,wr}msr() to
> >> access the perf MSRs safely. I know the vpmu MSR handling is in a
> >> tragic state, but this new get_msr subop is making the problem even more
> >> tangled.
> >>
> >
> > Adding a separate hypercall is fine.
>
> Which will then also avoid altering the behavior of the existing hypercall:
> You can't just assume e.g. input fields to be zeroed (or otherwise
> suitably set) by existing callers, when those were previously output only.
>
I did add a memset to zero it on the single caller I could find.
Tamas
>
Re: [PATCH] xen/x86: Make XEN_DOMCTL_get_vcpu_msrs more configurable
On Wed, Oct 26, 2022, 7:06 AM Andrew Cooper
wrote:
> On 24/10/2022 17:58, Tamas K Lengyel wrote:
> > Currently the XEN_DOMCTL_get_vcpu_msrs is only capable of gathering a
> handful
> > of predetermined vcpu MSRs. In our use-case gathering the vPMU MSRs by an
> > external privileged tool is necessary, thus we extend the domctl to
> allow for
> > querying for any guest MSRs. To remain compatible with the existing
> setup if
> > no specific MSR is requested via the domctl the default list is returned.
> >
> > Signed-off-by: Tamas K Lengyel
>
> Naming aside, XEN_DOMCTL_{get,set}_vcpu_msrs is supposed to be "get me
> all MSRs needed to migrate a vCPU". (I do intend to retire the
> hypercall as part of fixing the Xen side of migration, but that's ages
> away)
>
> It seems like what you want is something more like
> XEN_DOMCTL_{rd,wr}msr_list (convenient timing, given the recent ISE
> update). I think those would be better as a separate pair of
> hypercalls, rather than trying to repurpose an existing hypercall.
>
>
> As for actually getting the values, please fix up guest_{rd,wr}msr() to
> access the perf MSRs safely. I know the vpmu MSR handling is in a
> tragic state, but this new get_msr subop is making the problem even more
> tangled.
>
Adding a separate hypercall is fine. Unfortunately wiring it into
guest_rdmsr failed on the first attempt when I tried. This is because the
guest itself will hit that path when it reads its own vpmu msrs. The
guest_rdmsr actually fails in that path and a separate fall-back path is
where the vpmu do_rdmsr is called. Now if I wire in the vpmu msrs into
guest_rdmsr I short circuit the existing setup and it looked like a can of
worms. I would have to figure out who is trying to get the vpmu msrs and do
things differently based on that, and the only info we have is if v ==
current. That just looked fragile to me.
Tamas
>
Re: [PATCH] xen/x86: Make XEN_DOMCTL_get_vcpu_msrs more configurable
On Tue, Oct 25, 2022 at 4:13 AM Roger Pau Monné wrote: > > On Mon, Oct 24, 2022 at 12:58:54PM -0400, Tamas K Lengyel wrote: > > Currently the XEN_DOMCTL_get_vcpu_msrs is only capable of gathering a handful > > of predetermined vcpu MSRs. In our use-case gathering the vPMU MSRs by an > > external privileged tool is necessary, thus we extend the domctl to allow for > > querying for any guest MSRs. To remain compatible with the existing setup if > > no specific MSR is requested via the domctl the default list is returned. > > I'm afraid I would benefit from some extra description about why you > need to introduce a separate hook instead of using the existing > do_rdmsr hook in arch_vpmu_ops (which is already hooked into > guest_rdmsr()). > > Are the MSRs you are trying to fetch not accessible for the guest > itself to read? No, the reason we need this different hook is because do_rdmsr assumes the guest is reading the MSRs that are currently loaded. For external tools where v != current the vpmu context needs to be saved by pausing the vcpu first and then the MSR content returned from the saved context. Tamas
Re: [PATCH 06/10] x86/mem-sharing: copy GADDR based shared guest areas
> @@ -1987,7 +2056,10 @@ int mem_sharing_fork_reset(struct domain
>
> state:
> if ( reset_state )
> +{
> rc = copy_settings(d, pd);
> +/* TBD: What to do here with -ERESTART? */
Generally speaking the fork reset operation does not support "restarting".
While in the memory op path the error can be propagated back to the
toolstack and have it re-issue it, on the monitor reply path that's not
possible. But the important question is where does the -ERESTART come
from? What I think would happen here though is that -ERESTART may happen
during the initial fork op and that can fail, but if it succeeded, then
during reset it can't happen since everything would be already allocated
and mapped, the only thing during reset that would be done is the page
copying.
Tamas
