Re: [PATCH v2] swiotlb-xen: provide the "max_mapping_size" method
Looks good: Reviewed-by: Christoph Hellwig
Re: [PATCH 20/29] tools: add 9pfs device to xenstore-stubdom
On 07.11.23 20:18, Jason Andryuk wrote:
On Wed, Nov 1, 2023 at 8:23 AM Juergen Gross wrote:
Add a 9pfs device to Xenstore stubdom in order to allow it to do e.g.
logging into a dom0 file.
Use the following parameters for the new device:
- tag = "xen"
- type = "xenlogd"
- path = "/var/lib/xen/xenstore"
For now don't limit allowed file space or number of files.
Add a new libxl function for adding it similar to the function for
adding the console device.
Signed-off-by: Juergen Gross
diff --git a/tools/libs/light/libxl_9pfs.c b/tools/libs/light/libxl_9pfs.c
index 0b9d84dce9..3297389493 100644
--- a/tools/libs/light/libxl_9pfs.c
+++ b/tools/libs/light/libxl_9pfs.c
@@ -174,6 +174,35 @@ static void libxl__device_p9_add(libxl__egc *egc, uint32_t
domid,
aodev->callback(egc, aodev);
}
+int libxl_p9_add_xenstore(libxl_ctx *ctx, uint32_t domid, uint32_t backend,
+ libxl_p9_type type, char *tag, char *path,
+ unsigned int max_space, unsigned int max_files,
+ unsigned int max_open_files, bool auto_delete,
+ const libxl_asyncop_how *ao_how)
+{
+AO_CREATE(ctx, domid, ao_how);
+libxl__ao_device *aodev;
+libxl_device_p9 p9 = { .backend_domid = backend,
+ .tag = tag,
+ .path = path,
+ .security_model = "none",
While the xl.cfg man page states that only security_model="none" is
supported, it is possible to use other ones.The value isn't
inspected and it is just passed through Xenstore to QEMU. QEMU can
then operate however it operates. I just tested mapped-xattr and it's
working from some quick testing. So maybe libxl_p9_add_xenstore()
should take security_model as an argument, and then
init-xenstore-domain can pass in "none"?
Yes, good idea.
Everything else looks good, so either way:
Reviewed-by: Jason Andryuk
Thanks,
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH 16/29] tools/xl: support new 9pfs backend xenlogd
On 07.11.23 17:55, Jason Andryuk wrote:
On Wed, Nov 1, 2023 at 6:41 AM Juergen Gross wrote:
Add support for the new 9pfs backend "xenlogd". For this backend type
the tag defaults to "Xen" and the host side path to
"/var/log/xen/guests/".
Signed-off-by: Juergen Gross
diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
index ed983200c3..346532e117 100644
--- a/tools/xl/xl_parse.c
+++ b/tools/xl/xl_parse.c
@@ -2242,6 +2256,27 @@ void parse_config_data(const char *config_source,
libxl_string_list_dispose();
+if (p9->type == LIBXL_P9_TYPE_UNKNOWN) {
+p9->type = LIBXL_P9_TYPE_QEMU;
+}
+if (p9->type == LIBXL_P9_TYPE_QEMU &&
+(p9->max_space || p9->auto_delete)) {
Also check p9->max_open_files and p9->max_files?
Ah, yes, I added those later and forgot to adapt this check.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
[ovmf test] 183711: all pass - PUSHED
flight 183711 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/183711/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf bb18fb80abb9d35d01be5d693086a9ed4b9d65b5 baseline version: ovmf c96b4da2a079eb837ab3af9aeb86a97078b3bde6 Last test of basis 183700 2023-11-07 03:42:39 Z1 days Testing same since 183711 2023-11-08 03:11:06 Z0 days1 attempts People who touched revisions under test: Michael D Kinney jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git c96b4da2a0..bb18fb80ab bb18fb80abb9d35d01be5d693086a9ed4b9d65b5 -> xen-tested-master
xen | Successful pipeline for staging | bede1c7e
Pipeline #1064344124 has passed! Project: xen ( https://gitlab.com/xen-project/xen ) Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging ) Commit: bede1c7e ( https://gitlab.com/xen-project/xen/-/commit/bede1c7e3b790b63f1ff3ea3ee4e476b012fdf2c ) Commit Message: exclude-list: generalise exclude-list Currentl... Commit Author: Luca Fancellu ( https://gitlab.com/luca.fancellu ) Committed by: Stefano Stabellini Pipeline #1064344124 ( https://gitlab.com/xen-project/xen/-/pipelines/1064344124 ) triggered by Ganis ( https://gitlab.com/ganis ) successfully completed 129 jobs in 3 stages. -- You're receiving this email because of your account on gitlab.com.
[xen-unstable test] 183706: tolerable trouble: fail/pass/starved - PUSHED
flight 183706 xen-unstable real [real] flight 183710 xen-unstable real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/183706/ http://logs.test-lab.xenproject.org/osstest/logs/183710/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install fail pass in 183710-retest Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stop fail in 183710 like 183703 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 183703 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183703 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 183703 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183703 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 183703 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183703 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183703 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183703 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183703 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183703 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183703 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-amd64-amd64-dom0pvh-xl-amd 3 hosts-allocate starved n/a version targeted for testing: xen fab51099a1cdb6bfe5127b14a5d41c246ea1a2c7 baseline version: xen
[xen-unstable-smoke test] 183709: tolerable all pass - PUSHED
flight 183709 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/183709/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen bede1c7e3b790b63f1ff3ea3ee4e476b012fdf2c baseline version: xen 78a86b26868c12ae1cc3dd2a8bb9aa5eebaa41fd Last test of basis 183707 2023-11-07 18:02:03 Z0 days Testing same since 183709 2023-11-07 21:00:26 Z0 days1 attempts People who touched revisions under test: Federico Serafini Jan Beulich Julien Grall Luca Fancellu Michal Orzel Simone Ballarin Stefano Stabellini Stefano Stabellini jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 78a86b2686..bede1c7e3b bede1c7e3b790b63f1ff3ea3ee4e476b012fdf2c -> smoke
xen | Successful pipeline for staging | 78a86b26
Pipeline #1064177796 has passed! Project: xen ( https://gitlab.com/xen-project/xen ) Branch: staging ( https://gitlab.com/xen-project/xen/-/commits/staging ) Commit: 78a86b26 ( https://gitlab.com/xen-project/xen/-/commit/78a86b26868c12ae1cc3dd2a8bb9aa5eebaa41fd ) Commit Message: x86/spec-ctrl: Add SRSO whitepaper URL ... now... Commit Author: Andrew Cooper ( https://gitlab.com/andyhhp ) Pipeline #1064177796 ( https://gitlab.com/xen-project/xen/-/pipelines/1064177796 ) triggered by Ganis ( https://gitlab.com/ganis ) successfully completed 129 jobs in 3 stages. -- You're receiving this email because of your account on gitlab.com.
[xen-4.18-testing test] 183704: tolerable trouble: fail/pass/starved - PUSHED
flight 183704 xen-4.18-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/183704/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183663 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183663 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 183663 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183663 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 183663 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183663 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183663 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 183663 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183663 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183663 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183663 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183663 test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-dom0pvh-xl-amd 3 hosts-allocate starved n/a version targeted for testing: xen 3e12149eb214fee62fc86ee964d3142d0235330e baseline version: xen 6f9285975b1c6e82889a6118503ca367e3aa78fd Last test of basis 183663 2023-11-02 21:40:29 Z5 days Testing same since 183704 2023-11-07 10:10:25 Z0 days1 attempts People who
Re: [XEN PATCH v3] xen/string: address violations of MISRA C:2012 Rules 8.2 and 8.3
On Tue, 7 Nov 2023, Federico Serafini wrote: > Add missing parameter names and make function declarations and > definitions consistent. > Mismatches between parameter names "count" and "n" are resolved > in favor of "n", being the same name used by the C standard. > > No functional change. > > Signed-off-by: Federico Serafini Reviewed-by: Stefano Stabellini
[xen-unstable-smoke test] 183707: tolerable all pass - PUSHED
flight 183707 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/183707/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 78a86b26868c12ae1cc3dd2a8bb9aa5eebaa41fd baseline version: xen fab51099a1cdb6bfe5127b14a5d41c246ea1a2c7 Last test of basis 183705 2023-11-07 11:00:26 Z0 days Testing same since 183707 2023-11-07 18:02:03 Z0 days1 attempts People who touched revisions under test: Andrew Cooper jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git fab51099a1..78a86b2686 78a86b26868c12ae1cc3dd2a8bb9aa5eebaa41fd -> smoke
Re: [XEN PATCH v3 2/3] docs: make the docs for MISRA C:2012 Dir 4.1 visible to ECLAIR
+Julien, Andrew Julien and Andrew raised concerns on this patch on the Xen Matrix channel. Please provide further details. On Mon, 2 Oct 2023, Stefano Stabellini wrote: > On Mon, 2 Oct 2023, Nicola Vetrini wrote: > > To be able to check for the existence of the necessary subsections in > > the documentation for MISRA C:2012 Dir 4.1, ECLAIR needs to have a source > > file that is built. > > > > This file is generated from 'C-runtime-failures.rst' in docs/misra > > and the configuration is updated accordingly. > > > > Signed-off-by: Nicola Vetrini > > Acked-by: Stefano Stabellini > > > > --- > > Changes from RFC: > > - Dropped unused/useless code > > - Revised the sed command > > - Revised the clean target > > > > Changes in v2: > > - Added explanative comment to the makefile > > - printf instead of echo > > > > Changes in v3: > > - Terminate the generated file with a newline > > - Build it with -std=c99, so that the documentation > > for D1.1 applies. > > --- > > docs/Makefile | 7 ++- > > docs/misra/Makefile | 22 ++ > > 2 files changed, 28 insertions(+), 1 deletion(-) > > create mode 100644 docs/misra/Makefile > > > > diff --git a/docs/Makefile b/docs/Makefile > > index 966a104490ac..ff991a0c3ca2 100644 > > --- a/docs/Makefile > > +++ b/docs/Makefile > > @@ -43,7 +43,7 @@ DOC_PDF := $(patsubst %.pandoc,pdf/%.pdf,$(PANDOCSRC-y)) > > \ > > all: build > > > > .PHONY: build > > -build: html txt pdf man-pages figs > > +build: html txt pdf man-pages figs misra > > > > .PHONY: sphinx-html > > sphinx-html: > > @@ -66,9 +66,14 @@ endif > > .PHONY: pdf > > pdf: $(DOC_PDF) > > > > +.PHONY: misra > > +misra: > > + $(MAKE) -C misra > > + > > .PHONY: clean > > clean: clean-man-pages > > $(MAKE) -C figs clean > > + $(MAKE) -C misra clean > > rm -rf .word_count *.aux *.dvi *.bbl *.blg *.glo *.idx *~ > > rm -rf *.ilg *.log *.ind *.toc *.bak *.tmp core > > rm -rf html txt pdf sphinx/html > > diff --git a/docs/misra/Makefile b/docs/misra/Makefile > > new file mode 100644 > > index ..949458ff9e15 > > --- /dev/null > > +++ b/docs/misra/Makefile > > @@ -0,0 +1,22 @@ > > +TARGETS := C-runtime-failures.o > > + > > +all: $(TARGETS) > > + > > +# This Makefile will generate the object files indicated in TARGETS by > > taking > > +# the corresponding .rst file, converting its content to a C block comment > > and > > +# then compiling the resulting .c file. This is needed for the file's > > content to > > +# be available when performing static analysis with ECLAIR on the project. > > + > > +# sed is used in place of cat to prevent occurrences of '*/' > > +# in the .rst from breaking the compilation > > +$(TARGETS:.o=.c): %.c: %.rst > > + printf "/*\n\n" > $@.tmp > > + sed -e 's|\*/|*//*|g' $< >> $@.tmp > > + printf "\n\n*/\n" >> $@.tmp > > + mv $@.tmp $@ > > + > > +%.o: %.c > > + $(CC) -std=c99 -c $< -o $@ > > + > > +clean: > > + rm -f C-runtime-failures.c *.o *.tmp > > -- > > 2.34.1 > > >
Re: [PATCH 29/29] tools/xenstored: have a single do_control_memreport()
On Wed, Nov 1, 2023 at 6:55 AM Juergen Gross wrote: > > With 9pfs now available in Xenstore-stubdom, there is no reason to > have distinct do_control_memreport() variants for the daemon and the > stubdom implementations. > > Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: [XEN PATCH 07/10] arm/traps: address a violation of MISRA C:2012 Rule 8.2
On Tue, 7 Nov 2023, Julien Grall wrote:
> Hi Stefano,
>
> On 16/10/2023 10:02, Julien Grall wrote:
> > Hi,
> >
> > On 13/10/2023 16:24, Federico Serafini wrote:
> > > Add missing parameter name, no functional change.
> > >
> > > Signed-off-by: Federico Serafini
> > > ---
> > > xen/arch/arm/traps.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> > > index ce89f16404..5aa14d4707 100644
> > > --- a/xen/arch/arm/traps.c
> > > +++ b/xen/arch/arm/traps.c
> > > @@ -1236,7 +1236,7 @@ int do_bug_frame(const struct cpu_user_regs *regs,
> > > vaddr_t pc)
> > > if ( id == BUGFRAME_run_fn )
> > > {
> > > - void (*fn)(const struct cpu_user_regs *) = (void
> > > *)regs->BUG_FN_REG;
> > > + void (*fn)(const struct cpu_user_regs *regs) = (void
> > > *)regs->BUG_FN_REG;
> >
> > Now the line will be over 80 characters. I think we should introduce a
> > typedef. This would also help in the longer run to validate that the
> > function passed to run_in_exception_handle() has the expected prototype.
>
> I see this patch was committed in your for-4.19 branch. But this comment was
> unaddressed. Can you drop the patch because your branch is committed in
> staging?
I dropped the patch. Federico, please address Julien's feedback.
Re: [PATCH 28/29] tools/xenstored: support complete log capabilities in stubdom
On Wed, Nov 1, 2023 at 6:49 AM Juergen Gross wrote: > > With 9pfs being fully available in Xenstore-stubdom now, there is no > reason to not fully support all logging capabilities in stubdom. > > Open the logfile on stubdom only after the 9pfs file system has been > mounted. > > Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: [PATCH 27/29] tools/xenstored: add daemon_init() function
On Wed, Nov 1, 2023 at 5:50 AM Juergen Gross wrote: > > Some xenstored initialization needs to be done in the daemon case only, > so split it out into a new daemon_init() function being a stub in the > stubdom case. > > Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: [PATCH 26/29] tools/xenstored: add helpers for filename handling
On Wed, Nov 1, 2023 at 7:29 AM Juergen Gross wrote: > > Add some helpers for handling filenames which might need different > implementations between stubdom and daemon environments: > > - expansion of relative filenames (those are not really defined today, > just expand them to be relative to /var/lib/xen/xenstore) > - expansion of xenstore_daemon_rundir() (used e.g. for saving the state > file in case of live update - needs to be unchanged in the daemon > case, but should result in /var/lib/xen/xenstore for stubdom) > > Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: [PATCH 25/29] tools/xenstored: mount 9pfs device in stubdom
On Wed, Nov 1, 2023 at 7:48 AM Juergen Gross wrote: > > Mount the 9pfs device in stubdom enabling it to use files. > > This has to happen in a worker thread in order to allow the main thread > handling the required Xenstore accesses in parallel. > > Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: Support situation for nestedhvm
On 07/11/2023 7:53 pm, Elliott Mitchell wrote: > I ran into the nestedhvm via the following path. I was considering the > feasibility of shedding tasks from a desktop onto a server running Xen. > I was looking at `man xl.cfg` and noticed "nestedhvm". > > Since one of the tasks the computer handled was running other OSes in > fully simulated environments, this seemed to be something I was looking > for. No where did I ever see anything hinting "This configuration option > is completely unsupported and risky to use". This one is explicitly covered in SUPPORT.md, and has had XSAs out against it in the past for being unexpectedly active when it oughtn't to have been. > Things simply started exploding without any warnings. Things also explode if you try to create a VM with 10x more RAM than you have, or if you try `./xenwatchdogd --help`, or `xl debug-keys c`, or many other things. The xl manpage probably ought to state explicitly that the option is experimental, but that the extent of what I'd consider reasonable here. You can't solve educational matters with technical measures. ~Andrew
Re: [PATCH v5 9/9] xen/arm: Map ITS doorbell register to IOMMU page tables.
On 10/20/23 14:02, Julien Grall wrote:
> Hi Stewart,
>
> On 04/10/2023 15:55, Stewart Hildebrand wrote:
>> From: Rahul Singh
>
> This wants an explanation why this is needed.
I'll add an explanation
>
>> Signed-off-by: Rahul Singh
>
> Your signed-off-by is missing.
I'll add it
>
>> ---
>> v4->v5:
>> * new patch
>> ---
>> xen/arch/arm/vgic-v3-its.c | 12
>> 1 file changed, 12 insertions(+)
>>
>> diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
>> index 05429030b539..df8f045198a3 100644
>> --- a/xen/arch/arm/vgic-v3-its.c
>> +++ b/xen/arch/arm/vgic-v3-its.c
>> @@ -682,6 +682,18 @@ static int its_handle_mapd(struct virt_its *its,
>> uint64_t *cmdptr)
>> BIT(size, UL), valid);
>> if ( ret && valid )
>> return ret;
>> +
>> + if ( is_iommu_enabled(its->d) ) {
>
> Coding style.
I'll fix
>
>> + ret = map_mmio_regions(its->d,
>> gaddr_to_gfn(its->doorbell_address),
>> + PFN_UP(ITS_DOORBELL_OFFSET),
>> + maddr_to_mfn(its->doorbell_address));
>
>
> A couple of remarks. Firstly, we know the ITS doorbell at domain
> creation. So I think thish should be called from vgic_v3_its_init_virtual().
>
> Regardless that, any code related to device initialization belongs to
> gicv3_its_map_guest_device().
How about calling it from a map_hwdom_extra_mappings() hook?
For example see [1].
[1]
https://gitlab.com/xen-project/people/bmarquis/xen-arm-poc/-/commit/6232a0d53377009bb7fbc3c3ab81d0153734be6b
>
> Lastly, I know the IOMMU page-tables and CPU page-tables are currently
> shared. But strictly speaking, map_mmio_regions() is incorrect because
> the doorbell is only meant to be accessible by the device. So this
> should only be mapped in the IOMMU page-tables.
>
> In fact I vaguely recall that on some platforms you may get a lockup if
> the CPU attempts to write to the doorbell. So we may want to unshare
> page-tables in the future.
>
> For now, we want to use the correct interface (iommu_*) and write down
> the potential security impact (so we remember when exposing a virtual
> ITS to guests).
OK, I will use iommu_map()
>
>> + if ( ret < 0 )
>> + {
>> + printk(XENLOG_ERR "GICv3: Map ITS translation register d%d
>> failed.\n",
>> + its->d->domain_id);
>
> XENLOG_ERR is not ratelimited and therefore should not be called from
> emulation path. If you want to print an error, then you should use
> XENLOG_G_ERR.
>
> Also, for printing domain, the preferred is to using %pd with the domain
> as argument (here its->d.
I'll fix it
>
> But as this is emulation and therefore the current vCPU belongs to
> its->d, you could directly use gprintk(XENLOG_ERR, "...").
Will do
>
> Cheers,
>
> --
> Julien Grall
Support situation for nestedhvm
I ran into the nestedhvm via the following path. I was considering the feasibility of shedding tasks from a desktop onto a server running Xen. I was looking at `man xl.cfg` and noticed "nestedhvm". Since one of the tasks the computer handled was running other OSes in fully simulated environments, this seemed to be something I was looking for. No where did I ever see anything hinting "This configuration option is completely unsupported and risky to use". For an option like this, additional steps should have been needed to enable it. First, on Xen's command-line there needs to be something along the lines of "enable_unsupported=nestedhvm,others". Second, in xl.cfg perhaps there should be an `enable_unsupported` option which is a list of such options. Third, perhaps a build-time configuration option too? The issue is the above. At no point did I realize I had crossed the support boundary. Things simply started exploding without any warnings. -- (\___(\___(\__ --=> 8-) EHM <=-- __/)___/)___/) \BS (| ehem+sig...@m5p.com PGP 87145445 |) / \_CS\ | _ -O #include O- _ | / _/ 8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
Re: [PATCH] x86/x2apic: introduce a mixed physical/cluster mode
On Mon, Oct 30, 2023 at 04:27:22PM +0100, Roger Pau Monné wrote:
> On Mon, Oct 30, 2023 at 07:50:27AM -0700, Elliott Mitchell wrote:
> > On Tue, Oct 24, 2023 at 03:51:50PM +0200, Roger Pau Monne wrote:
> > > diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
> > > index 707deef98c27..15632cc7332e 100644
> > > --- a/xen/arch/x86/genapic/x2apic.c
> > > +++ b/xen/arch/x86/genapic/x2apic.c
> > > @@ -220,38 +239,56 @@ static struct notifier_block x2apic_cpu_nfb = {
> > > static int8_t __initdata x2apic_phys = -1;
> > > boolean_param("x2apic_phys", x2apic_phys);
> > >
> > > +enum {
> > > + unset, physical, cluster, mixed
> > > +} static __initdata x2apic_mode = unset;
> > > +
> > > +static int __init parse_x2apic_mode(const char *s)
> > > +{
> > > +if ( !cmdline_strcmp(s, "physical") )
> > > +x2apic_mode = physical;
> > > +else if ( !cmdline_strcmp(s, "cluster") )
> > > +x2apic_mode = cluster;
> > > +else if ( !cmdline_strcmp(s, "mixed") )
> > > +x2apic_mode = mixed;
> > > +else
> > > +return EINVAL;
> > > +
> > > +return 0;
> > > +}
> > > +custom_param("x2apic-mode", parse_x2apic_mode);
> > > +
> > > const struct genapic *__init apic_x2apic_probe(void)
> > > {
> > > -if ( x2apic_phys < 0 )
> > > +/* x2apic-mode option has preference over x2apic_phys. */
> > > +if ( x2apic_phys >= 0 && x2apic_mode == unset )
> > > +x2apic_mode = x2apic_phys ? physical : cluster;
> > > +
> > > +if ( x2apic_mode == unset )
> > > {
> > > -/*
> > > - * Force physical mode if there's no (full) interrupt remapping
> > > support:
> > > - * The ID in clustered mode requires a 32 bit destination field
> > > due to
> > > - * the usage of the high 16 bits to hold the cluster ID.
> > > - */
> > > -x2apic_phys = iommu_intremap != iommu_intremap_full ||
> > > - (acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL) ||
> > > - IS_ENABLED(CONFIG_X2APIC_PHYSICAL);
> > > -}
> > > -else if ( !x2apic_phys )
> > > -switch ( iommu_intremap )
> > > +if ( acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL )
> > > {
> >
> > Could this explain the issues with recent AMD processors/motherboards?
> >
> > Mainly the firmware had been setting this flag, but Xen was previously
> > ignoring it?
>
> No, not unless you pass {no-}x2apic_phys={false,0} on the Xen command
> line to force logical (clustered) destination mode.
>
> > As such Xen had been attempting to use cluster mode on an
> > x2APIC where that mode was broken for physical interrupts?
>
> No, not realy, x2apic_phys was already forced to true if
> acpi_gbl_FADT.flags & ACPI_FADT_APIC_PHYSICAL is set on the FADT (I
> just delete that line in this same chunk and move it here).
Okay, that was from a quick look at the patch. Given the symptoms and
workaround with recent AMD motherboards, this looked suspicious.
In that case it might be a bug in what AMD is providing to motherboard
manufacturers. Mainly this bit MUST be set, but AMD's implementation
leaves it unset.
Could also be if the setup is done correctly the bit can be cleared, but
multiple motherboard manufacturers are breaking this. Perhaps the steps
are fragile and AMD needed to provide better guidance.
Neowutran, are you still setup to and interested in doing
experimentation/testing with Xen on your AMD computer? Would you be up
for trying the patch here:
https://lore.kernel.org/xen-devel/20231106142739.19650-1-roger@citrix.com/raw
I have a suspicion this *might* fix the x2APIC issue everyone has been
seeing.
How plausible would it be to release this as a bugfix/workaround on 4.17?
I'm expecting a "no", but figured I should ask given how widespread the
issue is.
--
(\___(\___(\__ --=> 8-) EHM <=-- __/)___/)___/)
\BS (| ehem+sig...@m5p.com PGP 87145445 |) /
\_CS\ | _ -O #include O- _ | / _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
Re: [PATCH 22/29] tools/xenstored:
On Wed, Nov 1, 2023 at 7:15 AM Juergen Gross wrote: > > When [un]mapping the ring page of a Xenstore client, different actions > are required for "normal" guests and dom0. Today this distinction is > made at call site. > > Move this distinction into [un]map_interface() instead, avoiding code > duplication and preparing special handling for [un]mapping the stub > domain's ring page. > > Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: [PATCH 20/29] tools: add 9pfs device to xenstore-stubdom
On Wed, Nov 1, 2023 at 8:23 AM Juergen Gross wrote:
>
> Add a 9pfs device to Xenstore stubdom in order to allow it to do e.g.
> logging into a dom0 file.
>
> Use the following parameters for the new device:
>
> - tag = "xen"
> - type = "xenlogd"
> - path = "/var/lib/xen/xenstore"
>
> For now don't limit allowed file space or number of files.
>
> Add a new libxl function for adding it similar to the function for
> adding the console device.
>
> Signed-off-by: Juergen Gross
> diff --git a/tools/libs/light/libxl_9pfs.c b/tools/libs/light/libxl_9pfs.c
> index 0b9d84dce9..3297389493 100644
> --- a/tools/libs/light/libxl_9pfs.c
> +++ b/tools/libs/light/libxl_9pfs.c
> @@ -174,6 +174,35 @@ static void libxl__device_p9_add(libxl__egc *egc,
> uint32_t domid,
> aodev->callback(egc, aodev);
> }
>
> +int libxl_p9_add_xenstore(libxl_ctx *ctx, uint32_t domid, uint32_t backend,
> + libxl_p9_type type, char *tag, char *path,
> + unsigned int max_space, unsigned int max_files,
> + unsigned int max_open_files, bool auto_delete,
> + const libxl_asyncop_how *ao_how)
> +{
> +AO_CREATE(ctx, domid, ao_how);
> +libxl__ao_device *aodev;
> +libxl_device_p9 p9 = { .backend_domid = backend,
> + .tag = tag,
> + .path = path,
> + .security_model = "none",
While the xl.cfg man page states that only security_model="none" is
supported, it is possible to use other ones.The value isn't
inspected and it is just passed through Xenstore to QEMU. QEMU can
then operate however it operates. I just tested mapped-xattr and it's
working from some quick testing. So maybe libxl_p9_add_xenstore()
should take security_model as an argument, and then
init-xenstore-domain can pass in "none"?
Everything else looks good, so either way:
Reviewed-by: Jason Andryuk
Re: [PATCH 19/29] stubdom: extend xenstore stubdom configs
On Wed, Nov 1, 2023 at 5:48 AM Juergen Gross wrote: > > Extend the config files of the Xenstore stubdoms to include XENBUS > and 9PFRONT items in order to support file based logging. > > Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: [PATCH 17/29] tools/helpers: allocate xenstore event channel for xenstore stubdom
On Wed, Nov 1, 2023 at 5:53 AM Juergen Gross wrote: > > In order to prepare support of PV frontends in xenstore-stubdom, add > allocation of a Xenstore event channel to init-xenstore-domain.c. > > Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: [XEN PATCH 07/10] arm/traps: address a violation of MISRA C:2012 Rule 8.2
Hi Stefano,
On 16/10/2023 10:02, Julien Grall wrote:
Hi,
On 13/10/2023 16:24, Federico Serafini wrote:
Add missing parameter name, no functional change.
Signed-off-by: Federico Serafini
---
xen/arch/arm/traps.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ce89f16404..5aa14d4707 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1236,7 +1236,7 @@ int do_bug_frame(const struct cpu_user_regs
*regs, vaddr_t pc)
if ( id == BUGFRAME_run_fn )
{
- void (*fn)(const struct cpu_user_regs *) = (void
*)regs->BUG_FN_REG;
+ void (*fn)(const struct cpu_user_regs *regs) = (void
*)regs->BUG_FN_REG;
Now the line will be over 80 characters. I think we should introduce a
typedef. This would also help in the longer run to validate that the
function passed to run_in_exception_handle() has the expected prototype.
I see this patch was committed in your for-4.19 branch. But this comment
was unaddressed. Can you drop the patch because your branch is committed
in staging?
Cheers,
--
Julien Grall
Re: [RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
On 07/11/2023 14:45, Nicola Vetrini wrote:
Hi Julien,
Hi,
On 2023-11-07 13:44, Julien Grall wrote:
+in the community."
+-config=MC3R1.R15.2,reports+={deliberate,
"any_area(any_loc(text(^.*goto (again|retry).*$)))"}
+-doc_end
+
#
# Series 20.
#
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..7d1e1f0d09b3 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
statements are deliberate.
- Project-wide deviation; tagged as `disapplied` for ECLAIR.
+ * - R15.2
+ - The possible prevention of developer confusion as a result of
using
+ control flow structures other than backwards goto-s in some
instances was
+ deemed not strong enough to justify the additional complexity
introduced
+ in the code. Such instances are the uses of the following
labels:
+
+ - again
+ - retry
Have you investigated the possibility to use SAF-X in the code? If so,
what's the problem to use them?
Cheers,
This is another viable option: putting the SAF comment on top of the
label should suffice,
as shown below:
/* SAF-2-safe */
repeat:
++fmt; /* this also skips first '%' */
switch (*fmt) {
case '-': flags |= LEFT; goto repeat;
case '+': flags |= PLUS; goto repeat;
case ' ': flags |= SPACE; goto repeat;
case '#': flags |= SPECIAL; goto repeat;
case '0': flags |= ZEROPAD; goto repeat;
}
I think it ultimately boils down to whether Xen wants to promote the use
of certain labels
as the designated alternative when no other control flow mechanism is
clearer from a
readability perspective (in which case there should be a consistent
naming to capture and deviate
all of them, such as "retry") or do so on a case-by-case basis with a
SAF, which is ok,
I would prefer a case-by-case basis because it adds an additional
review. With deviating by keywords, the reviewrs/developpers may not be
aware of the deviation (which may be fine for some, but IMHO not this one).
but then
it needs someone to check each one and either fix them or mark them as ok.
Don't we technically already need to go through all the existing use of
ready & co even if we deviate by keyword?
Yet another route could be to mark with a SAF all those present right
now to establish a baseline.
How many do we have?
Cheers,
--
Julien Grall
Re: [PATCH 16/29] tools/xl: support new 9pfs backend xenlogd
On Wed, Nov 1, 2023 at 6:41 AM Juergen Gross wrote:
>
> Add support for the new 9pfs backend "xenlogd". For this backend type
> the tag defaults to "Xen" and the host side path to
> "/var/log/xen/guests/".
>
> Signed-off-by: Juergen Gross
> diff --git a/tools/xl/xl_parse.c b/tools/xl/xl_parse.c
> index ed983200c3..346532e117 100644
> --- a/tools/xl/xl_parse.c
> +++ b/tools/xl/xl_parse.c
> @@ -2242,6 +2256,27 @@ void parse_config_data(const char *config_source,
>
> libxl_string_list_dispose();
>
> +if (p9->type == LIBXL_P9_TYPE_UNKNOWN) {
> +p9->type = LIBXL_P9_TYPE_QEMU;
> +}
> +if (p9->type == LIBXL_P9_TYPE_QEMU &&
> +(p9->max_space || p9->auto_delete)) {
Also check p9->max_open_files and p9->max_files?
Regards,
Jason
[PATCH 7/7] tools/xg: Simplify hypercall stubs
Now there are no pending dependencies on the current form of the hypercall
stubs. Replace them with simpler forms that only take the xc_cpu_policy
object. This way the plumbing logic becomes a lot simpler, allowing the
policy to be extended without touching the plumbing code.
Signed-off-by: Alejandro Vallejo
---
tools/libs/guest/xg_cpuid_x86.c | 59 -
1 file changed, 22 insertions(+), 37 deletions(-)
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index e2a2659953..cf07a69764 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -125,16 +125,17 @@ int xc_cpu_policy_get_size(xc_interface *xch, uint32_t
*nr_leaves,
return ret;
}
-static int get_system_cpu_policy(xc_interface *xch, uint32_t index,
- uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
- uint32_t *nr_msrs, xen_msr_entry_t *msrs)
+static int get_system_cpu_policy_serialised(xc_interface *xch, uint32_t index,
+xc_cpu_policy_t *p)
{
struct xen_sysctl sysctl = {};
+xen_cpuid_leaf_t *leaves = p->leaves.buf;
+xen_msr_entry_t *msrs = p->msrs.buf;
DECLARE_HYPERCALL_BOUNCE(leaves,
- *nr_leaves * sizeof(*leaves),
+ p->leaves.allocated * sizeof(*leaves),
XC_HYPERCALL_BUFFER_BOUNCE_OUT);
DECLARE_HYPERCALL_BOUNCE(msrs,
- *nr_msrs * sizeof(*msrs),
+ p->msrs.allocated * sizeof(*msrs),
XC_HYPERCALL_BUFFER_BOUNCE_OUT);
int ret;
@@ -144,9 +145,9 @@ static int get_system_cpu_policy(xc_interface *xch,
uint32_t index,
sysctl.cmd = XEN_SYSCTL_get_cpu_policy;
sysctl.u.cpu_policy.index = index;
-sysctl.u.cpu_policy.nr_leaves = *nr_leaves;
+sysctl.u.cpu_policy.nr_leaves = p->leaves.allocated;
set_xen_guest_handle(sysctl.u.cpu_policy.leaves, leaves);
-sysctl.u.cpu_policy.nr_msrs = *nr_msrs;
+sysctl.u.cpu_policy.nr_msrs = p->msrs.allocated;
set_xen_guest_handle(sysctl.u.cpu_policy.msrs, msrs);
ret = do_sysctl(xch, );
@@ -156,23 +157,24 @@ static int get_system_cpu_policy(xc_interface *xch,
uint32_t index,
if ( !ret )
{
-*nr_leaves = sysctl.u.cpu_policy.nr_leaves;
-*nr_msrs = sysctl.u.cpu_policy.nr_msrs;
+p->leaves.len = sysctl.u.cpu_policy.nr_leaves;
+p->msrs.len = sysctl.u.cpu_policy.nr_msrs;
}
return ret;
}
-static int get_domain_cpu_policy(xc_interface *xch, uint32_t domid,
- uint32_t *nr_leaves, xen_cpuid_leaf_t *leaves,
- uint32_t *nr_msrs, xen_msr_entry_t *msrs)
+static int get_domain_cpu_policy_serialised(xc_interface *xch, uint32_t domid,
+xc_cpu_policy_t *p)
{
DECLARE_DOMCTL;
+xen_cpuid_leaf_t *leaves = p->leaves.buf;
+xen_msr_entry_t *msrs = p->msrs.buf;
DECLARE_HYPERCALL_BOUNCE(leaves,
- *nr_leaves * sizeof(*leaves),
+ p->leaves.allocated * sizeof(*leaves),
XC_HYPERCALL_BUFFER_BOUNCE_OUT);
DECLARE_HYPERCALL_BOUNCE(msrs,
- *nr_msrs * sizeof(*msrs),
+ p->msrs.allocated * sizeof(*msrs),
XC_HYPERCALL_BUFFER_BOUNCE_OUT);
int ret;
@@ -182,9 +184,9 @@ static int get_domain_cpu_policy(xc_interface *xch,
uint32_t domid,
domctl.cmd = XEN_DOMCTL_get_cpu_policy;
domctl.domain = domid;
-domctl.u.cpu_policy.nr_leaves = *nr_leaves;
+domctl.u.cpu_policy.nr_leaves = p->leaves.allocated;
set_xen_guest_handle(domctl.u.cpu_policy.leaves, leaves);
-domctl.u.cpu_policy.nr_msrs = *nr_msrs;
+domctl.u.cpu_policy.nr_msrs = p->msrs.allocated;
set_xen_guest_handle(domctl.u.cpu_policy.msrs, msrs);
ret = do_domctl(xch, );
@@ -194,8 +196,8 @@ static int get_domain_cpu_policy(xc_interface *xch,
uint32_t domid,
if ( !ret )
{
-*nr_leaves = domctl.u.cpu_policy.nr_leaves;
-*nr_msrs = domctl.u.cpu_policy.nr_msrs;
+p->leaves.len = domctl.u.cpu_policy.nr_leaves;
+p->msrs.len = domctl.u.cpu_policy.nr_msrs;
}
return ret;
@@ -745,22 +747,14 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t *policy)
int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
xc_cpu_policy_t *policy)
{
-unsigned int nr_leaves = policy->leaves.allocated;
-unsigned int nr_msrs = policy->msrs.allocated;
int rc;
-rc = get_system_cpu_policy(xch, policy_idx,
- _leaves, policy->leaves.buf,
- _msrs, policy->msrs.buf);
-if ( rc )
+if ( (rc = get_system_cpu_policy_serialised(xch,
[PATCH 3/7] tools/xg: Add self-(de)serialisation functions for cpu_policy
These allow a policy to internally (de)serialize itself, so that we don't
have to carry around serialization buffers when perfectly good ones are
present inside.
Both moved on top of the xend overrides as they are needed there in future
patches
Signed-off-by: Alejandro Vallejo
---
tools/libs/guest/xg_cpuid_x86.c | 90 ++---
1 file changed, 49 insertions(+), 41 deletions(-)
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 3545f3e530..ac75ce2b4e 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -254,6 +254,50 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t
domid,
return ret;
}
+static int cpu_policy_deserialise_on_self(xc_interface *xch, xc_cpu_policy_t
*p)
+{
+uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+int rc;
+
+rc = x86_cpuid_copy_from_buffer(>policy, p->leaves.buf, p->leaves.len,
+_leaf, _subleaf);
+if ( rc )
+{
+if ( err_leaf != -1 )
+ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d
= %s)",
+ err_leaf, err_subleaf, -rc, strerror(-rc));
+return rc;
+}
+
+rc = x86_msr_copy_from_buffer(>policy, p->msrs.buf, p->msrs.len,
_msr);
+if ( rc )
+{
+if ( err_msr != -1 )
+ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
+ err_msr, -rc, strerror(-rc));
+return rc;
+}
+
+return 0;
+}
+
+static int cpu_policy_serialise_on_self(xc_interface *xch, xc_cpu_policy_t *p)
+{
+uint32_t nr_leaves = p->leaves.allocated;
+uint32_t nr_msrs = p->msrs.allocated;
+int rc = xc_cpu_policy_serialise(xch, p,
+ p->leaves.buf, _leaves,
+ p->msrs.buf, _msrs);
+
+if ( !rc )
+{
+p->leaves.len = nr_leaves;
+p->msrs.len = nr_msrs;
+}
+
+return rc;
+}
+
static int compare_leaves(const void *l, const void *r)
{
const xen_cpuid_leaf_t *lhs = l;
@@ -883,35 +927,6 @@ void xc_cpu_policy_destroy(xc_cpu_policy_t *policy)
errno = err;
}
-static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy,
- unsigned int nr_leaves, unsigned int nr_entries)
-{
-uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-int rc;
-
-rc = x86_cpuid_copy_from_buffer(>policy, policy->leaves.buf,
-nr_leaves, _leaf, _subleaf);
-if ( rc )
-{
-if ( err_leaf != -1 )
-ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d
= %s)",
- err_leaf, err_subleaf, -rc, strerror(-rc));
-return rc;
-}
-
-rc = x86_msr_copy_from_buffer(>policy, policy->msrs.buf,
- nr_entries, _msr);
-if ( rc )
-{
-if ( err_msr != -1 )
-ERROR("Failed to deserialise MSR (err MSR %#x) (%d = %s)",
- err_msr, -rc, strerror(-rc));
-return rc;
-}
-
-return 0;
-}
-
int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
xc_cpu_policy_t *policy)
{
@@ -931,7 +946,7 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned
int policy_idx,
policy->leaves.len = nr_leaves;
policy->msrs.len = nr_msrs;
-rc = deserialize_policy(xch, policy, nr_leaves, nr_msrs);
+rc = cpu_policy_deserialise_on_self(xch, policy);
if ( rc )
{
errno = -rc;
@@ -960,7 +975,7 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t
domid,
policy->leaves.len = nr_leaves;
policy->msrs.len = nr_msrs;
-rc = deserialize_policy(xch, policy, nr_leaves, nr_msrs);
+rc = cpu_policy_deserialise_on_self(xch, policy);
if ( rc )
{
errno = -rc;
@@ -974,22 +989,15 @@ int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t
domid,
xc_cpu_policy_t *policy)
{
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-unsigned int nr_leaves = policy->leaves.allocated;
-unsigned int nr_msrs = policy->msrs.allocated;
int rc;
-rc = xc_cpu_policy_serialise(xch, policy,
- policy->leaves.buf, _leaves,
- policy->msrs.buf, _msrs);
+rc = cpu_policy_serialise_on_self(xch, policy);
if ( rc )
return rc;
-policy->leaves.len = nr_leaves;
-policy->msrs.len = nr_msrs;
-
rc = xc_set_domain_cpu_policy(xch, domid,
- nr_leaves, policy->leaves.buf,
- nr_msrs, policy->msrs.buf,
+ policy->leaves.len, policy->leaves.buf,
+ policy->msrs.len, policy->msrs.buf,
_leaf, _subleaf, _msr);
if ( rc )
{
--
2.34.1
[PATCH 0/7] Cleanup and code duplication removal in xenguest
NOTE: This series is still under test This series tries to bind the current xc_cpu_policy_t and its serialization buffers. Having them separate makes it hard to extend of modify the serialized policy structure and needlessly leaks details in places that don't need them, causing a lot of boilerplate and code duplication. The resulting code is >100LoC leaner. The series is not as aggressive as it could be, but it's enough to unlock future work regarding cpu policy extensions and ought to make toolstack interactions a lot faster. Patch 1: Remove the fixed buffers in xc_cpu_policy_t and create pointers populated during xc_cpu_policy_init() instead. Patch 2: Removes boilerplate by making use of the newly created buffers Patch 3: Adds a couple of helpers to keep the 2 forms inside the xc_cpu_policy_t object consistently in sync. Patch 4: Splits the common set_cpu_policy wrapper in 2. One to send the deserialized form of the policy object (after serializing it internally first) and another to send the serialized form directly. Patch 5: Uses the previous patches to avoid a lot of boilerplate in the main policy manipulation routine. Patch 6: Remove code duplication in the xend-style override setters Patch 7: Final cleanup so the get_cpu_policy wrappers can operate on policy objects directly Alejandro Vallejo (7): tools/xenguest: Dynamically allocate xc_cpu_policy_t contents tools/xg: Simplify write_x86_cpu_policy_records() tools/xg: Add self-(de)serialisation functions for cpu_policy tools/xg: Split xc_cpu_policy_set_domain() tools/xg: Streamline xc_cpuid_apply_policy() tools/xg: Simplify xc_cpuid_xend_policy() and xc_msr_policy() tools/xg: Simplify hypercall stubs tools/include/xenguest.h | 11 +- tools/libs/guest/xg_cpuid_x86.c | 626 +++ tools/libs/guest/xg_private.h| 17 +- tools/libs/guest/xg_sr_common_x86.c | 55 +-- tools/misc/xen-cpuid.c | 2 +- tools/tests/tsx/test-tsx.c | 61 +-- xen/include/xen/lib/x86/cpu-policy.h | 2 +- 7 files changed, 318 insertions(+), 456 deletions(-) -- 2.34.1
[PATCH 5/7] tools/xg: Streamline xc_cpuid_apply_policy()
Instantiates host, default and domain policies in order to clean up a lot
of boilerplate hypercalls. This is partial work in order to deduplicate
the same hypercalls being used when updating CPUID and MSR parts of the
policy.
Signed-off-by: Alejandro Vallejo
---
tools/include/xenguest.h| 1 +
tools/libs/guest/xg_cpuid_x86.c | 184
2 files changed, 93 insertions(+), 92 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index a66d9f7807..f0b58bb395 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -788,6 +788,7 @@ typedef struct xc_cpu_policy xc_cpu_policy_t;
/* Create and free a xc_cpu_policy object. */
xc_cpu_policy_t *xc_cpu_policy_init(xc_interface *xch);
+xc_cpu_policy_t *xc_cpu_policy_clone(const xc_cpu_policy_t *other);
void xc_cpu_policy_destroy(xc_cpu_policy_t *policy);
/* Retrieve a system policy, or get/set a domains policy. */
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index 8fafeb2a02..acc94fb16b 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -635,13 +635,14 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t
domid, bool restore,
int rc;
bool hvm;
xc_domaininfo_t di;
-unsigned int i, nr_leaves, nr_msrs;
-xen_cpuid_leaf_t *leaves = NULL;
-struct cpu_policy *p = NULL;
-uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
-uint32_t len = ARRAY_SIZE(host_featureset);
+uint32_t def_policy;
+/*
+ * Three full policies. The host, default for the domain type,
+ * and domain current.
+ */
+xc_cpu_policy_t *host = NULL, *def = NULL, *cur = NULL;
+/* Determine if domid is PV or HVM */
if ( (rc = xc_domain_getinfo_single(xch, domid, )) < 0 )
{
PERROR("Failed to obtain d%d info", domid);
@@ -650,48 +651,24 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t
domid, bool restore,
}
hvm = di.flags & XEN_DOMINF_hvm_guest;
-rc = xc_cpu_policy_get_size(xch, _leaves, _msrs);
-if ( rc )
-{
-PERROR("Failed to obtain policy info size");
-rc = -errno;
-goto out;
-}
-
-rc = -ENOMEM;
-if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL ||
- (p = calloc(1, sizeof(*p))) == NULL )
-goto out;
-
-/* Get the host policy. */
-rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
- , host_featureset);
-/* Tolerate "buffer too small", as we've got the bits we need. */
-if ( rc && errno != ENOBUFS )
+if ( !(host = xc_cpu_policy_init(xch)) ||
+ !(def = xc_cpu_policy_clone(host)) ||
+ !(cur = xc_cpu_policy_clone(host)) )
{
-PERROR("Failed to obtain host featureset");
-rc = -errno;
+PERROR("Failed to allocate policy state");
goto out;
}
-/* Get the domain's default policy. */
-nr_msrs = 0;
-rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-: XEN_SYSCTL_cpu_policy_pv_default,
- _leaves, leaves, _msrs, NULL);
-if ( rc )
-{
-PERROR("Failed to obtain %s default policy", hvm ? "hvm" : "pv");
-rc = -errno;
-goto out;
-}
+def_policy = hvm ? XEN_SYSCTL_cpu_policy_hvm_default
+ : XEN_SYSCTL_cpu_policy_pv_default;
-rc = x86_cpuid_copy_from_buffer(p, leaves, nr_leaves,
-_leaf, _subleaf);
-if ( rc )
+/* Get the domain type's default policy. */
+if ( (rc = xc_cpu_policy_get_domain(xch, domid, cur)) ||
+ (rc = xc_cpu_policy_get_system(xch, def_policy, def)) ||
+ (rc = xc_cpu_policy_get_system(xch, XEN_SYSCTL_cpu_policy_host,
host)) )
{
-ERROR("Failed to deserialise CPUID (err leaf %#x, subleaf %#x) (%d =
%s)",
- err_leaf, err_subleaf, -rc, strerror(-rc));
+PERROR("Failed to obtain d%d, %s and/or host policies",
+ domid, hvm ? "hvm" : "pv");
goto out;
}
@@ -711,18 +688,16 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t
domid, bool restore,
* - Re-enable features which have become (possibly) off by default.
*/
-p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
-p->feat.hle = test_bit(X86_FEATURE_HLE, host_featureset);
-p->feat.rtm = test_bit(X86_FEATURE_RTM, host_featureset);
+cur->policy.basic.rdrand = host->policy.basic.rdrand;
+cur->policy.feat.hle = host->policy.feat.hle;
+cur->policy.feat.rtm = host->policy.feat.rtm;
if ( hvm )
-{
-p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
-}
+cur->policy.feat.mpx = host->policy.feat.mpx;
-p->basic.max_leaf =
[PATCH 4/7] tools/xg: Split xc_cpu_policy_set_domain()
xc_cpu_policy_set_domain_from_serialised() converts the cpu policy into its
serialised form first and then sends that to Xen. Meanwhile,
xc_cpu_policy_domain_set_from_deserialised() uses whatever is already in
the internal serialisation buffer of the policy object.
This split helps in a future patch where modifications are done in the
serialized form and we don't want to do a serialization round-trip to
send it to Xen.
Signed-off-by: Alejandro Vallejo
---
tools/include/xenguest.h| 8 ++--
tools/libs/guest/xg_cpuid_x86.c | 24 ++--
tools/tests/tsx/test-tsx.c | 2 +-
3 files changed, 21 insertions(+), 13 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index 702965addc..a66d9f7807 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -795,8 +795,12 @@ int xc_cpu_policy_get_system(xc_interface *xch, unsigned
int policy_idx,
xc_cpu_policy_t *policy);
int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t domid,
xc_cpu_policy_t *policy);
-int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
- xc_cpu_policy_t *policy);
+int xc_cpu_policy_set_domain_from_serialised(xc_interface *xch,
+ uint32_t domid,
+ xc_cpu_policy_t *policy);
+int xc_cpu_policy_set_domain_from_deserialised(xc_interface *xch,
+ uint32_t domid,
+ xc_cpu_policy_t *policy);
/* Manipulate a policy via architectural representations. */
int xc_cpu_policy_serialise(xc_interface *xch, const xc_cpu_policy_t *policy,
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index ac75ce2b4e..8fafeb2a02 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -985,20 +985,24 @@ int xc_cpu_policy_get_domain(xc_interface *xch, uint32_t
domid,
return rc;
}
-int xc_cpu_policy_set_domain(xc_interface *xch, uint32_t domid,
- xc_cpu_policy_t *policy)
+int xc_cpu_policy_set_domain_from_deserialised(xc_interface *xch, uint32_t
domid,
+ xc_cpu_policy_t *policy)
{
-uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
int rc;
-
-rc = cpu_policy_serialise_on_self(xch, policy);
-if ( rc )
+if ( (rc = cpu_policy_serialise_on_self(xch, policy)) )
return rc;
-rc = xc_set_domain_cpu_policy(xch, domid,
- policy->leaves.len, policy->leaves.buf,
- policy->msrs.len, policy->msrs.buf,
- _leaf, _subleaf, _msr);
+return xc_cpu_policy_set_domain_from_serialised(xch, domid, policy);
+}
+
+int xc_cpu_policy_set_domain_from_serialised(xc_interface *xch, uint32_t domid,
+ xc_cpu_policy_t *policy)
+{
+uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+int rc = xc_set_domain_cpu_policy(xch, domid,
+ policy->leaves.len, policy->leaves.buf,
+ policy->msrs.len, policy->msrs.buf,
+ _leaf, _subleaf, _msr);
if ( rc )
{
ERROR("Failed to set domain %u policy (%d = %s)", domid, -rc,
diff --git a/tools/tests/tsx/test-tsx.c b/tools/tests/tsx/test-tsx.c
index 3371bb26f7..21b5640796 100644
--- a/tools/tests/tsx/test-tsx.c
+++ b/tools/tests/tsx/test-tsx.c
@@ -405,7 +405,7 @@ static void test_guest(struct xen_domctl_createdomain *c)
(bitmaskof(X86_FEATURE_HLE) | bitmaskof(X86_FEATURE_RTM)));
/* Set the new policy. */
-rc = xc_cpu_policy_set_domain(xch, domid, guest_policy);
+rc = xc_cpu_policy_set_domain_from_deserialised(xch, domid,
guest_policy);
if ( rc )
{
fail(" Failed to set domain policy: %d - %s\n",
--
2.34.1
[PATCH 2/7] tools/xg: Simplify write_x86_cpu_policy_records()
With the policy automatically getting appropriate serialised buffer sizes,
we can remove boilerplate from this function. Furthermore, the extra
dynamic allocations aren't needed anymore as the serialised buffers inside
the policy can be used instead.
Signed-off-by: Alejandro Vallejo
---
tools/libs/guest/xg_sr_common_x86.c | 55 +
1 file changed, 17 insertions(+), 38 deletions(-)
diff --git a/tools/libs/guest/xg_sr_common_x86.c
b/tools/libs/guest/xg_sr_common_x86.c
index ad63c675ed..c8fd64775f 100644
--- a/tools/libs/guest/xg_sr_common_x86.c
+++ b/tools/libs/guest/xg_sr_common_x86.c
@@ -44,55 +44,36 @@ int handle_x86_tsc_info(struct xc_sr_context *ctx, struct
xc_sr_record *rec)
int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
{
+int rc = -1;
xc_interface *xch = ctx->xch;
-struct xc_sr_record cpuid = { .type = REC_TYPE_X86_CPUID_POLICY, };
-struct xc_sr_record msrs = { .type = REC_TYPE_X86_MSR_POLICY, };
-uint32_t nr_leaves = 0, nr_msrs = 0;
-xc_cpu_policy_t *policy = NULL;
-int rc;
-
-if ( xc_cpu_policy_get_size(xch, _leaves, _msrs) < 0 )
-{
-PERROR("Unable to get CPU Policy size");
-return -1;
-}
+struct xc_sr_record record;
+xc_cpu_policy_t *policy = xc_cpu_policy_init(xch);
-cpuid.data = malloc(nr_leaves * sizeof(xen_cpuid_leaf_t));
-msrs.data = malloc(nr_msrs * sizeof(xen_msr_entry_t));
-policy = xc_cpu_policy_init(xch);
-if ( !cpuid.data || !msrs.data || !policy )
-{
-ERROR("Cannot allocate memory for CPU Policy");
-rc = -1;
-goto out;
-}
-
-if ( xc_cpu_policy_get_domain(xch, ctx->domid, policy) )
+if ( !policy ||
+ (rc = xc_cpu_policy_get_domain(xch, ctx->domid, policy)) )
{
PERROR("Unable to get d%d CPU Policy", ctx->domid);
-rc = -1;
-goto out;
-}
-if ( xc_cpu_policy_serialise(xch, policy, cpuid.data, _leaves,
- msrs.data, _msrs) )
-{
-PERROR("Unable to serialize d%d CPU Policy", ctx->domid);
-rc = -1;
goto out;
}
-cpuid.length = nr_leaves * sizeof(xen_cpuid_leaf_t);
-if ( cpuid.length )
+record = (struct xc_sr_record){
+.type = REC_TYPE_X86_CPUID_POLICY, .data = policy->leaves.buf,
+.length = policy->leaves.len * sizeof(*policy->leaves.buf),
+};
+if ( record.length )
{
-rc = write_record(ctx, );
+rc = write_record(ctx, );
if ( rc )
goto out;
}
-msrs.length = nr_msrs * sizeof(xen_msr_entry_t);
-if ( msrs.length )
+record = (struct xc_sr_record){
+.type = REC_TYPE_X86_MSR_POLICY, .data = policy->msrs.buf,
+.length = policy->msrs.len * sizeof(*policy->msrs.buf),
+};
+if ( record.length )
{
-rc = write_record(ctx, );
+rc = write_record(ctx, );
if ( rc )
goto out;
}
@@ -100,8 +81,6 @@ int write_x86_cpu_policy_records(struct xc_sr_context *ctx)
rc = 0;
out:
-free(cpuid.data);
-free(msrs.data);
xc_cpu_policy_destroy(policy);
return rc;
--
2.34.1
[PATCH 6/7] tools/xg: Simplify xc_cpuid_xend_policy() and xc_msr_policy()
Remove all duplication in CPUID and MSR xend-style overrides. They had an
incredible amount of overhead for no good reason.
After this patch, CPU policy application takes a number of hypercalls to
recover the policy state and then those are passed to the xend-style
override code so it can avoid them.
Furthermore, the ultimate reapplication of the policy to the domain in Xen
is done only once after both CPUID and MSRs have been fixed up.
BUG!!! apply_policy is sending the policy after deserialise when it poked
at it in its serialised form.
Signed-off-by: Alejandro Vallejo
---
tools/libs/guest/xg_cpuid_x86.c | 261 +---
1 file changed, 38 insertions(+), 223 deletions(-)
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index acc94fb16b..e2a2659953 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -312,102 +312,32 @@ static int compare_leaves(const void *l, const void *r)
return 0;
}
-static xen_cpuid_leaf_t *find_leaf(
-xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
-const struct xc_xend_cpuid *xend)
+static xen_cpuid_leaf_t *find_leaf(xc_cpu_policy_t *p,
+ const struct xc_xend_cpuid *xend)
{
const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
-return bsearch(, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
+return bsearch(, p->leaves.buf, p->leaves.len,
+ sizeof(*p->leaves.buf), compare_leaves);
}
static int xc_cpuid_xend_policy(
-xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
+xc_interface *xch, uint32_t domid,
+const struct xc_xend_cpuid *xend,
+xc_cpu_policy_t *host,
+xc_cpu_policy_t *def,
+xc_cpu_policy_t *cur)
{
-int rc;
-bool hvm;
-xc_domaininfo_t di;
-unsigned int nr_leaves, nr_msrs;
-uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
-/*
- * Three full policies. The host, default for the domain type,
- * and domain current.
- */
-xen_cpuid_leaf_t *host = NULL, *def = NULL, *cur = NULL;
-unsigned int nr_host, nr_def, nr_cur;
-
-if ( (rc = xc_domain_getinfo_single(xch, domid, )) < 0 )
-{
-PERROR("Failed to obtain d%d info", domid);
-rc = -errno;
-goto fail;
-}
-hvm = di.flags & XEN_DOMINF_hvm_guest;
-
-rc = xc_cpu_policy_get_size(xch, _leaves, _msrs);
-if ( rc )
-{
-PERROR("Failed to obtain policy info size");
-rc = -errno;
-goto fail;
-}
-
-rc = -ENOMEM;
-if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
- (def = calloc(nr_leaves, sizeof(*def))) == NULL ||
- (cur = calloc(nr_leaves, sizeof(*cur))) == NULL )
-{
-ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
-goto fail;
-}
-
-/* Get the domain's current policy. */
-nr_msrs = 0;
-nr_cur = nr_leaves;
-rc = get_domain_cpu_policy(xch, domid, _cur, cur, _msrs, NULL);
-if ( rc )
-{
-PERROR("Failed to obtain d%d current policy", domid);
-rc = -errno;
-goto fail;
-}
-
-/* Get the domain type's default policy. */
-nr_msrs = 0;
-nr_def = nr_leaves;
-rc = get_system_cpu_policy(xch, hvm ? XEN_SYSCTL_cpu_policy_hvm_default
-: XEN_SYSCTL_cpu_policy_pv_default,
- _def, def, _msrs, NULL);
-if ( rc )
-{
-PERROR("Failed to obtain %s def policy", hvm ? "hvm" : "pv");
-rc = -errno;
-goto fail;
-}
-
-/* Get the host policy. */
-nr_msrs = 0;
-nr_host = nr_leaves;
-rc = get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
- _host, host, _msrs, NULL);
-if ( rc )
-{
-PERROR("Failed to obtain host policy");
-rc = -errno;
-goto fail;
-}
-
-rc = -EINVAL;
for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
{
-xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
-const xen_cpuid_leaf_t *def_leaf = find_leaf(def, nr_def, xend);
-const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
+xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, xend);
+const xen_cpuid_leaf_t *def_leaf = find_leaf(def, xend);
+const xen_cpuid_leaf_t *host_leaf = find_leaf(host, xend);
if ( cur_leaf == NULL || def_leaf == NULL || host_leaf == NULL )
{
ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
-goto fail;
+return -EINVAL;
}
for ( unsigned int i = 0; i < ARRAY_SIZE(xend->policy); i++ )
@@ -436,7 +366,7 @@ static int xc_cpuid_xend_policy(
{
ERROR("Bad character '%c' in policy[%d] string '%s'",
xend->policy[i][j], i, xend->policy[i]);
-goto fail;
+
[PATCH 1/7] tools/xenguest: Dynamically allocate xc_cpu_policy_t contents
This is part of a larger effort to reduce technical debt in this area and
prevent policy internals from leaking in plumbing code that needs not be
aware of them.
This patch turns the internal static buffers into dynamic ones and performs
the allocations based on policy sizes probed using a hypercall. This is
meant to help dealing with mismatched versions of toolstack/Xen, as no
assumptions are made with regards of the sizes of the buffers. With this,
we are now able to use these buffers for serialization instead of
out-of-band data structures.
The scheme is simple and involves keeping track of the buffer occupancy
through the "len" field and its capacity through the "allocated" field;
think "vector" in higher-level languages. Both trackers refer to entries
rather than octets because most of the code deals in those terms.
While at this, make a minor change to MSR_MAX_SERIALISED_ENTRIES so it's
unsigned in order to please the max() macro.
Signed-off-by: Alejandro Vallejo
---
tools/include/xenguest.h | 2 +-
tools/libs/guest/xg_cpuid_x86.c | 86 +---
tools/libs/guest/xg_private.h| 17 +-
tools/libs/guest/xg_sr_common_x86.c | 2 +-
tools/misc/xen-cpuid.c | 2 +-
tools/tests/tsx/test-tsx.c | 61 +++-
xen/include/xen/lib/x86/cpu-policy.h | 2 +-
7 files changed, 119 insertions(+), 53 deletions(-)
diff --git a/tools/include/xenguest.h b/tools/include/xenguest.h
index e01f494b77..702965addc 100644
--- a/tools/include/xenguest.h
+++ b/tools/include/xenguest.h
@@ -787,7 +787,7 @@ xen_pfn_t *xc_map_m2p(xc_interface *xch,
typedef struct xc_cpu_policy xc_cpu_policy_t;
/* Create and free a xc_cpu_policy object. */
-xc_cpu_policy_t *xc_cpu_policy_init(void);
+xc_cpu_policy_t *xc_cpu_policy_init(xc_interface *xch);
void xc_cpu_policy_destroy(xc_cpu_policy_t *policy);
/* Retrieve a system policy, or get/set a domains policy. */
diff --git a/tools/libs/guest/xg_cpuid_x86.c b/tools/libs/guest/xg_cpuid_x86.c
index f2b1e80901..3545f3e530 100644
--- a/tools/libs/guest/xg_cpuid_x86.c
+++ b/tools/libs/guest/xg_cpuid_x86.c
@@ -839,15 +839,48 @@ out:
return rc;
}
-xc_cpu_policy_t *xc_cpu_policy_init(void)
+xc_cpu_policy_t *xc_cpu_policy_init(xc_interface *xch)
{
-return calloc(1, sizeof(struct xc_cpu_policy));
+uint32_t nr_leaves, nr_msrs;
+xc_cpu_policy_t *p = calloc(1, sizeof(*p));
+if ( !p )
+return NULL;
+
+if ( xc_cpu_policy_get_size(xch, _leaves, _msrs) )
+goto fail;
+
+p->leaves.allocated = max(nr_leaves, CPUID_MAX_SERIALISED_LEAVES);
+p->leaves.buf = calloc(p->leaves.allocated, sizeof(*p->leaves.buf));
+if ( !p->leaves.buf )
+goto fail;
+
+p->msrs.allocated = max(nr_msrs, MSR_MAX_SERIALISED_ENTRIES);
+p->msrs.buf = calloc(p->msrs.allocated, sizeof(*p->msrs.buf));
+if ( !p->msrs.buf )
+goto fail;
+
+/* Success */
+return p;
+
+ fail:
+xc_cpu_policy_destroy(p);
+return NULL;
}
void xc_cpu_policy_destroy(xc_cpu_policy_t *policy)
{
-if ( policy )
-free(policy);
+int err = errno;
+
+if ( !policy )
+return;
+
+if ( policy->leaves.buf )
+free(policy->leaves.buf);
+if ( policy->msrs.buf )
+free(policy->msrs.buf);
+
+free(policy);
+errno = err;
}
static int deserialize_policy(xc_interface *xch, xc_cpu_policy_t *policy,
@@ -856,7 +889,7 @@ static int deserialize_policy(xc_interface *xch,
xc_cpu_policy_t *policy,
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
int rc;
-rc = x86_cpuid_copy_from_buffer(>policy, policy->leaves,
+rc = x86_cpuid_copy_from_buffer(>policy, policy->leaves.buf,
nr_leaves, _leaf, _subleaf);
if ( rc )
{
@@ -866,7 +899,7 @@ static int deserialize_policy(xc_interface *xch,
xc_cpu_policy_t *policy,
return rc;
}
-rc = x86_msr_copy_from_buffer(>policy, policy->msrs,
+rc = x86_msr_copy_from_buffer(>policy, policy->msrs.buf,
nr_entries, _msr);
if ( rc )
{
@@ -882,18 +915,22 @@ static int deserialize_policy(xc_interface *xch,
xc_cpu_policy_t *policy,
int xc_cpu_policy_get_system(xc_interface *xch, unsigned int policy_idx,
xc_cpu_policy_t *policy)
{
-unsigned int nr_leaves = ARRAY_SIZE(policy->leaves);
-unsigned int nr_msrs = ARRAY_SIZE(policy->msrs);
+unsigned int nr_leaves = policy->leaves.allocated;
+unsigned int nr_msrs = policy->msrs.allocated;
int rc;
-rc = get_system_cpu_policy(xch, policy_idx, _leaves, policy->leaves,
- _msrs, policy->msrs);
+rc = get_system_cpu_policy(xch, policy_idx,
+ _leaves, policy->leaves.buf,
+ _msrs, policy->msrs.buf);
if ( rc )
{
PERROR("Failed to obtain %u policy", policy_idx);
return
Re: [PATCH 15/29] tools/libs/light: add backend type for 9pfs PV devices
On Wed, Nov 1, 2023 at 5:51 AM Juergen Gross wrote: > > Make the backend type of 9pfs PV devices configurable. The default is > "qemu" with the related Xenstore backend-side directory being "9pfs". > > Add another type "xenlogd" with the related Xenstore backend-side > directory "xen_9pfs". > > As additional security features it is possible to specify: > - "max-space" for limiting the maximum space consumed on the filesystem > in MBs > - "max-files" for limiting the maximum number of files in the > filesystem > - "max-open-files" for limiting the maximum number of concurrent open > files > > For convenience "auto-delete" is available to let the backend delete the > oldest file of the guest in case otherwise "max-space" or "max-files" > would be violated. > > The xenlogd daemon will be started by libxenlight automatically when > the first "xen_9pfs" device is being created. > > Signed-off-by: Juergen Gross With Xentore paths updated to "libxl/..." as mentioned elsewhere: Reviewed-by: Jason Andryuk
[xen-unstable test] 183703: tolerable trouble: fail/pass/starved - PUSHED
flight 183703 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/183703/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 183695 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 183695 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 183695 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 183695 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 183695 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 183695 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 183695 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 183695 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 183695 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 183695 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 183695 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 183695 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-amd64-amd64-dom0pvh-xl-amd 3 hosts-allocate starved n/a version targeted for testing: xen de1cc5102b487e1a4bf321ac138b64c6ce1f0c0a baseline version: xen 1f849edc2f9ca7dc2f9ed7b0585c31bd6b81d7ef Last test of basis 183695 2023-11-06 19:37:11 Z0 days Testing same since 183703 2023-11-07 05:01:48 Z0 days1 attempts People who
Re: [PATCH 14/29] tools/xenlogd: add 9pfs read request support
On Tue, Nov 7, 2023 at 9:49 AM Juergen Gross wrote:
>
> On 07.11.23 15:31, Jason Andryuk wrote:
> > On Wed, Nov 1, 2023 at 5:35 AM Juergen Gross wrote:
> >>
> >> Add the read request of the 9pfs protocol.
> >>
> >> For now support only reading plain files (no directories).
> >>
> >> Signed-off-by: Juergen Gross
> >> ---
> >> tools/xenlogd/io.c | 60 ++
> >> 1 file changed, 60 insertions(+)
> >>
> >> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> >> index 6b4692ca67..b3f9f96bcc 100644
> >> --- a/tools/xenlogd/io.c
> >> +++ b/tools/xenlogd/io.c
> >
> >> @@ -1011,6 +1012,61 @@ static void p9_create(device *device, struct
> >> p9_header *hdr)
> >> fill_buffer(device, hdr->cmd + 1, hdr->tag, "QU", , );
> >> }
> >>
> >> +static void p9_read(device *device, struct p9_header *hdr)
> >> +{
> >> +uint32_t fid;
> >> +uint64_t off;
> >> +unsigned int len;
> >> +uint32_t count;
> >> +void *buf;
> >> +struct p9_fid *fidp;
> >> +int ret;
> >> +
> >> +ret = fill_data(device, "ULU", , , );
> >> +if ( ret != 3 )
> >> +{
> >> +p9_error(device, hdr->tag, EINVAL);
> >> +return;
> >> +}
> >> +
> >> +fidp = find_fid(device, fid);
> >> +if ( !fidp || !fidp->opened )
> >> +{
> >> +p9_error(device, hdr->tag, EBADF);
> >> +return;
> >> +}
> >> +
> >
> > I think you want to mode check here for readability.
>
> Same reasoning as for the write case: read() will do it for me.
>
> >
> >> +if ( fidp->isdir )
> >> +{
> >> +p9_error(device, hdr->tag, EOPNOTSUPP);
> >> +return;
> >
> > Hmmm, 9P2000.u supports read on a dir.
> > https://ericvh.github.io/9p-rfc/rfc9p2000.html#anchor30
> > """
> > For directories, read returns an integral number of direc- tory
> > entries exactly as in stat (see stat(5)), one for each member of the
> > directory. The read request message must have offset equal to zero or
> > the value of offset in the previous read on the directory, plus the
> > number of bytes returned in the previous read. In other words, seeking
> > other than to the beginning is illegal in a directory (see seek(2)).
> > """
>
> This is part of "only operations needed for Xenstore-stubdom are implemented."
> For supporting Linux guests or other stubdoms directory reading must be added,
> of course.
>
> >
> >> +}
> >> +else
> >
> > Since the above is a return, maybe remove the else and un-indent?
> > Though maybe you don't want to do that if you want to implement read
> > on a dir.
>
> Correct.
>
> >
> >> +{
> >> +len = count;
> >> +buf = device->buffer + sizeof(*hdr) + sizeof(uint32_t);
> >> +
> >> +while ( len != 0 )
> >> +{
> >> +ret = pread(fidp->fd, buf, len, off);
> >> +if ( ret <= 0 )
> >> +break;
> >> +len -= ret;
> >> +buf += ret;
> >> +off += ret;
> >> +}
> >> +if ( ret && len == count )
> >
> > This seems wrong to me - should this be ( ret < 0 && len == count ) to
> > indicate an error on the first pread? Any partial reads would still
> > return their data?
>
> If len == count there was no partial read, as this would have reduced len.
Right. I found it confusing since ret > 0 is not an error. As you
wrote, len != count in that case though.
Preferably with ret < 0:
Reviewed-by: Jason Andryuk
Regards,
Jason
Re: [PATCH 12/29] tools/xenlogd: add 9pfs stat request support
On Tue, Nov 7, 2023 at 9:48 AM Jason Andryuk wrote: > > On Tue, Nov 7, 2023 at 9:42 AM Juergen Gross wrote: > > > > On 07.11.23 15:04, Jason Andryuk wrote: > > > On Wed, Nov 1, 2023 at 5:34 AM Juergen Gross wrote: > > >> > > >> Add the stat request of the 9pfs protocol. > > >> > > >> Signed-off-by: Juergen Gross Reviewed-by: Jason Andryuk
Re: [PATCH 13/29] tools/xenlogd: add 9pfs write request support
On Tue, Nov 7, 2023 at 9:43 AM Juergen Gross wrote:
>
> On 07.11.23 15:10, Jason Andryuk wrote:
> > On Wed, Nov 1, 2023 at 5:54 AM Juergen Gross wrote:
> >>
> >> Add the write request of the 9pfs protocol.
> >>
> >> Signed-off-by: Juergen Gross
> >> ---
> >> tools/xenlogd/io.c | 50 ++
> >> 1 file changed, 50 insertions(+)
> >>
> >> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> >> index 6e92667fab..6b4692ca67 100644
> >> --- a/tools/xenlogd/io.c
> >> +++ b/tools/xenlogd/io.c
> >
> >> @@ -1010,6 +1011,51 @@ static void p9_create(device *device, struct
> >> p9_header *hdr)
> >> fill_buffer(device, hdr->cmd + 1, hdr->tag, "QU", , );
> >> }
> >>
> >> +static void p9_write(device *device, struct p9_header *hdr)
> >> +{
> >> +uint32_t fid;
> >> +uint64_t off;
> >> +unsigned int len;
> >> +uint32_t written;
> >> +void *buf;
> >> +struct p9_fid *fidp;
> >> +int ret;
> >> +
> >> +ret = fill_data(device, "ULD", , , , device->buffer);
> >> +if ( ret != 3 )
> >> +{
> >> +p9_error(device, hdr->tag, EINVAL);
> >> +return;
> >> +}
> >> +
> >> +fidp = find_fid(device, fid);
> >> +if ( !fidp || !fidp->opened || fidp->isdir )
> >
> > I think you want an additional check that the fidp is writable.
>
> The open was done with the correct mode. If fidp isn't writable, the write()
> will fail with the correct errno.
Oh, right.
Reviewed-by: Jason Andryuk
[XEN PATCH v3] xen/string: address violations of MISRA C:2012 Rules 8.2 and 8.3
Add missing parameter names and make function declarations and
definitions consistent.
Mismatches between parameter names "count" and "n" are resolved
in favor of "n", being the same name used by the C standard.
No functional change.
Signed-off-by: Federico Serafini
---
Changes in v3:
- applied changes discussed in the following thread
https://lists.xenproject.org/archives/html/xen-devel/2023-08/msg00318.html
Changes in v2:
- memset() adjusted.
---
xen/include/xen/string.h | 42
xen/lib/memcpy.c | 6 +++---
xen/lib/memmove.c| 12 ++--
xen/lib/memset.c | 6 +++---
4 files changed, 33 insertions(+), 33 deletions(-)
diff --git a/xen/include/xen/string.h b/xen/include/xen/string.h
index b4d2217a96..bd4a8f48e9 100644
--- a/xen/include/xen/string.h
+++ b/xen/include/xen/string.h
@@ -12,27 +12,27 @@
#define strncpy __xen_has_no_strncpy__
#define strncat __xen_has_no_strncat__
-size_t strlcpy(char *, const char *, size_t);
-size_t strlcat(char *, const char *, size_t);
-int strcmp(const char *, const char *);
-int strncmp(const char *, const char *, size_t);
-int strcasecmp(const char *, const char *);
-int strncasecmp(const char *, const char *, size_t);
-char *strchr(const char *, int);
-char *strrchr(const char *, int);
-char *strstr(const char *, const char *);
-size_t strlen(const char *);
-size_t strnlen(const char *, size_t);
-char *strpbrk(const char *, const char *);
-char *strsep(char **, const char *);
-size_t strspn(const char *, const char *);
-
-void *memset(void *, int, size_t);
-void *memcpy(void *, const void *, size_t);
-void *memmove(void *, const void *, size_t);
-int memcmp(const void *, const void *, size_t);
-void *memchr(const void *, int, size_t);
-void *memchr_inv(const void *, int, size_t);
+size_t strlcpy(char *dest, const char *src, size_t size);
+size_t strlcat(char *dest, const char *src, size_t size);
+int strcmp(const char *cs, const char *ct);
+int strncmp(const char *cs, const char *ct, size_t count);
+int strcasecmp(const char *s1, const char *s2);
+int strncasecmp(const char *s1, const char *s2, size_t len);
+char *strchr(const char *s, int c);
+char *strrchr(const char *s, int c);
+char *strstr(const char *s1, const char *s2);
+size_t strlen(const char *s);
+size_t strnlen(const char *s, size_t count);
+char *strpbrk(const char *cs,const char *ct);
+char *strsep(char **s, const char *ct);
+size_t strspn(const char *s, const char *accept);
+
+void *memset(void *s, int c, size_t n);
+void *memcpy(void *dest, const void *src, size_t n);
+void *memmove(void *dest, const void *src, size_t n);
+int memcmp(const void *cs, const void *ct, size_t count);
+void *memchr(const void *s, int c, size_t n);
+void *memchr_inv(const void *s, int c, size_t n);
#include
diff --git a/xen/lib/memcpy.c b/xen/lib/memcpy.c
index afb322797d..5476121c0d 100644
--- a/xen/lib/memcpy.c
+++ b/xen/lib/memcpy.c
@@ -8,16 +8,16 @@
* memcpy - Copy one area of memory to another
* @dest: Where to copy to
* @src: Where to copy from
- * @count: The size of the area.
+ * @n: The size of the area.
*
* You should not use this function to access IO space, use memcpy_toio()
* or memcpy_fromio() instead.
*/
-void *(memcpy)(void *dest, const void *src, size_t count)
+void *(memcpy)(void *dest, const void *src, size_t n)
{
char *tmp = (char *) dest, *s = (char *) src;
- while (count--)
+ while (n--)
*tmp++ = *s++;
return dest;
diff --git a/xen/lib/memmove.c b/xen/lib/memmove.c
index 1ab79dfb28..99804352e6 100644
--- a/xen/lib/memmove.c
+++ b/xen/lib/memmove.c
@@ -8,23 +8,23 @@
* memmove - Copy one area of memory to another
* @dest: Where to copy to
* @src: Where to copy from
- * @count: The size of the area.
+ * @n: The size of the area.
*
* Unlike memcpy(), memmove() copes with overlapping areas.
*/
-void *(memmove)(void *dest, const void *src, size_t count)
+void *(memmove)(void *dest, const void *src, size_t n)
{
char *tmp, *s;
if (dest <= src) {
tmp = (char *) dest;
s = (char *) src;
- while (count--)
+ while (n--)
*tmp++ = *s++;
} else {
- tmp = (char *) dest + count;
- s = (char *) src + count;
- while (count--)
+ tmp = (char *) dest + n;
+ s = (char *) src + n;
+ while (n--)
*--tmp = *--s;
}
diff --git a/xen/lib/memset.c b/xen/lib/memset.c
index e86afafd02..48a072cb51 100644
--- a/xen/lib/memset.c
+++ b/xen/lib/memset.c
@@ -8,15 +8,15 @@
* memset - Fill a region of memory with the given value
* @s: Pointer to the start of the area.
* @c: The byte to fill the area with
- * @count: The size of the area.
+ * @n: The size of the area.
*
* Do not use memset() to access IO space, use memset_io() instead.
*/
-void
[xen-unstable-smoke test] 183705: tolerable all pass - PUSHED
flight 183705 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/183705/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen fab51099a1cdb6bfe5127b14a5d41c246ea1a2c7 baseline version: xen de1cc5102b487e1a4bf321ac138b64c6ce1f0c0a Last test of basis 183697 2023-11-06 23:00:27 Z0 days Testing same since 183705 2023-11-07 11:00:26 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Henry Wang Michal Orzel Roger Pau Monne Roger Pau Monné jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git de1cc5102b..fab51099a1 fab51099a1cdb6bfe5127b14a5d41c246ea1a2c7 -> smoke
Re: [PATCH 14/29] tools/xenlogd: add 9pfs read request support
On 07.11.23 15:31, Jason Andryuk wrote:
On Wed, Nov 1, 2023 at 5:35 AM Juergen Gross wrote:
Add the read request of the 9pfs protocol.
For now support only reading plain files (no directories).
Signed-off-by: Juergen Gross
---
tools/xenlogd/io.c | 60 ++
1 file changed, 60 insertions(+)
diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
index 6b4692ca67..b3f9f96bcc 100644
--- a/tools/xenlogd/io.c
+++ b/tools/xenlogd/io.c
@@ -1011,6 +1012,61 @@ static void p9_create(device *device, struct p9_header
*hdr)
fill_buffer(device, hdr->cmd + 1, hdr->tag, "QU", , );
}
+static void p9_read(device *device, struct p9_header *hdr)
+{
+uint32_t fid;
+uint64_t off;
+unsigned int len;
+uint32_t count;
+void *buf;
+struct p9_fid *fidp;
+int ret;
+
+ret = fill_data(device, "ULU", , , );
+if ( ret != 3 )
+{
+p9_error(device, hdr->tag, EINVAL);
+return;
+}
+
+fidp = find_fid(device, fid);
+if ( !fidp || !fidp->opened )
+{
+p9_error(device, hdr->tag, EBADF);
+return;
+}
+
I think you want to mode check here for readability.
Same reasoning as for the write case: read() will do it for me.
+if ( fidp->isdir )
+{
+p9_error(device, hdr->tag, EOPNOTSUPP);
+return;
Hmmm, 9P2000.u supports read on a dir.
https://ericvh.github.io/9p-rfc/rfc9p2000.html#anchor30
"""
For directories, read returns an integral number of direc- tory
entries exactly as in stat (see stat(5)), one for each member of the
directory. The read request message must have offset equal to zero or
the value of offset in the previous read on the directory, plus the
number of bytes returned in the previous read. In other words, seeking
other than to the beginning is illegal in a directory (see seek(2)).
"""
This is part of "only operations needed for Xenstore-stubdom are implemented."
For supporting Linux guests or other stubdoms directory reading must be added,
of course.
+}
+else
Since the above is a return, maybe remove the else and un-indent?
Though maybe you don't want to do that if you want to implement read
on a dir.
Correct.
+{
+len = count;
+buf = device->buffer + sizeof(*hdr) + sizeof(uint32_t);
+
+while ( len != 0 )
+{
+ret = pread(fidp->fd, buf, len, off);
+if ( ret <= 0 )
+break;
+len -= ret;
+buf += ret;
+off += ret;
+}
+if ( ret && len == count )
This seems wrong to me - should this be ( ret < 0 && len == count ) to
indicate an error on the first pread? Any partial reads would still
return their data?
If len == count there was no partial read, as this would have reduced len.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH 12/29] tools/xenlogd: add 9pfs stat request support
On Tue, Nov 7, 2023 at 9:42 AM Juergen Gross wrote:
>
> On 07.11.23 15:04, Jason Andryuk wrote:
> > On Wed, Nov 1, 2023 at 5:34 AM Juergen Gross wrote:
> >>
> >> Add the stat request of the 9pfs protocol.
> >>
> >> Signed-off-by: Juergen Gross
> >> ---
> >> tools/xenlogd/io.c | 89 ++
> >> 1 file changed, 89 insertions(+)
> >>
> >> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> >> index 34f137be1b..6e92667fab 100644
> >> --- a/tools/xenlogd/io.c
> >> +++ b/tools/xenlogd/io.c
> >> @@ -33,6 +33,7 @@
> >
> >> +static void fill_p9_stat(struct p9_stat *p9s, struct stat *st, const char
> >> *name)
> >> +{
> >> +memset(p9s, 0, sizeof(*p9s));
> >> +fill_qid(NULL, >qid, st);
> >> +p9s->mode = st->st_mode & 0777;
> >> +if ( S_ISDIR(st->st_mode) )
> >> +p9s->mode |= P9_CREATE_PERM_DIR;
> >> +p9s->atime = st->st_atime;
> >> +p9s->mtime = st->st_mtime;
> >> +p9s->length = st->st_size;
> >> +p9s->name = name;
> >> +p9s->uid = "";
> >> +p9s->gid = "";
> >> +p9s->muid = "";
> >> +p9s->extension = "";
> >> +p9s->n_uid = 0;
> >> +p9s->n_gid = 0;
> >
> > If the daemon is running as root and managing the directories, these
> > probably match. Still, do we want uid & gid to be populated from the
> > stat struct?
>
> I wouldn't want to do that. In the end the permissions of the daemon are
> relevant for being able to access the files. There is no need to leak any
> uids and gids from the host to the guests.
Ok.
> >
> >> +p9s->n_muid = 0;
> >> +
> >> +/*
> >> + * Size of individual fields without the size field, including 5
> >> 2-byte
> >> + * string length fields.
> >> + */
> >> +p9s->size = 71 + strlen(p9s->name);
> >> +}
> >> +
> >> +static void p9_stat(device *device, struct p9_header *hdr)
> >> +{
> >> +uint32_t fid;
> >> +struct p9_fid *fidp;
> >> +struct p9_stat p9s;
> >> +struct stat st;
> >> +uint16_t total_length;
> >
> > total_length = 0;
> >
> > Otherwise it is used uninitialized.
>
> I don't think so. There is a single user just after setting the variable.
Whoops - you are right. Sorry for the noise.
Regards,
Jason
Re: [RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
Hi Julien,
On 2023-11-07 13:44, Julien Grall wrote:
Hi Nicola,
On 07/11/2023 10:33, Nicola Vetrini wrote:
As explained in the deviation record, code constructs such as
"goto retry" and "goto again" are sometimes the best balance between
code complexity and the understandability of the control flow
by developers; as such, these construct are allowed to deviate
from Rule 15.2.
Signed-off-by: Nicola Vetrini
---
automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++
docs/misra/deviations.rst| 10 ++
2 files changed, 20 insertions(+)
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a27..8b1f622f8f82 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -270,6 +270,16 @@ statements are deliberate"
-config=MC3R1.R14.3,statements={deliberate ,
"wrapped(any(),node(if_stmt))" }
-doc_end
+#
+# Series 15
+#
+
+-doc_begin="The additional complexity introduced in the code by using
control flow structures other than backwards goto-s
+were deemed not to justify the possible prevention of developer
confusion, given the very torough review process estabilished
Typoes: s/torough/thorough/ s/estabilished/established/
Thanks
+in the community."
+-config=MC3R1.R15.2,reports+={deliberate,
"any_area(any_loc(text(^.*goto (again|retry).*$)))"}
+-doc_end
+
#
# Series 20.
#
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..7d1e1f0d09b3 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
statements are deliberate.
- Project-wide deviation; tagged as `disapplied` for ECLAIR.
+ * - R15.2
+ - The possible prevention of developer confusion as a result of
using
+ control flow structures other than backwards goto-s in some
instances was
+ deemed not strong enough to justify the additional complexity
introduced
+ in the code. Such instances are the uses of the following
labels:
+
+ - again
+ - retry
Have you investigated the possibility to use SAF-X in the code? If so,
what's the problem to use them?
Cheers,
This is another viable option: putting the SAF comment on top of the
label should suffice,
as shown below:
/* SAF-2-safe */
repeat:
++fmt; /* this also skips first '%' */
switch (*fmt) {
case '-': flags |= LEFT; goto repeat;
case '+': flags |= PLUS; goto repeat;
case ' ': flags |= SPACE; goto repeat;
case '#': flags |= SPECIAL; goto repeat;
case '0': flags |= ZEROPAD; goto repeat;
}
I think it ultimately boils down to whether Xen wants to promote the use
of certain labels
as the designated alternative when no other control flow mechanism is
clearer from a
readability perspective (in which case there should be a consistent
naming to capture and deviate
all of them, such as "retry") or do so on a case-by-case basis with a
SAF, which is ok, but then
it needs someone to check each one and either fix them or mark them as
ok.
Yet another route could be to mark with a SAF all those present right
now to establish a baseline.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [PATCH 13/29] tools/xenlogd: add 9pfs write request support
On 07.11.23 15:10, Jason Andryuk wrote:
On Wed, Nov 1, 2023 at 5:54 AM Juergen Gross wrote:
Add the write request of the 9pfs protocol.
Signed-off-by: Juergen Gross
---
tools/xenlogd/io.c | 50 ++
1 file changed, 50 insertions(+)
diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
index 6e92667fab..6b4692ca67 100644
--- a/tools/xenlogd/io.c
+++ b/tools/xenlogd/io.c
@@ -1010,6 +1011,51 @@ static void p9_create(device *device, struct p9_header
*hdr)
fill_buffer(device, hdr->cmd + 1, hdr->tag, "QU", , );
}
+static void p9_write(device *device, struct p9_header *hdr)
+{
+uint32_t fid;
+uint64_t off;
+unsigned int len;
+uint32_t written;
+void *buf;
+struct p9_fid *fidp;
+int ret;
+
+ret = fill_data(device, "ULD", , , , device->buffer);
+if ( ret != 3 )
+{
+p9_error(device, hdr->tag, EINVAL);
+return;
+}
+
+fidp = find_fid(device, fid);
+if ( !fidp || !fidp->opened || fidp->isdir )
I think you want an additional check that the fidp is writable.
The open was done with the correct mode. If fidp isn't writable, the write()
will fail with the correct errno.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH 12/29] tools/xenlogd: add 9pfs stat request support
On 07.11.23 15:04, Jason Andryuk wrote:
On Wed, Nov 1, 2023 at 5:34 AM Juergen Gross wrote:
Add the stat request of the 9pfs protocol.
Signed-off-by: Juergen Gross
---
tools/xenlogd/io.c | 89 ++
1 file changed, 89 insertions(+)
diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
index 34f137be1b..6e92667fab 100644
--- a/tools/xenlogd/io.c
+++ b/tools/xenlogd/io.c
@@ -33,6 +33,7 @@
+static void fill_p9_stat(struct p9_stat *p9s, struct stat *st, const char
*name)
+{
+memset(p9s, 0, sizeof(*p9s));
+fill_qid(NULL, >qid, st);
+p9s->mode = st->st_mode & 0777;
+if ( S_ISDIR(st->st_mode) )
+p9s->mode |= P9_CREATE_PERM_DIR;
+p9s->atime = st->st_atime;
+p9s->mtime = st->st_mtime;
+p9s->length = st->st_size;
+p9s->name = name;
+p9s->uid = "";
+p9s->gid = "";
+p9s->muid = "";
+p9s->extension = "";
+p9s->n_uid = 0;
+p9s->n_gid = 0;
If the daemon is running as root and managing the directories, these
probably match. Still, do we want uid & gid to be populated from the
stat struct?
I wouldn't want to do that. In the end the permissions of the daemon are
relevant for being able to access the files. There is no need to leak any
uids and gids from the host to the guests.
+p9s->n_muid = 0;
+
+/*
+ * Size of individual fields without the size field, including 5 2-byte
+ * string length fields.
+ */
+p9s->size = 71 + strlen(p9s->name);
+}
+
+static void p9_stat(device *device, struct p9_header *hdr)
+{
+uint32_t fid;
+struct p9_fid *fidp;
+struct p9_stat p9s;
+struct stat st;
+uint16_t total_length;
total_length = 0;
Otherwise it is used uninitialized.
I don't think so. There is a single user just after setting the variable.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature.asc
Description: OpenPGP digital signature
Re: [PATCH 14/29] tools/xenlogd: add 9pfs read request support
On Wed, Nov 1, 2023 at 5:35 AM Juergen Gross wrote:
>
> Add the read request of the 9pfs protocol.
>
> For now support only reading plain files (no directories).
>
> Signed-off-by: Juergen Gross
> ---
> tools/xenlogd/io.c | 60 ++
> 1 file changed, 60 insertions(+)
>
> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> index 6b4692ca67..b3f9f96bcc 100644
> --- a/tools/xenlogd/io.c
> +++ b/tools/xenlogd/io.c
> @@ -1011,6 +1012,61 @@ static void p9_create(device *device, struct p9_header
> *hdr)
> fill_buffer(device, hdr->cmd + 1, hdr->tag, "QU", , );
> }
>
> +static void p9_read(device *device, struct p9_header *hdr)
> +{
> +uint32_t fid;
> +uint64_t off;
> +unsigned int len;
> +uint32_t count;
> +void *buf;
> +struct p9_fid *fidp;
> +int ret;
> +
> +ret = fill_data(device, "ULU", , , );
> +if ( ret != 3 )
> +{
> +p9_error(device, hdr->tag, EINVAL);
> +return;
> +}
> +
> +fidp = find_fid(device, fid);
> +if ( !fidp || !fidp->opened )
> +{
> +p9_error(device, hdr->tag, EBADF);
> +return;
> +}
> +
I think you want to mode check here for readability.
> +if ( fidp->isdir )
> +{
> +p9_error(device, hdr->tag, EOPNOTSUPP);
> +return;
Hmmm, 9P2000.u supports read on a dir.
https://ericvh.github.io/9p-rfc/rfc9p2000.html#anchor30
"""
For directories, read returns an integral number of direc- tory
entries exactly as in stat (see stat(5)), one for each member of the
directory. The read request message must have offset equal to zero or
the value of offset in the previous read on the directory, plus the
number of bytes returned in the previous read. In other words, seeking
other than to the beginning is illegal in a directory (see seek(2)).
"""
> +}
> +else
Since the above is a return, maybe remove the else and un-indent?
Though maybe you don't want to do that if you want to implement read
on a dir.
> +{
> +len = count;
> +buf = device->buffer + sizeof(*hdr) + sizeof(uint32_t);
> +
> +while ( len != 0 )
> +{
> +ret = pread(fidp->fd, buf, len, off);
> +if ( ret <= 0 )
> +break;
> +len -= ret;
> +buf += ret;
> +off += ret;
> +}
> +if ( ret && len == count )
This seems wrong to me - should this be ( ret < 0 && len == count ) to
indicate an error on the first pread? Any partial reads would still
return their data?
Regards,
Jason
> +{
> +p9_error(device, hdr->tag, errno);
> +return;
> +}
> +
> +buf = device->buffer + sizeof(*hdr) + sizeof(uint32_t);
> +len = count - len;
> +fill_buffer(device, hdr->cmd + 1, hdr->tag, "D", , buf);
> +}
> +}
> +
> static void p9_write(device *device, struct p9_header *hdr)
> {
> uint32_t fid;
Re: Informal voting proposal
Thanks for the feedback Julien, see my reply below. Many thanks, Kelly Choi Open Source Community Manager XenServer, Cloud Software Group On Tue, Nov 7, 2023 at 8:23 AM Julien Grall wrote: > Hi Stefano, > > On 07/11/2023 04:18, Stefano Stabellini wrote: > > On Mon, 6 Nov 2023, Julien Grall wrote: > >> Hi Kelly, > >> > >> On 06/11/2023 16:40, Kelly Choi wrote: > >>> Hi all, > >>> > >>> As an open-source community, there will always be differences of > opinion in > >>> approaches and the way we think. It is imperative, however, that we > view > >>> this diversity as a source of strength rather than a hindrance. > >>> > >>> Recent deliberations within our project have led to certain matters > being > >>> put on hold due to an inability to reach a consensus. While formal > voting > >>> procedures serve their purpose, they can be time-consuming and may not > >>> always lead to meaningful progress. > >>> > >>> Having received agreement from a few maintainers already, I would like > to > >>> propose the following: > >> > >> Thanks for the sending the proposal. While I like the idea of informal > vote to > >> move faster, I would like to ask some clarifications. > >> > >>> *Informal voting method:* > >>> > >>> 1. Each project should ideally have more than 2 maintainers to > >>> facilitate impartial discussions. Projects lacking this > configuration > >>> will > >>> be addressed at a later stage. > >>> 2. Anyone in the community is welcome to voice their opinions, > ideas, > >>> and concerns about any patch or contribution. > >>> 3. If members cannot agree, the majority informal vote of the > >>> maintainers will be the decision that stands. For instance, if, > after > >>> careful consideration of all suggestions and concerns, 2 out of 3 > >>> maintainers endorse a solution within the x86 subsystem, it shall > be the > >>> decision we move forward with. > >> > >> How do you know that all the options have been carefully considered? > > > > I don't think there is a hard rule on this. We follow the discussion on > > the list the same way as we do today. > > To me the fact we need to write down the informal rules means that > something already gone wrong before. So I feel that rules should be > unambiguous to avoid any problem afterwards. > In this case 'all options' would mean the different choices that maintainers have discussed and considered, before calling an informal vote. The reason for the informal vote is that 'all options' have been considered, but a decision can not be made. Whilst there can be many options, the informal voting method aims to reduce this by enabling maintainers to call a vote and propose two options, so the project can move forward. Again there will be times for that call for flexibility, but we should always aim to have a vote for two of the best solutions to avoid the project coming to another standstill. > > > > > While I like the informal vote proposal and effectively we have already > > been following it in many areas of the project, I don't think we should > > change the current processes from that point of view. > > I am confused. Are you suggesting that we should not write down how > informal votes works? > I cannot speak for Stefano, but the informal vote process should be written down as an 'aspirational guideline' meaning it's a process we ought to follow in the best interest of the project. The formal voting process will still be in place. > > > > > > >>> 4. Naturally, there may be exceptional circumstances, as such, a > formal > >>> vote may be warranted but should happen only a few times a year > for > >>> serious > >>> cases only. > >> Similarly here, you are suggesting that a formal vote can be called. > But it is > >> not super clear what would be the condition it could be used and how it > can be > >> called. > > > > The formal vote is basically the same we already have today. It would > > follow the existing voting rules and guidelines already part of the > > governance. > > Reading through the governance, I couldn't find anywhere indicating in > which condition the formal votes can be triggered. Hence my question here. > There's a difficulty here in the sense that there isn't a specific guideline around what condition a formal vote can be triggered. Until that guideline is implemented, reviewed, and updated, I would suggest that a formal vote is called only in cases where serious damage would come to the project and the community. Again, this would be down to reasonable judgement so I would trust committers/maintainers on this, and should happen less than a few times a year. > > >> For instance, per the informal rule, if 2 out of 3 maintainers accept. > Then it > >> would be sensible for the patch to be merged as soon as the 5 days > windows is > >> closed. Yet the 3rd maintainer technically object it. So could that > maintainer > >> request a formal vote? If so, how long do they have? > > > >
Re: [PATCH 13/29] tools/xenlogd: add 9pfs write request support
On Wed, Nov 1, 2023 at 5:54 AM Juergen Gross wrote:
>
> Add the write request of the 9pfs protocol.
>
> Signed-off-by: Juergen Gross
> ---
> tools/xenlogd/io.c | 50 ++
> 1 file changed, 50 insertions(+)
>
> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> index 6e92667fab..6b4692ca67 100644
> --- a/tools/xenlogd/io.c
> +++ b/tools/xenlogd/io.c
> @@ -1010,6 +1011,51 @@ static void p9_create(device *device, struct p9_header
> *hdr)
> fill_buffer(device, hdr->cmd + 1, hdr->tag, "QU", , );
> }
>
> +static void p9_write(device *device, struct p9_header *hdr)
> +{
> +uint32_t fid;
> +uint64_t off;
> +unsigned int len;
> +uint32_t written;
> +void *buf;
> +struct p9_fid *fidp;
> +int ret;
> +
> +ret = fill_data(device, "ULD", , , , device->buffer);
> +if ( ret != 3 )
> +{
> +p9_error(device, hdr->tag, EINVAL);
> +return;
> +}
> +
> +fidp = find_fid(device, fid);
> +if ( !fidp || !fidp->opened || fidp->isdir )
I think you want an additional check that the fidp is writable.
Regards,
Jason
Re: [PATCH 12/29] tools/xenlogd: add 9pfs stat request support
On Wed, Nov 1, 2023 at 5:34 AM Juergen Gross wrote:
>
> Add the stat request of the 9pfs protocol.
>
> Signed-off-by: Juergen Gross
> ---
> tools/xenlogd/io.c | 89 ++
> 1 file changed, 89 insertions(+)
>
> diff --git a/tools/xenlogd/io.c b/tools/xenlogd/io.c
> index 34f137be1b..6e92667fab 100644
> --- a/tools/xenlogd/io.c
> +++ b/tools/xenlogd/io.c
> @@ -33,6 +33,7 @@
> +static void fill_p9_stat(struct p9_stat *p9s, struct stat *st, const char
> *name)
> +{
> +memset(p9s, 0, sizeof(*p9s));
> +fill_qid(NULL, >qid, st);
> +p9s->mode = st->st_mode & 0777;
> +if ( S_ISDIR(st->st_mode) )
> +p9s->mode |= P9_CREATE_PERM_DIR;
> +p9s->atime = st->st_atime;
> +p9s->mtime = st->st_mtime;
> +p9s->length = st->st_size;
> +p9s->name = name;
> +p9s->uid = "";
> +p9s->gid = "";
> +p9s->muid = "";
> +p9s->extension = "";
> +p9s->n_uid = 0;
> +p9s->n_gid = 0;
If the daemon is running as root and managing the directories, these
probably match. Still, do we want uid & gid to be populated from the
stat struct?
> +p9s->n_muid = 0;
> +
> +/*
> + * Size of individual fields without the size field, including 5 2-byte
> + * string length fields.
> + */
> +p9s->size = 71 + strlen(p9s->name);
> +}
> +
> +static void p9_stat(device *device, struct p9_header *hdr)
> +{
> +uint32_t fid;
> +struct p9_fid *fidp;
> +struct p9_stat p9s;
> +struct stat st;
> +uint16_t total_length;
total_length = 0;
Otherwise it is used uninitialized.
Regards,
Jason
Re: [PULL 00/15] xenfv.for-upstream queue
Applied, thanks. Please update the changelog at https://wiki.qemu.org/ChangeLog/8.2 for any user-visible changes. signature.asc Description: PGP signature
Re: [RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
Hi Nicola,
On 07/11/2023 10:33, Nicola Vetrini wrote:
As explained in the deviation record, code constructs such as
"goto retry" and "goto again" are sometimes the best balance between
code complexity and the understandability of the control flow
by developers; as such, these construct are allowed to deviate
from Rule 15.2.
Signed-off-by: Nicola Vetrini
---
automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++
docs/misra/deviations.rst| 10 ++
2 files changed, 20 insertions(+)
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a27..8b1f622f8f82 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -270,6 +270,16 @@ statements are deliberate"
-config=MC3R1.R14.3,statements={deliberate , "wrapped(any(),node(if_stmt))" }
-doc_end
+#
+# Series 15
+#
+
+-doc_begin="The additional complexity introduced in the code by using control
flow structures other than backwards goto-s
+were deemed not to justify the possible prevention of developer confusion,
given the very torough review process estabilished
Typoes: s/torough/thorough/ s/estabilished/established/
+in the community."
+-config=MC3R1.R15.2,reports+={deliberate, "any_area(any_loc(text(^.*goto
(again|retry).*$)))"}
+-doc_end
+
#
# Series 20.
#
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..7d1e1f0d09b3 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
statements are deliberate.
- Project-wide deviation; tagged as `disapplied` for ECLAIR.
+ * - R15.2
+ - The possible prevention of developer confusion as a result of using
+ control flow structures other than backwards goto-s in some instances
was
+ deemed not strong enough to justify the additional complexity introduced
+ in the code. Such instances are the uses of the following labels:
+
+ - again
+ - retry
Have you investigated the possibility to use SAF-X in the code? If so,
what's the problem to use them?
Cheers,
--
Julien Grall
Re: [XEN PATCH][for-4.19 v2] xen: replace occurrences of SAF-1-safe with asmlinkage attribute
Hi Nicola, On 07/11/2023 10:30, Nicola Vetrini wrote: The comment-based justifications for MISRA C:2012 Rule 8.4 are replaced by the asmlinkage pseudo-attribute, for the sake of uniformity. Add missing 'xen/compiler.h' #include-s where needed. The text in docs/misra/deviations.rst and docs/misra/safe.json is modified to reflect this change. Signed-off-by: Nicola Vetrini With one note below: Acked-by: Julien Grall --- Changes in v2: - Edit safe.json. - Remove mention of SAF-1-safe in deviations.rst. --- docs/misra/deviations.rst | 5 ++--- docs/misra/safe.json| 2 +- xen/arch/arm/cpuerrata.c| 7 +++ xen/arch/arm/setup.c| 5 ++--- xen/arch/arm/smpboot.c | 3 +-- xen/arch/arm/traps.c| 21 +++-- xen/arch/x86/boot/cmdline.c | 5 +++-- xen/arch/x86/boot/reloc.c | 7 --- xen/arch/x86/extable.c | 3 +-- xen/arch/x86/setup.c| 3 +-- xen/arch/x86/traps.c| 27 +-- xen/common/efi/boot.c | 5 ++--- 12 files changed, 36 insertions(+), 57 deletions(-) diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst index d468da2f5ce9..0fa0475db0eb 100644 --- a/docs/misra/deviations.rst +++ b/docs/misra/deviations.rst @@ -134,9 +134,8 @@ Deviations related to MISRA C:2012 Rules: - Tagged as `safe` for ECLAIR. * - R8.4 - - Functions and variables used only by asm modules are either marked with - the `asmlinkage` macro or with a SAF-1-safe textual deviation - (see safe.json). This will require a rebase on top https://lore.kernel.org/all/b914ac93-2499-4bfd-a60a-51a9f1c17...@xen.org/ with the updated wording. Also, it is a good idea to mention any dependency with any patches that are not yet in 'staging' (The for-next branch from Stefano doesn't count). This helps the committers to know which order the patches should be committed and also the reviewers to apply the patches for review. Cheers, -- Julien Grall
Re: [RFC PATCH 3/4] xen/arm: GICv3: address MISRA C:2012 Rule 15.2
Hi Nicola,
On 07/11/2023 10:33, Nicola Vetrini wrote:
The backwards jump due to the "goto retry;" statement
can be transformed into a loop, without losing much in terms
of readability.
Signed-off-by: Stefano Stabellini
Signed-off-by: Nicola Vetrini
---
This specific patch was provided by Stefano, I just added the
commit message.
If that's the case, then Stefano should be the Author (at the moment
this is you).
---
xen/arch/arm/gic-v3-its.c | 81 ---
1 file changed, 42 insertions(+), 39 deletions(-)
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 8afcd9783bc8..4ba3f869ddf2 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -341,6 +341,7 @@ static int its_map_baser(void __iomem *basereg, uint64_t
regc,
unsigned int pagesz = 2;/* try 64K pages first, then go down. */
unsigned int table_size;
void *buffer;
+bool retry = false;
retry is false so...
attr = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
attr |= GIC_BASER_CACHE_SameAsInner <<
GITS_BASER_OUTER_CACHEABILITY_SHIFT;
@@ -351,55 +352,57 @@ static int its_map_baser(void __iomem *basereg, uint64_t
regc,
* it back and see what sticks (page size, cacheability and shareability
* attributes), retrying if necessary.
*/
-retry:
-table_size = ROUNDUP(nr_items * entry_size,
- BIT(BASER_PAGE_BITS(pagesz), UL));
-/* The BASE registers support at most 256 pages. */
-table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
+while ( retry )
... you will never end in the loop. I also think that name 'retry' is
confusing as the first time is not retry.
It would be clearer if we use a
do {
} while (retry)
That said, I actually prefer the goto version because some of the lines
within the loop are now over 80 characters and splitting them would make
the code harder to read.
Below an example, with the new indentation it is just over 80 characters.
if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )
One possibly would be to move the logic within the loop in a separate
function.
Cheers,
--
Julien Grall
Re: [PATCH v3 2/2] xen/arm32: head Split and move MMU-specific head.S to mmu/head.S
Hi Ayan,
On 07/11/2023 12:02, Ayan Kumar Halder wrote:
>
>
> The MMU specific code in head.S will not be used on MPU systems.
> Instead of introducing more #ifdefs which will bring complexity
> to the code, move MMU related code to mmu/head.S and keep common
> code in head.S. Two notes while moving:
> - As "fail" in original head.S is very simple and this name is too
>easy to be conflicted, duplicate it in mmu/head.S instead of
>exporting it.
> - Realigned ".macro ret" so that the alignment matches to the other
>macros.
> - Rename puts to asm_puts, putn to asm_putn (this denotes that the
>macros are used within the context of assembly only).
> - Use ENTRY() for enable_secondary_cpu_mm, enable_boot_cpu_mm,
>setup_fixmap, asm_puts, asm_putn as they will be used externally.
>
> Also move the assembly macros shared by head.S and mmu/head.S to
> macros.h.
>
> This is based on 6734327d76be ("xen/arm64: Split and move MMU-specific head.S
> to mmu/head.S").
>
> Signed-off-by: Ayan Kumar Halder
For now, I won't provide Rb given that the baseline is still under development
and not very clear to me.
Just a few remarks:
[...]
> -
> #ifdef CONFIG_EARLY_PRINTK
> /*
> * Initialize the UART. Should only be called on the boot CPU.
> @@ -912,14 +298,14 @@ ENDPROC(init_uart)
> * r11: Early UART base address
You could follow the arm64 patch and add:
"Note: This function must be called from assembly."
to make the usage of this function clear.
Same for asm_putn.
> * Clobbers r0-r1
> */
> -puts:
> +ENTRY(asm_puts)
> early_uart_ready r11, r1
> ldrb r1, [r0], #1 /* Load next char */
> teq r1, #0 /* Exit on nul */
> moveq pc, lr
> early_uart_transmit r11, r1
> -b puts
> -ENDPROC(puts)
> +b asm_puts
> +ENDPROC(asm_puts)
>
> /*
> * Print a 32-bit number in hex. Specific to the PL011 UART.
The second sentence can be dropped. I don't see anything PL011 specific here.
> @@ -927,7 +313,7 @@ ENDPROC(puts)
> * r11: Early UART base address
> * Clobbers r0-r3
> */
> -putn:
> +ENTRY(asm_putn)
> adr_l r1, hex
> mov r3, #8
> 1:
> @@ -939,7 +325,7 @@ putn:
> subs r3, r3, #1
> bne 1b
> mov pc, lr
> -ENDPROC(putn)
> +ENDPROC(asm_putn)
>
> RODATA_STR(hex, "0123456789abcdef")
>
> @@ -947,8 +333,8 @@ RODATA_STR(hex, "0123456789abcdef")
>
> ENTRY(early_puts)
> init_uart:
> -puts:
> -putn: mov pc, lr
> +asm_puts:
> +asm_putn: mov pc, lr
Both asm_puts and asm_putn are used only within #ifdef so you can drop the stubs
[...]
> diff --git a/xen/arch/arm/include/asm/arm32/macros.h
> b/xen/arch/arm/include/asm/arm32/macros.h
> index a4e20aa520..c6e390cc5f 100644
> --- a/xen/arch/arm/include/asm/arm32/macros.h
> +++ b/xen/arch/arm/include/asm/arm32/macros.h
> @@ -1,8 +1,62 @@
> #ifndef __ASM_ARM_ARM32_MACROS_H
> #define __ASM_ARM_ARM32_MACROS_H
>
> -.macro ret
> +.macro ret
> mov pc, lr
> -.endm
> +.endm
>
> +/*
> + * Move an immediate constant into a 32-bit register using movw/movt
> + * instructions.
> + */
> +.macro mov_w reg, word
> +movw \reg, #:lower16:\word
> +movt \reg, #:upper16:\word
> +.endm
> +
> +/*
> + * Pseudo-op for PC relative adr , where is
> + * within the range +/- 4GB of the PC.
> + *
> + * @dst: destination register
> + * @sym: name of the symbol
> + */
> +.macro adr_l, dst, sym
> +mov_w \dst, \sym - .Lpc\@
> +.set .Lpc\@, .+ 8 /* PC bias */
> +add \dst, \dst, pc
> +.endm
> +
> +#ifdef CONFIG_EARLY_PRINTK
> +/*
> + * Macro to print a string to the UART, if there is one.
> + *
> + * Clobbers r0 - r3
> + */
> +#define PRINT(_s) \
> +mov r3, lr ;\
> +adr_l r0, 98f ;\
> +blputs ;\
This should be a call to asm_puts
~Michal
Re: [RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop
On 07/11/2023 10:33 am, Nicola Vetrini wrote:
> The backwards goto in the vsnprintf function can be replaced
> with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2.
>
> Signed-off-by: Nicola Vetrini
> ---
> xen/common/vsprintf.c | 20
> 1 file changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
> index c49631c0a4d8..603bae44177a 100644
> --- a/xen/common/vsprintf.c
> +++ b/xen/common/vsprintf.c
> @@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
> va_list args)
> }
>
> for (; *fmt ; ++fmt) {
> +bool repeat = true;
> +
> if (*fmt != '%') {
> if (str < end)
> *str = *fmt;
> @@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
> va_list args)
>
> /* process flags */
> flags = 0;
> -repeat:
> -++fmt; /* this also skips first '%' */
> -switch (*fmt) {
> -case '-': flags |= LEFT; goto repeat;
> -case '+': flags |= PLUS; goto repeat;
> -case ' ': flags |= SPACE; goto repeat;
> -case '#': flags |= SPECIAL; goto repeat;
> -case '0': flags |= ZEROPAD; goto repeat;
> +while ( repeat ) {
> +++fmt; /* this also skips the first '%' */
> +switch (*fmt) {
> +case '-': flags |= LEFT; break;
> +case '+': flags |= PLUS; break;
> +case ' ': flags |= SPACE; break;
> +case '#': flags |= SPECIAL; break;
> +case '0': flags |= ZEROPAD; break;
> +default: repeat = false; break;
> +}
I'm firmly against this change. It takes a simple and clear piece of
code and replaces it with something harder to follow because you have to
look elsewhere to figure how the variable works.
Labels with names such as repeat/again/retry are clearly forming a
loop(ish).
I see in patch 4 that you exempt again/retry. That list needs to
include repeat, and this patch wants dropping.
~Andrew
Re: [PATCH v3 1/2] xen/arm32: head: Introduce enable_{boot,secondary}_cpu_mm()
Hi Ayan,
On 07/11/2023 12:02, Ayan Kumar Halder wrote:
> All the MMU related functionality have been clubbed together in
> enable_boot_cpu_mm() for booting primary cpu and enable_secondary_cpu_mm() for
> booting secondary cpus.
> This is done in preparation for moving the code related to MMU in MMU specific
> file and in order to support non MMU cpus in future.
>
> This is based on d2f8df5b3ede ("xen/arm64: head.S: Introduce
> enable_{boot,secondary}_cpu_mm()").
>
> Signed-off-by: Ayan Kumar Halder
> Reviewed-by: Michal Orzel
> Acked-by: Julien Grall
> ---
>
> Changes from :-
>
> v1 - 1. Added a proper commit message.
> 2. Some style and other fixes suggested in v1.
>
> v2 - 1. Updated the comment on top of enable_boot_cpu_mm() and
> enable_secondary_cpu_mm() ie mentioned the input and output registers.
> 2. Updated the comment inside enable_boot_cpu_mm().
>
> xen/arch/arm/arm32/head.S | 102 ++
> 1 file changed, 80 insertions(+), 22 deletions(-)
>
> diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
> index 2d7e690bf5..2204ea6dce 100644
> --- a/xen/arch/arm/arm32/head.S
> +++ b/xen/arch/arm/arm32/head.S
> @@ -201,13 +201,11 @@ past_zImage:
>
> blcheck_cpu_mode
> blcpu_init
> -blcreate_page_tables
>
> -/* Address in the runtime mapping to jump to after the MMU is
> enabled */
> mov_w lr, primary_switched
> -b enable_mmu
> +b enable_boot_cpu_mm
> +
> primary_switched:
> -blsetup_fixmap
> #ifdef CONFIG_EARLY_PRINTK
> /* Use a virtual address to access the UART. */
> mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> @@ -249,27 +247,11 @@ GLOBAL(init_secondary)
> #endif
> blcheck_cpu_mode
> blcpu_init
> -blcreate_page_tables
>
> -/* Address in the runtime mapping to jump to after the MMU is
> enabled */
> mov_w lr, secondary_switched
> -b enable_mmu
> -secondary_switched:
> -/*
> - * Non-boot CPUs need to move on to the proper pagetables, which were
> - * setup in prepare_secondary_mm.
> - *
> - * XXX: This is not compliant with the Arm Arm.
> - */
> -mov_w r4, init_ttbr /* VA of HTTBR value stashed by CPU 0 */
> -ldrd r4, r5, [r4] /* Actual value */
> -dsb
> -mcrr CP64(r4, r5, HTTBR)
> -dsb
> -isb
> -flush_xen_tlb_local r0
> -pt_enforce_wxn r0
> +b enable_secondary_cpu_mm
>
> +secondary_switched:
> #ifdef CONFIG_EARLY_PRINTK
> /* Use a virtual address to access the UART. */
> mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
> @@ -692,6 +674,82 @@ ready_to_switch:
> mov pc, lr
> ENDPROC(switch_to_runtime_mapping)
>
> +/*
> + * Enable mm (turn on the data cache and the MMU) for secondary CPUs.
> + * The function will return to the virtual address provided in LR (e.g. the
> + * runtime mapping).
> + *
> + * Inputs:
> + * r9 : paddr(start)
> + * r10: phys offset
> + * lr : Virtual address to return to.
> + *
> + * Output:
> + * r12: Was a temporary mapping created?
> + *
> + * Clobbers r0 - r6
> + */
> +enable_secondary_cpu_mm:
> +mov r6, lr
> +
> +blcreate_page_tables
> +
> +/* Address in the runtime mapping to jump to after the MMU is
> enabled */
> +mov_w lr, 1f
> +b enable_mmu
> +1:
> +/*
> + * Non-boot CPUs need to move on to the proper pagetables, which were
> + * setup in prepare_secondary_mm.
> + *
> + * XXX: This is not compliant with the Arm Arm.
> + */
> +mov_w r4, init_ttbr /* VA of HTTBR value stashed by CPU 0 */
> +ldrd r4, r5, [r4] /* Actual value */
> +dsb
> +mcrr CP64(r4, r5, HTTBR)
> +dsb
> +isb
> +flush_xen_tlb_local r0
> +pt_enforce_wxn r0
> +
> +/* Return to the virtual address requested by the caller. */
> +mov pc, r6
> +ENDPROC(enable_secondary_cpu_mm)
> +
> +/*
> + * Enable mm (turn on the data cache and the MMU) for the boot CPU.
> + * The function will return to the virtual address provided in LR (e.g. the
> + * runtime mapping).
> + *
> + * Inputs:
> + * r9 : paddr(start)
> + * r10: phys offset
> + * lr : Virtual address to return to.
> + *
> + * Output:
> + * r12: Was a temporary mapping created?
> + *
> + * Clobbers r0 - r6
> + */
> +enable_boot_cpu_mm:
> +mov r6, lr
> +
> +blcreate_page_tables
> +
> +/* Address in the runtime mapping to jump to after the MMU is
> enabled */
> +mov_w lr, 1f
> +b enable_mmu
> +1:
> +/*
> + * Prepare the fixmap. The function will return to the virtual
> address
> + * requested by the caller.
> + */
This comment should be above branch instruction and
[libvirt test] 183702: tolerable all pass - PUSHED
flight 183702 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/183702/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 183679 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 183679 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 183679 test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 15 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass version targeted for testing: libvirt 85f58711865385c3ba6414e398a7d08c60d362bd baseline version: libvirt 9231566146c0614d1339dd94cf98f82f1d1baee2 Last test of basis 183679 2023-11-04 04:21:29 Z3 days Testing same since 183702 2023-11-07 04:18:58 Z0 days1 attempts People who touched revisions under test: "Sergey A." Andrea Bolognani Daniel P. Berrangé Fedora Weblate Translation Laine Stump Michal Privoznik Sergey A Temuri Doghonadze Weblate 김인수 jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt pass test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-qcow2 pass test-arm64-arm64-libvirt-raw pass test-armhf-armhf-libvirt-raw pass test-amd64-i386-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images
Re: [RFC PATCH 0/4] address MISRA C:2012 Rule 15.2
On 2023-11-07 11:52, Jan Beulich wrote: On 07.11.2023 11:33, Nicola Vetrini wrote: This series is aimed at presenting some strategies that can be used to deal with violations of Rule 15.2: "The goto statement shall jump to a label declared later in the same function". I don't recall this rule being discussed on any of the meetings. This series is aimed mainly at collecting opinions on the possible resolution strategies, and based on the feedback decide what to focus the discussion on in the meetings. The rule's rationale is about possible developer confusion, therefore it could be argued that there is no substantial gain in complying with it, given the torough review process in place. To be honest, forward goto have potential of developer confusion as well: All other entities need to be declared / defined before they can be used. Just labels don't. (Or have I missed any other outlier?) IOW if I saw Misra make any rule here, I think it ought to suggest to avoid using "goto" altogether. Jan There is Rule 15.1 that says precisely that "do not use goto", but it's advisory and has never been proposed (there are likely hundreds of violations, and some are perhaps legitimate uses of goto). MISRA says that, if 15.1 is not followed, then a constrained use of goto is regulated by subsequent rules 15.2 and 15.3. -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
[PATCH v3 2/2] xen/arm32: head Split and move MMU-specific head.S to mmu/head.S
The MMU specific code in head.S will not be used on MPU systems.
Instead of introducing more #ifdefs which will bring complexity
to the code, move MMU related code to mmu/head.S and keep common
code in head.S. Two notes while moving:
- As "fail" in original head.S is very simple and this name is too
easy to be conflicted, duplicate it in mmu/head.S instead of
exporting it.
- Realigned ".macro ret" so that the alignment matches to the other
macros.
- Rename puts to asm_puts, putn to asm_putn (this denotes that the
macros are used within the context of assembly only).
- Use ENTRY() for enable_secondary_cpu_mm, enable_boot_cpu_mm,
setup_fixmap, asm_puts, asm_putn as they will be used externally.
Also move the assembly macros shared by head.S and mmu/head.S to
macros.h.
This is based on 6734327d76be ("xen/arm64: Split and move MMU-specific head.S
to mmu/head.S").
Signed-off-by: Ayan Kumar Halder
---
Changes from v1 :-
1. Added a commit message
2. Moved load_paddr to mmu/head.S
v2 :-
1. Renamed puts to asm_puts and putn to asm_putn. Exported asm_putn().
2. Moved XEN_TEMPORARY_OFFSET to head.S.
3. Some style issues.
xen/arch/arm/arm32/head.S | 628 +---
xen/arch/arm/arm32/mmu/Makefile | 1 +
xen/arch/arm/arm32/mmu/head.S | 573 +
xen/arch/arm/include/asm/arm32/macros.h | 58 ++-
4 files changed, 637 insertions(+), 623 deletions(-)
create mode 100644 xen/arch/arm/arm32/mmu/head.S
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 2204ea6dce..1448d092d1 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -22,86 +22,10 @@
#define ZIMAGE_MAGIC_NUMBER 0x016f2818
-#define PT_PT 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
-#define PT_MEM0xf7d /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=0 P=1 */
-#define PT_MEM_L3 0xf7f /* nG=1 AF=1 SH=11 AP=01 NS=1 ATTR=111 T=1 P=1 */
-#define PT_DEV0xe71 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=0 P=1 */
-#define PT_DEV_L3 0xe73 /* nG=1 AF=1 SH=10 AP=01 NS=1 ATTR=100 T=1 P=1 */
-
-#define PT_UPPER(x) (PT_##x & 0xf00)
-#define PT_LOWER(x) (PT_##x & 0x0ff)
-
-/* Convenience defines to get slot used by Xen mapping. */
-#define XEN_FIRST_SLOT first_table_offset(XEN_VIRT_START)
-#define XEN_SECOND_SLOT second_table_offset(XEN_VIRT_START)
-
-/* Offset between the early boot xen mapping and the runtime xen mapping */
-#define XEN_TEMPORARY_OFFSET (TEMPORARY_XEN_VIRT_START - XEN_VIRT_START)
-
#if defined(CONFIG_EARLY_PRINTK) && defined(CONFIG_EARLY_PRINTK_INC)
#include CONFIG_EARLY_PRINTK_INC
#endif
-/*
- * Move an immediate constant into a 32-bit register using movw/movt
- * instructions.
- */
-.macro mov_w reg, word
-movw \reg, #:lower16:\word
-movt \reg, #:upper16:\word
-.endm
-
-/*
- * Pseudo-op for PC relative adr , where is
- * within the range +/- 4GB of the PC.
- *
- * @dst: destination register
- * @sym: name of the symbol
- */
-.macro adr_l, dst, sym
-mov_w \dst, \sym - .Lpc\@
-.set .Lpc\@, .+ 8 /* PC bias */
-add \dst, \dst, pc
-.endm
-
-.macro load_paddr rb, sym
-mov_w \rb, \sym
-add \rb, \rb, r10
-.endm
-
-/*
- * Flush local TLBs
- *
- * @tmp: Scratch register
- *
- * See asm/arm32/flushtlb.h for the explanation of the sequence.
- */
-.macro flush_xen_tlb_local tmp
-dsb nshst
-mcr CP32(\tmp, TLBIALLH)
-dsb nsh
-isb
-.endm
-
-/*
- * Enforce Xen page-tables do not contain mapping that are both
- * Writable and eXecutables.
- *
- * This should be called on each secondary CPU.
- */
-.macro pt_enforce_wxn tmp
-mrc CP32(\tmp, HSCTLR)
-orr \tmp, \tmp, #SCTLR_Axx_ELx_WXN
-dsb
-mcr CP32(\tmp, HSCTLR)
-/*
- * The TLBs may cache SCTLR_EL2.WXN. So ensure it is synchronized
- * before flushing the TLBs.
- */
-isb
-flush_xen_tlb_local \tmp
-.endm
-
/*
* Common register usage in this file:
* r0 -
@@ -121,38 +45,6 @@
* r14 - LR
* r15 - PC
*/
-#ifdef CONFIG_EARLY_PRINTK
-/*
- * Macro to print a string to the UART, if there is one.
- *
- * Clobbers r0 - r3
- */
-#define PRINT(_s) \
-mov r3, lr ;\
-adr_l r0, 98f ;\
-blputs ;\
-mov lr, r3 ;\
-RODATA_STR(98, _s)
-
-/*
- * Macro to print the value of register \rb
- *
- * Clobbers r0 - r4
- */
-.macro print_reg rb
-mov r0, \rb
-mov r4, lr
-blputn
-mov lr, r4
-.endm
-
-#else /* CONFIG_EARLY_PRINTK */
-#define PRINT(s)
-
-.macro print_reg rb
-.endm
-
-#endif /* !CONFIG_EARLY_PRINTK */
.section .text.header, "ax", %progbits
.arm
@@ -355,480 +247,6 @@ cpu_init_done:
mov pc, r5/* Return address is in r5 */
ENDPROC(cpu_init)
-/*
- * Macro to find the slot number at a given
[PATCH v3 1/2] xen/arm32: head: Introduce enable_{boot,secondary}_cpu_mm()
All the MMU related functionality have been clubbed together in
enable_boot_cpu_mm() for booting primary cpu and enable_secondary_cpu_mm() for
booting secondary cpus.
This is done in preparation for moving the code related to MMU in MMU specific
file and in order to support non MMU cpus in future.
This is based on d2f8df5b3ede ("xen/arm64: head.S: Introduce
enable_{boot,secondary}_cpu_mm()").
Signed-off-by: Ayan Kumar Halder
Reviewed-by: Michal Orzel
Acked-by: Julien Grall
---
Changes from :-
v1 - 1. Added a proper commit message.
2. Some style and other fixes suggested in v1.
v2 - 1. Updated the comment on top of enable_boot_cpu_mm() and
enable_secondary_cpu_mm() ie mentioned the input and output registers.
2. Updated the comment inside enable_boot_cpu_mm().
xen/arch/arm/arm32/head.S | 102 ++
1 file changed, 80 insertions(+), 22 deletions(-)
diff --git a/xen/arch/arm/arm32/head.S b/xen/arch/arm/arm32/head.S
index 2d7e690bf5..2204ea6dce 100644
--- a/xen/arch/arm/arm32/head.S
+++ b/xen/arch/arm/arm32/head.S
@@ -201,13 +201,11 @@ past_zImage:
blcheck_cpu_mode
blcpu_init
-blcreate_page_tables
-/* Address in the runtime mapping to jump to after the MMU is enabled
*/
mov_w lr, primary_switched
-b enable_mmu
+b enable_boot_cpu_mm
+
primary_switched:
-blsetup_fixmap
#ifdef CONFIG_EARLY_PRINTK
/* Use a virtual address to access the UART. */
mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
@@ -249,27 +247,11 @@ GLOBAL(init_secondary)
#endif
blcheck_cpu_mode
blcpu_init
-blcreate_page_tables
-/* Address in the runtime mapping to jump to after the MMU is enabled
*/
mov_w lr, secondary_switched
-b enable_mmu
-secondary_switched:
-/*
- * Non-boot CPUs need to move on to the proper pagetables, which were
- * setup in prepare_secondary_mm.
- *
- * XXX: This is not compliant with the Arm Arm.
- */
-mov_w r4, init_ttbr /* VA of HTTBR value stashed by CPU 0 */
-ldrd r4, r5, [r4] /* Actual value */
-dsb
-mcrr CP64(r4, r5, HTTBR)
-dsb
-isb
-flush_xen_tlb_local r0
-pt_enforce_wxn r0
+b enable_secondary_cpu_mm
+secondary_switched:
#ifdef CONFIG_EARLY_PRINTK
/* Use a virtual address to access the UART. */
mov_w r11, EARLY_UART_VIRTUAL_ADDRESS
@@ -692,6 +674,82 @@ ready_to_switch:
mov pc, lr
ENDPROC(switch_to_runtime_mapping)
+/*
+ * Enable mm (turn on the data cache and the MMU) for secondary CPUs.
+ * The function will return to the virtual address provided in LR (e.g. the
+ * runtime mapping).
+ *
+ * Inputs:
+ * r9 : paddr(start)
+ * r10: phys offset
+ * lr : Virtual address to return to.
+ *
+ * Output:
+ * r12: Was a temporary mapping created?
+ *
+ * Clobbers r0 - r6
+ */
+enable_secondary_cpu_mm:
+mov r6, lr
+
+blcreate_page_tables
+
+/* Address in the runtime mapping to jump to after the MMU is enabled
*/
+mov_w lr, 1f
+b enable_mmu
+1:
+/*
+ * Non-boot CPUs need to move on to the proper pagetables, which were
+ * setup in prepare_secondary_mm.
+ *
+ * XXX: This is not compliant with the Arm Arm.
+ */
+mov_w r4, init_ttbr /* VA of HTTBR value stashed by CPU 0 */
+ldrd r4, r5, [r4] /* Actual value */
+dsb
+mcrr CP64(r4, r5, HTTBR)
+dsb
+isb
+flush_xen_tlb_local r0
+pt_enforce_wxn r0
+
+/* Return to the virtual address requested by the caller. */
+mov pc, r6
+ENDPROC(enable_secondary_cpu_mm)
+
+/*
+ * Enable mm (turn on the data cache and the MMU) for the boot CPU.
+ * The function will return to the virtual address provided in LR (e.g. the
+ * runtime mapping).
+ *
+ * Inputs:
+ * r9 : paddr(start)
+ * r10: phys offset
+ * lr : Virtual address to return to.
+ *
+ * Output:
+ * r12: Was a temporary mapping created?
+ *
+ * Clobbers r0 - r6
+ */
+enable_boot_cpu_mm:
+mov r6, lr
+
+blcreate_page_tables
+
+/* Address in the runtime mapping to jump to after the MMU is enabled
*/
+mov_w lr, 1f
+b enable_mmu
+1:
+/*
+ * Prepare the fixmap. The function will return to the virtual address
+ * requested by the caller.
+ */
+mov lr, r6
+
+b setup_fixmap
+ENDPROC(enable_boot_cpu_mm)
+
/*
* Remove the 1:1 map from the page-tables. It is not easy to keep track
* where the 1:1 map was mapped, so we will look for the top-level entry
--
2.25.1
[PATCH v3 0/2] xen/arm: Split MMU code as the prepration of MPU work form Arm32
Hi,
These are the set of patches based on top of
"[PATCH v8 0/8] xen/arm: Split MMU code as the prepration of MPU work".
This is the preparation work to add MPU support on Arm32.
There are two more dependencies for this series :-
1. "[XEN v1 1/4] xen/arm: arm32: Move pt_enforce_wxn() so that it can be
bundled with other MMU functionality"
is merged into "[PATCH v8 3/8] xen/arm: Fold mmu_init_secondary_cpu() to head.S"
as per the discussion on [1].
2. "[XEN v4] xen/arm32: head: Replace load_paddr with adr_l when they are
equivalent"
[1] -
https://lore.kernel.org/all/f098a07d-fa19-4b40-bfac-7b1215243...@xen.org/#t
Changes from :-
v1 - Dropped "[XEN v1 1/4] xen/arm: arm32: Move pt_enforce_wxn() so that it can
be bundled with other MMU functionality"
and "[XEN v1 4/4] xen/arm: traps.c: Enclose VMSA specific registers within
CONFIG_MMU".
v2 - Changes mentioned in individual patches.
Ayan Kumar Halder (2):
xen/arm32: head: Introduce enable_{boot,secondary}_cpu_mm()
xen/arm32: head Split and move MMU-specific head.S to mmu/head.S
xen/arch/arm/arm32/head.S | 578 +---
xen/arch/arm/arm32/mmu/Makefile | 1 +
xen/arch/arm/arm32/mmu/head.S | 573 +++
xen/arch/arm/include/asm/arm32/macros.h | 58 ++-
4 files changed, 641 insertions(+), 569 deletions(-)
create mode 100644 xen/arch/arm/arm32/mmu/head.S
--
2.25.1
Re: [kernel-6.5-bug] with xen-4.18~rc4 on ub-24.04 noble, booting xen domU shows UBSAN errors in blkback & netback drivers
On 06.11.2023 20:20, Pry Mar wrote: > These 2 errors show in dmesg late in boot process when xen domU launch: > 1) blkback > UBSAN: array-index-out-of-bounds in > /build/linux-D15vQj/linux-6.5.0/drivers/block/xen-blkback/blkback.c:1227:4 > > 2) netback > [ 55.209063] UBSAN: array-index-out-of-bounds in > /build/linux-D15vQj/linux-6.5.0/drivers/net/xen-netback/netback.c:289:3 > [ 55.209268] index 3 is out of range for type 'xen_netif_tx_sring_entry > [1]' > [ 55.209401] CPU: 3 PID: 0 Comm: swapper/3 Not tainted 6.5.0-9-generic > #9-Ubuntu > > The xen sources don't seem to generate these UBSAN errors and I included > the patch from staging-4.18. It seems only the xen related kernel drivers > are > doing UBSAN errors. While I have to admit that I find your report a little confusing, I kind of guess that what you're reporting would be addressed by https://lists.xen.org/archives/html/xen-devel/2023-07/msg01837.html (once it was put in suitable shape to be committed, and once it was then further pulled over into the Linux tree). Jan
Re: [RFC PATCH 0/4] address MISRA C:2012 Rule 15.2
On 07.11.2023 11:33, Nicola Vetrini wrote: > This series is aimed at presenting some strategies that can be used to deal > with > violations of Rule 15.2: > "The goto statement shall jump to a label declared later in the same > function". I don't recall this rule being discussed on any of the meetings. > The rule's rationale is about possible developer confusion, therefore it could > be argued that there is no substantial gain in complying with it, given the > torough review process in place. To be honest, forward goto have potential of developer confusion as well: All other entities need to be declared / defined before they can be used. Just labels don't. (Or have I missed any other outlier?) IOW if I saw Misra make any rule here, I think it ought to suggest to avoid using "goto" altogether. Jan
[RFC PATCH 3/4] xen/arm: GICv3: address MISRA C:2012 Rule 15.2
The backwards jump due to the "goto retry;" statement
can be transformed into a loop, without losing much in terms
of readability.
Signed-off-by: Stefano Stabellini
Signed-off-by: Nicola Vetrini
---
This specific patch was provided by Stefano, I just added the
commit message.
---
xen/arch/arm/gic-v3-its.c | 81 ---
1 file changed, 42 insertions(+), 39 deletions(-)
diff --git a/xen/arch/arm/gic-v3-its.c b/xen/arch/arm/gic-v3-its.c
index 8afcd9783bc8..4ba3f869ddf2 100644
--- a/xen/arch/arm/gic-v3-its.c
+++ b/xen/arch/arm/gic-v3-its.c
@@ -341,6 +341,7 @@ static int its_map_baser(void __iomem *basereg, uint64_t
regc,
unsigned int pagesz = 2;/* try 64K pages first, then go down. */
unsigned int table_size;
void *buffer;
+bool retry = false;
attr = GIC_BASER_InnerShareable << GITS_BASER_SHAREABILITY_SHIFT;
attr |= GIC_BASER_CACHE_SameAsInner << GITS_BASER_OUTER_CACHEABILITY_SHIFT;
@@ -351,55 +352,57 @@ static int its_map_baser(void __iomem *basereg, uint64_t
regc,
* it back and see what sticks (page size, cacheability and shareability
* attributes), retrying if necessary.
*/
-retry:
-table_size = ROUNDUP(nr_items * entry_size,
- BIT(BASER_PAGE_BITS(pagesz), UL));
-/* The BASE registers support at most 256 pages. */
-table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
+while ( retry )
+{
+table_size = ROUNDUP(nr_items * entry_size,
+BIT(BASER_PAGE_BITS(pagesz), UL));
+/* The BASE registers support at most 256 pages. */
+table_size = min(table_size, 256U << BASER_PAGE_BITS(pagesz));
-buffer = _xzalloc(table_size, BIT(BASER_PAGE_BITS(pagesz), UL));
-if ( !buffer )
-return -ENOMEM;
+buffer = _xzalloc(table_size, BIT(BASER_PAGE_BITS(pagesz), UL));
+if ( !buffer )
+return -ENOMEM;
-if ( !check_baser_phys_addr(buffer, BASER_PAGE_BITS(pagesz)) )
-{
-xfree(buffer);
-return -ERANGE;
-}
+if ( !check_baser_phys_addr(buffer, BASER_PAGE_BITS(pagesz)) )
+{
+xfree(buffer);
+return -ERANGE;
+}
-reg = attr;
-reg |= (pagesz << GITS_BASER_PAGE_SIZE_SHIFT);
-reg |= (table_size >> BASER_PAGE_BITS(pagesz)) - 1;
-reg |= regc & BASER_RO_MASK;
-reg |= GITS_VALID_BIT;
-reg |= encode_baser_phys_addr(virt_to_maddr(buffer),
- BASER_PAGE_BITS(pagesz));
+reg = attr;
+reg |= (pagesz << GITS_BASER_PAGE_SIZE_SHIFT);
+reg |= (table_size >> BASER_PAGE_BITS(pagesz)) - 1;
+reg |= regc & BASER_RO_MASK;
+reg |= GITS_VALID_BIT;
+reg |= encode_baser_phys_addr(virt_to_maddr(buffer),
+BASER_PAGE_BITS(pagesz));
-writeq_relaxed(reg, basereg);
-regc = readq_relaxed(basereg);
+writeq_relaxed(reg, basereg);
+regc = readq_relaxed(basereg);
-/* The host didn't like our attributes, just use what it returned. */
-if ( (regc & BASER_ATTR_MASK) != attr )
-{
-/* If we can't map it shareable, drop cacheability as well. */
-if ( (regc & GITS_BASER_SHAREABILITY_MASK) == GIC_BASER_NonShareable )
+/* The host didn't like our attributes, just use what it returned. */
+if ( (regc & BASER_ATTR_MASK) != attr )
{
-regc &= ~GITS_BASER_INNER_CACHEABILITY_MASK;
-writeq_relaxed(regc, basereg);
+/* If we can't map it shareable, drop cacheability as well. */
+if ( (regc & GITS_BASER_SHAREABILITY_MASK) ==
GIC_BASER_NonShareable )
+{
+regc &= ~GITS_BASER_INNER_CACHEABILITY_MASK;
+writeq_relaxed(regc, basereg);
+}
+attr = regc & BASER_ATTR_MASK;
}
-attr = regc & BASER_ATTR_MASK;
-}
-if ( (regc & GITS_BASER_INNER_CACHEABILITY_MASK) <= GIC_BASER_CACHE_nC )
-clean_and_invalidate_dcache_va_range(buffer, table_size);
+if ( (regc & GITS_BASER_INNER_CACHEABILITY_MASK) <= GIC_BASER_CACHE_nC
)
+clean_and_invalidate_dcache_va_range(buffer, table_size);
-/* If the host accepted our page size, we are done. */
-if ( ((regc >> GITS_BASER_PAGE_SIZE_SHIFT) & 0x3UL) == pagesz )
-return 0;
+/* If the host accepted our page size, we are done. */
+if ( ((regc >> GITS_BASER_PAGE_SIZE_SHIFT) & 0x3UL) == pagesz )
+return 0;
-xfree(buffer);
+xfree(buffer);
-if ( pagesz-- > 0 )
-goto retry;
+if ( pagesz-- > 0 )
+retry = true;
+}
/* None of the page sizes was accepted, give up */
return -EINVAL;
--
2.34.1
[RFC PATCH 2/4] x86/dom0: make goto jump forward
The jump to the label 'parse_error' becomes forward, rather
than backward; at the same time, the else branch can be eliminated.
This also fixes a violation of MISRA C:2012 Rule 15.2.
Signed-off-by: Nicola Vetrini
---
xen/arch/x86/dom0_build.c | 14 +++---
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index 09fb8b063ae7..f0191dc148a2 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -439,12 +439,7 @@ static void __init process_dom0_ioports_disable(struct
domain *dom0)
{
io_from = simple_strtoul(t, , 16);
if ( u == t )
-{
-parse_error:
-printk("Invalid ioport range <%s> "
- "in dom0_ioports_disable, skipping\n", t);
-continue;
-}
+goto parse_error;
if ( *u == '\0' )
io_to = io_from;
@@ -454,7 +449,12 @@ static void __init process_dom0_ioports_disable(struct
domain *dom0)
goto parse_error;
if ( (*u != '\0') || (io_to < io_from) || (io_to >= 65536) )
-goto parse_error;
+{
+parse_error:
+printk("Invalid ioport range <%s> "
+ "in dom0_ioports_disable, skipping\n", t);
+continue;
+}
printk("Disabling dom0 access to ioport range %04lx-%04lx\n",
io_from, io_to);
--
2.34.1
[RFC PATCH 4/4] automation/eclair: add deviation for certain backwards goto
As explained in the deviation record, code constructs such as
"goto retry" and "goto again" are sometimes the best balance between
code complexity and the understandability of the control flow
by developers; as such, these construct are allowed to deviate
from Rule 15.2.
Signed-off-by: Nicola Vetrini
---
automation/eclair_analysis/ECLAIR/deviations.ecl | 10 ++
docs/misra/deviations.rst| 10 ++
2 files changed, 20 insertions(+)
diff --git a/automation/eclair_analysis/ECLAIR/deviations.ecl
b/automation/eclair_analysis/ECLAIR/deviations.ecl
index fa56e5c00a27..8b1f622f8f82 100644
--- a/automation/eclair_analysis/ECLAIR/deviations.ecl
+++ b/automation/eclair_analysis/ECLAIR/deviations.ecl
@@ -270,6 +270,16 @@ statements are deliberate"
-config=MC3R1.R14.3,statements={deliberate , "wrapped(any(),node(if_stmt))" }
-doc_end
+#
+# Series 15
+#
+
+-doc_begin="The additional complexity introduced in the code by using control
flow structures other than backwards goto-s
+were deemed not to justify the possible prevention of developer confusion,
given the very torough review process estabilished
+in the community."
+-config=MC3R1.R15.2,reports+={deliberate, "any_area(any_loc(text(^.*goto
(again|retry).*$)))"}
+-doc_end
+
#
# Series 20.
#
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index 8511a189253b..7d1e1f0d09b3 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -208,6 +208,16 @@ Deviations related to MISRA C:2012 Rules:
statements are deliberate.
- Project-wide deviation; tagged as `disapplied` for ECLAIR.
+ * - R15.2
+ - The possible prevention of developer confusion as a result of using
+ control flow structures other than backwards goto-s in some instances
was
+ deemed not strong enough to justify the additional complexity introduced
+ in the code. Such instances are the uses of the following labels:
+
+ - again
+ - retry
+ - Tagged as `deliberate` for ECLAIR.
+
* - R20.7
- Code violating Rule 20.7 is safe when macro parameters are used:
(1) as function arguments;
--
2.34.1
[RFC PATCH 0/4] address MISRA C:2012 Rule 15.2
This series is aimed at presenting some strategies that can be used to deal with violations of Rule 15.2: "The goto statement shall jump to a label declared later in the same function". The rule's rationale is about possible developer confusion, therefore it could be argued that there is no substantial gain in complying with it, given the torough review process in place. Nonetheless, the proposed resolution strategies are the following: - use a loop instead of a goto (see patch 1 and 3) - make the jump due to the goto forward, rather than backward (see patch 2) - unconditionally allow certain constructs, such as "goto retry", whose presence in the codebase typically signifies that all other reasonable approaches (e.g, loops, forward jumps) have been considered and deemed inferior in terms of code readability. The latter strategy may be postponed until all goto-s with a certain label have been examined. An alternative strategy could be to allow certain files (most notably those under x86/x86_emulate) to have backward jumps, and resolve the remaining violations. Any feedback on this matter is welcome. Nicola Vetrini (4): xen/vsprintf: replace backwards jump with loop x86/dom0: make goto jump forward xen/arm: GICv3: address MISRA C:2012 Rule 15.2 automation/eclair: add deviation for certain backwards goto .../eclair_analysis/ECLAIR/deviations.ecl | 10 +++ docs/misra/deviations.rst | 10 +++ xen/arch/arm/gic-v3-its.c | 81 ++- xen/arch/x86/dom0_build.c | 14 ++-- xen/common/vsprintf.c | 20 +++-- 5 files changed, 81 insertions(+), 54 deletions(-) -- 2.34.1
[RFC PATCH 1/4] xen/vsprintf: replace backwards jump with loop
The backwards goto in the vsnprintf function can be replaced
with a loop, thereby fixing a violation of MISRA C:2012 Rule 15.2.
Signed-off-by: Nicola Vetrini
---
xen/common/vsprintf.c | 20
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index c49631c0a4d8..603bae44177a 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -495,6 +495,8 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
}
for (; *fmt ; ++fmt) {
+bool repeat = true;
+
if (*fmt != '%') {
if (str < end)
*str = *fmt;
@@ -504,14 +506,16 @@ int vsnprintf(char *buf, size_t size, const char *fmt,
va_list args)
/* process flags */
flags = 0;
-repeat:
-++fmt; /* this also skips first '%' */
-switch (*fmt) {
-case '-': flags |= LEFT; goto repeat;
-case '+': flags |= PLUS; goto repeat;
-case ' ': flags |= SPACE; goto repeat;
-case '#': flags |= SPECIAL; goto repeat;
-case '0': flags |= ZEROPAD; goto repeat;
+while ( repeat ) {
+++fmt; /* this also skips the first '%' */
+switch (*fmt) {
+case '-': flags |= LEFT; break;
+case '+': flags |= PLUS; break;
+case ' ': flags |= SPACE; break;
+case '#': flags |= SPECIAL; break;
+case '0': flags |= ZEROPAD; break;
+default: repeat = false; break;
+}
}
/* get field width */
--
2.34.1
[XEN PATCH][for-4.19 v2] xen: replace occurrences of SAF-1-safe with asmlinkage attribute
The comment-based justifications for MISRA C:2012 Rule 8.4 are replaced
by the asmlinkage pseudo-attribute, for the sake of uniformity.
Add missing 'xen/compiler.h' #include-s where needed.
The text in docs/misra/deviations.rst and docs/misra/safe.json
is modified to reflect this change.
Signed-off-by: Nicola Vetrini
---
Changes in v2:
- Edit safe.json.
- Remove mention of SAF-1-safe in deviations.rst.
---
docs/misra/deviations.rst | 5 ++---
docs/misra/safe.json| 2 +-
xen/arch/arm/cpuerrata.c| 7 +++
xen/arch/arm/setup.c| 5 ++---
xen/arch/arm/smpboot.c | 3 +--
xen/arch/arm/traps.c| 21 +++--
xen/arch/x86/boot/cmdline.c | 5 +++--
xen/arch/x86/boot/reloc.c | 7 ---
xen/arch/x86/extable.c | 3 +--
xen/arch/x86/setup.c| 3 +--
xen/arch/x86/traps.c| 27 +--
xen/common/efi/boot.c | 5 ++---
12 files changed, 36 insertions(+), 57 deletions(-)
diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst
index d468da2f5ce9..0fa0475db0eb 100644
--- a/docs/misra/deviations.rst
+++ b/docs/misra/deviations.rst
@@ -134,9 +134,8 @@ Deviations related to MISRA C:2012 Rules:
- Tagged as `safe` for ECLAIR.
* - R8.4
- - Functions and variables used only by asm modules are either marked with
- the `asmlinkage` macro or with a SAF-1-safe textual deviation
- (see safe.json).
+ - Functions and variables used only to interface with asm modules should
+ be marked with the `asmlinkage` macro.
- Tagged as `safe` for ECLAIR.
* - R8.6
diff --git a/docs/misra/safe.json b/docs/misra/safe.json
index 39c5c056c7d4..eaff0312e20c 100644
--- a/docs/misra/safe.json
+++ b/docs/misra/safe.json
@@ -16,7 +16,7 @@
"eclair": "MC3R1.R8.4"
},
"name": "Rule 8.4: asm-only definition",
-"text": "Functions and variables used only by asm modules do not
need to have a visible declaration prior to their definition."
+"text": "Not used anymore."
},
{
"id": "SAF-2-safe",
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c
index 9137958fb682..a28fa6ac78cc 100644
--- a/xen/arch/arm/cpuerrata.c
+++ b/xen/arch/arm/cpuerrata.c
@@ -370,10 +370,9 @@ custom_param("spec-ctrl", parse_spec_ctrl);
/* Arm64 only for now as for Arm32 the workaround is currently handled in C. */
#ifdef CONFIG_ARM_64
-/* SAF-1-safe */
-void __init arm_enable_wa2_handling(const struct alt_instr *alt,
-const uint32_t *origptr,
-uint32_t *updptr, int nr_inst)
+void asmlinkage __init arm_enable_wa2_handling(const struct alt_instr *alt,
+ const uint32_t *origptr,
+ uint32_t *updptr, int nr_inst)
{
BUG_ON(nr_inst != 1);
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 3f3a45719ccb..bedbce3c0d37 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -1077,9 +1077,8 @@ static bool __init is_dom0less_mode(void)
size_t __read_mostly dcache_line_bytes;
/* C entry point for boot CPU */
-/* SAF-1-safe */
-void __init start_xen(unsigned long boot_phys_offset,
- unsigned long fdt_paddr)
+void asmlinkage __init start_xen(unsigned long boot_phys_offset,
+ unsigned long fdt_paddr)
{
size_t fdt_size;
const char *cmdline;
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index ec76de3cac12..a32f610b47d9 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -303,8 +303,7 @@ smp_prepare_cpus(void)
}
/* Boot the current CPU */
-/* SAF-1-safe */
-void start_secondary(void)
+void asmlinkage start_secondary(void)
{
unsigned int cpuid = init_data.cpuid;
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 5aa14d470791..63d3bc7bd67b 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -161,8 +161,7 @@ void init_traps(void)
isb();
}
-/* SAF-1-safe */
-void __div0(void)
+void asmlinkage __div0(void)
{
printk("Division by zero in hypervisor.\n");
BUG();
@@ -1955,8 +1954,7 @@ static inline bool needs_ssbd_flip(struct vcpu *v)
* Actions that needs to be done after entering the hypervisor from the
* guest and before the interrupts are unmasked.
*/
-/* SAF-1-safe */
-void enter_hypervisor_from_guest_preirq(void)
+void asmlinkage enter_hypervisor_from_guest_preirq(void)
{
struct vcpu *v = current;
@@ -1970,8 +1968,7 @@ void enter_hypervisor_from_guest_preirq(void)
* guest and before we handle any request. Depending on the exception trap,
* this may be called with interrupts unmasked.
*/
-/* SAF-1-safe */
-void enter_hypervisor_from_guest(void)
+void asmlinkage enter_hypervisor_from_guest(void)
{
struct vcpu *v = current;
@@ -1999,8 +1996,7 @@ void
Re: [XEN PATCH][for-4.19] xen: replace occurrences of SAF-1-safe with asmlinkage attribute
On 2023-11-07 10:49, Julien Grall wrote: Hi, On 07/11/2023 08:36, Nicola Vetrini wrote: On 2023-11-06 23:57, Julien Grall wrote: Hi Nicola, On 03/11/2023 18:05, Nicola Vetrini wrote: The comment-based justifications for MISRA C:2012 Rule 8.4 are replaced by the asmlinkage pseudo-attribute, for the sake of uniformity. The deviation with a comment based on the SAF framework is also mentioned as a last resort. I don't see any reason to keep SAF-1 after this patch. So can this be removed? In documenting-violations.rst it's stated: "Entries in the database shall never be removed, even if they are not used anymore in the code (if a patch is removing or modifying the faulty line). This is to make sure that numbers are not reused which could lead to conflicts with old branches or misleading justifications." I read this as the number can not be re-used. But we could replace the description with "Not used anymore". that's why I kept SAF-1 in the safe.json file and added the remark about it being a last resort. Right, but this is confusing. What is the last resort? Why would one use it? It would be best to not mention SAF-1 at all in deviations.rst. Cheers, Ok, I'll submit a v2 -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH][for-4.19] xen: replace occurrences of SAF-1-safe with asmlinkage attribute
Hi, On 07/11/2023 08:36, Nicola Vetrini wrote: On 2023-11-06 23:57, Julien Grall wrote: Hi Nicola, On 03/11/2023 18:05, Nicola Vetrini wrote: The comment-based justifications for MISRA C:2012 Rule 8.4 are replaced by the asmlinkage pseudo-attribute, for the sake of uniformity. The deviation with a comment based on the SAF framework is also mentioned as a last resort. I don't see any reason to keep SAF-1 after this patch. So can this be removed? In documenting-violations.rst it's stated: "Entries in the database shall never be removed, even if they are not used anymore in the code (if a patch is removing or modifying the faulty line). This is to make sure that numbers are not reused which could lead to conflicts with old branches or misleading justifications." I read this as the number can not be re-used. But we could replace the description with "Not used anymore". that's why I kept SAF-1 in the safe.json file and added the remark about it being a last resort. Right, but this is confusing. What is the last resort? Why would one use it? It would be best to not mention SAF-1 at all in deviations.rst. Cheers, -- Julien Grall
Re: [PATCH v4 14/17] net: do not delete nics in net_cleanup()
On Mon, 2023-11-06 at 14:35 +, David Woodhouse wrote: > From: David Woodhouse > > In net_cleanup() we only need to delete the netdevs, as those may have > state which outlives Qemu when it exits, and thus may actually need to > be cleaned up on exit. > > The nics, on the other hand, are owned by the device which created them. > Most devices don't bother to clean up on exit because they don't have > any state which will outlive Qemu... but XenBus devices do need to clean > up their nodes in XenStore, and do have an exit handler to delete them. > > When the XenBus exit handler destroys the xen-net-device, it attempts > to delete its nic after net_cleanup() had already done so. And crashes. > > Fix this by only deleting netdevs as we walk the list. As the comment > notes, we can't use QTAILQ_FOREACH_SAFE() as each deletion may remove > *multiple* entries, including the "safely" saved 'next' pointer. But > we can store the *previous* entry, since nics are safe. > > Signed-off-by: David Woodhouse > Reviewed-by: Paul Durrant I've left this out of the pull request I've just sent, pending Jason's approval for it. As it's a bugfix, I don't think we strictly has to be in by *today*, right? We still have a little time? smime.p7s Description: S/MIME cryptographic signature
Re: [PATCH] ubsan: Fix pointer overflow error message
On 07/11/2023 9:14 am, Michal Orzel wrote:
> In __ubsan_handle_pointer_overflow(), fix the condition for determining
> whether a pointer operation overflowed or underflowed. Currently, the
> function reports "underflowed" when it should be reporting "overflowed"
> and vice versa.
>
> Example of incorrect error reporting:
> void *foo = (void *)__UINTPTR_MAX__;
> foo += 1;
>
> UBSAN:
> pointer operation underflowed to
>
> Fixes: 4e3fb2fb47d6 ("ubsan: add clang 5.0 support")
> Signed-off-by: Michal Orzel
> ---
> xen/common/ubsan/ubsan.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
> index 0fddacabda6a..a3a80fa99eec 100644
> --- a/xen/common/ubsan/ubsan.c
> +++ b/xen/common/ubsan/ubsan.c
> @@ -513,7 +513,7 @@ void __ubsan_handle_pointer_overflow(struct
> pointer_overflow_data *data,
> ubsan_prologue(>location, );
>
> pr_err("pointer operation %s %p to %p\n",
> -base > result ? "underflowed" : "overflowed",
> +base > result ? "overflowed" : "underflowed",
Lovely.
Acked-by: Andrew Cooper
Re: [PATCH v4 16/17] doc/sphinx/hxtool.py: add optional label argument to SRST directive
On Mon, 2023-11-06 at 14:35 +, David Woodhouse wrote: > From: David Woodhouse > > We can't just embed labels directly into files like qemu-options.hx which > are included from multiple top-level RST files, because Sphinx sees the > labels as duplicate: https://github.com/sphinx-doc/sphinx/issues/9707 > > So add an 'emitrefs' option to the Sphinx hxtool-doc directive, which is > set only in invocation.rst and not from the HTML rendition of the man > page. Along with an argument to the SRST directive which causes a label > of the form '.. _LABEL-reference-label:' to be emitted when the emitrefs > option is set. > > Signed-off-by: David Woodhouse > --- FWIW I've left this out of the pull request I just sent, and the Xen docs just tell the user to go *find* the -initrd documentation instead of being able to emit a direct link. We can fix that later. smime.p7s Description: S/MIME cryptographic signature
[PULL 08/15] hw/xen: do not repeatedly try to create a failing backend device
From: David Woodhouse
If xen_backend_device_create() fails to instantiate a device, the XenBus
code will just keep trying over and over again each time the bus is
re-enumerated, as long as the backend appears online and in
XenbusStateInitialising.
The only thing which prevents the XenBus code from recreating duplicates
of devices which already exist, is the fact that xen_device_realize()
sets the backend state to XenbusStateInitWait. If the attempt to create
the device doesn't get *that* far, that's when it will keep getting
retried.
My first thought was to handle errors by setting the backend state to
XenbusStateClosed, but that doesn't work for XenConsole which wants to
*ignore* any device of type != "ioemu" completely.
So, make xen_backend_device_create() *keep* the XenBackendInstance for a
failed device, and provide a new xen_backend_exists() function to allow
xen_bus_type_enumerate() to check whether one already exists before
creating a new one.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/xen/xen-backend.c | 27 +--
hw/xen/xen-bus.c | 3 ++-
include/hw/xen/xen-backend.h | 1 +
3 files changed, 24 insertions(+), 7 deletions(-)
diff --git a/hw/xen/xen-backend.c b/hw/xen/xen-backend.c
index 5b0fb76eae..b9bf70a9f5 100644
--- a/hw/xen/xen-backend.c
+++ b/hw/xen/xen-backend.c
@@ -101,6 +101,24 @@ static XenBackendInstance *xen_backend_list_find(XenDevice
*xendev)
return NULL;
}
+bool xen_backend_exists(const char *type, const char *name)
+{
+const XenBackendImpl *impl = xen_backend_table_lookup(type);
+XenBackendInstance *backend;
+
+if (!impl) {
+return false;
+}
+
+QLIST_FOREACH(backend, _list, entry) {
+if (backend->impl == impl && !strcmp(backend->name, name)) {
+return true;
+}
+}
+
+return false;
+}
+
static void xen_backend_list_remove(XenBackendInstance *backend)
{
QLIST_REMOVE(backend, entry);
@@ -122,11 +140,6 @@ void xen_backend_device_create(XenBus *xenbus, const char
*type,
backend->name = g_strdup(name);
impl->create(backend, opts, errp);
-if (*errp) {
-g_free(backend->name);
-g_free(backend);
-return;
-}
backend->impl = impl;
xen_backend_list_add(backend);
@@ -165,7 +178,9 @@ bool xen_backend_try_device_destroy(XenDevice *xendev,
Error **errp)
}
impl = backend->impl;
-impl->destroy(backend, errp);
+if (backend->xendev) {
+impl->destroy(backend, errp);
+}
xen_backend_list_remove(backend);
g_free(backend->name);
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 12ff782005..3ffd1a5333 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -209,7 +209,8 @@ static void xen_bus_type_enumerate(XenBus *xenbus, const
char *type)
NULL, "%u", ) != 1)
online = 0;
-if (online && state == XenbusStateInitialising) {
+if (online && state == XenbusStateInitialising &&
+!xen_backend_exists(type, backend[i])) {
Error *local_err = NULL;
xen_bus_backend_create(xenbus, type, backend[i], backend_path,
diff --git a/include/hw/xen/xen-backend.h b/include/hw/xen/xen-backend.h
index aac2fd454d..0f01631ae7 100644
--- a/include/hw/xen/xen-backend.h
+++ b/include/hw/xen/xen-backend.h
@@ -33,6 +33,7 @@ XenDevice *xen_backend_get_device(XenBackendInstance
*backend);
void xen_backend_register(const XenBackendInfo *info);
const char **xen_backend_get_types(unsigned int *nr);
+bool xen_backend_exists(const char *type, const char *name);
void xen_backend_device_create(XenBus *xenbus, const char *type,
const char *name, QDict *opts, Error **errp);
bool xen_backend_try_device_destroy(XenDevice *xendev, Error **errp);
--
2.41.0
[PULL 14/15] xen-platform: unplug AHCI disks
From: David Woodhouse
To support Xen guests using the Q35 chipset, the unplug protocol needs
to also remove AHCI disks.
Make pci_xen_ide_unplug() more generic, iterating over the children
of the PCI device and destroying the "ide-hd" devices. That works the
same for both AHCI and IDE, as does the detection of the primary disk
as unit 0 on the bus named "ide.0".
Then pci_xen_ide_unplug() can be used for both AHCI and IDE devices.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/i386/xen/xen_platform.c | 68 +-
1 file changed, 45 insertions(+), 23 deletions(-)
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index e2dd1b536a..ef7d3fc05f 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -169,39 +169,60 @@ static void pci_unplug_nics(PCIBus *bus)
*
* [1]
https://xenbits.xen.org/gitweb/?p=xen.git;a=blob;f=docs/misc/hvm-emulated-unplug.pandoc
*/
-static void pci_xen_ide_unplug(PCIDevice *d, bool aux)
+struct ide_unplug_state {
+bool aux;
+int nr_unplugged;
+};
+
+static int ide_dev_unplug(DeviceState *dev, void *_st)
{
-DeviceState *dev = DEVICE(d);
-PCIIDEState *pci_ide;
-int i;
+struct ide_unplug_state *st = _st;
IDEDevice *idedev;
IDEBus *idebus;
BlockBackend *blk;
+int unit;
+
+idedev = IDE_DEVICE(object_dynamic_cast(OBJECT(dev), "ide-hd"));
+if (!idedev) {
+return 0;
+}
-pci_ide = PCI_IDE(dev);
+idebus = IDE_BUS(qdev_get_parent_bus(dev));
-for (i = aux ? 1 : 0; i < 4; i++) {
-idebus = _ide->bus[i / 2];
-blk = idebus->ifs[i % 2].blk;
+unit = (idedev == idebus->slave);
+assert(unit || idedev == idebus->master);
-if (blk && idebus->ifs[i % 2].drive_kind != IDE_CD) {
-if (!(i % 2)) {
-idedev = idebus->master;
-} else {
-idedev = idebus->slave;
-}
+if (st->aux && !unit && !strcmp(BUS(idebus)->name, "ide.0")) {
+return 0;
+}
-blk_drain(blk);
-blk_flush(blk);
+blk = idebus->ifs[unit].blk;
+if (blk) {
+blk_drain(blk);
+blk_flush(blk);
-blk_detach_dev(blk, DEVICE(idedev));
-idebus->ifs[i % 2].blk = NULL;
-idedev->conf.blk = NULL;
-monitor_remove_blk(blk);
-blk_unref(blk);
-}
+blk_detach_dev(blk, DEVICE(idedev));
+idebus->ifs[unit].blk = NULL;
+idedev->conf.blk = NULL;
+monitor_remove_blk(blk);
+blk_unref(blk);
+}
+
+object_unparent(OBJECT(dev));
+st->nr_unplugged++;
+
+return 0;
+}
+
+static void pci_xen_ide_unplug(PCIDevice *d, bool aux)
+{
+struct ide_unplug_state st = { aux, 0 };
+DeviceState *dev = DEVICE(d);
+
+qdev_walk_children(dev, NULL, NULL, ide_dev_unplug, NULL, );
+if (st.nr_unplugged) {
+pci_device_reset(d);
}
-pci_device_reset(d);
}
static void unplug_disks(PCIBus *b, PCIDevice *d, void *opaque)
@@ -216,6 +237,7 @@ static void unplug_disks(PCIBus *b, PCIDevice *d, void
*opaque)
switch (pci_get_word(d->config + PCI_CLASS_DEVICE)) {
case PCI_CLASS_STORAGE_IDE:
+case PCI_CLASS_STORAGE_SATA:
pci_xen_ide_unplug(d, aux);
break;
--
2.41.0
[PULL 03/15] include: update Xen public headers to Xen 4.17.2 release
From: David Woodhouse
... in order to advertise the XEN_HVM_CPUID_UPCALL_VECTOR feature,
which will come in a subsequent commit.
Signed-off-by: David Woodhouse
Acked-by: Paul Durrant
---
hw/i386/kvm/xen_xenstore.c| 2 +-
include/hw/xen/interface/arch-arm.h | 37 +++---
include/hw/xen/interface/arch-x86/cpuid.h | 31 +---
.../hw/xen/interface/arch-x86/xen-x86_32.h| 19 +--
.../hw/xen/interface/arch-x86/xen-x86_64.h| 19 +--
include/hw/xen/interface/arch-x86/xen.h | 26 ++
include/hw/xen/interface/event_channel.h | 19 +--
include/hw/xen/interface/features.h | 19 +--
include/hw/xen/interface/grant_table.h| 19 +--
include/hw/xen/interface/hvm/hvm_op.h | 19 +--
include/hw/xen/interface/hvm/params.h | 19 +--
include/hw/xen/interface/io/blkif.h | 27 --
include/hw/xen/interface/io/console.h | 19 +--
include/hw/xen/interface/io/fbif.h| 19 +--
include/hw/xen/interface/io/kbdif.h | 19 +--
include/hw/xen/interface/io/netif.h | 25 +++---
include/hw/xen/interface/io/protocols.h | 19 +--
include/hw/xen/interface/io/ring.h| 49 ++-
include/hw/xen/interface/io/usbif.h | 19 +--
include/hw/xen/interface/io/xenbus.h | 19 +--
include/hw/xen/interface/io/xs_wire.h | 36 ++
include/hw/xen/interface/memory.h | 30 +---
include/hw/xen/interface/physdev.h| 23 ++---
include/hw/xen/interface/sched.h | 19 +--
include/hw/xen/interface/trace.h | 19 +--
include/hw/xen/interface/vcpu.h | 19 +--
include/hw/xen/interface/version.h| 19 +--
include/hw/xen/interface/xen-compat.h | 19 +--
include/hw/xen/interface/xen.h| 19 +--
29 files changed, 124 insertions(+), 523 deletions(-)
diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 8e716a7009..831da535fc 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -331,7 +331,7 @@ static void xs_error(XenXenstoreState *s, unsigned int id,
const char *errstr = NULL;
for (unsigned int i = 0; i < ARRAY_SIZE(xsd_errors); i++) {
-struct xsd_errors *xsd_error = _errors[i];
+const struct xsd_errors *xsd_error = _errors[i];
if (xsd_error->errnum == errnum) {
errstr = xsd_error->errstring;
diff --git a/include/hw/xen/interface/arch-arm.h
b/include/hw/xen/interface/arch-arm.h
index 94b31511dd..1528ced509 100644
--- a/include/hw/xen/interface/arch-arm.h
+++ b/include/hw/xen/interface/arch-arm.h
@@ -1,26 +1,9 @@
+/* SPDX-License-Identifier: MIT */
/**
* arch-arm.h
*
* Guest OS interface to ARM Xen.
*
- * Permission is hereby granted, free of charge, to any person obtaining a copy
- * of this software and associated documentation files (the "Software"), to
- * deal in the Software without restriction, including without limitation the
- * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or
- * sell copies of the Software, and to permit persons to whom the Software is
- * furnished to do so, subject to the following conditions:
- *
- * The above copyright notice and this permission notice shall be included in
- * all copies or substantial portions of the Software.
- *
- * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
- * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
- * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
- * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
- * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
- * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
- * DEALINGS IN THE SOFTWARE.
- *
* Copyright 2011 (C) Citrix Systems
*/
@@ -361,6 +344,7 @@ typedef uint64_t xen_callback_t;
#define PSR_DBG_MASK(1<<9)/* arm64: Debug Exception mask */
#define PSR_IT_MASK (0x0600fc00) /* Thumb If-Then Mask */
#define PSR_JAZELLE (1<<24) /* Jazelle Mode */
+#define PSR_Z (1<<30) /* Zero condition flag */
/* 32 bit modes */
#define PSR_MODE_USR 0x10
@@ -383,7 +367,15 @@ typedef uint64_t xen_callback_t;
#define PSR_MODE_EL1t 0x04
#define PSR_MODE_EL0t 0x00
-#define PSR_GUEST32_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC)
+/*
+ * We set PSR_Z to be able to boot Linux kernel versions with an invalid
+ * encoding of the first 8 NOP instructions. See commit a92882a4d270 in
+ * Linux.
+ *
+ * Note that PSR_Z is also set by U-Boot and QEMU -kernel when loading
+ * zImage kernels on aarch32.
+ */
+#define PSR_GUEST32_INIT
[PULL 07/15] hw/xen: add get_frontend_path() method to XenDeviceClass
From: David Woodhouse
The primary Xen console is special. The guest's side is set up for it by
the toolstack automatically and not by the standard PV init sequence.
Accordingly, its *frontend* doesn't appear in …/device/console/0 either;
instead it appears under …/console in the guest's XenStore node.
To allow the Xen console driver to override the frontend path for the
primary console, add a method to the XenDeviceClass which can be used
instead of the standard xen_device_get_frontend_path()
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/xen/xen-bus.c | 11 ++-
include/hw/xen/xen-bus.h | 2 ++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index ece8ec40cd..12ff782005 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -711,8 +711,17 @@ static void xen_device_frontend_create(XenDevice *xendev,
Error **errp)
{
ERRP_GUARD();
XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(xendev)));
+XenDeviceClass *xendev_class = XEN_DEVICE_GET_CLASS(xendev);
-xendev->frontend_path = xen_device_get_frontend_path(xendev);
+if (xendev_class->get_frontend_path) {
+xendev->frontend_path = xendev_class->get_frontend_path(xendev, errp);
+if (!xendev->frontend_path) {
+error_prepend(errp, "failed to create frontend: ");
+return;
+}
+} else {
+xendev->frontend_path = xen_device_get_frontend_path(xendev);
+}
/*
* The frontend area may have already been created by a legacy
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index f435898164..eb440880b5 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -33,6 +33,7 @@ struct XenDevice {
};
typedef struct XenDevice XenDevice;
+typedef char *(*XenDeviceGetFrontendPath)(XenDevice *xendev, Error **errp);
typedef char *(*XenDeviceGetName)(XenDevice *xendev, Error **errp);
typedef void (*XenDeviceRealize)(XenDevice *xendev, Error **errp);
typedef void (*XenDeviceFrontendChanged)(XenDevice *xendev,
@@ -46,6 +47,7 @@ struct XenDeviceClass {
/*< public >*/
const char *backend;
const char *device;
+XenDeviceGetFrontendPath get_frontend_path;
XenDeviceGetName get_name;
XenDeviceRealize realize;
XenDeviceFrontendChanged frontend_changed;
--
2.41.0
[PULL 10/15] hw/xen: add support for Xen primary console in emulated mode
From: David Woodhouse
The primary console is special because the toolstack maps a page into
the guest for its ring, and also allocates the guest-side event channel.
The guest's grant table is even primed to export that page using a known
grant ref#. Add support for all that in emulated mode, so that we can
have a primary console.
For reasons unclear, the backends running under real Xen don't just use
a mapping of the well-known GNTTAB_RESERVED_CONSOLE grant ref (which
would also be in the ring-ref node in XenStore). Instead, the toolstack
sets the ring-ref node of the primary console to the GFN of the guest
page. The backend is expected to handle that special case and map it
with foreignmem operations instead.
We don't have an implementation of foreignmem ops for emulated Xen mode,
so just make it map GNTTAB_RESERVED_CONSOLE instead. This would probably
work for real Xen too, but we can't work out how to make real Xen create
a primary console of type "ioemu" to make QEMU drive it, so we can't
test that; might as well leave it as it is for now under Xen.
Now at last we can boot the Xen PV shim and run PV kernels in QEMU.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/char/xen_console.c | 78
hw/i386/kvm/meson.build | 1 +
hw/i386/kvm/trace-events | 2 +
hw/i386/kvm/xen-stubs.c | 8 ++
hw/i386/kvm/xen_gnttab.c | 7 +-
hw/i386/kvm/xen_primary_console.c | 193 ++
hw/i386/kvm/xen_primary_console.h | 23
hw/i386/kvm/xen_xenstore.c| 10 ++
hw/xen/xen-bus.c | 5 +
include/hw/xen/xen-bus.h | 1 +
target/i386/kvm/xen-emu.c | 23 +++-
11 files changed, 328 insertions(+), 23 deletions(-)
create mode 100644 hw/i386/kvm/xen_primary_console.c
create mode 100644 hw/i386/kvm/xen_primary_console.h
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 4a419dc287..5cbee2f184 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -33,6 +33,8 @@
#include "hw/qdev-properties-system.h"
#include "hw/xen/interface/io/console.h"
#include "hw/xen/interface/io/xs_wire.h"
+#include "hw/xen/interface/grant_table.h"
+#include "hw/i386/kvm/xen_primary_console.h"
#include "trace.h"
struct buffer {
@@ -230,24 +232,47 @@ static bool xen_console_connect(XenDevice *xendev, Error
**errp)
return false;
}
-if (!con->dev) {
-xen_pfn_t mfn = (xen_pfn_t)con->ring_ref;
-con->sring = qemu_xen_foreignmem_map(xendev->frontend_id, NULL,
- PROT_READ | PROT_WRITE,
- 1, , NULL);
-if (!con->sring) {
-error_setg(errp, "failed to map console page");
-return false;
+switch (con->dev) {
+case 0:
+/*
+ * The primary console is special. For real Xen the ring-ref is
+ * actually a GFN which needs to be mapped as foreignmem.
+ */
+if (xen_mode != XEN_EMULATE) {
+xen_pfn_t mfn = (xen_pfn_t)con->ring_ref;
+con->sring = qemu_xen_foreignmem_map(xendev->frontend_id, NULL,
+ PROT_READ | PROT_WRITE,
+ 1, , NULL);
+if (!con->sring) {
+error_setg(errp, "failed to map console page");
+return false;
+}
+break;
}
-} else {
+
+/*
+ * For Xen emulation, we still follow the convention of ring-ref
+ * holding the GFN, but we map the fixed GNTTAB_RESERVED_CONSOLE
+ * grant ref because there is no implementation of foreignmem
+ * operations for emulated mode. The emulation code which handles
+ * the guest-side page and event channel also needs to be informed
+ * of the backend event channel port, in order to reconnect to it
+ * after a soft reset.
+ */
+xen_primary_console_set_be_port(
+xen_event_channel_get_local_port(con->event_channel));
+con->ring_ref = GNTTAB_RESERVED_CONSOLE;
+/* fallthrough */
+default:
con->sring = xen_device_map_grant_refs(xendev,
>ring_ref, 1,
PROT_READ | PROT_WRITE,
errp);
if (!con->sring) {
-error_prepend(errp, "failed to map grant ref: ");
+error_prepend(errp, "failed to map console grant ref: ");
return false;
}
+break;
}
trace_xen_console_connect(con->dev, con->ring_ref, port,
@@ -272,10 +297,14 @@ static void xen_console_disconnect(XenDevice *xendev,
Error **errp)
xen_device_unbind_event_channel(xendev, con->event_channel,
errp);
con->event_channel =
[PULL 05/15] hw/xen: populate store frontend nodes with XenStore PFN/port
From: David Woodhouse
This is kind of redundant since without being able to get these through
some other method (HVMOP_get_param) the guest wouldn't be able to access
XenStore in order to find them.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/i386/kvm/xen_xenstore.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/hw/i386/kvm/xen_xenstore.c b/hw/i386/kvm/xen_xenstore.c
index 831da535fc..b7c0407765 100644
--- a/hw/i386/kvm/xen_xenstore.c
+++ b/hw/i386/kvm/xen_xenstore.c
@@ -1434,6 +1434,7 @@ static void alloc_guest_port(XenXenstoreState *s)
int xen_xenstore_reset(void)
{
XenXenstoreState *s = xen_xenstore_singleton;
+GList *perms;
int err;
if (!s) {
@@ -1461,6 +1462,16 @@ int xen_xenstore_reset(void)
}
s->be_port = err;
+/* Create frontend store nodes */
+perms = g_list_append(NULL, xs_perm_as_string(XS_PERM_NONE, DOMID_QEMU));
+perms = g_list_append(perms, xs_perm_as_string(XS_PERM_READ, xen_domid));
+
+relpath_printf(s, perms, "store/port", "%u", s->guest_port);
+relpath_printf(s, perms, "store/ring-ref", "%lu",
+ XEN_SPECIAL_PFN(XENSTORE));
+
+g_list_free_full(perms, g_free);
+
/*
* We don't actually access the guest's page through the grant, because
* this isn't real Xen, and we can just use the page we gave it in the
--
2.41.0
[PULL 11/15] hw/xen: only remove peers of PCI NICs on unplug
From: David Woodhouse
When the Xen guest asks to unplug *emulated* NICs, it's kind of unhelpful
also to unplug the peer of the *Xen* PV NIC.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/i386/xen/xen_platform.c | 9 +++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/hw/i386/xen/xen_platform.c b/hw/i386/xen/xen_platform.c
index 17457ff3de..e2dd1b536a 100644
--- a/hw/i386/xen/xen_platform.c
+++ b/hw/i386/xen/xen_platform.c
@@ -140,9 +140,14 @@ static void unplug_nic(PCIBus *b, PCIDevice *d, void *o)
/* Remove the peer of the NIC device. Normally, this would be a tap device. */
static void del_nic_peer(NICState *nic, void *opaque)
{
-NetClientState *nc;
+NetClientState *nc = qemu_get_queue(nic);
+ObjectClass *klass = module_object_class_by_name(nc->model);
+
+/* Only delete peers of PCI NICs that we're about to delete */
+if (!klass || !object_class_dynamic_cast(klass, TYPE_PCI_DEVICE)) {
+return;
+}
-nc = qemu_get_queue(nic);
if (nc->peer)
qemu_del_net_client(nc->peer);
}
--
2.41.0
[PULL 13/15] hw/i386/pc: support '-nic' for xen-net-device
From: David Woodhouse
The default NIC creation seems a bit hackish to me. I don't understand
why each platform has to call pci_nic_init_nofail() from a point in the
code where it actually has a pointer to the PCI bus, and then we have
the special cases for things like ne2k_isa.
If qmp_device_add() can *find* the appropriate bus and instantiate
the device on it, why can't we just do that from generic code for
creating the default NICs too?
But that isn't a yak I want to shave today. Add a xenbus field to the
PCMachineState so that it can make its way from pc_basic_device_init()
to pc_nic_init() and be handled as a special case like ne2k_isa is.
Now we can launch emulated Xen guests with '-nic user'.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/i386/pc.c | 11 ---
hw/i386/pc_piix.c| 2 +-
hw/i386/pc_q35.c | 2 +-
hw/xen/xen-bus.c | 4 +++-
include/hw/i386/pc.h | 4 +++-
include/hw/xen/xen-bus.h | 2 +-
6 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 1aef21aa2c..188bc9d0f8 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1261,7 +1261,7 @@ void pc_basic_device_init(struct PCMachineState *pcms,
if (pcms->bus) {
pci_create_simple(pcms->bus, -1, "xen-platform");
}
-xen_bus_init();
+pcms->xenbus = xen_bus_init();
xen_be_init();
}
#endif
@@ -1289,7 +1289,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
pcms->vmport != ON_OFF_AUTO_ON);
}
-void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus)
+void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
+ BusState *xen_bus)
{
MachineClass *mc = MACHINE_CLASS(pcmc);
int i;
@@ -1299,7 +1300,11 @@ void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus,
PCIBus *pci_bus)
NICInfo *nd = _table[i];
const char *model = nd->model ? nd->model : mc->default_nic;
-if (g_str_equal(model, "ne2k_isa")) {
+if (xen_bus && (!nd->model || g_str_equal(model, "xen-net-device"))) {
+DeviceState *dev = qdev_new("xen-net-device");
+qdev_set_nic_properties(dev, nd);
+qdev_realize_and_unref(dev, xen_bus, _fatal);
+} else if (g_str_equal(model, "ne2k_isa")) {
pc_init_ne2k_isa(isa_bus, nd);
} else {
pci_nic_init_nofail(nd, pci_bus, model, NULL);
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 26e161beb9..eace854335 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -342,7 +342,7 @@ static void pc_init1(MachineState *machine,
pc_basic_device_init(pcms, isa_bus, x86ms->gsi, rtc_state, true,
0x4);
-pc_nic_init(pcmc, isa_bus, pci_bus);
+pc_nic_init(pcmc, isa_bus, pci_bus, pcms->xenbus);
if (pcmc->pci_enabled) {
pc_cmos_init(pcms, idebus[0], idebus[1], rtc_state);
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 597943ff1b..4f3e5412f6 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -340,7 +340,7 @@ static void pc_q35_init(MachineState *machine)
/* the rest devices to which pci devfn is automatically assigned */
pc_vga_init(isa_bus, host_bus);
-pc_nic_init(pcmc, isa_bus, host_bus);
+pc_nic_init(pcmc, isa_bus, host_bus, pcms->xenbus);
if (machine->nvdimms_state->is_enabled) {
nvdimm_init_acpi_state(machine->nvdimms_state, system_io,
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index cc6f1b362f..4973e7d9c9 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -1133,11 +1133,13 @@ static void xen_register_types(void)
type_init(xen_register_types)
-void xen_bus_init(void)
+BusState *xen_bus_init(void)
{
DeviceState *dev = qdev_new(TYPE_XEN_BRIDGE);
BusState *bus = qbus_new(TYPE_XEN_BUS, dev, NULL);
sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), _fatal);
qbus_set_bus_hotplug_handler(bus);
+
+return bus;
}
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 29a9724524..a10ceeabbf 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -33,6 +33,7 @@ typedef struct PCMachineState {
/* Pointers to devices and objects: */
PCIBus *bus;
+BusState *xenbus;
I2CBus *smbus;
PFlashCFI01 *flash[2];
ISADevice *pcspk;
@@ -184,7 +185,8 @@ void pc_basic_device_init(struct PCMachineState *pcms,
void pc_cmos_init(PCMachineState *pcms,
BusState *ide0, BusState *ide1,
ISADevice *s);
-void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus);
+void pc_nic_init(PCMachineClass *pcmc, ISABus *isa_bus, PCIBus *pci_bus,
+ BusState *xen_bus);
void pc_i8259_create(ISABus *isa_bus, qemu_irq *i8259_irqs);
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 38d40afa37..334ddd1ff6 100644
--- a/include/hw/xen/xen-bus.h
+++
[PULL 12/15] hw/xen: update Xen PV NIC to XenDevice model
From: David Woodhouse
This allows us to use Xen PV networking with emulated Xen guests, and to
add them on the command line or hotplug.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/net/meson.build| 2 +-
hw/net/trace-events | 11 +
hw/net/xen_nic.c | 484 +-
hw/xenpv/xen_machine_pv.c | 1 -
4 files changed, 381 insertions(+), 117 deletions(-)
diff --git a/hw/net/meson.build b/hw/net/meson.build
index 2632634df3..f64651c467 100644
--- a/hw/net/meson.build
+++ b/hw/net/meson.build
@@ -1,5 +1,5 @@
system_ss.add(when: 'CONFIG_DP8393X', if_true: files('dp8393x.c'))
-system_ss.add(when: 'CONFIG_XEN', if_true: files('xen_nic.c'))
+system_ss.add(when: 'CONFIG_XEN_BUS', if_true: files('xen_nic.c'))
system_ss.add(when: 'CONFIG_NE2000_COMMON', if_true: files('ne2000.c'))
# PCI network cards
diff --git a/hw/net/trace-events b/hw/net/trace-events
index 3abfd65e5b..3097742cc0 100644
--- a/hw/net/trace-events
+++ b/hw/net/trace-events
@@ -482,3 +482,14 @@ dp8393x_receive_oversize(int size) "oversize packet,
pkt_size is %d"
dp8393x_receive_not_netcard(void) "packet not for netcard"
dp8393x_receive_packet(int crba) "Receive packet at 0x%"PRIx32
dp8393x_receive_write_status(int crba) "Write status at 0x%"PRIx32
+
+# xen_nic.c
+xen_netdev_realize(int dev, const char *info, const char *peer) "vif%u info
'%s' peer '%s'"
+xen_netdev_unrealize(int dev) "vif%u"
+xen_netdev_create(int dev) "vif%u"
+xen_netdev_destroy(int dev) "vif%u"
+xen_netdev_disconnect(int dev) "vif%u"
+xen_netdev_connect(int dev, unsigned int tx, unsigned int rx, int port) "vif%u
tx %u rx %u port %u"
+xen_netdev_frontend_changed(const char *dev, int state) "vif%s state %d"
+xen_netdev_tx(int dev, int ref, int off, int len, unsigned int flags, const
char *c, const char *d, const char *m, const char *e) "vif%u ref %u off %u len
%u flags 0x%x%s%s%s%s"
+xen_netdev_rx(int dev, int idx, int status, int flags) "vif%u idx %d status %d
flags 0x%x"
diff --git a/hw/net/xen_nic.c b/hw/net/xen_nic.c
index 9bbf6599fc..af4ba3f1e6 100644
--- a/hw/net/xen_nic.c
+++ b/hw/net/xen_nic.c
@@ -20,6 +20,13 @@
*/
#include "qemu/osdep.h"
+#include "qemu/main-loop.h"
+#include "qemu/cutils.h"
+#include "qemu/log.h"
+#include "qemu/qemu-print.h"
+#include "qapi/qmp/qdict.h"
+#include "qapi/error.h"
+
#include
#include
#include
@@ -27,18 +34,26 @@
#include "net/net.h"
#include "net/checksum.h"
#include "net/util.h"
-#include "hw/xen/xen-legacy-backend.h"
+
+#include "hw/xen/xen-backend.h"
+#include "hw/xen/xen-bus-helper.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
#include "hw/xen/interface/io/netif.h"
+#include "hw/xen/interface/io/xs_wire.h"
+
+#include "trace.h"
/* - */
struct XenNetDev {
-struct XenLegacyDevice xendev; /* must be first */
-char *mac;
+struct XenDevice xendev; /* must be first */
+XenEventChannel *event_channel;
+int dev;
int tx_work;
-int tx_ring_ref;
-int rx_ring_ref;
+unsigned int tx_ring_ref;
+unsigned int rx_ring_ref;
struct netif_tx_sring *txs;
struct netif_rx_sring *rxs;
netif_tx_back_ring_t tx_ring;
@@ -47,6 +62,11 @@ struct XenNetDev {
NICState *nic;
};
+typedef struct XenNetDev XenNetDev;
+
+#define TYPE_XEN_NET_DEVICE "xen-net-device"
+OBJECT_DECLARE_SIMPLE_TYPE(XenNetDev, XEN_NET_DEVICE)
+
/* - */
static void net_tx_response(struct XenNetDev *netdev, netif_tx_request_t *txp,
int8_t st)
@@ -68,7 +88,8 @@ static void net_tx_response(struct XenNetDev *netdev,
netif_tx_request_t *txp, i
netdev->tx_ring.rsp_prod_pvt = ++i;
RING_PUSH_RESPONSES_AND_CHECK_NOTIFY(>tx_ring, notify);
if (notify) {
-xen_pv_send_notify(>xendev);
+xen_device_notify_event_channel(XEN_DEVICE(netdev),
+netdev->event_channel, NULL);
}
if (i == netdev->tx_ring.req_cons) {
@@ -104,13 +125,16 @@ static void net_tx_error(struct XenNetDev *netdev,
netif_tx_request_t *txp, RING
#endif
}
-static void net_tx_packets(struct XenNetDev *netdev)
+static bool net_tx_packets(struct XenNetDev *netdev)
{
+bool done_something = false;
netif_tx_request_t txreq;
RING_IDX rc, rp;
void *page;
void *tmpbuf = NULL;
+assert(qemu_mutex_iothread_locked());
+
for (;;) {
rc = netdev->tx_ring.req_cons;
rp = netdev->tx_ring.sring->req_prod;
@@ -122,49 +146,52 @@ static void net_tx_packets(struct XenNetDev *netdev)
}
memcpy(, RING_GET_REQUEST(>tx_ring, rc),
sizeof(txreq));
netdev->tx_ring.req_cons = ++rc;
+done_something = true;
#if 1
[PULL 00/15] xenfv.for-upstream queue
The following changes since commit 8aba939e77daca10eac99d9d467f65ba7df5ab3e: Merge tag 'pull-riscv-to-apply-20231107' of https://github.com/alistair23/qemu into staging (2023-11-07 11:08:16 +0800) are available in the Git repository at: git://git.infradead.org/users/dwmw2/qemu.git tags/pull-xenfv.for-upstream-20231107 for you to fetch changes up to cc9d10b9e89f0325c1a14955534d6b28ea586fba: docs: update Xen-on-KVM documentation (2023-11-07 08:58:02 +) Xen PV guest support for 8.2 Add Xen PV console and network support, the former of which enables the Xen "PV shim" to be used to support PV guests. Also clean up the block support and make it work when the user passes just 'drive file=IMAGE,if=xen' on the command line. Update the documentation to reflect all of these, taking the opportunity to simplify what it says about q35 by making unplug work for AHCI. Ignore the VCPU_SSHOTTMR_future timer flag, and advertise the 'fixed' per-vCPU upcall vector support, as newer upstream Xen do. David Woodhouse (15): i386/xen: Ignore VCPU_SSHOTTMR_future flag in set_singleshot_timer() hw/xen: Clean up event channel 'type_val' handling to use union include: update Xen public headers to Xen 4.17.2 release i386/xen: advertise XEN_HVM_CPUID_UPCALL_VECTOR in CPUID hw/xen: populate store frontend nodes with XenStore PFN/port hw/xen: automatically assign device index to block devices hw/xen: add get_frontend_path() method to XenDeviceClass hw/xen: do not repeatedly try to create a failing backend device hw/xen: update Xen console to XenDevice model hw/xen: add support for Xen primary console in emulated mode hw/xen: only remove peers of PCI NICs on unplug hw/xen: update Xen PV NIC to XenDevice model hw/i386/pc: support '-nic' for xen-net-device xen-platform: unplug AHCI disks docs: update Xen-on-KVM documentation MAINTAINERS| 2 +- blockdev.c | 15 +- docs/system/i386/xen.rst | 107 +++-- hw/block/xen-block.c | 118 - hw/char/trace-events | 8 + hw/char/xen_console.c | 572 +++-- hw/i386/kvm/meson.build| 1 + hw/i386/kvm/trace-events | 2 + hw/i386/kvm/xen-stubs.c| 8 + hw/i386/kvm/xen_evtchn.c | 151 +++ hw/i386/kvm/xen_gnttab.c | 7 +- hw/i386/kvm/xen_primary_console.c | 193 + hw/i386/kvm/xen_primary_console.h | 23 + hw/i386/kvm/xen_xenstore.c | 23 +- hw/i386/pc.c | 11 +- hw/i386/pc_piix.c | 2 +- hw/i386/pc_q35.c | 2 +- hw/i386/xen/xen_platform.c | 77 ++-- hw/net/meson.build | 2 +- hw/net/trace-events| 11 + hw/net/xen_nic.c | 484 - hw/xen/xen-backend.c | 27 +- hw/xen/xen-bus.c | 23 +- hw/xen/xen-legacy-backend.c| 1 - hw/xen/xen_devconfig.c | 28 -- hw/xenpv/xen_machine_pv.c | 10 - include/hw/i386/pc.h | 4 +- include/hw/xen/interface/arch-arm.h| 37 +- include/hw/xen/interface/arch-x86/cpuid.h | 31 +- include/hw/xen/interface/arch-x86/xen-x86_32.h | 19 +- include/hw/xen/interface/arch-x86/xen-x86_64.h | 19 +- include/hw/xen/interface/arch-x86/xen.h| 26 +- include/hw/xen/interface/event_channel.h | 19 +- include/hw/xen/interface/features.h| 19 +- include/hw/xen/interface/grant_table.h | 19 +- include/hw/xen/interface/hvm/hvm_op.h | 19 +- include/hw/xen/interface/hvm/params.h | 19 +- include/hw/xen/interface/io/blkif.h| 27 +- include/hw/xen/interface/io/console.h | 19 +- include/hw/xen/interface/io/fbif.h | 19 +- include/hw/xen/interface/io/kbdif.h| 19 +- include/hw/xen/interface/io/netif.h| 25 +- include/hw/xen/interface/io/protocols.h| 19 +- include/hw/xen/interface/io/ring.h | 49 +-- include/hw/xen/interface/io/usbif.h| 19 +- include/hw/xen/interface/io/xenbus.h | 19 +- include/hw/xen/interface/io/xs_wire.h | 36 +- include/hw/xen/interface/memory.h | 30 +- include/hw/xen/interface/physdev.h | 23 +- include/hw/xen/interface/sched.h | 19 +- include/hw/xen
[PULL 06/15] hw/xen: automatically assign device index to block devices
From: David Woodhouse
There's no need to force the user to assign a vdev. We can automatically
assign one, starting at xvda and searching until we find the first disk
name that's unused.
This means we can now allow '-drive if=xen,file=xxx' to work without an
explicit separate -driver argument, just like if=virtio.
Rip out the legacy handling from the xenpv machine, which was scribbling
over any disks configured by the toolstack, and didn't work with anything
but raw images.
Signed-off-by: David Woodhouse
Acked-by: Kevin Wolf
Reviewed-by: Paul Durrant
---
blockdev.c | 15 +++-
hw/block/xen-block.c| 118 ++--
hw/xen/xen_devconfig.c | 28 ---
hw/xenpv/xen_machine_pv.c | 9 ---
include/hw/xen/xen-legacy-backend.h | 1 -
5 files changed, 125 insertions(+), 46 deletions(-)
diff --git a/blockdev.c b/blockdev.c
index 1517dc6210..e9b7e38dc4 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -255,13 +255,13 @@ void drive_check_orphaned(void)
* Ignore default drives, because we create certain default
* drives unconditionally, then leave them unclaimed. Not the
* users fault.
- * Ignore IF_VIRTIO, because it gets desugared into -device,
- * so we can leave failing to -device.
+ * Ignore IF_VIRTIO or IF_XEN, because it gets desugared into
+ * -device, so we can leave failing to -device.
* Ignore IF_NONE, because leaving unclaimed IF_NONE remains
* available for device_add is a feature.
*/
if (dinfo->is_default || dinfo->type == IF_VIRTIO
-|| dinfo->type == IF_NONE) {
+|| dinfo->type == IF_XEN || dinfo->type == IF_NONE) {
continue;
}
if (!blk_get_attached_dev(blk)) {
@@ -977,6 +977,15 @@ DriveInfo *drive_new(QemuOpts *all_opts,
BlockInterfaceType block_default_type,
qemu_opt_set(devopts, "driver", "virtio-blk", _abort);
qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
_abort);
+} else if (type == IF_XEN) {
+QemuOpts *devopts;
+devopts = qemu_opts_create(qemu_find_opts("device"), NULL, 0,
+ _abort);
+qemu_opt_set(devopts, "driver",
+ (media == MEDIA_CDROM) ? "xen-cdrom" : "xen-disk",
+ _abort);
+qemu_opt_set(devopts, "drive", qdict_get_str(bs_opts, "id"),
+ _abort);
}
filename = qemu_opt_get(legacy_opts, "file");
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index bfa53960c3..6d64ede94f 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -27,13 +27,119 @@
#include "sysemu/block-backend.h"
#include "sysemu/iothread.h"
#include "dataplane/xen-block.h"
+#include "hw/xen/interface/io/xs_wire.h"
#include "trace.h"
+#define XVDA_MAJOR 202
+#define XVDQ_MAJOR (1 << 20)
+#define XVDBGQCV_MAJOR ((1 << 21) - 1)
+#define HDA_MAJOR 3
+#define HDC_MAJOR 22
+#define SDA_MAJOR 8
+
+
+static int vdev_to_diskno(unsigned int vdev_nr)
+{
+switch (vdev_nr >> 8) {
+case XVDA_MAJOR:
+case SDA_MAJOR:
+return (vdev_nr >> 4) & 0x15;
+
+case HDA_MAJOR:
+return (vdev_nr >> 6) & 1;
+
+case HDC_MAJOR:
+return ((vdev_nr >> 6) & 1) + 2;
+
+case XVDQ_MAJOR ... XVDBGQCV_MAJOR:
+return (vdev_nr >> 8) & 0xf;
+
+default:
+return -1;
+}
+}
+
+#define MAX_AUTO_VDEV 4096
+
+/*
+ * Find a free device name in the xvda → xvdfan range and set it in
+ * blockdev->props.vdev. Our definition of "free" is that there must
+ * be no other disk or partition with the same disk number.
+ *
+ * You are technically permitted to have all of hda, hda1, sda, sda1,
+ * xvda and xvda1 as *separate* PV block devices with separate backing
+ * stores. That doesn't make it a good idea. This code will skip xvda
+ * if *any* of those "conflicting" devices already exists.
+ *
+ * The limit of xvdfan (disk 4095) is fairly arbitrary just to avoid a
+ * stupidly sized bitmap, but Linux as of v6.6 doesn't support anything
+ * higher than that anyway.
+ */
+static bool xen_block_find_free_vdev(XenBlockDevice *blockdev, Error **errp)
+{
+XenBus *xenbus = XEN_BUS(qdev_get_parent_bus(DEVICE(blockdev)));
+unsigned long used_devs[BITS_TO_LONGS(MAX_AUTO_VDEV)];
+XenBlockVdev *vdev = >props.vdev;
+char fe_path[XENSTORE_ABS_PATH_MAX + 1];
+char **existing_frontends;
+unsigned int nr_existing = 0;
+unsigned int vdev_nr;
+int i, disk = 0;
+
+snprintf(fe_path, sizeof(fe_path), "/local/domain/%u/device/vbd",
+ blockdev->xendev.frontend_id);
+
+existing_frontends = qemu_xen_xs_directory(xenbus->xsh, XBT_NULL, fe_path,
+ _existing);
+if (!existing_frontends && errno != ENOENT) {
+error_setg_errno(errp, errno, "cannot read %s", fe_path);
+
[PULL 09/15] hw/xen: update Xen console to XenDevice model
From: David Woodhouse
This allows (non-primary) console devices to be created on the command
line and hotplugged.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/char/trace-events| 8 +
hw/char/xen_console.c | 532 +++-
hw/xen/xen-legacy-backend.c | 1 -
3 files changed, 411 insertions(+), 130 deletions(-)
diff --git a/hw/char/trace-events b/hw/char/trace-events
index babf4d35ea..7a398c82a5 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -105,3 +105,11 @@ cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
# sh_serial.c
sh_serial_read(char *id, unsigned size, uint64_t offs, uint64_t val) " %s size
%d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
sh_serial_write(char *id, unsigned size, uint64_t offs, uint64_t val) "%s size
%d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
+
+# xen_console.c
+xen_console_connect(unsigned int idx, unsigned int ring_ref, unsigned int
port, unsigned int limit) "idx %u ring_ref %u port %u limit %u"
+xen_console_disconnect(unsigned int idx) "idx %u"
+xen_console_unrealize(unsigned int idx) "idx %u"
+xen_console_realize(unsigned int idx, const char *chrdev) "idx %u chrdev %s"
+xen_console_device_create(unsigned int idx) "idx %u"
+xen_console_device_destroy(unsigned int idx) "idx %u"
diff --git a/hw/char/xen_console.c b/hw/char/xen_console.c
index 810dae3f44..4a419dc287 100644
--- a/hw/char/xen_console.c
+++ b/hw/char/xen_console.c
@@ -20,15 +20,20 @@
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include
#include
#include "qapi/error.h"
#include "sysemu/sysemu.h"
#include "chardev/char-fe.h"
-#include "hw/xen/xen-legacy-backend.h"
-
+#include "hw/xen/xen-backend.h"
+#include "hw/xen/xen-bus-helper.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
#include "hw/xen/interface/io/console.h"
+#include "hw/xen/interface/io/xs_wire.h"
+#include "trace.h"
struct buffer {
uint8_t *data;
@@ -39,16 +44,22 @@ struct buffer {
};
struct XenConsole {
-struct XenLegacyDevice xendev; /* must be first */
+struct XenDevice xendev; /* must be first */
+XenEventChannel *event_channel;
+int dev;
struct buffer buffer;
-char console[XEN_BUFSIZE];
-int ring_ref;
+char *fe_path;
+unsigned int ring_ref;
void *sring;
CharBackend chr;
int backlog;
};
+typedef struct XenConsole XenConsole;
+
+#define TYPE_XEN_CONSOLE_DEVICE "xen-console"
+OBJECT_DECLARE_SIMPLE_TYPE(XenConsole, XEN_CONSOLE_DEVICE)
-static void buffer_append(struct XenConsole *con)
+static bool buffer_append(XenConsole *con)
{
struct buffer *buffer = >buffer;
XENCONS_RING_IDX cons, prod, size;
@@ -60,7 +71,7 @@ static void buffer_append(struct XenConsole *con)
size = prod - cons;
if ((size == 0) || (size > sizeof(intf->out)))
-return;
+return false;
if ((buffer->capacity - buffer->size) < size) {
buffer->capacity += (size + 1024);
@@ -73,7 +84,7 @@ static void buffer_append(struct XenConsole *con)
xen_mb();
intf->out_cons = cons;
-xen_pv_send_notify(>xendev);
+xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
if (buffer->max_capacity &&
buffer->size > buffer->max_capacity) {
@@ -89,6 +100,7 @@ static void buffer_append(struct XenConsole *con)
if (buffer->consumed > buffer->max_capacity - over)
buffer->consumed = buffer->max_capacity - over;
}
+return true;
}
static void buffer_advance(struct buffer *buffer, size_t len)
@@ -100,7 +112,7 @@ static void buffer_advance(struct buffer *buffer, size_t
len)
}
}
-static int ring_free_bytes(struct XenConsole *con)
+static int ring_free_bytes(XenConsole *con)
{
struct xencons_interface *intf = con->sring;
XENCONS_RING_IDX cons, prod, space;
@@ -118,13 +130,13 @@ static int ring_free_bytes(struct XenConsole *con)
static int xencons_can_receive(void *opaque)
{
-struct XenConsole *con = opaque;
+XenConsole *con = opaque;
return ring_free_bytes(con);
}
static void xencons_receive(void *opaque, const uint8_t *buf, int len)
{
-struct XenConsole *con = opaque;
+XenConsole *con = opaque;
struct xencons_interface *intf = con->sring;
XENCONS_RING_IDX prod;
int i, max;
@@ -141,10 +153,10 @@ static void xencons_receive(void *opaque, const uint8_t
*buf, int len)
}
xen_wmb();
intf->in_prod = prod;
-xen_pv_send_notify(>xendev);
+xen_device_notify_event_channel(XEN_DEVICE(con), con->event_channel, NULL);
}
-static void xencons_send(struct XenConsole *con)
+static bool xencons_send(XenConsole *con)
{
ssize_t len, size;
@@ -159,174 +171,436 @@ static void xencons_send(struct XenConsole *con)
if (len < 1) {
if (!con->backlog) {
con->backlog = 1;
-
[PULL 04/15] i386/xen: advertise XEN_HVM_CPUID_UPCALL_VECTOR in CPUID
From: David Woodhouse
This will allow Linux guests (since v6.0) to use the per-vCPU upcall
vector delivered as MSI through the local APIC.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
target/i386/kvm/kvm.c | 4
1 file changed, 4 insertions(+)
diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
index 770e81d56e..11b8177eff 100644
--- a/target/i386/kvm/kvm.c
+++ b/target/i386/kvm/kvm.c
@@ -1837,6 +1837,10 @@ int kvm_arch_init_vcpu(CPUState *cs)
c->eax |= XEN_HVM_CPUID_VCPU_ID_PRESENT;
c->ebx = cs->cpu_index;
}
+
+if (cs->kvm_state->xen_version >= XEN_VERSION(4, 17)) {
+c->eax |= XEN_HVM_CPUID_UPCALL_VECTOR;
+}
}
r = kvm_xen_init_vcpu(cs);
--
2.41.0
[PULL 01/15] i386/xen: Ignore VCPU_SSHOTTMR_future flag in set_singleshot_timer()
From: David Woodhouse
Upstream Xen now ignores this flag¹, since the only guest kernel ever to
use it was buggy.
¹ https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=19c6cbd909
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
target/i386/kvm/xen-emu.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/target/i386/kvm/xen-emu.c b/target/i386/kvm/xen-emu.c
index 75b2c557b9..1dc9ab0d91 100644
--- a/target/i386/kvm/xen-emu.c
+++ b/target/i386/kvm/xen-emu.c
@@ -1077,17 +1077,13 @@ static int vcpuop_stop_periodic_timer(CPUState *target)
* Must always be called with xen_timers_lock held.
*/
static int do_set_singleshot_timer(CPUState *cs, uint64_t timeout_abs,
- bool future, bool linux_wa)
+ bool linux_wa)
{
CPUX86State *env = _CPU(cs)->env;
int64_t now = kvm_get_current_ns();
int64_t qemu_now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
int64_t delta = timeout_abs - now;
-if (future && timeout_abs < now) {
-return -ETIME;
-}
-
if (linux_wa && unlikely((int64_t)timeout_abs < 0 ||
(delta > 0 && (uint32_t)(delta >> 50) != 0))) {
/*
@@ -1129,9 +1125,13 @@ static int vcpuop_set_singleshot_timer(CPUState *cs,
uint64_t arg)
}
QEMU_LOCK_GUARD(_CPU(cs)->env.xen_timers_lock);
-return do_set_singleshot_timer(cs, sst.timeout_abs_ns,
- !!(sst.flags & VCPU_SSHOTTMR_future),
- false);
+
+/*
+ * We ignore the VCPU_SSHOTTMR_future flag, just as Xen now does.
+ * The only guest that ever used it, got it wrong.
+ * https://xenbits.xen.org/gitweb/?p=xen.git;a=commitdiff;h=19c6cbd909
+ */
+return do_set_singleshot_timer(cs, sst.timeout_abs_ns, false);
}
static int vcpuop_stop_singleshot_timer(CPUState *cs)
@@ -1156,7 +1156,7 @@ static bool kvm_xen_hcall_set_timer_op(struct
kvm_xen_exit *exit, X86CPU *cpu,
err = vcpuop_stop_singleshot_timer(CPU(cpu));
} else {
QEMU_LOCK_GUARD(_CPU(cpu)->env.xen_timers_lock);
-err = do_set_singleshot_timer(CPU(cpu), timeout, false, true);
+err = do_set_singleshot_timer(CPU(cpu), timeout, true);
}
exit->u.hcall.result = err;
return true;
@@ -1844,7 +1844,7 @@ int kvm_put_xen_state(CPUState *cs)
QEMU_LOCK_GUARD(>xen_timers_lock);
if (env->xen_singleshot_timer_ns) {
ret = do_set_singleshot_timer(cs, env->xen_singleshot_timer_ns,
-false, false);
+ false);
if (ret < 0) {
return ret;
}
--
2.41.0
[PULL 02/15] hw/xen: Clean up event channel 'type_val' handling to use union
From: David Woodhouse
A previous implementation of this stuff used a 64-bit field for all of
the port information (vcpu/type/type_val) and did atomic exchanges on
them. When I implemented that in Qemu I regretted my life choices and
just kept it simple with locking instead.
So there's no need for the XenEvtchnPort to be so simplistic. We can
use a union for the pirq/virq/interdomain information, which lets us
keep a separate bit for the 'remote domain' in interdomain ports. A
single bit is enough since the only possible targets are loopback or
qemu itself.
So now we can ditch PORT_INFO_TYPEVAL_REMOTE_QEMU and the horrid
manual masking, although the in-memory representation is identical
so there's no change in the saved state ABI.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
hw/i386/kvm/xen_evtchn.c | 151 ++-
1 file changed, 70 insertions(+), 81 deletions(-)
diff --git a/hw/i386/kvm/xen_evtchn.c b/hw/i386/kvm/xen_evtchn.c
index b2b4be9983..02b8cbf8df 100644
--- a/hw/i386/kvm/xen_evtchn.c
+++ b/hw/i386/kvm/xen_evtchn.c
@@ -58,7 +58,15 @@ OBJECT_DECLARE_SIMPLE_TYPE(XenEvtchnState, XEN_EVTCHN)
typedef struct XenEvtchnPort {
uint32_t vcpu; /* Xen/ACPI vcpu_id */
uint16_t type; /* EVTCHNSTAT_ */
-uint16_t type_val; /* pirq# / virq# / remote port according to type */
+union {
+uint16_t val; /* raw value for serialization etc. */
+uint16_t pirq;
+uint16_t virq;
+struct {
+uint16_t port:15;
+uint16_t to_qemu:1; /* Only two targets; qemu or loopback */
+} interdomain;
+} u;
} XenEvtchnPort;
/* 32-bit compatibility definitions, also used natively in 32-bit build */
@@ -105,14 +113,6 @@ struct xenevtchn_handle {
int fd;
};
-/*
- * For unbound/interdomain ports there are only two possible remote
- * domains; self and QEMU. Use a single high bit in type_val for that,
- * and the low bits for the remote port number (or 0 for unbound).
- */
-#define PORT_INFO_TYPEVAL_REMOTE_QEMU 0x8000
-#define PORT_INFO_TYPEVAL_REMOTE_PORT_MASK 0x7FFF
-
/*
* These 'emuirq' values are used by Xen in the LM stream... and yes, I am
* insane enough to think about guest-transparent live migration from actual
@@ -210,16 +210,16 @@ static int xen_evtchn_post_load(void *opaque, int
version_id)
XenEvtchnPort *p = >port_table[i];
if (p->type == EVTCHNSTAT_pirq) {
-assert(p->type_val);
-assert(p->type_val < s->nr_pirqs);
+assert(p->u.pirq);
+assert(p->u.pirq < s->nr_pirqs);
/*
* Set the gsi to IRQ_UNBOUND; it may be changed to an actual
* GSI# below, or to IRQ_MSI_EMU when the MSI table snooping
* catches up with it.
*/
-s->pirq[p->type_val].gsi = IRQ_UNBOUND;
-s->pirq[p->type_val].port = i;
+s->pirq[p->u.pirq].gsi = IRQ_UNBOUND;
+s->pirq[p->u.pirq].port = i;
}
}
/* Rebuild s->pirq[].gsi mapping */
@@ -243,7 +243,7 @@ static const VMStateDescription xen_evtchn_port_vmstate = {
.fields = (VMStateField[]) {
VMSTATE_UINT32(vcpu, XenEvtchnPort),
VMSTATE_UINT16(type, XenEvtchnPort),
-VMSTATE_UINT16(type_val, XenEvtchnPort),
+VMSTATE_UINT16(u.val, XenEvtchnPort),
VMSTATE_END_OF_LIST()
}
};
@@ -605,14 +605,13 @@ static void unbind_backend_ports(XenEvtchnState *s)
for (i = 1; i < s->nr_ports; i++) {
p = >port_table[i];
-if (p->type == EVTCHNSTAT_interdomain &&
-(p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU)) {
-evtchn_port_t be_port = p->type_val &
PORT_INFO_TYPEVAL_REMOTE_PORT_MASK;
+if (p->type == EVTCHNSTAT_interdomain && p->u.interdomain.to_qemu) {
+evtchn_port_t be_port = p->u.interdomain.port;
if (s->be_handles[be_port]) {
/* This part will be overwritten on the load anyway. */
p->type = EVTCHNSTAT_unbound;
-p->type_val = PORT_INFO_TYPEVAL_REMOTE_QEMU;
+p->u.interdomain.port = 0;
/* Leave the backend port open and unbound too. */
if (kvm_xen_has_cap(EVTCHN_SEND)) {
@@ -650,30 +649,22 @@ int xen_evtchn_status_op(struct evtchn_status *status)
switch (p->type) {
case EVTCHNSTAT_unbound:
-if (p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU) {
-status->u.unbound.dom = DOMID_QEMU;
-} else {
-status->u.unbound.dom = xen_domid;
-}
+status->u.unbound.dom = p->u.interdomain.to_qemu ? DOMID_QEMU
+ : xen_domid;
break;
case EVTCHNSTAT_interdomain:
-if (p->type_val & PORT_INFO_TYPEVAL_REMOTE_QEMU) {
-status->u.interdomain.dom = DOMID_QEMU;
-} else {
-
[PULL 15/15] docs: update Xen-on-KVM documentation
From: David Woodhouse
Add notes about console and network support, and how to launch PV guests.
Clean up the disk configuration examples now that that's simpler, and
remove the comment about IDE unplug on q35/AHCI now that it's fixed.
Update the -initrd option documentation to explain how to quote commas
in module command lines, and reference it when documenting PV guests.
Also update stale avocado test filename in MAINTAINERS.
Signed-off-by: David Woodhouse
Reviewed-by: Paul Durrant
---
MAINTAINERS | 2 +-
docs/system/i386/xen.rst | 107 +--
qemu-options.hx | 12 -
3 files changed, 90 insertions(+), 31 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 59b92ee640..fd6b362311 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -490,7 +490,7 @@ S: Supported
F: include/sysemu/kvm_xen.h
F: target/i386/kvm/xen*
F: hw/i386/kvm/xen*
-F: tests/avocado/xen_guest.py
+F: tests/avocado/kvm_xen_guest.py
Guest CPU Cores (other accelerators)
diff --git a/docs/system/i386/xen.rst b/docs/system/i386/xen.rst
index f06765e88c..81898768ba 100644
--- a/docs/system/i386/xen.rst
+++ b/docs/system/i386/xen.rst
@@ -15,46 +15,24 @@ Setup
-
Xen mode is enabled by setting the ``xen-version`` property of the KVM
-accelerator, for example for Xen 4.10:
+accelerator, for example for Xen 4.17:
.. parsed-literal::
- |qemu_system| --accel kvm,xen-version=0x4000a,kernel-irqchip=split
+ |qemu_system| --accel kvm,xen-version=0x40011,kernel-irqchip=split
Additionally, virtual APIC support can be advertised to the guest through the
``xen-vapic`` CPU flag:
.. parsed-literal::
- |qemu_system| --accel kvm,xen-version=0x4000a,kernel-irqchip=split --cpu
host,+xen_vapic
+ |qemu_system| --accel kvm,xen-version=0x40011,kernel-irqchip=split --cpu
host,+xen-vapic
When Xen support is enabled, QEMU changes hypervisor identification (CPUID
0x4000..0x400A) to Xen. The KVM identification and features are not
advertised to a Xen guest. If Hyper-V is also enabled, the Xen identification
moves to leaves 0x4100..0x410A.
-The Xen platform device is enabled automatically for a Xen guest. This allows
-a guest to unplug all emulated devices, in order to use Xen PV block and
network
-drivers instead. Under Xen, the boot disk is typically available both via IDE
-emulation, and as a PV block device. Guest bootloaders typically use IDE to
load
-the guest kernel, which then unplugs the IDE and continues with the Xen PV
block
-device.
-
-This configuration can be achieved as follows
-
-.. parsed-literal::
-
- |qemu_system| -M pc --accel kvm,xen-version=0x4000a,kernel-irqchip=split \\
- -drive file=${GUEST_IMAGE},if=none,id=disk,file.locking=off -device
xen-disk,drive=disk,vdev=xvda \\
- -drive file=${GUEST_IMAGE},index=2,media=disk,file.locking=off,if=ide
-
-It is necessary to use the pc machine type, as the q35 machine uses AHCI
instead
-of legacy IDE, and AHCI disks are not unplugged through the Xen PV unplug
-mechanism.
-
-VirtIO devices can also be used; Linux guests may need to be dissuaded from
-umplugging them by adding 'xen_emul_unplug=never' on their command line.
-
Properties
--
@@ -63,7 +41,10 @@ The following properties exist on the KVM accelerator object:
``xen-version``
This property contains the Xen version in ``XENVER_version`` form, with the
major version in the top 16 bits and the minor version in the low 16 bits.
- Setting this property enables the Xen guest support.
+ Setting this property enables the Xen guest support. If Xen version 4.5 or
+ greater is specified, the HVM leaf in Xen CPUID is populated. Xen version
+ 4.6 enables the vCPU ID in CPUID, and version 4.17 advertises vCPU upcall
+ vector support to the guest.
``xen-evtchn-max-pirq``
Xen PIRQs represent an emulated physical interrupt, either GSI or MSI, which
@@ -83,8 +64,78 @@ The following properties exist on the KVM accelerator object:
through simultaneous grants. For guests with large numbers of PV devices and
high throughput, it may be desirable to increase this value.
-OS requirements
+Xen paravirtual devices
+---
+
+The Xen PCI platform device is enabled automatically for a Xen guest. This
+allows a guest to unplug all emulated devices, in order to use paravirtual
+block and network drivers instead.
+
+Those paravirtual Xen block, network (and console) devices can be created
+through the command line, and/or hot-plugged.
+
+To provide a Xen console device, define a character device and then a device
+of type ``xen-console`` to connect to it. For the Xen console equivalent of
+the handy ``-serial mon:stdio`` option, for example:
+
+.. parsed-literal::
+ -chardev stdio,mux=on,id=char0,signal=off -mon char0 \\
+ -device xen-console,chardev=char0
+
+The Xen network device is ``xen-net-device``, which becomes the default NIC
[PATCH] ubsan: Fix pointer overflow error message
In __ubsan_handle_pointer_overflow(), fix the condition for determining
whether a pointer operation overflowed or underflowed. Currently, the
function reports "underflowed" when it should be reporting "overflowed"
and vice versa.
Example of incorrect error reporting:
void *foo = (void *)__UINTPTR_MAX__;
foo += 1;
UBSAN:
pointer operation underflowed to
Fixes: 4e3fb2fb47d6 ("ubsan: add clang 5.0 support")
Signed-off-by: Michal Orzel
---
xen/common/ubsan/ubsan.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/common/ubsan/ubsan.c b/xen/common/ubsan/ubsan.c
index 0fddacabda6a..a3a80fa99eec 100644
--- a/xen/common/ubsan/ubsan.c
+++ b/xen/common/ubsan/ubsan.c
@@ -513,7 +513,7 @@ void __ubsan_handle_pointer_overflow(struct
pointer_overflow_data *data,
ubsan_prologue(>location, );
pr_err("pointer operation %s %p to %p\n",
- base > result ? "underflowed" : "overflowed",
+ base > result ? "overflowed" : "underflowed",
_p(base), _p(result));
ubsan_epilogue();
--
2.25.1
Re: [XEN PATCH][for-4.19] xen: replace occurrences of SAF-1-safe with asmlinkage attribute
On 2023-11-06 23:57, Julien Grall wrote: Hi Nicola, On 03/11/2023 18:05, Nicola Vetrini wrote: The comment-based justifications for MISRA C:2012 Rule 8.4 are replaced by the asmlinkage pseudo-attribute, for the sake of uniformity. The deviation with a comment based on the SAF framework is also mentioned as a last resort. I don't see any reason to keep SAF-1 after this patch. So can this be removed? In documenting-violations.rst it's stated: "Entries in the database shall never be removed, even if they are not used anymore in the code (if a patch is removing or modifying the faulty line). This is to make sure that numbers are not reused which could lead to conflicts with old branches or misleading justifications." that's why I kept SAF-1 in the safe.json file and added the remark about it being a last resort. I am ok with that remark becoming not to use SAF-1 in new code at all (I probably didn't go back to check your reply when writing the patch). Add missing 'xen/compiler.h' #include-s where needed. The text in docs/misra/deviations.rst is modified to reflect this change. Signed-off-by: Nicola Vetrini --- docs/misra/deviations.rst | 6 +++--- xen/arch/arm/cpuerrata.c| 7 +++ xen/arch/arm/setup.c| 5 ++--- xen/arch/arm/smpboot.c | 3 +-- xen/arch/arm/traps.c| 21 +++-- xen/arch/x86/boot/cmdline.c | 5 +++-- xen/arch/x86/boot/reloc.c | 7 --- xen/arch/x86/extable.c | 3 +-- xen/arch/x86/setup.c| 3 +-- xen/arch/x86/traps.c| 27 +-- xen/common/efi/boot.c | 5 ++--- 11 files changed, 36 insertions(+), 56 deletions(-) diff --git a/docs/misra/deviations.rst b/docs/misra/deviations.rst index d468da2f5ce9..ed5d36c08647 100644 --- a/docs/misra/deviations.rst +++ b/docs/misra/deviations.rst @@ -134,9 +134,9 @@ Deviations related to MISRA C:2012 Rules: - Tagged as `safe` for ECLAIR. * - R8.4 - - Functions and variables used only by asm modules are either marked with - the `asmlinkage` macro or with a SAF-1-safe textual deviation - (see safe.json). I thought we agreed to a different wording [1]. So is this really based on last version? + - Functions and variables used only to interface with asm modules should + be marked with the `asmlinkage` macro. If that's not possible, consider + using the SAF-1-safe textual deviation (see safe.json). See above. Actually, I am a bit surprised that SAF-1 is still mentioned given that I have now requested multiple that it should be removed and I haven't yet seen a reason to keep it. Cheers, [1] https://lore.kernel.org/all/b914ac93-2499-4bfd-a60a-51a9f1c17...@xen.org/ -- Nicola Vetrini, BSc Software Engineer, BUGSENG srl (https://bugseng.com)
Re: Informal voting proposal
Hi Stefano, On 07/11/2023 04:18, Stefano Stabellini wrote: On Mon, 6 Nov 2023, Julien Grall wrote: Hi Kelly, On 06/11/2023 16:40, Kelly Choi wrote: Hi all, As an open-source community, there will always be differences of opinion in approaches and the way we think. It is imperative, however, that we view this diversity as a source of strength rather than a hindrance. Recent deliberations within our project have led to certain matters being put on hold due to an inability to reach a consensus. While formal voting procedures serve their purpose, they can be time-consuming and may not always lead to meaningful progress. Having received agreement from a few maintainers already, I would like to propose the following: Thanks for the sending the proposal. While I like the idea of informal vote to move faster, I would like to ask some clarifications. *Informal voting method:* 1. Each project should ideally have more than 2 maintainers to facilitate impartial discussions. Projects lacking this configuration will be addressed at a later stage. 2. Anyone in the community is welcome to voice their opinions, ideas, and concerns about any patch or contribution. 3. If members cannot agree, the majority informal vote of the maintainers will be the decision that stands. For instance, if, after careful consideration of all suggestions and concerns, 2 out of 3 maintainers endorse a solution within the x86 subsystem, it shall be the decision we move forward with. How do you know that all the options have been carefully considered? I don't think there is a hard rule on this. We follow the discussion on > the list the same way as we do today. To me the fact we need to write down the informal rules means that something already gone wrong before. So I feel that rules should be unambiguous to avoid any problem afterwards. While I like the informal vote proposal and effectively we have already been following it in many areas of the project, I don't think we should change the current processes from that point of view. I am confused. Are you suggesting that we should not write down how informal votes works? 4. Naturally, there may be exceptional circumstances, as such, a formal vote may be warranted but should happen only a few times a year for serious cases only. Similarly here, you are suggesting that a formal vote can be called. But it is not super clear what would be the condition it could be used and how it can be called. The formal vote is basically the same we already have today. It would follow the existing voting rules and guidelines already part of the governance. Reading through the governance, I couldn't find anywhere indicating in which condition the formal votes can be triggered. Hence my question here. For instance, per the informal rule, if 2 out of 3 maintainers accept. Then it would be sensible for the patch to be merged as soon as the 5 days windows is closed. Yet the 3rd maintainer technically object it. So could that maintainer request a formal vote? If so, how long do they have? It is difficult to write down a process that works in all cases, and if we did it would probably end up being slower rather than faster. In my view if someone doesn't agree with his other co-maintainers and he is outvoted (e.g. 2/3 of the maintainers prefer a different option), the individual is entitled to raise a request for a vote with the committers, which is the same as we already have today. Ideally a formal vote would be rare, maybe once or twice a year, so I hope we won't need to optimize the formal vote. Ok. So the expectation is that all the maintainers will accept the informal votes in the majority of the cases. If this is not the case, then we will revise the rules. Is that correct? 5. Informal votes can be as easy as 2 out of 3 maintainers providing their Acked-by/Reviewed-by tag. Alternatively, Maintainers can call an informal vote by simply emailing the thread with "informal vote proposed, option 1 and option 2." 6. *All maintainers should reply with their vote within 5 working days.* While I understand we want to move fast, this means that a maintainer that is away for PTO would not have the opportunity to answer. PTO is a bit of a special case because we typically know when one of the maintainers is on PTO. If it is a short PTO and if the vacationing maintainer could have a strong opinion on the subject then it would make sense to wait. If it is a long leave of absence (several weeks or months) then I don't think we can wait. So I think the 5 working days is OK as a rule of thumb, but of course it shouldn't be used to work around objections of a maintainer by waiting for him to go on holiday :-) Well... It has been done before ;). That's why I think it is important to write down because at least it is not left to interpretation. Cheers, -- Julien Grall
