Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations
On 25.08.2022 20:09, Stefano Stabellini wrote: > But first, let's confirm whether this change: > > > #define dt_for_each_property_node(dn, pp) \ > -for ( pp = dn->properties; pp != NULL; pp = pp->next ) > +for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next ) > > > is sufficient to make the violation go away in Eclair or cppcheck. I am > assuming it is not sufficient, but let's confirm. Well, even if for the lhs of assignments there was an exception, this still wouldn't be sufficient. The minimum needed is #define dt_for_each_property_node(dn, pp) \ for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next ) Jan
[ovmf test] 172782: regressions - FAIL
flight 172782 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/172782/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172136 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172136 version targeted for testing: ovmf 0ede7cad73dda686afa2ea0eb2a787f48ec666aa baseline version: ovmf 444260d45ec2a84e8f8c192b3539a3cd5591d009 Last test of basis 172136 2022-08-04 06:43:42 Z 21 days Failing since172151 2022-08-05 02:40:28 Z 21 days 171 attempts Testing same since 172773 2022-08-25 13:41:54 Z0 days4 attempts People who touched revisions under test: Abdul Lateef Attar Abner Chang Chasel Chiu Czajkowski, Maciej Dimitrije Pavlov Dun Tan Edward Pickup Foster Nong Gregx Yeh Igor Kulchytskyy James Lu Jose Marinho KasimX Liu Kavya Konstantin Aladyshev Liu, Zhiguang Maciej Czajkowski Michael D Kinney Ray Ni Sainadh Nagolu Sami Mujawar Shengfengx Xue Zhiguang Liu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt fail build-i386-libvirt fail 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 Not pushing. (No revision log; it would be 962 lines long.)
[linux-5.4 test] 172776: regressions - FAIL
flight 172776 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/172776/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172128 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172128 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 172128 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 172128 test-armhf-armhf-xl 18 guest-start/debian.repeat fail REGR. vs. 172128 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-armhf-armhf-xl-credit1 18 guest-start/debian.repeat fail blocked in 172128 test-armhf-armhf-xl-multivcpu 18 guest-start/debian.repeatfail like 172108 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 172108 test-armhf-armhf-xl-credit2 14 guest-start fail like 172128 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 172128 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 172128 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 172128 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeatfail like 172128 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 172128 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 172128 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 172128 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 172128 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 172128 test-amd64-i386-xl-pvshim14 guest-start fail 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-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-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-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 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-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 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-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armh
[linux-linus test] 172774: regressions - FAIL
flight 172774 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/172774/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172133 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172133 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 172133 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 172133 Tests which are failing intermittently (not blocking): test-amd64-amd64-pygrub 12 debian-di-install fail pass in 172764 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qcow2 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-raw 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 172133 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 172133 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 172133 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 172133 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 172133 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 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-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 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 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 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-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 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 version targeted for testing: linuxc40e8341e3b3bb27e3a65b06b5b454626234c4f0 baseline version: linuxb44f2fd87919b5ae6e1756d4c7ba2cbba22238e1 Last test of basis 172133 2022-08-04 05:14:48 Z 21 days Failing since172152 2022-08-05 04:01:26 Z 20 days 48 attempts Testing same since 172743 2022-08-24 03:29:51 Z1 days4 attempts 1512 people touched revisions under test, not listing them all jobs: build-amd64-xsm pass build-arm64-xsm pass
[ovmf test] 172780: regressions - FAIL
flight 172780 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/172780/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172136 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172136 version targeted for testing: ovmf 0ede7cad73dda686afa2ea0eb2a787f48ec666aa baseline version: ovmf 444260d45ec2a84e8f8c192b3539a3cd5591d009 Last test of basis 172136 2022-08-04 06:43:42 Z 21 days Failing since172151 2022-08-05 02:40:28 Z 20 days 170 attempts Testing same since 172773 2022-08-25 13:41:54 Z0 days3 attempts People who touched revisions under test: Abdul Lateef Attar Abner Chang Chasel Chiu Czajkowski, Maciej Dimitrije Pavlov Dun Tan Edward Pickup Foster Nong Gregx Yeh Igor Kulchytskyy James Lu Jose Marinho KasimX Liu Kavya Konstantin Aladyshev Liu, Zhiguang Maciej Czajkowski Michael D Kinney Ray Ni Sainadh Nagolu Sami Mujawar Shengfengx Xue Zhiguang Liu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt fail build-i386-libvirt fail 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 Not pushing. (No revision log; it would be 962 lines long.)
[xen-unstable test] 172772: tolerable FAIL
flight 172772 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/172772/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a build-amd64-libvirt 6 libvirt-buildfail like 172762 build-i386-libvirt6 libvirt-buildfail like 172762 build-arm64-libvirt 6 libvirt-buildfail like 172762 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 172762 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 172762 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 172762 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 172762 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 172762 build-armhf-libvirt 6 libvirt-buildfail like 172762 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeatfail like 172762 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 172762 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 172762 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 172762 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 172762 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 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-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-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-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 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-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-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 15 migrate-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 version targeted for testing: xen c3bd0b83ea5b7c0da65426874
[PATCH] Add support for ESRT loading under Xen
This is needed for fwupd to work in Qubes OS.
Signed-off-by: Demi Marie Obenour
---
drivers/firmware/efi/esrt.c | 34 --
drivers/xen/efi.c | 33 +
include/linux/efi.h | 10 ++
3 files changed, 67 insertions(+), 10 deletions(-)
diff --git a/drivers/firmware/efi/esrt.c b/drivers/firmware/efi/esrt.c
index
2a2f52b017e736dd995c69e8aeb5fbd7761732e5..c0fc149a838044cc16bb08a374a0c8ea6b7dcbff
100644
--- a/drivers/firmware/efi/esrt.c
+++ b/drivers/firmware/efi/esrt.c
@@ -244,22 +244,36 @@ void __init efi_esrt_init(void)
struct efi_system_resource_table tmpesrt;
size_t size, max, entry_size, entries_size;
efi_memory_desc_t md;
- int rc;
phys_addr_t end;
- if (!efi_enabled(EFI_MEMMAP))
- return;
-
pr_debug("esrt-init: loading.\n");
if (!esrt_table_exists())
return;
- rc = efi_mem_desc_lookup(efi.esrt, &md);
- if (rc < 0 ||
- (!(md.attribute & EFI_MEMORY_RUNTIME) &&
-md.type != EFI_BOOT_SERVICES_DATA &&
-md.type != EFI_RUNTIME_SERVICES_DATA)) {
- pr_warn("ESRT header is not in the memory map.\n");
+ if (efi_enabled(EFI_MEMMAP)) {
+ if (efi_mem_desc_lookup(efi.esrt, &md) < 0 ||
+ (!(md.attribute & EFI_MEMORY_RUNTIME) &&
+md.type != EFI_BOOT_SERVICES_DATA &&
+md.type != EFI_RUNTIME_SERVICES_DATA)) {
+ pr_warn("ESRT header is not in the memory map.\n");
+ return;
+ }
+ } else if (IS_ENABLED(CONFIG_XEN_EFI) && efi_enabled(EFI_PARAVIRT)) {
+ if (!xen_efi_mem_desc_lookup(efi.esrt, &md)) {
+ pr_warn("Failed to lookup ESRT header in Xen memory
map\n");
+ return;
+ }
+
+ /* Recent Xen versions relocate the ESRT to memory of type
+* EfiRuntimeServicesData, which Xen will not reuse. If the
ESRT
+* is not in EfiRuntimeServicesData memory, it has not been
reserved
+* by Xen and might be allocated to other guests, so it cannot
+* safely be used. */
+ if (md.type != EFI_RUNTIME_SERVICES_DATA) {
+ pr_warn("Xen did not reserve ESRT, ignoring it\n");
+ return;
+ }
+ } else {
return;
}
diff --git a/drivers/xen/efi.c b/drivers/xen/efi.c
index
d1ff2186ebb48a7c0981ecb6d4afcbbb25ffcea0..b313f213822f0fd5ba6448f6f6f453cfda4c7e23
100644
--- a/drivers/xen/efi.c
+++ b/drivers/xen/efi.c
@@ -26,6 +26,7 @@
#include
#include
+#include
#include
#include
@@ -40,6 +41,38 @@
#define efi_data(op) (op.u.efi_runtime_call)
+static_assert(XEN_PAGE_SHIFT == EFI_PAGE_SHIFT,
+ "Mismatch between EFI_PAGE_SHIFT and XEN_PAGE_SHIFT");
+
+bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *md)
+{
+ struct xen_platform_op op = {
+ .cmd = XENPF_firmware_info,
+ .u.firmware_info = {
+ .type = XEN_FW_EFI_INFO,
+ .index = XEN_FW_EFI_MEM_INFO,
+ .u.efi_info.mem.addr = phys_addr,
+ .u.efi_info.mem.size = ((u64)-1ULL) - phys_addr,
+ }
+ };
+ union xenpf_efi_info *info = &op.u.firmware_info.u.efi_info;
+ int rc;
+
+ memset(md, 0, sizeof(*md)); /* initialize md even on failure */
+ rc = HYPERVISOR_platform_op(&op);
+ if (rc) {
+ pr_warn("Could not obtain information on address %llu from Xen:
"
+ "error %d\n", phys_addr, rc);
+ return false;
+ }
+
+ md->attribute = info->mem.attr;
+ md->type = info->mem.type;
+ md->num_pages = info->mem.size >> XEN_PAGE_SHIFT;
+ md->phys_addr = info->mem.addr;
+ return true;
+}
+
static efi_status_t xen_efi_get_time(efi_time_t *tm, efi_time_cap_t *tc)
{
struct xen_platform_op op = INIT_EFI_OP(get_time);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index
d2b84c2fec39f0268324d1a38a73ed67786973c9..0598869cdc924aef0e2b9cacc4450b728e1a98c7
100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1327,1 +1327,11 @@ struct linux_efi_coco_secret_area {
+#if IS_ENABLED(CONFIG_XEN_EFI)
+extern bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t *out_md);
+#else
+static inline bool xen_efi_mem_desc_lookup(u64 phys_addr, efi_memory_desc_t
*out_md)
+{
+ BUILD_BUG();
+ return false;
+}
+#endif
+
#endif /* _LINUX_EFI_H */
--
Sincerely,
Demi Marie Obenour (she/her/hers)
Invisible Things Lab
[ovmf test] 172777: regressions - FAIL
flight 172777 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/172777/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172136 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172136 version targeted for testing: ovmf 0ede7cad73dda686afa2ea0eb2a787f48ec666aa baseline version: ovmf 444260d45ec2a84e8f8c192b3539a3cd5591d009 Last test of basis 172136 2022-08-04 06:43:42 Z 21 days Failing since172151 2022-08-05 02:40:28 Z 20 days 169 attempts Testing same since 172773 2022-08-25 13:41:54 Z0 days2 attempts People who touched revisions under test: Abdul Lateef Attar Abner Chang Chasel Chiu Czajkowski, Maciej Dimitrije Pavlov Dun Tan Edward Pickup Foster Nong Gregx Yeh Igor Kulchytskyy James Lu Jose Marinho KasimX Liu Kavya Konstantin Aladyshev Liu, Zhiguang Maciej Czajkowski Michael D Kinney Ray Ni Sainadh Nagolu Sami Mujawar Shengfengx Xue Zhiguang Liu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt fail build-i386-libvirt fail 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 Not pushing. (No revision log; it would be 962 lines long.)
Re: [PATCH] Make XEN_FW_EFI_MEM_INFO easier to use
On Thu, Aug 25, 2022 at 09:59:56AM +0200, Jan Beulich wrote: > On 24.08.2022 23:04, Demi Marie Obenour wrote: > > The XEN_FW_EFI_MEM_INFO platform op has very surprising behavior: it > > only sets info->mem.size if the initial value was *larger* than the size > > of the memory region. > > And intentionally so - the caller didn't ask for any bigger region, > after all. That needs to be documented, then. I thought it provided the full region that contained the address. > > This is not particularly useful and cost me most > > of a day of debugging. It also has some integer overflow problems, > > though as the data comes from dom0 or the firmware (both of which are > > trusted) these are not security issues. > > I'm afraid we're trusting the firmware in this regard elsewhere as > well. So if there was a need to change that, I guess it would need > changing everywhere, not just here. But we trust the E820 map as > well, when on non-EFI platforms, so I don't see why we would need > to change that. In any event such would want to be a separate > change imo. That is valid. > > Fix both of these problems by unconditionally setting the memory region > > size > > If you were to report a larger ending address, why would you not also > report a smaller starting address? > > But before you go that route - I don't think we can change the API > now that it has been in use this way for many years. If a "give me > the full enclosing range" variant is wanted, it will need to be > fully separate. Does anyone use this API? -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab signature.asc Description: PGP signature
[qemu-mainline test] 172768: regressions - FAIL
flight 172768 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/172768/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172123 build-i386-libvirt6 libvirt-buildfail REGR. vs. 172123 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 172123 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 172123 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 172123 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 172123 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 172123 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 172123 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 172123 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 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-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 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-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 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-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-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-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass version targeted for testing: qemuu9a99f964b152f8095949bbddca7841744ad418da baseline version: qemuu2480f3bbd03814b0651a1f74959f5c6631ee5819 Last test of basis 172123 2022-08-03 18:10:07 Z 22 days Failing since172148 2022-08-04 21:39:38 Z 20 days 48 attempts Testing same since 172768 2022-08-25 07:03:08 Z0 days1 attempts ---
[xen-unstable-smoke test] 172775: tolerable FAIL - PUSHED
flight 172775 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/172775/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 1 build-check(1) blocked n/a build-amd64-libvirt 6 libvirt-buildfail like 172752 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 cbb35e72802f3a285c382a995ef647b59e5caf2f baseline version: xen c3bd0b83ea5b7c0da6542687436042eeea1e7909 Last test of basis 172752 2022-08-24 13:00:27 Z1 days Testing same since 172775 2022-08-25 15:01:50 Z0 days1 attempts People who touched revisions under test: Jan Beulich jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt fail test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt blocked 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 c3bd0b83ea..cbb35e7280 cbb35e72802f3a285c382a995ef647b59e5caf2f -> smoke
Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations
CC MISRA C working group Short summary: we are discussing whether the following is sufficient to address MISRA C Rule 20.7, and also in general for safety: #define dt_for_each_property_node(dn, pp) \ -for ( pp = dn->properties; pp != NULL; pp = pp->next ) +for ( pp = (dn)->properties; pp != NULL; pp = (pp)->next ) as you can see "dn" has been parenthesizing because is used as a rhs, while "pp" has *not* because it is used as lhs. More below. On Thu, 25 Aug 2022, Jan Beulich wrote: > On 25.08.2022 10:02, Xenia Ragiadakou wrote: > > On 8/22/22 14:48, Jan Beulich wrote: > >> On 22.08.2022 12:43, Xenia Ragiadakou wrote: > >>> On 8/22/22 12:59, Jan Beulich wrote: > On 19.08.2022 21:43, Xenia Ragiadakou wrote: > > In macros dt_for_each_property_node(), dt_for_each_device_node() and > > dt_for_each_child_node(), add parentheses around the macro parameters > > that > > have the arrow operator applied, to prevent against unintended > > expansions. > > Why is this relevant only when -> is used? For comparisons and the rhs of > assignments it's as relevant, ad even for the lhs of assignments I doubt > it can be generally omitted. > >>> > >>> Yes, I agree with you but some older patches that I sent that were > >>> adding parentheses around the lhs of the assignments were not accepted > >>> and I thought that the rhs of the assignments as well these comparisons > >>> fall to the same category. > >>> > >>> Personally, I would expect to see parentheses, also, around the macro > >>> parameters that are used as the lhs or the rhs of assignments, the > >>> operands of comparison or the arguments of a function. > >>> Not only because they can prevent against unintentional bugs but because > >>> the parentheses help me to identify more easily the macro parameters > >>> when reading a macro definition. I totally understand that for other > >>> people parentheses may reduce readability. > >> > >> Afair Julien's comments were very specific to the lhs of assignments. > >> So at the very least everything else ought to be parenthesized imo. > >> > > > > So, IIUC, the only deviations from the MISRA C 2012 Rule 20.7 will be > > for macro parameters used as the lhs of assignments and function arguments? > > Afaic I don't consider that discussion settled. > > > This feels a bit of a hack to parenthesize the macro parameters that are > > used as the rhs of an assignment but not those used as the lhs. > > lhs and rhs of assignments are quite different, and hence making such a > distinction wouldn't look to be a "hack" to me. In fact I've always > considered this part of the language somewhat strange: To me > parenthesizing e.g. an identifier already makes it (visually) an > rvalue. Leaving aside odd (and easy to spot as odd) uses at the call > sizes, I don't think I can come up with a case where parentheses are > really needed. Anything needing parenthesizing actually yields an > rvalue afaics, thus causing a diagnostic anyway. Although I can see where you are coming from, parenthesizing an identifier doesn't actually make it an rvalue. Also it is a lot simpler to understand, review, and apply a policy that says: "all macro parameters are parenthesized" compared to a policy that says: "most macro paremeters are parenthesized, let's go into the details of which ones are parenthesized and which ones are not, including examples and corner cases" For simplicity, I would go with the simplest version, the MISRA version. I am assuming that the MISRA Rule 20.7 requires that "pp" is also parenthesized. Roberto, is that correct? > > From previous discussions on the topic, I understood that the > > parentheses are considered needed only when they eliminate operator > > precedence problems, while for the wrong parameter format bugs we can > > rely on the reviewers. > > > > I think we need to decide if the rule will be applied as is and if not > > which will be the deviations along with their justification and add it > > to the document. > > Yes. But this shouldn't hinder adjustments for all other, non- > controversial cases. It looks like we need a discussion and see where the majority of the team is on this issue. I prefer the original MISRA version for simplicity, but I also think it is OK if we make a small customization to it. In that case, we would add the extra explanation and details to docs/misra/rules.rst. However, as we make the decision we also need to take into account that if we don't go with the vanilla MISRA rule, there is a price to pay: all the MISRA scanners, including cppcheck, Eclair, Coverity and others would probably still flag these issues as violations polluting the results and making the scanners less useful. We might have to mark each "deviation" with a one-line in-code comment on top, or we would have to disable automatic scanning for Rule 20.7 altogether. Either option is not great. This is actually the main reason w
Re: [PATCH v2 7/7] xen/arm: introduce new xen,enhanced property value
On Thu, 25 Aug 2022, Bertrand Marquis wrote:
> > On 25 Aug 2022, at 10:37, Julien Grall wrote:
> > On 25/08/2022 08:39, Bertrand Marquis wrote:
> >> Hi,
> >>> On 25 Aug 2022, at 02:10, Stefano Stabellini
> >>> wrote:
> >>>
> >>> On Wed, 24 Aug 2022, Julien Grall wrote:
> On 24/08/2022 22:59, Stefano Stabellini wrote:
> > On Wed, 24 Aug 2022, Rahul Singh wrote:
> >>> On 24 Aug 2022, at 4:36 pm, Julien Grall wrote:
> >>> On 24/08/2022 15:42, Rahul Singh wrote:
> > On 24 Aug 2022, at 1:59 pm, Julien Grall wrote:
> >
> >
> >
> > On 24/08/2022 13:15, Rahul Singh wrote:
> >> Hi Julien,
> >
> > Hi Rahul,
> >
> >> Please let me know your view on this.
> >> diff --git a/xen/arch/arm/domain_build.c
> >> b/xen/arch/arm/domain_build.c
> >> index bfe7bc6b36..a1e23eee59 100644
> >> --- a/xen/arch/arm/domain_build.c
> >> +++ b/xen/arch/arm/domain_build.c
> >> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
> >> domain *d,
> >>if ( rc == -EILSEQ ||
> >> rc == -ENODATA ||
> >> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) )
> >> - {
> >> -if ( hardware_domain )
> >>kinfo.dom0less_enhanced = true;
> >> -else
> >> - panic(“Tried to use xen,enhanced without dom0\n”);
> >> - }
> >
> > You can't use "xen,enhanced" without dom0. In fact, you will end up
> > to dereference NULL in alloc_xenstore_evtchn(). That's because
> > "xen,enhanced" means the domain will be able to use Xenstored.
> >
> > Now if you want to support your feature without a dom0. Then I think
> > we want to introduce an option which would be the same as
> > "xen,enhanced" but doesn't expose Xenstored.
> If we modify the patch as below we can use the "xen,enhanced" for
> domUs without dom0.
> I tested the patch and its works fine. Do you see any issue with this
> approach?
> >>>
> >>> Yes. For two reasons:
> >>> 1) It is muddying the meaning of "xen,enhanced". In particular a user
> >>> may not realize that Xenstore is not available if dom0 is not present.
> >>> 2) It would be more complicated to handle the case where Xenstored
> >>> lives
> >>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm
> >>> yet,
> >>> but I don't want to close the door.
> >>>
> >>> So if you want to support create "xen,xen" without all the rest. Then
> >>> I
> >>> think we need a different property value. I don't have a good
> >>> suggestion
> >>> for the name.
> >>
> >> Is that okay if we use the earlier approach, when user set
> >> "xen,enhanced
> >> = evtchn” we will not call alloc_xenstore_evtchn()
> >> but we create hypervisor node with all fields.
> >
> > Thinking more about this, today xen,enhanced has the implication that:
> >
> > - the guest will get a regular and complete "xen,xen" node in device
> > tree
> > - xenstore and PV drivers will be available (full Xen interfaces
> > support)
> >
> > We don't necessarely imply that dom0 is required (from a domU point of
> > view) but we do imply that xenstore+evtchn+gnttab will be available to
> > the domU.
> >
> > Now, static event channels are different. They don't require xenstore
> > and they don't require gnttab.
> >
> > It is as if the current xen,enhanced node actually meant:
> >
> > xen,enhanced = "xenstore,gnttab,evtchn";
>
> Correct.
>
> >
> > and now we are only enabling a subset:
> >
> > xen,enhanced = "evtchn";
> >
> > Is that a correct understanding?
>
> Yes with some cavears (see below).
>
> >
> >
> > If so, we can clarify that:
> >
> > xen,enhanced;
> >
> > it is a convenient shortend for:
> >
> > xen,enhanced = "xenstore,gnttab,evtchn";
> >
> > and that other combinations are also acceptable, e.g.:
> >
> > xen,enhanced = "gnttab";
> > xen,enhanced = "evtchn";
> > xen,enhanced = "evtchn,gnttab";
> >
> > It is OK to panic if the user specifies an option that is currently
> > unsupported (e.g. "gnttab").
>
> So today, if you create the node "xen,xen", the guest will expect to be
> able
> to use both grant-table and event channel.
>
> Therefore, in the list above, the only configuration we can sensibly
> support
> without any major rework is "evtchn,gnttab".
>
> If we want to support "evtchn" or "gnttab" only. Then we likely need to
> define
> a new binding (or new version) because neither "regs" nor "interrupts"
> are
> o
[ovmf test] 172773: regressions - trouble: broken/fail/pass
flight 172773 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/172773/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm broken build-amd64-xsm 4 host-install(4)broken REGR. vs. 172136 build-i386-libvirt6 libvirt-buildfail REGR. vs. 172136 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172136 version targeted for testing: ovmf 0ede7cad73dda686afa2ea0eb2a787f48ec666aa baseline version: ovmf 444260d45ec2a84e8f8c192b3539a3cd5591d009 Last test of basis 172136 2022-08-04 06:43:42 Z 21 days Failing since172151 2022-08-05 02:40:28 Z 20 days 168 attempts Testing same since 172773 2022-08-25 13:41:54 Z0 days1 attempts People who touched revisions under test: Abdul Lateef Attar Abner Chang Chasel Chiu Czajkowski, Maciej Dimitrije Pavlov Dun Tan Edward Pickup Foster Nong Gregx Yeh Igor Kulchytskyy James Lu Jose Marinho KasimX Liu Kavya Konstantin Aladyshev Liu, Zhiguang Maciej Czajkowski Michael D Kinney Ray Ni Sainadh Nagolu Sami Mujawar Shengfengx Xue Zhiguang Liu jobs: build-amd64-xsm broken build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt fail build-i386-libvirt fail 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 broken-job build-amd64-xsm broken broken-step build-amd64-xsm host-install(4) Not pushing. (No revision log; it would be 962 lines long.)
[linux-5.4 test] 172766: regressions - FAIL
flight 172766 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/172766/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172128 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172128 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 172128 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 172128 Tests which are failing intermittently (not blocking): test-amd64-i386-xl-qemuu-debianhvm-amd64 7 xen-install fail in 172757 pass in 172766 test-amd64-i386-examine 6 xen-install fail in 172757 pass in 172766 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-install fail in 172757 pass in 172766 test-armhf-armhf-xl-rtds 14 guest-startfail pass in 172757 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-armhf-armhf-xl-credit1 18 guest-start/debian.repeat fail blocked in 172128 test-armhf-armhf-xl-credit2 14 guest-start fail in 172757 like 172128 test-armhf-armhf-xl-credit1 14 guest-start fail in 172757 like 172128 test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 172757 like 172128 test-armhf-armhf-xl-rtds15 migrate-support-check fail in 172757 never pass test-armhf-armhf-xl-rtds 16 saverestore-support-check fail in 172757 never pass test-armhf-armhf-xl-vhd 14 migrate-support-check fail in 172757 never pass test-armhf-armhf-xl-vhd 15 saverestore-support-check fail in 172757 never pass test-armhf-armhf-xl-credit2 18 guest-start/debian.repeatfail like 172108 test-armhf-armhf-xl-vhd 13 guest-start fail like 172108 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 172108 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 172128 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 172128 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 172128 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 172128 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 172128 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 172128 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 172128 test-armhf-armhf-xl-multivcpu 14 guest-start fail like 172128 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 172128 test-amd64-i386-xl-pvshim14 guest-start fail 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-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 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-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-ar
Re: “Backend has not unmapped grant” errors
Hi Juergen, Thank you for the quick and nice reply! On Thu, 25 Aug 2022 08:20:33 +0200 Juergen Gross wrote: > [...] > > Could you please send it as two proper patches (one for each driver) with > the correct "Fixes:" tags? Sure, just posted: https://lore.kernel.org/xen-devel/20220825161511.94922-2...@kernel.org/ Thanks, SJ > > > Juergen
[PATCH 0/2] xen-blk{front,back}: Advertise feature-persistent as user requested
Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
when connect") made blkback to advertise its support of the persistent
grants feature only if the user sets the 'feature_persistent' parameter
of the driver and the frontend advertised its support of the feature.
However, following commit 402c43ea6b34 ("xen-blkfront: Apply
'feature_persistent' parameter when connect") made the blkfront to work
in the same way. That is, blkfront also advertises its support of the
persistent grants feature only if the user sets the 'feature_persistent'
parameter of the driver and the backend advertised its support of the
feature.
Hence blkback and blkfront will never advertise their support of the
feature but wait until the other advertises the support, even though
users set the 'feature_persistent' parameters of the drivers. As a
result, the persistent grants feature is disabled always regardless of
the 'feature_persistent' values[1].
The problem comes from the misuse of the semantic of the advertisement
of the feature. The advertisement of the feature should means only
availability of the feature not the decision for using the feature.
However, current behavior is working in the wrong way.
This patchset fixes the issue by making both blkback and blkfront
advertise their support of the feature as user requested via
'feature_persistent' parameter regardless of the otherend's support of
the feature.
[1]
https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f...@suse.com/
SeongJae Park (2):
xen-blkback: Advertise feature-persistent as user requested
xen-blkfront: Advertise feature-persistent as user requested
drivers/block/xen-blkback/common.h | 3 +++
drivers/block/xen-blkback/xenbus.c | 6 --
drivers/block/xen-blkfront.c | 8 ++--
3 files changed, 13 insertions(+), 4 deletions(-)
--
2.25.1
[PATCH 2/2] xen-blkfront: Advertise feature-persistent as user requested
Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
when connect") made blkback to advertise its support of the persistent
grants feature only if the user sets the 'feature_persistent' parameter
of the driver and the frontend advertised its support of the feature.
However, following commit 402c43ea6b34 ("xen-blkfront: Apply
'feature_persistent' parameter when connect") made the blkfront to work
in the same way. That is, blkfront also advertises its support of the
persistent grants feature only if the user sets the 'feature_persistent'
parameter of the driver and the backend advertised its support of the
feature.
Hence blkback and blkfront will never advertise their support of the
feature but wait until the other advertises the support, even though
users set the 'feature_persistent' parameters of the drivers. As a
result, the persistent grants feature is disabled always regardless of
the 'feature_persistent' values[1].
The problem comes from the misuse of the semantic of the advertisement
of the feature. The advertisement of the feature should means only
availability of the feature not the decision for using the feature.
However, current behavior is working in the wrong way.
This commit fixes the issue by making the blkfront advertises its
support of the feature as user requested via 'feature_persistent'
parameter regardless of the otherend's support of the feature.
[1]
https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f...@suse.com/
Fixes: 402c43ea6b34 ("xen-blkfront: Apply 'feature_persistent' parameter when
connect")
Cc: # 5.10.x
Reported-by: Marek Marczykowski-Górecki
Suggested-by: Juergen Gross
Signed-off-by: SeongJae Park
---
drivers/block/xen-blkfront.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 8e56e69fb4c4..dfae08115450 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -213,6 +213,9 @@ struct blkfront_info
unsigned int feature_fua:1;
unsigned int feature_discard:1;
unsigned int feature_secdiscard:1;
+ /* Connect-time cached feature_persistent parameter */
+ unsigned int feature_persistent_parm:1;
+ /* Persistent grants feature negotiation result */
unsigned int feature_persistent:1;
unsigned int bounce:1;
unsigned int discard_granularity;
@@ -1848,7 +1851,7 @@ static int talk_to_blkback(struct xenbus_device *dev,
goto abort_transaction;
}
err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
- info->feature_persistent);
+ info->feature_persistent_parm);
if (err)
dev_warn(&dev->dev,
"writing persistent grants feature to xenbus");
@@ -2281,7 +2284,8 @@ static void blkfront_gather_backend_features(struct
blkfront_info *info)
if (xenbus_read_unsigned(info->xbdev->otherend, "feature-discard", 0))
blkfront_setup_discard(info);
- if (feature_persistent)
+ info->feature_persistent_parm = feature_persistent;
+ if (info->feature_persistent_parm)
info->feature_persistent =
!!xenbus_read_unsigned(info->xbdev->otherend,
"feature-persistent", 0);
--
2.25.1
[PATCH 1/2] xen-blkback: Advertise feature-persistent as user requested
Commit e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter
when connect") made blkback to advertise its support of the persistent
grants feature only if the user sets the 'feature_persistent' parameter
of the driver and the frontend advertised its support of the feature.
However, following commit 402c43ea6b34 ("xen-blkfront: Apply
'feature_persistent' parameter when connect") made the blkfront to work
in the same way. That is, blkfront also advertises its support of the
persistent grants feature only if the user sets the 'feature_persistent'
parameter of the driver and the backend advertised its support of the
feature.
Hence blkback and blkfront will never advertise their support of the
feature but wait until the other advertises the support, even though
users set the 'feature_persistent' parameters of the drivers. As a
result, the persistent grants feature is disabled always regardless of
the 'feature_persistent' values[1].
The problem comes from the misuse of the semantic of the advertisement
of the feature. The advertisement of the feature should means only
availability of the feature not the decision for using the feature.
However, current behavior is working in the wrong way.
This commit fixes the issue by making the blkback advertises its support
of the feature as user requested via 'feature_persistent' parameter
regardless of the otherend's support of the feature.
[1]
https://lore.kernel.org/xen-devel/bd818aba-4857-bc07-dc8a-e9b2f8c5f...@suse.com/
Fixes: e94c6101e151 ("xen-blkback: Apply 'feature_persistent' parameter when
connect")
Cc: # 5.10.x
Reported-by: Marek Marczykowski-Górecki
Suggested-by: Juergen Gross
Signed-off-by: SeongJae Park
---
drivers/block/xen-blkback/common.h | 3 +++
drivers/block/xen-blkback/xenbus.c | 6 --
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/block/xen-blkback/common.h
b/drivers/block/xen-blkback/common.h
index bda5c815e441..a28473470e66 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -226,6 +226,9 @@ struct xen_vbd {
sector_tsize;
unsigned intflush_support:1;
unsigned intdiscard_secure:1;
+ /* Connect-time cached feature_persistent parameter value */
+ unsigned intfeature_gnt_persistent_parm:1;
+ /* Persistent grants feature negotiation result */
unsigned intfeature_gnt_persistent:1;
unsigned intoverflow_max_grants:1;
};
diff --git a/drivers/block/xen-blkback/xenbus.c
b/drivers/block/xen-blkback/xenbus.c
index ee7ad2fb432d..c0227dfa4688 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -907,7 +907,7 @@ static void connect(struct backend_info *be)
xen_blkbk_barrier(xbt, be, be->blkif->vbd.flush_support);
err = xenbus_printf(xbt, dev->nodename, "feature-persistent", "%u",
- be->blkif->vbd.feature_gnt_persistent);
+ be->blkif->vbd.feature_gnt_persistent_parm);
if (err) {
xenbus_dev_fatal(dev, err, "writing %s/feature-persistent",
dev->nodename);
@@ -1085,7 +1085,9 @@ static int connect_ring(struct backend_info *be)
return -ENOSYS;
}
- blkif->vbd.feature_gnt_persistent = feature_persistent &&
+ blkif->vbd.feature_gnt_persistent_parm = feature_persistent;
+ blkif->vbd.feature_gnt_persistent =
+ blkif->vbd.feature_gnt_persistent_parm &&
xenbus_read_unsigned(dev->otherend, "feature-persistent", 0);
blkif->vbd.overflow_max_grants = 0;
--
2.25.1
Re: [PATCH v5 6/9] drivers/char: mark DMA buffers as reserved for the XHCI
On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote: > The important part is to include those buffers in IOMMU page table > relevant for the USB controller. Otherwise, DbC will stop working as > soon as IOMMU is enabled, regardless of to which domain device assigned > (be it xen or dom0). > If the device is passed through to dom0 or other domain (see later > patches), that domain will effectively have access to those buffers too. > It does give such domain yet another way to DoS the system (as is the > case when having PCI device assigned already), but also possibly steal > the console ring content. Thus, such domain should be a trusted one. > In any case, prevent anything else being placed on those pages by adding > artificial padding. > > Signed-off-by: Marek Marczykowski-Górecki Acked-by: Jan Beulich
Re: [PATCH v5 3/9] IOMMU: add common API for device reserved memory
On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote: > Add API similar to rmrr= and ivmd= arguments, but in a common code. This > will allow drivers to register reserved memory regardless of the IOMMU > vendor. > The direct reason for this API is xhci-dbc console driver (aka xue), > that needs to use DMA. But future change may unify command line > arguments for user-supplied reserved memory, and it may be useful for > other drivers in the future too. > > This commit just introduces an API, subsequent patches will plug it in > appropriate places. The reserved memory ranges needs to be saved > locally, because at the point when they are collected, Xen doesn't know > yet which IOMMU driver will be used. > > Signed-off-by: Marek Marczykowski-Górecki Acked-by: Jan Beulich
Re: [PATCH v5 1/9] drivers/char: separate dbgp=xhci to dbc=xhci option
On 22.08.2022 17:27, Marek Marczykowski-Górecki wrote:
> This allows configuring EHCI and XHCI consoles separately,
> simultaneously.
>
> Suggested-by: Jan Beulich
But was I maybe confused, and much less of a change would suffice? After
all ...
> --- a/xen/drivers/char/xhci-dbc.c
> +++ b/xen/drivers/char/xhci-dbc.c
> @@ -1058,9 +1058,9 @@ static struct xhci_dbc_ctx ctx __aligned(16);
> static uint8_t out_wrk_buf[DBC_WORK_RING_CAP];
> static struct xhci_string_descriptor str_buf[DBC_STRINGS_COUNT];
>
> -static char __initdata opt_dbgp[30];
> +static char __initdata opt_dbc[30];
>
> -string_param("dbgp", opt_dbgp);
> +string_param("dbc", opt_dbc);
>
> void __init xhci_dbc_uart_init(void)
> {
> @@ -1068,25 +1068,25 @@ void __init xhci_dbc_uart_init(void)
> struct dbc *dbc = &uart->dbc;
> const char *e;
>
> -if ( strncmp(opt_dbgp, "xhci", 4) )
> +if ( strncmp(opt_dbc, "xhci", 4) )
> return;
... this already avoids mixing up who's going to parse what. So right
now I think that ...
> @@ -1102,7 +1102,7 @@ void __init xhci_dbc_uart_init(void)
> dbc->dbc_str = str_buf;
>
> if ( dbc_open(dbc) )
> -serial_register_uart(SERHND_DBGP, &dbc_uart_driver, &dbc_uart);
> +serial_register_uart(SERHND_DBC, &dbc_uart_driver, &dbc_uart);
> }
... this and other SERHND_* related changes are enough, and there's no
need for a separate "dbc=" option.
> --- a/xen/include/xen/serial.h
> +++ b/xen/include/xen/serial.h
> @@ -95,6 +95,7 @@ struct uart_driver {
> # define SERHND_COM1(0<<0)
> # define SERHND_COM2(1<<0)
> # define SERHND_DBGP(2<<0)
> +# define SERHND_DBC (3<<0)
Please also update the comment just out of context.
Jan
Re: Ryzen 6000 (Mobile)
On 25.08.2022 13:10, Dylanger Daly wrote:
> Yes please, I have Qubes's Build System setup with sourcehut so I can add
> patches at will, however please be aware Qubes currently uses Xen 4.14.
>
> I'll take a look and see if I can access that location
>
> With the added logging I should be able to trigger the crash and get to the
> bottom of it
Here's the (trivial) patch. It's against out version of 4.14, but I expect
to apply fine. Further logging would likely need to go in the kernel,
since - if my analysis+guessing is right - we wouldn't even see a mapping
attempt in Xen.
Jan
--- sle15sp3.orig/xen/arch/x86/e820.c
+++ sle15sp3/xen/arch/x86/e820.c
@@ -700,3 +700,15 @@ unsigned long __init init_e820(const cha
return find_max_pfn();
}
+
+#include //temp
+static int __init ryzen6000_init(void) {//temp
+ if(e820_all_mapped(0x7AF67000, 0x7AF68000, E820_NVS)) {
+ const uint32_t*p = map_domain_page(_mfn(0x7AF67));
+ printk("0x7AF67000: %08x %08x %08x %08x\n", p[0], p[1], p[2], p[3]);
+ printk("0x7AF67010: %08x %08x %08x %08x\n", p[4], p[5], p[6], p[7]);
+ unmap_domain_page(p);
+ }
+ return 0;
+}
+__initcall(ryzen6000_init);
Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table
Hi Julien, On Thu, Aug 25, 2022 at 01:59:06PM +0100, Julien Grall wrote: [...] > > Seems to me, to support para virtualization driver model (like virtio), > > Dom0 needs to provide the device driver backend, and DomUs enables > > the forend device drivers. In this case, the most hardware interrupts > > (SPIs) are routed to Dom0. > > That's correct. Most of the shared interrupts will be routed to dom0. Thanks for confirmation. > > To support passthrough driver model (VFIO), Xen needs to configure the > > hardware GICv3 to directly route hardware interrupt to the virtual CPU > > interface. > > Do you mean GICv4 rather than GICv3? In the latter, all the interrupts will > be received in Xen and then routed to the domain by updating the LRs. Thanks for clarification. So GICv3 relies on hypervisor to set LR, and VM can use virtural interface to response (ACK/EOI) the interrupt. GICv4 can directly forward the SPI to the CPU virtual interface (without hypervisor's interfering). > > But here I still cannot create the concept that how GIC RD tables play > > roles to support the para virtualization or passthrough mode. > > I am not sure what you are actually asking. The pending tables are just > memory you give to the GICv3 to record the state of the interrupts. For more specific, Xen has its own RD pending table, and we can use this pending table to set state for SGI/PPI/LPI for a specific CPU interface. Xen works as hypervisor, it saves and restores the pending table according to switched in VM context, right? On the other hand, what's the purpose for Linux kernel's GIC RD pending table? Is it only used for nested virtulisation? I mean if Linux kernel's GIC RD pending table is not used for the drivers in Dom0 or DomU, then it's useless to pass it from the primary kernel to secondary kernel; as result, we don't need to reserve the persistent memory for the pending table in this case. Thanks, Leo
Re: [PATCH v4] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.2022 16:19, Juergen Gross wrote:
> The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
> potentially with pages being NULL, leading to a NULL dereference.
>
> Additionally lock_pages() doesn't check for pin_user_pages_fast()
> having been completely successful, resulting in potentially not
> locking all pages into memory. This could result in sporadic failures
> when using the related memory in user mode.
>
> Fix all of that by calling unlock_pages() always with the real number
> of pinned pages, which will be zero in case pages being NULL, and by
> checking the number of pages pinned by pin_user_pages_fast() matching
> the expected number of pages.
>
> Cc:
> Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
> Reported-by: Rustam Subkhankulov
> Signed-off-by: Juergen Gross
Reviewed-by: Jan Beulich
Re: [PATCH v2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion
Hi Jan, On 25/08/2022 15:34, Jan Beulich wrote: On 25.08.2022 16:31, Julien Grall wrote: On 24/08/2022 13:44, Bertrand Marquis wrote: On 24 Aug 2022, at 13:33, Jan Beulich wrote: While Arm64 does so uniformly, for Arm32 only strchr() currently handles this properly. Add the necessary conversion also to strrchr(), memchr(), and memset(). As to the placement in memset(): Putting the new insn at the beginning of the function is apparently deemed more "obvious". It could be placed later, as the code reachable without ever making it to the "1" label only ever does byte stores. Signed-off-by: Jan Beulich Reviewed-by: Bertrand Marquis It is now committed. But then perhaps not pushed yet? Yes. I tend to send the message just after I apply it. I will push when I am done with a batch (in this case, this is the only patch I pushed). Cheers, -- Julien Grall
Re: [PATCH v2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion
On 25.08.2022 16:31, Julien Grall wrote: > On 24/08/2022 13:44, Bertrand Marquis wrote: >>> On 24 Aug 2022, at 13:33, Jan Beulich wrote: >>> >>> While Arm64 does so uniformly, for Arm32 only strchr() currently handles >>> this properly. Add the necessary conversion also to strrchr(), memchr(), >>> and memset(). >>> >>> As to the placement in memset(): Putting the new insn at the beginning >>> of the function is apparently deemed more "obvious". It could be placed >>> later, as the code reachable without ever making it to the "1" label >>> only ever does byte stores. >>> >>> Signed-off-by: Jan Beulich >> Reviewed-by: Bertrand Marquis > > It is now committed. But then perhaps not pushed yet? Jan
[linux-linus test] 172764: regressions - FAIL
flight 172764 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/172764/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172133 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172133 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 172133 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 172133 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qcow2 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-raw 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 172133 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 172133 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 172133 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 172133 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 172133 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 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-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 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 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 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-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 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-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass version targeted for testing: linuxc40e8341e3b3bb27e3a65b06b5b454626234c4f0 baseline version: linuxb44f2fd87919b5ae6e1756d4c7ba2cbba22238e1 Last test of basis 172133 2022-08-04 05:14:48 Z 21 days Failing since172152 2022-08-05 04:01:26 Z 20 days 47 attempts Testing same since 172743 2022-08-24 03:29:51 Z1 days3 attempts 1512 people touched revisions under test, not listing them all jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64
Re: [PATCH v2] Arm32: correct string.h functions for "int" -> "unsigned char" conversion
On 24/08/2022 13:44, Bertrand Marquis wrote: On 24 Aug 2022, at 13:33, Jan Beulich wrote: While Arm64 does so uniformly, for Arm32 only strchr() currently handles this properly. Add the necessary conversion also to strrchr(), memchr(), and memset(). As to the placement in memset(): Putting the new insn at the beginning of the function is apparently deemed more "obvious". It could be placed later, as the code reachable without ever making it to the "1" label only ever does byte stores. Signed-off-by: Jan Beulich Reviewed-by: Bertrand Marquis It is now committed. Cheers, -- Julien Grall
[PATCH v4] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
potentially with pages being NULL, leading to a NULL dereference.
Additionally lock_pages() doesn't check for pin_user_pages_fast()
having been completely successful, resulting in potentially not
locking all pages into memory. This could result in sporadic failures
when using the related memory in user mode.
Fix all of that by calling unlock_pages() always with the real number
of pinned pages, which will be zero in case pages being NULL, and by
checking the number of pages pinned by pin_user_pages_fast() matching
the expected number of pages.
Cc:
Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
Reported-by: Rustam Subkhankulov
Signed-off-by: Juergen Gross
---
V2:
- use "pinned" as parameter for unlock_pages() (Jan Beulich)
- drop label "unlock" again (Jan Beulich)
- add check for complete success of pin_user_pages_fast()
V3:
- continue after partial success of pin_user_pages_fast() (Jan Beulich)
V4:
- fix case of multiple partial successes for one buffer (Jan Beulich)
---
drivers/xen/privcmd.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 3369734108af..e88e8f6f0a33 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -581,27 +581,30 @@ static int lock_pages(
struct privcmd_dm_op_buf kbufs[], unsigned int num,
struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
{
- unsigned int i;
+ unsigned int i, off = 0;
- for (i = 0; i < num; i++) {
+ for (i = 0; i < num; ) {
unsigned int requested;
int page_count;
requested = DIV_ROUND_UP(
offset_in_page(kbufs[i].uptr) + kbufs[i].size,
- PAGE_SIZE);
+ PAGE_SIZE) - off;
if (requested > nr_pages)
return -ENOSPC;
page_count = pin_user_pages_fast(
- (unsigned long) kbufs[i].uptr,
+ (unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
requested, FOLL_WRITE, pages);
- if (page_count < 0)
- return page_count;
+ if (page_count <= 0)
+ return page_count ? : -EFAULT;
*pinned += page_count;
nr_pages -= page_count;
pages += page_count;
+
+ off = (requested == page_count) ? 0 : off + page_count;
+ i += !off;
}
return 0;
@@ -677,10 +680,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
}
rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
- if (rc < 0) {
- nr_pages = pinned;
+ if (rc < 0)
goto out;
- }
for (i = 0; i < kdata.num; i++) {
set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
@@ -692,7 +693,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
xen_preemptible_hcall_end();
out:
- unlock_pages(pages, nr_pages);
+ unlock_pages(pages, pinned);
kfree(xbufs);
kfree(pages);
kfree(kbufs);
--
2.35.3
[ovmf test] 172771: regressions - FAIL
flight 172771 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/172771/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172136 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172136 version targeted for testing: ovmf 4d83ee04f44a8dc9e6425a719b39c9d378730ca1 baseline version: ovmf 444260d45ec2a84e8f8c192b3539a3cd5591d009 Last test of basis 172136 2022-08-04 06:43:42 Z 21 days Failing since172151 2022-08-05 02:40:28 Z 20 days 167 attempts Testing same since 172746 2022-08-24 05:42:04 Z1 days 10 attempts People who touched revisions under test: Abdul Lateef Attar Abner Chang Chasel Chiu Czajkowski, Maciej Dimitrije Pavlov Dun Tan Edward Pickup Foster Nong Gregx Yeh Igor Kulchytskyy James Lu Jose Marinho KasimX Liu Kavya Konstantin Aladyshev Liu, Zhiguang Maciej Czajkowski Michael D Kinney Ray Ni Sainadh Nagolu Sami Mujawar Shengfengx Xue Zhiguang Liu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt fail build-i386-libvirt fail 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 Not pushing. (No revision log; it would be 926 lines long.)
Re: [PATCH v3 6/6] xen: introduce a Kconfig option to configure NUMA nodes number
On 22.08.2022 04:58, Wei Chen wrote: > Current NUMA nodes number is a hardcode configuration. This > configuration is difficult for an administrator to change > unless changing the code. > > So in this patch, we introduce this new Kconfig option for > administrators to change NUMA nodes number conveniently. > Also considering that not all architectures support NUMA, > this Kconfig option only can be visible on NUMA enabled > architectures. Non-NUMA supported architectures can still > use 1 as MAX_NUMNODES. Especially the uses of "NUMA nodes number" make this read somewhat odd. If I was to re-write all of this, it would become something like: Currently the maximum number of NUMA nodes is a hardcoded value. This provides little flexibility unless changing the code. Introduce a new Kconfig option to change the maximum number of NUMA nodes conveniently. Also considering that not all architectures support NUMA, this Kconfig option is only visible on NUMA enabled architectures. Architectures not supporting NUMA still use 1 for MAX_NUMNODES. > As NODES_SHIFT is currently unused, we're taking this > opportunity to remove it. > > Signed-off-by: Wei Chen Acked-by: Jan Beulich Note that there's an alternative with less #ifdef-ary: config NR_NUMA_NODES int "Maximum number of NUMA nodes supported" if NUMA range 2 64 if NUMA default "1" if !NUMA default "64" But I can see reasons why one might deem it better for there to not be any CONFIG_NR_NUMA_NODES in the resulting .config when !NUMA. Jan
Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table
Hi Leo, On 25/08/2022 12:50, Leo Yan wrote: On Thu, Aug 25, 2022 at 10:07:18AM +0100, Julien Grall wrote: [...] Xen directly passes ACPI MADT table from UEFI to Linux kernel to Dom0, (see functions acpi_create_madt() and gic_make_hwdom_madt()), which means the Linux kernel Dom0 uses the same ACPI table to initialize GICv3 driver, but since Linux kernel Dom0 accesses GIC memory region as IPA, it still trap to Xen in EL2 for stage 2 translation, so finally Xen can emulate the GICv3 device for Dom0. In the default setup, dom0 is also the hardware domain. So it owns all of the devices but the ones used by Xen (e.g. interrupt controller, SMMU). Therefore, dom0 will use the same memory layout as the host. At which point, it is a lot more convenient to re-use the host ACPI tables and rewrite only what's necessary. We cannot purely talk about interrupt handling without connecting with device driver model. Seems to me, to support para virtualization driver model (like virtio), Dom0 needs to provide the device driver backend, and DomUs enables the forend device drivers. In this case, the most hardware interrupts (SPIs) are routed to Dom0. That's correct. Most of the shared interrupts will be routed to dom0. To support passthrough driver model (VFIO), Xen needs to configure the hardware GICv3 to directly route hardware interrupt to the virtual CPU interface. Do you mean GICv4 rather than GICv3? In the latter, all the interrupts will be received in Xen and then routed to the domain by updating the LRs. But here I still cannot create the concept that how GIC RD tables play roles to support the para virtualization or passthrough mode. I am not sure what you are actually asking. The pending tables are just memory you give to the GICv3 to record the state of the interrupts. Cheers, -- Julien Grall
Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table
Hi Leo, On 25/08/2022 12:24, Leo Yan wrote: On Thu, Aug 25, 2022 at 10:07:18AM +0100, Julien Grall wrote: [...] In other words, let's assume the Dom0 kernel panic and its secondary kernel is launched by kexec, is it necessarily for the secondary kernel to reuse the primary kernel's RD pending page? No. If the answer is no, then I think it's feasible to pass the same ACPI table or DT binding for virtual GICv3 from primary kernel to secondary kernel, then the second kernel can initialize the VGIC and allocate a new RD tables (and trap to Xen in EL2 to handle the new allocated RD tables). How about you think for this? I think that would work. Cheers, -- Julien Grall
Re: [PATCH v3 5/6] xen/x86: move NUMA scan nodes codes from x86 to common
On 22.08.2022 04:58, Wei Chen wrote:
> --- a/xen/common/numa.c
> +++ b/xen/common/numa.c
> @@ -13,6 +13,21 @@
> #include
> #include
>
> +static nodemask_t __initdata processor_nodes_parsed;
> +static nodemask_t __initdata memory_nodes_parsed;
> +static struct node __initdata nodes[MAX_NUMNODES];
> +
> +static int __ro_after_init num_node_memblks;
unsigned int?
> @@ -36,6 +51,308 @@ bool numa_disabled(void)
> return numa_off || arch_numa_disabled(false);
> }
>
> +void __init numa_set_processor_nodes_parsed(nodeid_t node)
> +{
> +node_set(node, processor_nodes_parsed);
> +}
> +
> +unsigned int valid_numa_range(paddr_t start, paddr_t end, nodeid_t node)
bool (and then true/false below)?
> +{
> +unsigned int i;
> +
> +for ( i = 0; i < num_node_memblks; i++ )
> +{
> +struct node *nd = &node_memblk_range[i];
const?
> +if ( nd->start <= start && nd->end >= end &&
> + memblk_nodeid[i] == node )
> +return 1;
> +}
> +
> +return 0;
> +}
> +
> +static
> +enum conflicts __init conflicting_memblks(nodeid_t nid, paddr_t start,
May I ask that you re-flow this to either
static enum conflicts __init
conflicting_memblks(nodeid_t nid, paddr_t start,
or
static enum conflicts __init conflicting_memblks(
nodeid_t nid, paddr_t start,
?
> + paddr_t end, paddr_t nd_start,
> + paddr_t nd_end, unsigned int
> *mblkid)
> +{
> +unsigned int i;
> +
> +/*
> + * Scan all recorded nodes' memory blocks to check conflicts:
> + * Overlap or interleave.
> + */
> +for ( i = 0; i < num_node_memblks; i++ )
> +{
> +struct node *nd = &node_memblk_range[i];
const?
> +bool __init numa_memblks_available(void)
> +{
> +return num_node_memblks < NR_NODE_MEMBLKS;
> +}
This is kind of clumsy, but I have no better suggestion.
> +/*
> + * This function will be called by NUMA memory affinity initialization to
> + * update NUMA node's memory range. In this function, we assume all memory
> + * regions belonging to a single node are in one chunk. Holes (or MMIO
> + * ranges) between them will be included in the node.
> + *
> + * So in numa_update_node_memblks, if there are multiple banks for each
> + * node, start and end are stretched to cover the holes between them, and
> + * it works as long as memory banks of different NUMA nodes don't interleave.
> + */
> +int __init numa_update_node_memblks(nodeid_t node, unsigned int arch_nid,
The function only ever returns 0 or -EINVAL - please consider switching
to "bool" return type.
> +paddr_t start, paddr_t size,
> +const char *prefix,
> +bool hotplug)
> +{
> +unsigned int i;
> +paddr_t end = start + size;
> +paddr_t nd_start = start;
> +paddr_t nd_end = end;
> +struct node *nd = &nodes[node];
> +
> +/*
> + * For the node that already has some memory blocks, we will
> + * expand the node memory range temporarily to check memory
> + * interleaves with other nodes. We will not use this node
> + * temp memory range to check overlaps, because it will mask
> + * the overlaps in same node.
> + *
> + * Node with 0 bytes memory doesn't need this expandsion.
> + */
> +if ( nd->start != nd->end )
> +{
> +if ( nd_start > nd->start )
> +nd_start = nd->start;
> +
> +if ( nd_end < nd->end )
> +nd_end = nd->end;
> +}
> +
> +/* It is fine to add this area to the nodes data it will be used later*/
Please adjust style here.
> +switch ( conflicting_memblks(node, start, end, nd_start, nd_end, &i) )
> +{
> +case OVERLAP:
> +if ( memblk_nodeid[i] == node )
> +{
> +bool mismatch = !(hotplug) != !test_bit(i, memblk_hotplug);
> +
> +printk("%sNUMA: %s %u [%"PRIpaddr", %"PRIpaddr"] overlaps with
> itself [%"PRIpaddr", %"PRIpaddr"]\n",
> + mismatch ? KERN_ERR : KERN_WARNING, prefix,
> + arch_nid, start, end - 1,
> + node_memblk_range[i].start, node_memblk_range[i].end - 1);
> +if ( mismatch )
> +return -EINVAL;
> +break;
> +}
> +
> +printk(KERN_ERR
> + "NUMA: %s %u [%"PRIpaddr", %"PRIpaddr"] overlaps with %s %u
> [%"PRIpaddr", %"PRIpaddr"]\n",
> + prefix, arch_nid, start, end - 1, prefix,
> + numa_node_to_arch_nid(memblk_nodeid[i]),
> + node_memblk_range[i].start, node_memblk_range[i].end - 1);
> +return -EINVAL;
> +
> +
> +case INTERLEAVE:
> +printk(KERN_ERR
> + "NUMA： %s %u: [%"PRIpaddr", %"PRIpaddr"] interleaves with %s
> %u memblk [%"PRIpaddr", %"PRIpaddr"]\n",
> + prefix, arch_nid, nd_start, nd_end - 1,
> + prefix, numa_node_
[libvirt test] 172765: regressions - FAIL
flight 172765 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/172765/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 151777 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a version targeted for testing: libvirt 4c0310677a48fe3a64d560737b07469725e40ae4 baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 776 days Failing since151818 2020-07-11 04:18:52 Z 775 days 757 attempts Testing same since 172765 2022-08-25 04:20:31 Z0 days1 attempts People who touched revisions under test: Adolfo Jayme Barrientos Aleksandr Alekseev Aleksei Zakharov Amneesh Singh Andika Triwidada Andrea Bolognani Andrew Melnychenko Ani Sinha Balázs Meskó Barrett Schonefeld Bastian Germann Bastien Orivel BiaoXiang Ye Bihong Yu Binfeng Wu Bjoern Walk Boris Fiuczynski Brad Laue Brian Turek Bruno Haible Chris Mayo Christian Borntraeger Christian Ehrhardt Christian Kirbach Christian Schoenebeck Christophe de Dinechin Christophe Fergeau Claudio Fontana Cole Robinson Collin Walling Cornelia Huck Cédric Bosdonnat Côme Borsoi Daniel Henrique Barboza Daniel Letai Daniel P. Berrange Daniel P. Berrangé Dario Faggioli David Michael Didik Supriadi dinglimin Divya Garg Dmitrii Shcherbakov Dmytro Linkin Eiichi Tsukata Emilio Herrera Eric Farman Erik Skultety Eugenio Pérez Fabian Affolter Fabian Freyer Fabiano Fidêncio Fangge Jin Farhan Ali Fedora Weblate Translation Florian Schmidt Franck Ridel Gavi Teitz gongwei Guoyi Tu Göran Uddeborg Halil Pasic Han Han Hao Wang Haonan Wang Hela Basa Helmut Grohne Hiroki Narukawa Hyman Huang(黄勇) Ian Wienand Ioanna Alifieraki Ivan Teterevkov Jakob Meng Jamie Strandboge Jamie Strandboge Jan Kuparinen jason lee Jean-Baptiste Holcroft Jia Zhou Jianan Gao Jim Fehlig Jin Yan Jing Qi Jinsheng Zhang Jiri Denemark Joachim Falk John Ferlan John Levon John Levon Jonathan Watt Jonathon Jongsma Julio Faracco Justin Gatzen Ján Tomko Kashyap Chamarthy Kevin Locke Kim InSoo Koichi Murase Kristina Hanicova Laine Stump Laszlo Ersek Lee Yarwood Lei Yang Lena Voytek Liang Yan Liang Yan Liao Pingfang Lin Ma Lin Ma Lin Ma Liu Yiding Lubomir Rintel Ludek Janda Luke Yue Luyao Zhong luzhipeng Marc Hartmayer Marc-André Lureau Marek Marczykowski-Górecki Mark Mielke Markus Schade Martin Kletzander Martin Pitt Masayoshi Mizuma Matej Cepl Matt Coleman Matt Coleman Mauro Matteo Cascella Max Goodhart Maxim Nestratov Meina Li Michal Privoznik Michał Smyk Milo Casagrande minglei.liu Moshe Levi Moteen Shah Moteen Shah Muha Aliss Nathan Neal Gompa Nick Chevsky Nick Shyrokovskiy Nickys Music Group Nico Pache Nicolas Lécureuil Nicolas Lécureuil Nikolay Shirokovskiy Nikolay Shirokovskiy Nikolay Shirokovskiy Niteesh Dubey Olaf Hering Oleksandr Tyshchenko Olesya Gerasimenko
Re: [PATCH v3] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.2022 14:10, Juergen Gross wrote:
> On 25.08.22 13:58, Jan Beulich wrote:
>> On 25.08.2022 13:40, Juergen Gross wrote:
>>> --- a/drivers/xen/privcmd.c
>>> +++ b/drivers/xen/privcmd.c
>>> @@ -581,7 +581,7 @@ static int lock_pages(
>>> struct privcmd_dm_op_buf kbufs[], unsigned int num,
>>> struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
>>> {
>>> - unsigned int i;
>>> + unsigned int i, off = 0;
>>>
>>> for (i = 0; i < num; i++) {
>>> unsigned int requested;
>>> @@ -589,19 +589,23 @@ static int lock_pages(
>>>
>>> requested = DIV_ROUND_UP(
>>> offset_in_page(kbufs[i].uptr) + kbufs[i].size,
>>> - PAGE_SIZE);
>>> + PAGE_SIZE) - off;
>>> if (requested > nr_pages)
>>> return -ENOSPC;
>>>
>>> page_count = pin_user_pages_fast(
>>> - (unsigned long) kbufs[i].uptr,
>>> + (unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
>>> requested, FOLL_WRITE, pages);
>>> - if (page_count < 0)
>>> - return page_count;
>>> + if (page_count <= 0)
>>> + return page_count ? : -EFAULT;
>>>
>>> *pinned += page_count;
>>> nr_pages -= page_count;
>>> pages += page_count;
>>> +
>>> + off = requested - page_count;
>>> + if (off)
>>> + i--;
>>> }
>>
>> Initially I thought this would go wrong only on the 3rd iteration, but
>> meanwhile I think it's wrong already on the 2nd. What I think you need
>> is
>>
>> if (page_count < requested)
>> i--;
>> off += page_count;
>>
>> or with the i++ from the loop header absorbed here
>>
>> if (page_count == requested)
>> i++;
>> off += page_count;
>>
>> Plus of course off needs resetting to zero whenever i advances. I.e.
>>
>> if (page_count == requested) {
>> i++;
>> off = 0;
>> } else {
>> off += page_count;
>> }
>
> Yeah, or:
>
> off = (page_count == requested) ? 0 : off + page_count;
> i += !off;
I wasn't daring to suggest something like that ;-)
Jan
Re: [PATCH v3 4/6] xen/x86: use arch_get_ram_range to get information from E820 map
On 22.08.2022 04:58, Wei Chen wrote:
> @@ -96,3 +97,27 @@ unsigned int __init arch_get_dma_bitsize(void)
> flsl(node_start_pfn(node) + node_spanned_pages(node) / 4 -
> 1)
> + PAGE_SHIFT, 32);
> }
> +
> +/*
> + * This function provides the ability for caller to get one RAM entry
> + * from architectural memory map by index.
> + *
> + * This function will return zero if it can return a proper RAM entry.
> + * otherwise it will return -ENOENT for out of scope index, or return
> + * -EINVAL for non-RAM type memory entry.
> + *
> + * Note: the range is exclusive at the end, e.g. [start, end).
> + */
> +int __init arch_get_ram_range(unsigned int idx, paddr_t *start, paddr_t *end)
Since the comment is intended to apply to all architectures providing,
I think it should go with the declaration (once) rather than the
definition (at least one instance per arch).
> +{
> +if ( idx >= e820.nr_map )
> +return -ENOENT;
> +
> +if ( e820.map[idx].type != E820_RAM )
> +return -EINVAL;
EINVAL is so heavily (over)loaded that I'm inclined to ask to use e.g.
-ENODATA here.
> --- a/xen/arch/x86/srat.c
> +++ b/xen/arch/x86/srat.c
> @@ -428,18 +428,22 @@ acpi_numa_memory_affinity_init(const struct
> acpi_srat_mem_affinity *ma)
> Make sure the PXMs cover all memory. */
> static int __init nodes_cover_memory(void)
> {
> - int i;
> + unsigned int i;
>
> - for (i = 0; i < e820.nr_map; i++) {
> + for (i = 0; ; i++) {
> int j, found;
> paddr_t start, end;
>
> - if (e820.map[i].type != E820_RAM) {
> + /* Try to loop memory map from index 0 to end to get RAM
> ranges. */
> + found = arch_get_ram_range(i, &start, &end);
> +
> + /* Index relate entry is not RAM, skip it. */
> + if (found == -EINVAL)
> continue;
> - }
>
> - start = e820.map[i].addr;
> - end = e820.map[i].addr + e820.map[i].size;
> + /* Reach the end of arch's memory map */
> + if (found == -ENOENT)
> + break;
What if an arch returns a 3rd error indicator? The way you've written
it code below would assume success and use uninitialized data. I'd
like to suggest to only special-case -ENOENT and treat all other
errors the same. But of course the variable (re)used doesn't really
fit that:
/* Reach the end of arch's memory map */
if (found == -ENOENT)
break;
/* Index relate entry is not RAM, skip it. */
if (found)
continue;
because here really you mean "not found". Since in fact "found" would
want to be of "bool" type in the function, and "j" would want to be
"unsigned int" just like "i" is, I recommend introducing a new local
variable, e.g. "err".
Jan
> do {
> found = 0;
Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
On 25.08.22 13:42, Borislav Petkov wrote: On Thu, Aug 25, 2022 at 12:41:05PM +0200, Juergen Gross wrote: Maybe the alternative reasoning is much faster to understand: if the Cyrix set_all() could be called, the AMD and Centaur ones would be callable, too. Right. Those being called would result in a NULL deref, so why should we keep the Cyrix one? I know you're eager to remove dead code - I'd love that too. But before we do that, we need to find out whether some Cyrix hw out there would not need this. Back to reasoning #1. Only the use_intel() case calls the code in question with reg == ~0. And use_intel() is clearly not Cyrix. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v3] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.22 13:58, Jan Beulich wrote:
On 25.08.2022 13:40, Juergen Gross wrote:
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -581,7 +581,7 @@ static int lock_pages(
struct privcmd_dm_op_buf kbufs[], unsigned int num,
struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
{
- unsigned int i;
+ unsigned int i, off = 0;
for (i = 0; i < num; i++) {
unsigned int requested;
@@ -589,19 +589,23 @@ static int lock_pages(
requested = DIV_ROUND_UP(
offset_in_page(kbufs[i].uptr) + kbufs[i].size,
- PAGE_SIZE);
+ PAGE_SIZE) - off;
if (requested > nr_pages)
return -ENOSPC;
page_count = pin_user_pages_fast(
- (unsigned long) kbufs[i].uptr,
+ (unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
requested, FOLL_WRITE, pages);
- if (page_count < 0)
- return page_count;
+ if (page_count <= 0)
+ return page_count ? : -EFAULT;
*pinned += page_count;
nr_pages -= page_count;
pages += page_count;
+
+ off = requested - page_count;
+ if (off)
+ i--;
}
Initially I thought this would go wrong only on the 3rd iteration, but
meanwhile I think it's wrong already on the 2nd. What I think you need
is
if (page_count < requested)
i--;
off += page_count;
or with the i++ from the loop header absorbed here
if (page_count == requested)
i++;
off += page_count;
Plus of course off needs resetting to zero whenever i advances. I.e.
if (page_count == requested) {
i++;
off = 0;
} else {
off += page_count;
}
Yeah, or:
off = (page_count == requested) ? 0 : off + page_count;
i += !off;
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
[xen-unstable test] 172762: tolerable FAIL - PUSHED
flight 172762 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/172762/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds18 guest-start/debian.repeat fail REGR. vs. 172754 Tests which did not succeed, but are not blocking: test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a build-amd64-libvirt 6 libvirt-buildfail like 172754 build-i386-libvirt6 libvirt-buildfail like 172754 build-arm64-libvirt 6 libvirt-buildfail like 172754 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 172754 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 172754 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 172754 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 172754 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 172754 build-armhf-libvirt 6 libvirt-buildfail like 172754 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 172754 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 172754 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 172754 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 172754 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 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-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-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-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 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-arm64-arm64-xl-vhd 14 migrate-support-checkfail never pass test-arm64-arm64-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 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-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-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 15 migrate-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 version targeted
Re: Backport request
On Wed, Aug 24, 2022 at 03:52:27PM +0200, Juergen Gross wrote:
> On 24.08.22 14:10, Greg Kroah-Hartman wrote:
> > On Wed, Aug 24, 2022 at 01:20:22PM +0200, Juergen Gross wrote:
> > > Hi Greg,
> > >
> > > stable kernels 5.18 and 5.15 seem to be missing upstream patch
> > > c64cc2802a78 ("x86/entry: Move CLD to the start of the idtentry macro").
> > > This is a prerequisite patch for 64cbd0acb582 ("x86/entry: Don't call
> > > error_entry() for XENPV"), which is included in 5.15.y and 5.18.y.
> > >
> > > Could you please take c64cc2802a78 for 5.15 and 5.18?
> >
> > 5.18 is end-of-life, so that's impossible to do now :(
> >
> > For 5.15.y, the commit does not apply cleanly, can you provide a working
> > backport?
>
> Attached.
Thanks, now queued up.
greg k-h
Re: [PATCH v3] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.2022 13:40, Juergen Gross wrote:
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -581,7 +581,7 @@ static int lock_pages(
> struct privcmd_dm_op_buf kbufs[], unsigned int num,
> struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
> {
> - unsigned int i;
> + unsigned int i, off = 0;
>
> for (i = 0; i < num; i++) {
> unsigned int requested;
> @@ -589,19 +589,23 @@ static int lock_pages(
>
> requested = DIV_ROUND_UP(
> offset_in_page(kbufs[i].uptr) + kbufs[i].size,
> - PAGE_SIZE);
> + PAGE_SIZE) - off;
> if (requested > nr_pages)
> return -ENOSPC;
>
> page_count = pin_user_pages_fast(
> - (unsigned long) kbufs[i].uptr,
> + (unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
> requested, FOLL_WRITE, pages);
> - if (page_count < 0)
> - return page_count;
> + if (page_count <= 0)
> + return page_count ? : -EFAULT;
>
> *pinned += page_count;
> nr_pages -= page_count;
> pages += page_count;
> +
> + off = requested - page_count;
> + if (off)
> + i--;
> }
Initially I thought this would go wrong only on the 3rd iteration, but
meanwhile I think it's wrong already on the 2nd. What I think you need
is
if (page_count < requested)
i--;
off += page_count;
or with the i++ from the loop header absorbed here
if (page_count == requested)
i++;
off += page_count;
Plus of course off needs resetting to zero whenever i advances. I.e.
if (page_count == requested) {
i++;
off = 0;
} else {
off += page_count;
}
Jan
Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table
On Thu, Aug 25, 2022 at 10:07:18AM +0100, Julien Grall wrote: [...] > > Xen directly passes ACPI MADT table from UEFI to Linux kernel to Dom0, > > (see functions acpi_create_madt() and gic_make_hwdom_madt()), which > > means the Linux kernel Dom0 uses the same ACPI table to initialize GICv3 > > driver, but since Linux kernel Dom0 accesses GIC memory region as IPA, > > it still trap to Xen in EL2 for stage 2 translation, so finally Xen > > can emulate the GICv3 device for Dom0. > > In the default setup, dom0 is also the hardware domain. So it owns all of > the devices but the ones used by Xen (e.g. interrupt controller, SMMU). > > Therefore, dom0 will use the same memory layout as the host. At which point, > it is a lot more convenient to re-use the host ACPI tables and rewrite only > what's necessary. We cannot purely talk about interrupt handling without connecting with device driver model. Seems to me, to support para virtualization driver model (like virtio), Dom0 needs to provide the device driver backend, and DomUs enables the forend device drivers. In this case, the most hardware interrupts (SPIs) are routed to Dom0. To support passthrough driver model (VFIO), Xen needs to configure the hardware GICv3 to directly route hardware interrupt to the virtual CPU interface. But here I still cannot create the concept that how GIC RD tables play roles to support the para virtualization or passthrough mode. Thanks, Leo
Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
On Thu, Aug 25, 2022 at 12:41:05PM +0200, Juergen Gross wrote: > Maybe the alternative reasoning is much faster to understand: if the > Cyrix set_all() could be called, the AMD and Centaur ones would be callable, > too. Right. > Those being called would result in a NULL deref, so why should we keep > the Cyrix one? I know you're eager to remove dead code - I'd love that too. But before we do that, we need to find out whether some Cyrix hw out there would not need this. I know, I know, they should've complained by now ... maybe they have but we haven't heard about it. What it most likely looks like is that those machines - a commit from before git commit 8fbdcb188e31ac901e216b466b97e90e8b057daa Author: Dave Jones Date: Wed Aug 14 21:14:22 2002 -0700 [PATCH] Modular x86 MTRR driver. talks about +/* + * On Cyrix 6x86(MX) and M II the ARR3 is special: it has connection + * with the SMM (System Management Mode) mode. So we need the following: + * Check whether SMI_LOCK (CCR3 bit 0) is set + * if it is set, write a warning message: ARR3 cannot be changed! + * (it cannot be changed until the next processor reset) which sounds like old rust. And which no one uses or such machines are long dead already. Wikipedia says: https://en.wikipedia.org/wiki/Cyrix_6x86 "The Cyrix 6x86 is a line of sixth-generation, 32-bit x86 microprocessors designed and released by Cyrix in 1995..." So I'm thinking removing it would be ok... -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
[PATCH v3] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
potentially with pages being NULL, leading to a NULL dereference.
Additionally lock_pages() doesn't check for pin_user_pages_fast()
having been completely successful, resulting in potentially not
locking all pages into memory. This could result in sporadic failures
when using the related memory in user mode.
Fix all of that by calling unlock_pages() always with the real number
of pinned pages, which will be zero in case pages being NULL, and by
checking the number of pages pinned by pin_user_pages_fast() matching
the expected number of pages.
Cc:
Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
Reported-by: Rustam Subkhankulov
Signed-off-by: Juergen Gross
---
V2:
- use "pinned" as parameter for unlock_pages() (Jan Beulich)
- drop label "unlock" again (Jan Beulich)
- add check for complete success of pin_user_pages_fast()
V3:
- continue after partial success of pin_user_pages_fast() (Jan Beulich)
---
drivers/xen/privcmd.c | 20 +++-
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 3369734108af..1ca7e3ea6fd4 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -581,7 +581,7 @@ static int lock_pages(
struct privcmd_dm_op_buf kbufs[], unsigned int num,
struct page *pages[], unsigned int nr_pages, unsigned int *pinned)
{
- unsigned int i;
+ unsigned int i, off = 0;
for (i = 0; i < num; i++) {
unsigned int requested;
@@ -589,19 +589,23 @@ static int lock_pages(
requested = DIV_ROUND_UP(
offset_in_page(kbufs[i].uptr) + kbufs[i].size,
- PAGE_SIZE);
+ PAGE_SIZE) - off;
if (requested > nr_pages)
return -ENOSPC;
page_count = pin_user_pages_fast(
- (unsigned long) kbufs[i].uptr,
+ (unsigned long)kbufs[i].uptr + off * PAGE_SIZE,
requested, FOLL_WRITE, pages);
- if (page_count < 0)
- return page_count;
+ if (page_count <= 0)
+ return page_count ? : -EFAULT;
*pinned += page_count;
nr_pages -= page_count;
pages += page_count;
+
+ off = requested - page_count;
+ if (off)
+ i--;
}
return 0;
@@ -677,10 +681,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
}
rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
- if (rc < 0) {
- nr_pages = pinned;
+ if (rc < 0)
goto out;
- }
for (i = 0; i < kdata.num; i++) {
set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
@@ -692,7 +694,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
xen_preemptible_hcall_end();
out:
- unlock_pages(pages, nr_pages);
+ unlock_pages(pages, pinned);
kfree(xbufs);
kfree(pages);
kfree(kbufs);
--
2.35.3
Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table
Hi Julien, On Thu, Aug 25, 2022 at 10:07:18AM +0100, Julien Grall wrote: [...] > > In other words, let's assume the Dom0 kernel panic and its secondary > > kernel is launched by kexec, is it necessarily for the secondary > > kernel to reuse the primary kernel's RD pending page? > > No. If the answer is no, then I think it's feasible to pass the same ACPI table or DT binding for virtual GICv3 from primary kernel to secondary kernel, then the second kernel can initialize the VGIC and allocate a new RD tables (and trap to Xen in EL2 to handle the new allocated RD tables). How about you think for this? Thanks a lot for quick response. Leo
Re: [PATCH 2/2] xen/arm: Handle reserved heap pages in boot and heap allocator
Hi Henry,
On 24/08/2022 09:31, Henry Wang wrote:
>
> This commit firstly adds a global variable `reserved_heap`.
> This newly introduced global variable is set at the device tree
> parsing time if the reserved heap ranges are defined in the device
> tree chosen node.
>
Did you consider putting reserved_heap into bootinfo structure?
It would help to avoid introducing new global variables that are only used
in places making use of the bootinfo anyway.
> For Arm32, In `setup_mm`, if the reserved heap is enabled, we use
> the reserved heap region for both domheap and xenheap allocation.
>
> For Arm64, In `setup_mm`, if the reserved heap is enabled and used,
> we make sure that only these reserved heap pages are added to the
> boot allocator. These reserved heap pages in the boot allocator are
> added to the heap allocator at `end_boot_allocator()`.
>
> If the reserved heap is disabled, we stick to current page allocation
> strategy at boot time.
>
> Also, take the chance to correct a "double not" print in Arm32
> `setup_mm()`.
>
> Signed-off-by: Henry Wang
> ---
> With reserved heap enabled, for Arm64, naming of global variables such
> as `xenheap_mfn_start` and `xenheap_mfn_end` seems to be ambiguous,
> wondering if we should rename these variables.
> ---
> Changes from RFC to v1:
> - Rebase on top of latest `setup_mm()` changes.
> - Added Arm32 logic in `setup_mm()`.
> ---
> xen/arch/arm/bootfdt.c | 2 +
> xen/arch/arm/include/asm/setup.h | 2 +
> xen/arch/arm/setup.c | 79 +---
> 3 files changed, 67 insertions(+), 16 deletions(-)
>
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 33704ca487..ab73b6e212 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -325,6 +325,8 @@ static int __init process_chosen_node(const void *fdt,
> int node,
> true);
> if ( rc )
> return rc;
> +
> +reserved_heap = true;
> }
>
> printk("Checking for initrd in /chosen\n");
> diff --git a/xen/arch/arm/include/asm/setup.h
> b/xen/arch/arm/include/asm/setup.h
> index e80f3d6201..00536a6d55 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -92,6 +92,8 @@ extern struct bootinfo bootinfo;
>
> extern domid_t max_init_domid;
>
> +extern bool reserved_heap;
> +
> void copy_from_paddr(void *dst, paddr_t paddr, unsigned long len);
>
> size_t estimate_efi_size(unsigned int mem_nr_banks);
> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
> index 500307edc0..fe76cf6325 100644
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -73,6 +73,8 @@ integer_param("xenheap_megabytes", opt_xenheap_megabytes);
>
> domid_t __read_mostly max_init_domid;
>
> +bool __read_mostly reserved_heap;
> +
> static __used void init_done(void)
> {
> /* Must be done past setting system_state. */
> @@ -699,8 +701,10 @@ static void __init populate_boot_allocator(void)
> #ifdef CONFIG_ARM_32
> static void __init setup_mm(void)
> {
> -paddr_t ram_start, ram_end, ram_size, e;
> -unsigned long ram_pages;
> +paddr_t ram_start, ram_end, ram_size, e, bank_start, bank_end, bank_size;
> +paddr_t reserved_heap_start = ~0, reserved_heap_end = 0,
> +reserved_heap_size = 0;
> +unsigned long ram_pages, reserved_heap_pages = 0;
> unsigned long heap_pages, xenheap_pages, domheap_pages;
> unsigned int i;
> const uint32_t ctr = READ_CP32(CTR);
> @@ -720,9 +724,9 @@ static void __init setup_mm(void)
>
> for ( i = 1; i < bootinfo.mem.nr_banks; i++ )
> {
> -paddr_t bank_start = bootinfo.mem.bank[i].start;
> -paddr_t bank_size = bootinfo.mem.bank[i].size;
> -paddr_t bank_end = bank_start + bank_size;
> +bank_start = bootinfo.mem.bank[i].start;
> +bank_size = bootinfo.mem.bank[i].size;
> +bank_end = bank_start + bank_size;
>
> ram_size = ram_size + bank_size;
> ram_start = min(ram_start,bank_start);
> @@ -731,6 +735,25 @@ static void __init setup_mm(void)
>
> total_pages = ram_pages = ram_size >> PAGE_SHIFT;
>
> +if ( reserved_heap )
> +{
> +for ( i = 0 ; i < bootinfo.reserved_mem.nr_banks; i++ )
> +{
> +if ( bootinfo.reserved_mem.bank[i].xen_heap )
> +{
> +bank_start = bootinfo.reserved_mem.bank[i].start;
> +bank_size = bootinfo.reserved_mem.bank[i].size;
> +bank_end = bank_start + bank_size;
> +
> +reserved_heap_size += bank_size;
> +reserved_heap_start = min(reserved_heap_start, bank_start);
You do not need reserved_heap_start as you do not use it at any place later on.
In your current implementation you just need reserved_heap_size and
reserved_heap_end.
> +reserved_heap_end = max(reserved_heap_end, bank_end);
> +}
> +}
> +
> +
Re: [PATCH v2] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.22 12:22, Jan Beulich wrote: On 25.08.2022 12:13, Juergen Gross wrote: On 25.08.22 11:50, Jan Beulich wrote: On 25.08.2022 11:26, Juergen Gross wrote: --- a/drivers/xen/privcmd.c +++ b/drivers/xen/privcmd.c @@ -602,6 +602,10 @@ static int lock_pages( *pinned += page_count; nr_pages -= page_count; pages += page_count; + + /* Exact reason isn't known, EFAULT is one possibility. */ + if (page_count < requested) + return -EFAULT; } I don't really know the inner workings of pin_user_pages_fast() nor what future plans there are with it. To be as independent of its behavior as possible, how about bailing here only when page_count actually is zero (i.e. no forward progress)? This would require to rework the loop in lock_pages() to be able to handle only a partial buffer. Oh, I see - I've misread the code as if the loop was capping each iteration's count to the capacity of some internal buffer (as iirc is being done elsewhere). So ... This would add some complexity, but OTOH I'd get an exact error code back in case of failure. ... perhaps not worth it then, ... I'll have a try and see how the result would look like. ... unless you think this might be relevant in certain cases. Not sure, but the resulting code is looking fine IMO. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: Ryzen 6000 (Mobile)
Hi Jan, Yes please, I have Qubes's Build System setup with sourcehut so I can add patches at will, however please be aware Qubes currently uses Xen 4.14. I'll take a look and see if I can access that location With the added logging I should be able to trigger the crash and get to the bottom of it Thank you for your help Jan
Re: [PATCH v3 2/6] xen/x86: move generically usable NUMA code from x86 to common
On 22.08.2022 04:58, Wei Chen wrote:
> --- /dev/null
> +++ b/xen/common/numa.c
> @@ -0,0 +1,440 @@
> +/*
> + * Generic VM initialization for NUMA setups.
> + * Copyright 2002,2003 Andi Kleen, SuSE Labs.
> + * Adapted for Xen: Ryan Harper
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +struct node_data __ro_after_init node_data[MAX_NUMNODES];
> +
> +/* Mapping from pdx to node id */
> +unsigned int __ro_after_init memnode_shift;
> +unsigned long __ro_after_init memnodemapsize;
> +uint8_t *__ro_after_init memnodemap;
> +static uint8_t __ro_after_init _memnodemap[64];
> +
> +nodeid_t __ro_after_init cpu_to_node[NR_CPUS] = {
I don't think this can be __ro_after_init, or you'll break CPU
hotplug.
> +[0 ... NR_CPUS-1] = NUMA_NO_NODE
> +};
> +
> +cpumask_t __ro_after_init node_to_cpumask[MAX_NUMNODES];
Same here.
> +nodemask_t __read_mostly node_online_map = { { [0] = 1UL } };
> +
> +bool __read_mostly numa_off;
This, otoh, can be, or have I missed a place where it's written by a
non-__init function?
> +bool numa_disabled(void)
> +{
> +return numa_off || arch_numa_disabled(false);
> +}
> +
> +/*
> + * Given a shift value, try to populate memnodemap[]
> + * Returns :
> + * 1 if OK
> + * 0 if memnodmap[] too small (of shift too small)
> + * -1 if node overlap or lost ram (shift too big)
> + */
> +static int __init populate_memnodemap(const struct node *nodes,
> + nodeid_t numnodes, unsigned int shift,
I don't think you can use nodeid_t for a variable holding a node count.
Think of what would happen if there were 256 nodes, the IDs of which
all fit in nodeid_t. (Same again further down.)
> + nodeid_t *nodeids)
> +{
> +unsigned long spdx, epdx;
> +nodeid_t i;
This is likely inefficient for a loop counter variable. Note how you
use "unsigned int" in e.g. extract_lsb_from_nodes().
> +unsigned int __init compute_hash_shift(const struct node *nodes,
> + nodeid_t numnodes, nodeid_t *nodeids)
> +{
> +unsigned int shift;
> +
> +shift = extract_lsb_from_nodes(nodes, numnodes);
> +if ( memnodemapsize <= ARRAY_SIZE(_memnodemap) )
> +memnodemap = _memnodemap;
> +else if ( allocate_cachealigned_memnodemap() )
> +return -1;
With this the function can't very well have "unsigned int" return type.
> +void __init numa_init_array(void)
> +{
> +int rr, i;
"unsigned int" for i and perhaps nodeid_t for rr?
> +static int __init numa_emulation(unsigned long start_pfn,
> + unsigned long end_pfn)
> +{
> +unsigned int i;
> +struct node nodes[MAX_NUMNODES];
> +uint64_t sz = pfn_to_paddr(end_pfn - start_pfn) / numa_fake;
> +
> +/* Kludge needed for the hash function */
> +if ( hweight64(sz) > 1 )
> +{
> +u64 x = 1;
uint64_t and a blank line between declaration(s) and statement(s)
please.
> +while ( (x << 1) < sz )
> +x <<= 1;
> +if ( x < sz / 2 )
> +printk(KERN_ERR "Numa emulation unbalanced. Complain to
> maintainer\n");
> +sz = x;
> +}
> +
> +memset(&nodes, 0, sizeof(nodes));
> +for ( i = 0; i < numa_fake; i++ )
> +{
> +nodes[i].start = pfn_to_paddr(start_pfn) + i * sz;
> +if ( i == numa_fake - 1 )
> +sz = pfn_to_paddr(end_pfn) - nodes[i].start;
> +nodes[i].end = nodes[i].start + sz;
> +printk(KERN_INFO "Faking node %u at %"PRIx64"-%"PRIx64"
> (%"PRIu64"MB)\n",
> + i, nodes[i].start, nodes[i].end,
> + (nodes[i].end - nodes[i].start) >> 20);
> +node_set_online(i);
> +}
> +memnode_shift = compute_hash_shift(nodes, numa_fake, NULL);
> +if ( memnode_shift < 0 )
Does the compiler not warn here, comparing an unsigned value for being
negative?
> --- a/xen/include/xen/numa.h
> +++ b/xen/include/xen/numa.h
> @@ -18,4 +18,70 @@
>(((d)->vcpu != NULL && (d)->vcpu[0] != NULL) \
> ? vcpu_to_node((d)->vcpu[0]) : NUMA_NO_NODE)
>
> +/* The following content can be used when NUMA feature is enabled */
> +#ifdef CONFIG_NUMA
> +
> +extern nodeid_t cpu_to_node[NR_CPUS];
> +extern cpumask_t node_to_cpumask[];
> +
> +#define cpu_to_node(cpu)(cpu_to_node[cpu])
> +#define parent_node(node) (node)
> +#define node_to_first_cpu(node) (__ffs(node_to_cpumask[node]))
> +#define node_to_cpumask(node) (node_to_cpumask[node])
Please could you take the opportunity and drop unnecessary parentheses
from here? Afaict only parent_node() need them to be kept.
> +struct node {
> +paddr_t start, end;
> +};
> +
> +extern unsigned int compute_hash_shift(const struct node *nodes,
> + nodeid_t numnodes, nodeid_t *nodeids);
> +
> +#define VIRTUAL_BUG_ON(x)
> +
> +extern bool numa_off;
> +extern void numa_add_cpu(unsigned int cpu);
Please ca
[ovmf test] 172769: regressions - FAIL
flight 172769 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/172769/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172136 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172136 version targeted for testing: ovmf 4d83ee04f44a8dc9e6425a719b39c9d378730ca1 baseline version: ovmf 444260d45ec2a84e8f8c192b3539a3cd5591d009 Last test of basis 172136 2022-08-04 06:43:42 Z 21 days Failing since172151 2022-08-05 02:40:28 Z 20 days 166 attempts Testing same since 172746 2022-08-24 05:42:04 Z1 days9 attempts People who touched revisions under test: Abdul Lateef Attar Abner Chang Chasel Chiu Czajkowski, Maciej Dimitrije Pavlov Dun Tan Edward Pickup Foster Nong Gregx Yeh Igor Kulchytskyy James Lu Jose Marinho KasimX Liu Kavya Konstantin Aladyshev Liu, Zhiguang Maciej Czajkowski Michael D Kinney Ray Ni Sainadh Nagolu Sami Mujawar Shengfengx Xue Zhiguang Liu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt fail build-i386-libvirt fail 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 Not pushing. (No revision log; it would be 926 lines long.)
Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
On 25.08.22 12:31, Borislav Petkov wrote:
On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote:
The Cyrix cpu specific MTRR function cyrix_set_all() will never be
called, as the struct mtrr_ops set_all() callback will only be called
in the use_intel() case, which would require the use_intel_if member
of struct mtrr_ops to be set, which isn't the case for Cyrix.
Doing some git archeology:
So the commit which added mtrr_aps_delayed_init is
d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init")
from 2009.
The IPI callback before it, looked like this:
static void ipi_handler(void *info)
{
#ifdef CONFIG_SMP
struct set_mtrr_data *data = info;
unsigned long flags;
local_irq_save(flags);
atomic_dec(&data->count);
while (!atomic_read(&data->gate))
cpu_relax();
/* The master has cleared me to execute */
if (data->smp_reg != ~0U) {
mtrr_if->set(data->smp_reg, data->smp_base,
data->smp_size, data->smp_type);
} else {
mtrr_if->set_all();
^
and that else branch would call ->set_all() on Cyrix too.
Suresh's patch changed it to do:
- } else {
+ } else if (mtrr_aps_delayed_init) {
+ /*
+* Initialize the MTRRs inaddition to the synchronisation.
+*/
mtrr_if->set_all();
BUT below in the set_mtrr() call, it did:
/*
* HACK!
* We use this same function to initialize the mtrrs on boot.
* The state of the boot cpu's mtrrs has been saved, and we want
* to replicate across all the APs.
* If we're doing that @reg is set to something special...
*/
if (reg != ~0U)
mtrr_if->set(reg, base, size, type);
else if (!mtrr_aps_delayed_init)
mtrr_if->set_all();
^^^
and that would be the Cyrix case.
But then
192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous")
came and "cleaned" all up by removing the "HACK" and doing ->set_all()
only in the rendezvous handler:
+ } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
mtrr_if->set_all();
}
Which begs the question: why doesn't the second part of the else test
match on Cyrix? The "|| !cpu_online(smp_processor_id())" case.
If only we had a Cyrix machine somewhere...
Maybe the alternative reasoning is much faster to understand: if the
Cyrix set_all() could be called, the AMD and Centaur ones would be callable,
too. Those being called would result in a NULL deref, so why should we keep
the Cyrix one?
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2 02/10] x86/mtrr: remove unused cyrix_set_all() function
On 25.08.22 12:31, Borislav Petkov wrote:
On Sat, Aug 20, 2022 at 11:25:25AM +0200, Juergen Gross wrote:
The Cyrix cpu specific MTRR function cyrix_set_all() will never be
called, as the struct mtrr_ops set_all() callback will only be called
in the use_intel() case, which would require the use_intel_if member
of struct mtrr_ops to be set, which isn't the case for Cyrix.
Doing some git archeology:
So the commit which added mtrr_aps_delayed_init is
d0af9eed5aa9 ("x86, pat/mtrr: Rendezvous all the cpus for MTRR/PAT init")
from 2009.
The IPI callback before it, looked like this:
static void ipi_handler(void *info)
{
#ifdef CONFIG_SMP
struct set_mtrr_data *data = info;
unsigned long flags;
local_irq_save(flags);
atomic_dec(&data->count);
while (!atomic_read(&data->gate))
cpu_relax();
/* The master has cleared me to execute */
if (data->smp_reg != ~0U) {
mtrr_if->set(data->smp_reg, data->smp_base,
data->smp_size, data->smp_type);
} else {
mtrr_if->set_all();
^
and that else branch would call ->set_all() on Cyrix too.
Suresh's patch changed it to do:
- } else {
+ } else if (mtrr_aps_delayed_init) {
+ /*
+* Initialize the MTRRs inaddition to the synchronisation.
+*/
mtrr_if->set_all();
BUT below in the set_mtrr() call, it did:
/*
* HACK!
* We use this same function to initialize the mtrrs on boot.
* The state of the boot cpu's mtrrs has been saved, and we want
* to replicate across all the APs.
* If we're doing that @reg is set to something special...
*/
if (reg != ~0U)
mtrr_if->set(reg, base, size, type);
else if (!mtrr_aps_delayed_init)
mtrr_if->set_all();
^^^
and that would be the Cyrix case.
But then
192d8857427d ("x86, mtrr: use stop_machine APIs for doing MTRR rendezvous")
came and "cleaned" all up by removing the "HACK" and doing ->set_all()
only in the rendezvous handler:
+ } else if (mtrr_aps_delayed_init || !cpu_online(smp_processor_id())) {
mtrr_if->set_all();
}
Which begs the question: why doesn't the second part of the else test
match on Cyrix? The "|| !cpu_online(smp_processor_id())" case.
If only we had a Cyrix machine somewhere...
You are missing one aspect here: there is no call path for Cyrix CPUs using
reg == ~0U.
So the condition of the "else if" will never be evaluated with Cyrix.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.2022 12:13, Juergen Gross wrote: > On 25.08.22 11:50, Jan Beulich wrote: >> On 25.08.2022 11:26, Juergen Gross wrote: >>> --- a/drivers/xen/privcmd.c >>> +++ b/drivers/xen/privcmd.c >>> @@ -602,6 +602,10 @@ static int lock_pages( >>> *pinned += page_count; >>> nr_pages -= page_count; >>> pages += page_count; >>> + >>> + /* Exact reason isn't known, EFAULT is one possibility. */ >>> + if (page_count < requested) >>> + return -EFAULT; >>> } >> >> I don't really know the inner workings of pin_user_pages_fast() >> nor what future plans there are with it. To be as independent of >> its behavior as possible, how about bailing here only when >> page_count actually is zero (i.e. no forward progress)? > > This would require to rework the loop in lock_pages() to be able to > handle only a partial buffer. Oh, I see - I've misread the code as if the loop was capping each iteration's count to the capacity of some internal buffer (as iirc is being done elsewhere). So ... > This would add some complexity, but OTOH I'd get an exact error code > back in case of failure. ... perhaps not worth it then, ... > I'll have a try and see how the result would look like. ... unless you think this might be relevant in certain cases. Jan
Re: [PATCH v3 1/6] xen/x86: Provide helpers for common code to access acpi_numa
On 22.08.2022 04:58, Wei Chen wrote:
> --- a/xen/arch/x86/include/asm/numa.h
> +++ b/xen/arch/x86/include/asm/numa.h
> @@ -32,8 +32,9 @@ extern void numa_add_cpu(int cpu);
> extern void numa_init_array(void);
> extern bool numa_off;
>
> -
> -extern int srat_disabled(void);
> +extern int arch_numa_setup(const char *opt);
> +extern bool arch_numa_disabled(bool init_as_disable);
What is the parameter name intended to mean? Since the only caller
passes "false", this also isn't really possible to guess from the
use(s) in this patch. In any event perhaps best for the parameter
to be introduced only once it's actually needed.
> --- a/xen/arch/x86/numa.c
> +++ b/xen/arch/x86/numa.c
> @@ -50,9 +50,31 @@ nodemask_t __read_mostly node_online_map = { { [0] = 1UL }
> };
> bool numa_off;
> s8 acpi_numa = 0;
>
> -int srat_disabled(void)
> +int __init arch_numa_setup(const char *opt)
> {
> -return numa_off || acpi_numa < 0;
> +#ifdef CONFIG_ACPI_NUMA
> +if ( !strncmp(opt, "noacpi", 6) )
> +{
> +numa_off = false;
> +acpi_numa = -1;
> +return 0;
With this "return" ...
> +}
> +else
... this "else" is unnecessary and hence would better be dropped,
not the least to ...
> +#endif
> +return -EINVAL;
... avoid the otherwise ambiguous indentation of this line.
Jan
Re: [PATCH v2] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.22 11:50, Jan Beulich wrote:
On 25.08.2022 11:26, Juergen Gross wrote:
The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
potentially with pages being NULL, leading to a NULL dereference.
Additionally lock_pages() doesn't check for pin_user_pages_fast()
having been completely successful, resulting in potentially not
locking all pages into memory. This could result in sporadic failures
when using the related memory in user mode.
Fix all of that by calling unlock_pages() always with the real number
of pinned pages, which will be zero in case pages being NULL, and by
checking the number of patches pinned by pin_user_pages_fast()
Nit: s/patches/pages/
matching the expected number of pages.
Cc:
Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
Reported-by: Rustam Subkhankulov
Signed-off-by: Juergen Gross
Reviewed-by: Jan Beulich
I have a question / suggestion, though:
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -602,6 +602,10 @@ static int lock_pages(
*pinned += page_count;
nr_pages -= page_count;
pages += page_count;
+
+ /* Exact reason isn't known, EFAULT is one possibility. */
+ if (page_count < requested)
+ return -EFAULT;
}
I don't really know the inner workings of pin_user_pages_fast()
nor what future plans there are with it. To be as independent of
its behavior as possible, how about bailing here only when
page_count actually is zero (i.e. no forward progress)?
This would require to rework the loop in lock_pages() to be able to
handle only a partial buffer.
This would add some complexity, but OTOH I'd get an exact error code
back in case of failure.
I'll have a try and see how the result would look like.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v2] x86/public: move XEN_ACPI_ in a new header
On 25.08.2022 11:48, Bertrand Marquis wrote:
> When Xen is compiled for x86 on an arm machine, libacpi build is failing
> due to a wrong include path:
> - arch-x86/xen.h includes xen.h
> - xen.h includes arch-arm.h (as __i386__ and __x86_64__ are not defined
> but arm ones are).
>
> To solve this issue move XEN_ACPI_ definitions in a new header
> guest-acpi.h that can be included cleanly by mk_dsdt.c.
> Inside this header, only protect the definitions using ifdef
> __XEN_TOOLS__ as the defines are not used anywhere in the hypervisor and
> are not expected to be.
>
> Previous users needing any of the XEN_ACPI_ definitions will now need to
> include arch-x86/guest-acpi.h instead of arch-x86/xen.h
>
> Fixes: d6ac8e22c7c5 ("acpi/x86: define ACPI IO registers for PVH guests")
> Signed-off-by: Bertrand Marquis
Reviewed-by: Jan Beulich
> For the release manager:
> - risk: very low, the definitions moved are only used in mk_dsdt and
> external users would just have to include the new header.
> - advantage: we can now compile xen for x86 on arm build machines
I'll give it a little for Henry to possibly release-ack this, but since
strictly speaking this is a bug fix, I think it could also go in without
(as long as not actually objected to, of course).
Jan
Re: [PATCH v2] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.2022 11:26, Juergen Gross wrote:
> The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
> potentially with pages being NULL, leading to a NULL dereference.
>
> Additionally lock_pages() doesn't check for pin_user_pages_fast()
> having been completely successful, resulting in potentially not
> locking all pages into memory. This could result in sporadic failures
> when using the related memory in user mode.
>
> Fix all of that by calling unlock_pages() always with the real number
> of pinned pages, which will be zero in case pages being NULL, and by
> checking the number of patches pinned by pin_user_pages_fast()
Nit: s/patches/pages/
> matching the expected number of pages.
>
> Cc:
> Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
> Reported-by: Rustam Subkhankulov
> Signed-off-by: Juergen Gross
Reviewed-by: Jan Beulich
I have a question / suggestion, though:
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -602,6 +602,10 @@ static int lock_pages(
> *pinned += page_count;
> nr_pages -= page_count;
> pages += page_count;
> +
> + /* Exact reason isn't known, EFAULT is one possibility. */
> + if (page_count < requested)
> + return -EFAULT;
> }
I don't really know the inner workings of pin_user_pages_fast()
nor what future plans there are with it. To be as independent of
its behavior as possible, how about bailing here only when
page_count actually is zero (i.e. no forward progress)?
Jan
[PATCH v2] x86/public: move XEN_ACPI_ in a new header
When Xen is compiled for x86 on an arm machine, libacpi build is failing
due to a wrong include path:
- arch-x86/xen.h includes xen.h
- xen.h includes arch-arm.h (as __i386__ and __x86_64__ are not defined
but arm ones are).
To solve this issue move XEN_ACPI_ definitions in a new header
guest-acpi.h that can be included cleanly by mk_dsdt.c.
Inside this header, only protect the definitions using ifdef
__XEN_TOOLS__ as the defines are not used anywhere in the hypervisor and
are not expected to be.
Previous users needing any of the XEN_ACPI_ definitions will now need to
include arch-x86/guest-acpi.h instead of arch-x86/xen.h
Fixes: d6ac8e22c7c5 ("acpi/x86: define ACPI IO registers for PVH guests")
Signed-off-by: Bertrand Marquis
---
The x86 header is including ../xen.h before the ifndef/define so that it
gets included back by xen.h. This is wrongly making the assumption that
we are using an x86 compiler which is not the case when building the
tools for x86 on an arm host.
Moving the definitions to an independent header is making things cleaner
but some might need to include a new header but the risk is low.
For the release manager:
- risk: very low, the definitions moved are only used in mk_dsdt and
external users would just have to include the new header.
- advantage: we can now compile xen for x86 on arm build machines
---
Changes in v2:
- fix commit message
- remove ifdef __XEN__ protecting the definitions
- fix name in description and ifdef guards of the file
- fix description
Changes in v1:
- was "libacpi: Fix cross building x86 on arm"
- move XEN_ACPI_ definitions in a new header guest-acpi.h
- adapt mk_dsdt.c
- remove todo in public header
---
tools/libacpi/mk_dsdt.c | 2 +-
xen/include/public/arch-x86/guest-acpi.h | 50
xen/include/public/arch-x86/xen.h| 6 ---
3 files changed, 51 insertions(+), 7 deletions(-)
create mode 100644 xen/include/public/arch-x86/guest-acpi.h
diff --git a/tools/libacpi/mk_dsdt.c b/tools/libacpi/mk_dsdt.c
index c5ba4c0b2fd3..1176da80ef44 100644
--- a/tools/libacpi/mk_dsdt.c
+++ b/tools/libacpi/mk_dsdt.c
@@ -18,7 +18,7 @@
#include
#include
#if defined(CONFIG_X86)
-#include
+#include
#include
#elif defined(CONFIG_ARM_64)
#include
diff --git a/xen/include/public/arch-x86/guest-acpi.h
b/xen/include/public/arch-x86/guest-acpi.h
new file mode 100644
index ..3d79a31fd865
--- /dev/null
+++ b/xen/include/public/arch-x86/guest-acpi.h
@@ -0,0 +1,50 @@
+/**
+ * arch-x86/guest-acpi.h
+ *
+ * Guest ACPI interface to x86 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.
+ *
+ */
+
+#ifndef __XEN_PUBLIC_ARCH_X86_GUEST_ACPI_H__
+#define __XEN_PUBLIC_ARCH_X86_GUEST_ACPI_H__
+
+#ifdef __XEN_TOOLS__
+
+/* Location of online VCPU bitmap. */
+#define XEN_ACPI_CPU_MAP 0xaf00
+#define XEN_ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS + 7) / 8)
+
+/* GPE0 bit set during CPU hotplug */
+#define XEN_ACPI_GPE0_CPUHP_BIT 2
+
+#endif /* __XEN_TOOLS__ */
+
+#endif /* __XEN_PUBLIC_ARCH_X86_GUEST_ACPI_H__ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/public/arch-x86/xen.h
b/xen/include/public/arch-x86/xen.h
index 58a1e87ee971..546dd4496ac6 100644
--- a/xen/include/public/arch-x86/xen.h
+++ b/xen/include/public/arch-x86/xen.h
@@ -325,12 +325,6 @@ struct xen_arch_domainconfig {
/* Max XEN_X86_* constant. Used for ABI checking. */
#define XEN_X86_MISC_FLAGS_MAX XEN_X86_ASSISTED_X2APIC
-/* Location of online VCPU bitmap. */
-#define XEN_ACPI_CPU_MAP 0xaf00
-#define XEN_ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS + 7) / 8)
-
-/* GPE0 bit set during CPU hotplug */
-#define XEN_ACPI_GPE0_CPUHP_BIT 2
#endif
/*
--
2.25.1
Re: [PATCH v2 7/7] xen/arm: introduce new xen,enhanced property value
Hi Julien,
> On 25 Aug 2022, at 10:37, Julien Grall wrote:
>
>
>
> On 25/08/2022 08:39, Bertrand Marquis wrote:
>> Hi,
>>> On 25 Aug 2022, at 02:10, Stefano Stabellini wrote:
>>>
>>> On Wed, 24 Aug 2022, Julien Grall wrote:
On 24/08/2022 22:59, Stefano Stabellini wrote:
> On Wed, 24 Aug 2022, Rahul Singh wrote:
>>> On 24 Aug 2022, at 4:36 pm, Julien Grall wrote:
>>> On 24/08/2022 15:42, Rahul Singh wrote:
> On 24 Aug 2022, at 1:59 pm, Julien Grall wrote:
>
>
>
> On 24/08/2022 13:15, Rahul Singh wrote:
>> Hi Julien,
>
> Hi Rahul,
>
>> Please let me know your view on this.
>> diff --git a/xen/arch/arm/domain_build.c
>> b/xen/arch/arm/domain_build.c
>> index bfe7bc6b36..a1e23eee59 100644
>> --- a/xen/arch/arm/domain_build.c
>> +++ b/xen/arch/arm/domain_build.c
>> @@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
>> domain *d,
>>if ( rc == -EILSEQ ||
>> rc == -ENODATA ||
>> (rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) )
>> - {
>> -if ( hardware_domain )
>>kinfo.dom0less_enhanced = true;
>> -else
>> - panic(“Tried to use xen,enhanced without dom0\n”);
>> - }
>
> You can't use "xen,enhanced" without dom0. In fact, you will end up
> to dereference NULL in alloc_xenstore_evtchn(). That's because
> "xen,enhanced" means the domain will be able to use Xenstored.
>
> Now if you want to support your feature without a dom0. Then I think
> we want to introduce an option which would be the same as
> "xen,enhanced" but doesn't expose Xenstored.
If we modify the patch as below we can use the "xen,enhanced" for
domUs without dom0.
I tested the patch and its works fine. Do you see any issue with this
approach?
>>>
>>> Yes. For two reasons:
>>> 1) It is muddying the meaning of "xen,enhanced". In particular a user
>>> may not realize that Xenstore is not available if dom0 is not present.
>>> 2) It would be more complicated to handle the case where Xenstored lives
>>> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
>>> but I don't want to close the door.
>>>
>>> So if you want to support create "xen,xen" without all the rest. Then I
>>> think we need a different property value. I don't have a good suggestion
>>> for the name.
>>
>> Is that okay if we use the earlier approach, when user set "xen,enhanced
>> = evtchn” we will not call alloc_xenstore_evtchn()
>> but we create hypervisor node with all fields.
>
> Thinking more about this, today xen,enhanced has the implication that:
>
> - the guest will get a regular and complete "xen,xen" node in device tree
> - xenstore and PV drivers will be available (full Xen interfaces support)
>
> We don't necessarely imply that dom0 is required (from a domU point of
> view) but we do imply that xenstore+evtchn+gnttab will be available to
> the domU.
>
> Now, static event channels are different. They don't require xenstore
> and they don't require gnttab.
>
> It is as if the current xen,enhanced node actually meant:
>
> xen,enhanced = "xenstore,gnttab,evtchn";
Correct.
>
> and now we are only enabling a subset:
>
> xen,enhanced = "evtchn";
>
> Is that a correct understanding?
Yes with some cavears (see below).
>
>
> If so, we can clarify that:
>
> xen,enhanced;
>
> it is a convenient shortend for:
>
> xen,enhanced = "xenstore,gnttab,evtchn";
>
> and that other combinations are also acceptable, e.g.:
>
> xen,enhanced = "gnttab";
> xen,enhanced = "evtchn";
> xen,enhanced = "evtchn,gnttab";
>
> It is OK to panic if the user specifies an option that is currently
> unsupported (e.g. "gnttab").
So today, if you create the node "xen,xen", the guest will expect to be
able
to use both grant-table and event channel.
Therefore, in the list above, the only configuration we can sensibly
support
without any major rework is "evtchn,gnttab".
If we want to support "evtchn" or "gnttab" only. Then we likely need to
define
a new binding (or new version) because neither "regs" nor "interrupts" are
optional (although a guest OS is free to ignore them).
>>>
>>> Yes I think you are right. I also broadly agree with the rest of your
>>> reply.
>>>
>>> Thinking about it and given the above, we only need 2 "levels" of
>>> enhancement:
>>>
>>> 1) everything: xenstore, gnttab, evtchn
>>> 2) gnttab, evtchn, but not xenstore
>>>
>>> Nothing else is reall
Re: [PATCH v2 7/7] xen/arm: introduce new xen,enhanced property value
On 25/08/2022 08:39, Bertrand Marquis wrote:
Hi,
On 25 Aug 2022, at 02:10, Stefano Stabellini wrote:
On Wed, 24 Aug 2022, Julien Grall wrote:
On 24/08/2022 22:59, Stefano Stabellini wrote:
On Wed, 24 Aug 2022, Rahul Singh wrote:
On 24 Aug 2022, at 4:36 pm, Julien Grall wrote:
On 24/08/2022 15:42, Rahul Singh wrote:
On 24 Aug 2022, at 1:59 pm, Julien Grall wrote:
On 24/08/2022 13:15, Rahul Singh wrote:
Hi Julien,
Hi Rahul,
Please let me know your view on this.
diff --git a/xen/arch/arm/domain_build.c
b/xen/arch/arm/domain_build.c
index bfe7bc6b36..a1e23eee59 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
domain *d,
if ( rc == -EILSEQ ||
rc == -ENODATA ||
(rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) )
- {
-if ( hardware_domain )
kinfo.dom0less_enhanced = true;
-else
- panic(“Tried to use xen,enhanced without dom0\n”);
- }
You can't use "xen,enhanced" without dom0. In fact, you will end up
to dereference NULL in alloc_xenstore_evtchn(). That's because
"xen,enhanced" means the domain will be able to use Xenstored.
Now if you want to support your feature without a dom0. Then I think
we want to introduce an option which would be the same as
"xen,enhanced" but doesn't expose Xenstored.
If we modify the patch as below we can use the "xen,enhanced" for
domUs without dom0.
I tested the patch and its works fine. Do you see any issue with this
approach?
Yes. For two reasons:
1) It is muddying the meaning of "xen,enhanced". In particular a user
may not realize that Xenstore is not available if dom0 is not present.
2) It would be more complicated to handle the case where Xenstored lives
in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
but I don't want to close the door.
So if you want to support create "xen,xen" without all the rest. Then I
think we need a different property value. I don't have a good suggestion
for the name.
Is that okay if we use the earlier approach, when user set "xen,enhanced
= evtchn” we will not call alloc_xenstore_evtchn()
but we create hypervisor node with all fields.
Thinking more about this, today xen,enhanced has the implication that:
- the guest will get a regular and complete "xen,xen" node in device tree
- xenstore and PV drivers will be available (full Xen interfaces support)
We don't necessarely imply that dom0 is required (from a domU point of
view) but we do imply that xenstore+evtchn+gnttab will be available to
the domU.
Now, static event channels are different. They don't require xenstore
and they don't require gnttab.
It is as if the current xen,enhanced node actually meant:
xen,enhanced = "xenstore,gnttab,evtchn";
Correct.
and now we are only enabling a subset:
xen,enhanced = "evtchn";
Is that a correct understanding?
Yes with some cavears (see below).
If so, we can clarify that:
xen,enhanced;
it is a convenient shortend for:
xen,enhanced = "xenstore,gnttab,evtchn";
and that other combinations are also acceptable, e.g.:
xen,enhanced = "gnttab";
xen,enhanced = "evtchn";
xen,enhanced = "evtchn,gnttab";
It is OK to panic if the user specifies an option that is currently
unsupported (e.g. "gnttab").
So today, if you create the node "xen,xen", the guest will expect to be able
to use both grant-table and event channel.
Therefore, in the list above, the only configuration we can sensibly support
without any major rework is "evtchn,gnttab".
If we want to support "evtchn" or "gnttab" only. Then we likely need to define
a new binding (or new version) because neither "regs" nor "interrupts" are
optional (although a guest OS is free to ignore them).
Yes I think you are right. I also broadly agree with the rest of your
reply.
Thinking about it and given the above, we only need 2 "levels" of
enhancement:
1) everything: xenstore, gnttab, evtchn
2) gnttab, evtchn, but not xenstore
Nothing else is really possible because, as Julien pointed out,
"xen,enhanced" implies the xen,xen node in the domU device tree and in
turn that node implies both evtchn and gnttab.
So we could say that xen,enhanced always includes gnttab and Xenstore is
optional.
Not really, Xenstore has always been part of the story in Xen. So I
think making it optional for "xen,enhanced" is going to make more
difficult for user to understand what the meaning of the option (in
particular that in the future we may want to support Xenstored in a
separate domain).
So I think we just need to add a way to express 2). We could do
something like:
xen,enhanced = "evtchn,gnttab";
I am a bit puzzled here as gnttab is always there.
What do you mean?
Or we could use a new separate option like Julien initially suggested,
e.g.:
xen,enhanced-no-xenstore;
"xen,enhanced-no-xenstore" is a terrible name actually, but just to
explain what I am thinking :-)
I think most common use c
Re: Porting xen on rpi4
Hi Vipul, > On 25 Aug 2022, at 09:56, Vipul Suneja wrote: > > Hi Bertrand, > > Thanks! > > No, I couldn't see /dev/loop0. Can you please guide me to create it? First thing to try is “modprobe loop" It that does not work (ie module not found) you should check in your linux config if BLK_DEV_LOOP is enabled. > > I didn't change dom0 configurations, it's default generated by yocto. > > I will append this "IMAGE_FSTYPES:append = " wic.gz”" in local.conf & will > update you. > Cheers Bertrand > Regards, > Vipul Kumar > > On Thu, Aug 25, 2022 at 1:25 PM Bertrand Marquis > wrote: > Hi Vipul, > > > On 25 Aug 2022, at 08:31, Vipul Suneja wrote: > > > > Hi Stefano, > > > > Thanks! > > > > As suggested, I changed the guest1.cfg file. Below are the contents of > > config file > > > > kernel = "/home/root/Image" > > cmdline = "console=hvc0 earlyprintk=xen sync_console root=/dev/xvda" > > memory = "1024" > > name = "guest1" > > vcpus = 1 > > serial="pty" > > disk = [ > > 'file:/home/root/xen-guest-image-minimal-raspberrypi4-64.ext3,xvda,w' ] > > vif=[ 'mac=00:11:22:66:88:22,bridge=xenbr0,type=netfront', ] > > > > Its failing with below logs: > > > > root@raspberrypi4-64:~# xl create -c guest1.cfg > > Parsing config from guest1.cfg > > Invalid parameter `type'. > > libxl: error: libxl_exec.c:117:libxl_report_child_exitstatus: > > /etc/xen/scripts/block add [742] exited with error status 1 > > libxl: error: libxl_device.c:1265:device_hotplug_child_death_cb: script: > > losetup /dev/loop0 /home/root/xen-guest-image-minimal-raspberrypi4-64.ext3 > > failed > > libxl: error: libxl_create.c:1643:domcreate_launch_dm: Domain 1:unable to > > add disk devices > > libxl: error: libxl_exec.c:117:libxl_report_child_exitstatus: > > /etc/xen/scripts/block remove [793] exited with error status 1 > > libxl: error: libxl_device.c:1265:device_hotplug_child_death_cb: script: > > /etc/xen/scripts/block failed; error detected. > > libxl: error: libxl_domain.c:1183:libxl__destroy_domid: Domain > > 1:Non-existant domain > > libxl: error: libxl_domain.c:1137:domain_destroy_callback: Domain 1:Unable > > to destroy guest > > libxl: error: libxl_domain.c:1064:domain_destroy_cb: Domain 1:Destruction > > of domain failed > > I think you have a loop issue. > > Could you check if /dev/loop0 exists ? > > Did you change something on the dom0 linux configuration generated by Yocto ? > > We are using Yocto on RPI4 here without any issue like that, only difference > with > your setup is that we generate a wic image to have a real disk image instead > of > using the ext3/ext4 one. > > Should be possible to do the same on your side by adding the following in > local.conf: > IMAGE_FSTYPES:append = " wic.gz” > > > > > Even after removing 'type=netfront' from vif it's failing. > > This option is only for hvm on x86, so you can remove it from your > configuration. > > > One more doubt here, could this mac address be a dummy or actual here? > > This is a dummy one you set for the guest network interface and this is the > Mac > address other devices on your network will see so it must be fully valid (and > not conflicting with other devices on your network). > > Cheers > Bertrand > > > > > Regards, > > Vipul Kumar > > > > On Thu, Aug 25, 2022 at 2:36 AM Stefano Stabellini > > wrote: > > On Wed, 24 Aug 2022, Vipul Suneja wrote: > > > Hi Bertrand, > > > Thanks for your response! > > > > > > I builded the guest image on yocto kirkstone source which has FSTYPE > > > ext3. Guest image generated is > > > xen-guest-image-minimal-raspberrypi4-64.ext3. > > > Below is the content of guest.cfg file > > > > > >kernel = "/home/root/Image" > > >cmdline = "console=hvc0 earlyprintk=xen sync_console root=/dev/xvda" > > >memory = "256" > > >name = "guest1" > > >vcpus = 1 > > >serial="pty" > > >disk = [ 'phy:/dev/loop0,xvda,w' ] > > >vif=[ 'mac=00:11:22:66:88:22,bridge=xenbr0,type=netfront', ] > > > > > > I am trying to mount xen-guest-image-minimal-raspberrypi4-64.ext3 to a > > > virtual device & then will run the guest VM by command "xl create -c > > > guest.cfg". But facing issue while trying to mount. > > > > You don't actually need to mount > > xen-guest-image-minimal-raspberrypi4-64.ext3 anywhere to use it to run > > your guest VM with "xl create". > > > > It is enough to do this instead, as Bertrand suggested: > > > > disk=["file:/path/to/file/xen-guest-image-minimal-raspberrypi4-64.ext3,xvda,w"] > > > > No need to call losetup or mount. Just xl create -c. > > > > More answers below. > > > > > > > Regards, > > > Vipul Kumar > > > > > > On Wed, Aug 24, 2022 at 8:06 PM Bertrand Marquis > > > wrote: > > > Hi Vipul, > > > > > > > On 24 Aug 2022, at 15:16, Vipul Suneja > > > wrote: > > > > > > > > Hi, > > > > > > > > I am porting xen hypervisor on rpi4 with yocto kirkstone sources. > > > Followed the basic steps to build xen-
[PATCH v2] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
potentially with pages being NULL, leading to a NULL dereference.
Additionally lock_pages() doesn't check for pin_user_pages_fast()
having been completely successful, resulting in potentially not
locking all pages into memory. This could result in sporadic failures
when using the related memory in user mode.
Fix all of that by calling unlock_pages() always with the real number
of pinned pages, which will be zero in case pages being NULL, and by
checking the number of patches pinned by pin_user_pages_fast()
matching the expected number of pages.
Cc:
Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
Reported-by: Rustam Subkhankulov
Signed-off-by: Juergen Gross
---
V2:
- use "pinned" as parameter for unlock_pages() (Jan Beulich)
- drop label "unlock" again (Jan Beulich)
- add check for complete success of pin_user_pages_fast()
---
drivers/xen/privcmd.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 3369734108af..7dc62510635e 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -602,6 +602,10 @@ static int lock_pages(
*pinned += page_count;
nr_pages -= page_count;
pages += page_count;
+
+ /* Exact reason isn't known, EFAULT is one possibility. */
+ if (page_count < requested)
+ return -EFAULT;
}
return 0;
@@ -677,10 +681,8 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
}
rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
- if (rc < 0) {
- nr_pages = pinned;
+ if (rc < 0)
goto out;
- }
for (i = 0; i < kdata.num; i++) {
set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
@@ -692,7 +694,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
xen_preemptible_hcall_end();
out:
- unlock_pages(pages, nr_pages);
+ unlock_pages(pages, pinned);
kfree(xbufs);
kfree(pages);
kfree(kbufs);
--
2.35.3
Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table
Hi, On 25/08/2022 08:59, Leo Yan wrote: On Fri, Aug 19, 2022 at 01:10:10PM +0100, Marc Zyngier wrote: [...] In the context of Xen, dom0 doesn't have direct access to the host ITS because we are emulating it. So I think it doesn't matter for us because we can fix our implementation if it is affected. That said, kexec-ing dom0 (or any other domain) on Xen on Arm would require some work to be supported. OOI, @leo is it something you are investigating? Now I am working on automative reference platform; the first thing for me is to resolve the kernel oops. For long term, I think the kexec/kdump would be important to be supported, to be clear, so far supporting kexec/kdump for Xen/Linux is not priority for my work. Also thanks a lot for Ard and Mark's replying. To be honest, I missed many prerequisites (e.g. redistributor configurations for GIC in hypervisor) and seems Xen uses a different way by emulating GICv3 controller for guest OS. So now I am bit puzzle what's for next step or just keep as it is? If i understand Julien's remark correctly, the dom0 GICv3 is emulated, and so it should not suffer from the issue that we are working around here. Before proceeding discussion, I would like step back to get clear for the GIC implementation in Xen, otherwise, it's really hard for me to catch up the dicussion :) For me it's clear that Xen emulates GICv3 for DomU, but I am still confused how GICv3 works for Dom0. Xen directly passes ACPI MADT table from UEFI to Linux kernel to Dom0, (see functions acpi_create_madt() and gic_make_hwdom_madt()), which means the Linux kernel Dom0 uses the same ACPI table to initialize GICv3 driver, but since Linux kernel Dom0 accesses GIC memory region as IPA, it still trap to Xen in EL2 for stage 2 translation, so finally Xen can emulate the GICv3 device for Dom0. In the default setup, dom0 is also the hardware domain. So it owns all of the devices but the ones used by Xen (e.g. interrupt controller, SMMU). Therefore, dom0 will use the same memory layout as the host. At which point, it is a lot more convenient to re-use the host ACPI tables and rewrite only what's necessary. This is quite different from DomU. Xen prepares a DT node for GICv3 rather than directly passing ACPI table, so DomU kernel initializes GICv3 driver based on the DT binding. DomUs memory layout is defined by Xen. So we need to create the Device-Tree and ACPI tables (both are supported) from scrartch. Simply to say, no matter Dom0 using ACPI table or DomU using DT binding, at the end Xen emulates GICv3 device for all of them. Correct. In both situations the GICv3 will be owned by Xen and we will emulate the bits that are not virtualized by the CPUs (e.g. re-distributors). Another thing is not clear for me is that I can see Xen allocates redistributor pending page (see gicv3_lpi_set_pendtable()), after Dom0 or DomU kernel boots up, kernel allocates another RD pending page; so the question is how these two different pending pages co-work together. Xen allocates the pending pages that will be used by the host. Dom0/DomU will be allocating pending pages that will be used by the virtual GICv3. In other words, let's assume the Dom0 kernel panic and its secondary kernel is launched by kexec, is it necessarily for the secondary kernel to reuse the primary kernel's RD pending page? No. Or in this case it's no matter for the RD pending page in Dom0 and it's safe for Xen always maintains its own RD pending page in EL2? Dom0 doesn't have direct access to the host GICv3 (this will be controlled by Xen). What we expose to dom0 is a virtual GICv3. So effectively we have two different GICv3 and each of them will require their own set of pending table. The problem is that there is no way to distinguish a system that suffers from GICR LPI tables being immutable from one that allows the LPI configuration being changed (either because the HW allows it or because the hypervisor plays other games). Let me ask a stupid question. Seems to me, GICR LPI tables can be configured as below options - The hypervisor pre-allocates GICR LPI tables and pass the memory region info to Dom0 kernel; - The hypervisor doesn't allocate GICR LPI tables, and then Dom0 kernel allocates GICR LPI tables for the virtual GICv3, and Dom0 directly write data to the GICR LPI tables and the table is used by physical GICv3; - The hypervisor pre-allocates GICR LPI tables for phycial GICv3 and Dom0 kernel allocates another GICR LPI tables for virtual GICv3, and Xen needs to sync between these two tables. To be clear, all of above three options are hypothesis. So please correct me if anything is wrong (or even total are wrong?!). I will defer this question to Marc. Cheers, -- Julien Grall
Re: Porting xen on rpi4
Hi Bertrand, Thanks! No, I couldn't see /dev/loop0. Can you please guide me to create it? I didn't change dom0 configurations, it's default generated by yocto. I will append this "IMAGE_FSTYPES:append = " wic.gz”" in local.conf & will update you. Regards, Vipul Kumar On Thu, Aug 25, 2022 at 1:25 PM Bertrand Marquis wrote: > Hi Vipul, > > > On 25 Aug 2022, at 08:31, Vipul Suneja wrote: > > > > Hi Stefano, > > > > Thanks! > > > > As suggested, I changed the guest1.cfg file. Below are the contents of > config file > > > > kernel = "/home/root/Image" > > cmdline = "console=hvc0 earlyprintk=xen sync_console root=/dev/xvda" > > memory = "1024" > > name = "guest1" > > vcpus = 1 > > serial="pty" > > disk = [ > 'file:/home/root/xen-guest-image-minimal-raspberrypi4-64.ext3,xvda,w' ] > > vif=[ 'mac=00:11:22:66:88:22,bridge=xenbr0,type=netfront', ] > > > > Its failing with below logs: > > > > root@raspberrypi4-64:~# xl create -c guest1.cfg > > Parsing config from guest1.cfg > > Invalid parameter `type'. > > libxl: error: libxl_exec.c:117:libxl_report_child_exitstatus: > /etc/xen/scripts/block add [742] exited with error status 1 > > libxl: error: libxl_device.c:1265:device_hotplug_child_death_cb: script: > losetup /dev/loop0 /home/root/xen-guest-image-minimal-raspberrypi4-64.ext3 > failed > > libxl: error: libxl_create.c:1643:domcreate_launch_dm: Domain 1:unable > to add disk devices > > libxl: error: libxl_exec.c:117:libxl_report_child_exitstatus: > /etc/xen/scripts/block remove [793] exited with error status 1 > > libxl: error: libxl_device.c:1265:device_hotplug_child_death_cb: script: > /etc/xen/scripts/block failed; error detected. > > libxl: error: libxl_domain.c:1183:libxl__destroy_domid: Domain > 1:Non-existant domain > > libxl: error: libxl_domain.c:1137:domain_destroy_callback: Domain > 1:Unable to destroy guest > > libxl: error: libxl_domain.c:1064:domain_destroy_cb: Domain > 1:Destruction of domain failed > > I think you have a loop issue. > > Could you check if /dev/loop0 exists ? > > Did you change something on the dom0 linux configuration generated by > Yocto ? > > We are using Yocto on RPI4 here without any issue like that, only > difference with > your setup is that we generate a wic image to have a real disk image > instead of > using the ext3/ext4 one. > > Should be possible to do the same on your side by adding the following in > local.conf: > IMAGE_FSTYPES:append = " wic.gz” > > > > > Even after removing 'type=netfront' from vif it's failing. > > This option is only for hvm on x86, so you can remove it from your > configuration. > > > One more doubt here, could this mac address be a dummy or actual here? > > This is a dummy one you set for the guest network interface and this is > the Mac > address other devices on your network will see so it must be fully valid > (and > not conflicting with other devices on your network). > > Cheers > Bertrand > > > > > Regards, > > Vipul Kumar > > > > On Thu, Aug 25, 2022 at 2:36 AM Stefano Stabellini < > sstabell...@kernel.org> wrote: > > On Wed, 24 Aug 2022, Vipul Suneja wrote: > > > Hi Bertrand, > > > Thanks for your response! > > > > > > I builded the guest image on yocto kirkstone source which has FSTYPE > ext3. Guest image generated is > > > xen-guest-image-minimal-raspberrypi4-64.ext3. > > > Below is the content of guest.cfg file > > > > > >kernel = "/home/root/Image" > > >cmdline = "console=hvc0 earlyprintk=xen sync_console > root=/dev/xvda" > > >memory = "256" > > >name = "guest1" > > >vcpus = 1 > > >serial="pty" > > >disk = [ 'phy:/dev/loop0,xvda,w' ] > > >vif=[ 'mac=00:11:22:66:88:22,bridge=xenbr0,type=netfront', ] > > > > > > I am trying to mount xen-guest-image-minimal-raspberrypi4-64.ext3 to a > virtual device & then will run the guest VM by command "xl create -c > > > guest.cfg". But facing issue while trying to mount. > > > > You don't actually need to mount > > xen-guest-image-minimal-raspberrypi4-64.ext3 anywhere to use it to run > > your guest VM with "xl create". > > > > It is enough to do this instead, as Bertrand suggested: > > > > > disk=["file:/path/to/file/xen-guest-image-minimal-raspberrypi4-64.ext3,xvda,w"] > > > > No need to call losetup or mount. Just xl create -c. > > > > More answers below. > > > > > > > Regards, > > > Vipul Kumar > > > > > > On Wed, Aug 24, 2022 at 8:06 PM Bertrand Marquis < > bertrand.marq...@arm.com> wrote: > > > Hi Vipul, > > > > > > > On 24 Aug 2022, at 15:16, Vipul Suneja > wrote: > > > > > > > > Hi, > > > > > > > > I am porting xen hypervisor on rpi4 with yocto kirkstone > sources. Followed the basic steps to build xen-image-minimal & > > > xen-guest-image-minimal. I could flash sd card with xen minimal > image & could see dom0 up. I copied "Image", > > > "xen-guest-image-minimal" .ext3 file & guest.cfg to > "/home/root". After that created a bridge with below step: > > > > > > > > killall -SIGUSR2 udhcpc >
Re: Ryzen 6000 (Mobile)
On 24.08.2022 20:15, Dylanger Daly wrote:
> I'm sorry I didn't get where in /sys/firmware you'd like to take a look at.
It's been a long time since I last needed to access that, when it
was still /proc/mem and/or /proc/kmem. Their modern equivalents might
be /sys/devices/virtual/mem/{,k}mem ... But if that's not usable to
get at the needed data, perhaps we should go with logging it by way
of a patch to Xen. Please let me know if I need to hand you a patch
to do so.
> Sometimes when I power the laptop off I can see it's crashing somewhere in
> ACPI/weird address issue
In ACPI or in EFI? In the latter case suppressing the use of the EFI
runtime service for shutdown/reboot may help ("efi=no-rs" to disable
all runtime services use might be a good first try).
> Is there anyone else struggling with AMD Ryzen 6000 on Xen?
Don't know.
Jan
Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations
On 25.08.2022 10:02, Xenia Ragiadakou wrote: > On 8/22/22 14:48, Jan Beulich wrote: >> On 22.08.2022 12:43, Xenia Ragiadakou wrote: >>> On 8/22/22 12:59, Jan Beulich wrote: On 19.08.2022 21:43, Xenia Ragiadakou wrote: > In macros dt_for_each_property_node(), dt_for_each_device_node() and > dt_for_each_child_node(), add parentheses around the macro parameters that > have the arrow operator applied, to prevent against unintended expansions. Why is this relevant only when -> is used? For comparisons and the rhs of assignments it's as relevant, ad even for the lhs of assignments I doubt it can be generally omitted. >>> >>> Yes, I agree with you but some older patches that I sent that were >>> adding parentheses around the lhs of the assignments were not accepted >>> and I thought that the rhs of the assignments as well these comparisons >>> fall to the same category. >>> >>> Personally, I would expect to see parentheses, also, around the macro >>> parameters that are used as the lhs or the rhs of assignments, the >>> operands of comparison or the arguments of a function. >>> Not only because they can prevent against unintentional bugs but because >>> the parentheses help me to identify more easily the macro parameters >>> when reading a macro definition. I totally understand that for other >>> people parentheses may reduce readability. >> >> Afair Julien's comments were very specific to the lhs of assignments. >> So at the very least everything else ought to be parenthesized imo. >> > > So, IIUC, the only deviations from the MISRA C 2012 Rule 20.7 will be > for macro parameters used as the lhs of assignments and function arguments? Afaic I don't consider that discussion settled. > This feels a bit of a hack to parenthesize the macro parameters that are > used as the rhs of an assignment but not those used as the lhs. lhs and rhs of assignments are quite different, and hence making such a distinction wouldn't look to be a "hack" to me. In fact I've always considered this part of the language somewhat strange: To me parenthesizing e.g. an identifier already makes it (visually) an rvalue. Leaving aside odd (and easy to spot as odd) uses at the call sizes, I don't think I can come up with a case where parentheses are really needed. Anything needing parenthesizing actually yields an rvalue afaics, thus causing a diagnostic anyway. > From previous discussions on the topic, I understood that the > parentheses are considered needed only when they eliminate operator > precedence problems, while for the wrong parameter format bugs we can > rely on the reviewers. > > I think we need to decide if the rule will be applied as is and if not > which will be the deviations along with their justification and add it > to the document. Yes. But this shouldn't hinder adjustments for all other, non- controversial cases. Jan
Re: [PATCH 4/4] x86/hvmloader: Move various helpers to being static inlines
On 24.08.2022 12:59, Andrew Cooper wrote:
> The IO port, MSR, IO-APIC and LAPIC accessors compile typically to single or
> pairs of instructions, which is less overhead than even the stack manipulation
> to call the helpers.
>
> Move the implementations from util.c to being static inlines in util.h
>
> In addition, turn ioapic_base_address into a constant as it is never modified
> from 0xfec0 (substantially shrinks the IO-APIC logic), and make use of the
> "A" constraint for WRMSR/RDMSR like we already do for RDSTC.
Nit: RDTSC
> Bloat-o-meter reports a net:
> add/remove: 0/13 grow/shrink: 1/19 up/down: 6/-743 (-737)
>
> No functional change.
>
> Signed-off-by: Andrew Cooper
Reviewed-by: Jan Beulich
albeit with several further nits/suggestions:
> --- a/tools/firmware/hvmloader/util.h
> +++ b/tools/firmware/hvmloader/util.h
> @@ -7,6 +7,7 @@
> #include
> #include
> #include
> +#include "config.h"
> #include "e820.h"
>
> /* Request un-prefixed values from errno.h. */
> @@ -61,28 +62,91 @@ static inline int test_and_clear_bit(int nr, volatile
> void *addr)
> }
>
> /* MSR access */
> -void wrmsr(uint32_t idx, uint64_t v);
> -uint64_t rdmsr(uint32_t idx);
> +static inline void wrmsr(uint32_t idx, uint64_t v)
> +{
> +asm volatile ( "wrmsr" :: "c" (idx), "A" (v) : "memory" );
The addition of the "memory" clobber imo wants mentioning in the
description, so it's clear that it's intentional (and why).
> +}
> +
> +static inline uint64_t rdmsr(uint32_t idx)
> +{
> +uint64_t res;
> +
> +asm volatile ( "rdmsr" : "=A" (res) : "c" (idx) );
> +
> +return res;
> +}
>
> /* I/O output */
> -void outb(uint16_t addr, uint8_t val);
> -void outw(uint16_t addr, uint16_t val);
> -void outl(uint16_t addr, uint32_t val);
> +static inline void outb(uint16_t addr, uint8_t val)
> +{
> +asm volatile ( "outb %%al, %%dx" :: "d" (addr), "a" (val) );
I'm inclined to ask to use "outb %1, %0" here (and similarly below).
I also wonder whether at least all the OUTs shouldn't also gain a
"memory" clobber.
> +}
> +
> +static inline void outw(uint16_t addr, uint16_t val)
> +{
> +asm volatile ( "outw %%ax, %%dx" :: "d" (addr), "a" (val) );
> +}
> +
> +static inline void outl(uint16_t addr, uint32_t val)
> +{
> +asm volatile ( "outl %%eax, %%dx" :: "d" (addr), "a" (val) );
> +}
>
> /* I/O input */
> -uint8_t inb(uint16_t addr);
> -uint16_t inw(uint16_t addr);
> -uint32_t inl(uint16_t addr);
> +static inline uint8_t inb(uint16_t addr)
> +{
> +uint8_t val;
> +
> +asm volatile ( "inb %%dx,%%al" : "=a" (val) : "d" (addr) );
Would you mind adding blanks after the comma here and below?
> +
> +return val;
> +}
> +
> +static inline uint16_t inw(uint16_t addr)
> +{
> +uint16_t val;
> +
> +asm volatile ( "inw %%dx,%%ax" : "=a" (val) : "d" (addr) );
> +
> +return val;
> +}
> +
> +static inline uint32_t inl(uint16_t addr)
> +{
> +uint32_t val;
> +
> +asm volatile ( "inl %%dx,%%eax" : "=a" (val) : "d" (addr) );
> +
> +return val;
> +}
>
> /* CMOS access */
> uint8_t cmos_inb(uint8_t idx);
> void cmos_outb(uint8_t idx, uint8_t val);
>
> /* APIC access */
> -uint32_t ioapic_read(uint32_t reg);
> -void ioapic_write(uint32_t reg, uint32_t val);
> -uint32_t lapic_read(uint32_t reg);
> -void lapic_write(uint32_t reg, uint32_t val);
> +static inline uint32_t ioapic_read(uint32_t reg)
> +{
> +*(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x00) = reg;
> +return *(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x10);
> +}
> +
> +static inline void ioapic_write(uint32_t reg, uint32_t val)
> +{
> +*(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x00) = reg;
> +*(volatile uint32_t *)(IOAPIC_BASE_ADDRESS + 0x10) = val;
> +}
> +
> +#define LAPIC_BASE_ADDRESS 0xfee0
Seeing this #define here, does there anything stand in the way of
putting IOAPIC_BASE_ADDRESS next to the inline functions as well?
Jan
> +static inline uint32_t lapic_read(uint32_t reg)
> +{
> +return *(volatile uint32_t *)(LAPIC_BASE_ADDRESS + reg);
> +}
> +
> +static inline void lapic_write(uint32_t reg, uint32_t val)
> +{
> +*(volatile uint32_t *)(LAPIC_BASE_ADDRESS + reg) = val;
> +}
>
> /* PCI access */
> uint32_t pci_read(uint32_t devfn, uint32_t reg, uint32_t len);
Re: [PATCH] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 25.08.22 09:38, Jan Beulich wrote:
On 24.08.2022 16:26, Juergen Gross wrote:
The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
potentially with pages being NULL, leading to a NULL dereference.
Fix that by calling unlock_pages only if lock_pages() was at least
partially successful.
Cc:
Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
Reported-by: Rustam Subkhankulov
Signed-off-by: Juergen Gross
Reviewed-by: Jan Beulich
albeit I wonder whether you did consider the variant actually
reducing code size (and avoiding the need for yet another label),
...
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -679,7 +679,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
if (rc < 0) {
nr_pages = pinned;
... dropping this line and ...
- goto out;
+ goto unlock;
}
for (i = 0; i < kdata.num; i++) {
@@ -691,8 +691,9 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
xen_preemptible_hcall_end();
-out:
+ unlock:
unlock_pages(pages, nr_pages);
... passing "pinned" here.
Looking into this I found another problem: NOT using pinned is wrong, as
lock_pages() doesn't guarantee that all pages were really locked. I think
lock_pages() should return an error, in case pin_user_pages_fast() didn't
lock as many pages es expected.
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH 3/4] x86/hvmloader: Don't override stddef.h
On 24.08.2022 12:59, Andrew Cooper wrote:
> Since c/s 73b13705af7c ("firmware: provide a stand alone set of headers"),
> we've had an implementation of offsetof() which isn't undefined behaviour.
> Actually use it.
>
> No functional change.
>
> Signed-off-by: Andrew Cooper
Acked-by: Jan Beulich
Re: [PATCH 7/7] xen/device_tree: Fix MISRA C 2012 Rule 20.7 violations
On 8/22/22 14:48, Jan Beulich wrote: On 22.08.2022 12:43, Xenia Ragiadakou wrote: On 8/22/22 12:59, Jan Beulich wrote: On 19.08.2022 21:43, Xenia Ragiadakou wrote: In macros dt_for_each_property_node(), dt_for_each_device_node() and dt_for_each_child_node(), add parentheses around the macro parameters that have the arrow operator applied, to prevent against unintended expansions. Why is this relevant only when -> is used? For comparisons and the rhs of assignments it's as relevant, ad even for the lhs of assignments I doubt it can be generally omitted. Yes, I agree with you but some older patches that I sent that were adding parentheses around the lhs of the assignments were not accepted and I thought that the rhs of the assignments as well these comparisons fall to the same category. Personally, I would expect to see parentheses, also, around the macro parameters that are used as the lhs or the rhs of assignments, the operands of comparison or the arguments of a function. Not only because they can prevent against unintentional bugs but because the parentheses help me to identify more easily the macro parameters when reading a macro definition. I totally understand that for other people parentheses may reduce readability. Afair Julien's comments were very specific to the lhs of assignments. So at the very least everything else ought to be parenthesized imo. So, IIUC, the only deviations from the MISRA C 2012 Rule 20.7 will be for macro parameters used as the lhs of assignments and function arguments? This feels a bit of a hack to parenthesize the macro parameters that are used as the rhs of an assignment but not those used as the lhs. From previous discussions on the topic, I understood that the parentheses are considered needed only when they eliminate operator precedence problems, while for the wrong parameter format bugs we can rely on the reviewers. I think we need to decide if the rule will be applied as is and if not which will be the deviations along with their justification and add it to the document. -- Xenia
Re: [PATCH] Make XEN_FW_EFI_MEM_INFO easier to use
On 24.08.2022 23:04, Demi Marie Obenour wrote: > The XEN_FW_EFI_MEM_INFO platform op has very surprising behavior: it > only sets info->mem.size if the initial value was *larger* than the size > of the memory region. And intentionally so - the caller didn't ask for any bigger region, after all. > This is not particularly useful and cost me most > of a day of debugging. It also has some integer overflow problems, > though as the data comes from dom0 or the firmware (both of which are > trusted) these are not security issues. I'm afraid we're trusting the firmware in this regard elsewhere as well. So if there was a need to change that, I guess it would need changing everywhere, not just here. But we trust the E820 map as well, when on non-EFI platforms, so I don't see why we would need to change that. In any event such would want to be a separate change imo. > Fix both of these problems by unconditionally setting the memory region > size If you were to report a larger ending address, why would you not also report a smaller starting address? But before you go that route - I don't think we can change the API now that it has been in use this way for many years. If a "give me the full enclosing range" variant is wanted, it will need to be fully separate. Jan > and by computing it in a way that is immune to integer overflow. > The new code is slightly longer, but it is much easier to understand and > use.
Re: [PATCH] xen/arm: acpi: Support memory reserve configuration table
On Fri, Aug 19, 2022 at 01:10:10PM +0100, Marc Zyngier wrote: [...] > > > > In the context of Xen, dom0 doesn't have direct access to the host ITS > > > > because we are emulating it. So I think it doesn't matter for us > > > > because we > > > > can fix our implementation if it is affected. > > > > > > > > That said, kexec-ing dom0 (or any other domain) on Xen on Arm would > > > > require > > > > some work to be supported. OOI, @leo is it something you are > > > > investigating? > > > > > > > > > Now I am working on automative reference platform; the first thing for > > > me is to resolve the kernel oops. > > > > > > For long term, I think the kexec/kdump would be important to be > > > supported, to be clear, so far supporting kexec/kdump for Xen/Linux is > > > not priority for my work. > > > > > > Also thanks a lot for Ard and Mark's replying. To be honest, I missed > > > many prerequisites (e.g. redistributor configurations for GIC in > > > hypervisor) and seems Xen uses a different way by emulating GICv3 > > > controller for guest OS. So now I am bit puzzle what's for next step > > > or just keep as it is? > > > > > > > If i understand Julien's remark correctly, the dom0 GICv3 is emulated, > > and so it should not suffer from the issue that we are working around > > here. Before proceeding discussion, I would like step back to get clear for the GIC implementation in Xen, otherwise, it's really hard for me to catch up the dicussion :) For me it's clear that Xen emulates GICv3 for DomU, but I am still confused how GICv3 works for Dom0. Xen directly passes ACPI MADT table from UEFI to Linux kernel to Dom0, (see functions acpi_create_madt() and gic_make_hwdom_madt()), which means the Linux kernel Dom0 uses the same ACPI table to initialize GICv3 driver, but since Linux kernel Dom0 accesses GIC memory region as IPA, it still trap to Xen in EL2 for stage 2 translation, so finally Xen can emulate the GICv3 device for Dom0. This is quite different from DomU. Xen prepares a DT node for GICv3 rather than directly passing ACPI table, so DomU kernel initializes GICv3 driver based on the DT binding. Simply to say, no matter Dom0 using ACPI table or DomU using DT binding, at the end Xen emulates GICv3 device for all of them. Another thing is not clear for me is that I can see Xen allocates redistributor pending page (see gicv3_lpi_set_pendtable()), after Dom0 or DomU kernel boots up, kernel allocates another RD pending page; so the question is how these two different pending pages co-work together. In other words, let's assume the Dom0 kernel panic and its secondary kernel is launched by kexec, is it necessarily for the secondary kernel to reuse the primary kernel's RD pending page? Or in this case it's no matter for the RD pending page in Dom0 and it's safe for Xen always maintains its own RD pending page in EL2? > The problem is that there is no way to distinguish a system that > suffers from GICR LPI tables being immutable from one that allows the > LPI configuration being changed (either because the HW allows it or > because the hypervisor plays other games). Let me ask a stupid question. Seems to me, GICR LPI tables can be configured as below options - The hypervisor pre-allocates GICR LPI tables and pass the memory region info to Dom0 kernel; - The hypervisor doesn't allocate GICR LPI tables, and then Dom0 kernel allocates GICR LPI tables for the virtual GICv3, and Dom0 directly write data to the GICR LPI tables and the table is used by physical GICv3; - The hypervisor pre-allocates GICR LPI tables for phycial GICv3 and Dom0 kernel allocates another GICR LPI tables for virtual GICv3, and Xen needs to sync between these two tables. To be clear, all of above three options are hypothesis. So please correct me if anything is wrong (or even total are wrong?!). Thanks a lot for suggestions! Leo P.s. sorry for truncating Marc's following comments, just want to focus on above questions.
[ovmf test] 172767: regressions - FAIL
flight 172767 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/172767/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-libvirt6 libvirt-buildfail REGR. vs. 172136 build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172136 version targeted for testing: ovmf 4d83ee04f44a8dc9e6425a719b39c9d378730ca1 baseline version: ovmf 444260d45ec2a84e8f8c192b3539a3cd5591d009 Last test of basis 172136 2022-08-04 06:43:42 Z 21 days Failing since172151 2022-08-05 02:40:28 Z 20 days 165 attempts Testing same since 172746 2022-08-24 05:42:04 Z1 days8 attempts People who touched revisions under test: Abdul Lateef Attar Abner Chang Chasel Chiu Czajkowski, Maciej Dimitrije Pavlov Dun Tan Edward Pickup Foster Nong Gregx Yeh Igor Kulchytskyy James Lu Jose Marinho KasimX Liu Kavya Konstantin Aladyshev Liu, Zhiguang Maciej Czajkowski Michael D Kinney Ray Ni Sainadh Nagolu Sami Mujawar Shengfengx Xue Zhiguang Liu jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt fail build-i386-libvirt fail 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 Not pushing. (No revision log; it would be 926 lines long.)
Re: [PATCH v1] x86/public: move XEN_ACPI_ in a new header
Hi Jan,
> On 25 Aug 2022, at 08:47, Jan Beulich wrote:
>
> On 24.08.2022 17:29, Bertrand Marquis wrote:
>> When Xen is compiled for x86 on an arm machine, libacpi build is failing
>> due to a wrong include path:
>> - arch-x86/xen.h includes xen.h
>> - xen.h includes arch-arm.h (as __i386__ and __x86_64__ are not defined
>> but arm ones are).
>>
>> To solve this issue move XEN_ACPI_ definitions in a new header
>> guest-acpi.h that can be included cleanly by mk_dsdt.c
>>
>> Previous users needing any of the XEN_ACPI_ definitions will now need to
>> include arch-x86/guest-acpi.h instead of arch-x86/xen.h
>>
>> Fixes: d6ac8e22c7c5 ("acpi/x86: define ACPI IO registers for PVH
>> guests")
>
> Nit: Please don't wrap this line.
Ok
>
>> Signed-off-by: Bertrand Marquis
>> ---
>> The x86 header is including ../xen.h before the ifndef/define so that it
>> gets included back by xen.h. This is wrongly making the assumption that
>> we are using an x86 compiler which is not the case when building the
>> tools for x86 on an arm host.
>> Moving the definitions to an independent header is making things cleaner
>> but some might need to include a new header but the risk is low.
>>
>> For the release manager:
>> - risk: very low, the definitions moved are only used in mk_dsdt and
>> external users would just have to include the new header.
>> - advantage: we can now compile xen for x86 on arm build machines
>
> You will want to actually Cc him on v2, so he can ack the change (or
> not).
Ack
>
>> --- /dev/null
>> +++ b/xen/include/public/arch-x86/guest-acpi.h
>> @@ -0,0 +1,50 @@
>> +/**
>> + * arch-x86/xen-acpi.h
>
> Stale file name.
Right, forgot to change the content after renaming, will fix.
>
>> + * XEN ACPI interface to x86 Xen.
>
> Perhaps also here s/XEN/Guest/.
Ok.
>
>> + * 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.
>> + *
>> + */
>> +
>> +#ifndef __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__
>> +#define __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__
>
> Please make the guard match the file name.
Yes
>
>> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
>
> While separating it out, may I suggest to limit this to just the tool
> stack? There's no use of these #define-s in the hypervisor, and none
> is to be expected. (Of course this will want justifying this way in
> the description.)
Ok
Thanks for the review
Cheers
Bertrand
>
> Jan
>
>> +/* Location of online VCPU bitmap. */
>> +#define XEN_ACPI_CPU_MAP 0xaf00
>> +#define XEN_ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS + 7) / 8)
>> +
>> +/* GPE0 bit set during CPU hotplug */
>> +#define XEN_ACPI_GPE0_CPUHP_BIT 2
>> +
>> +#endif
>> +
>> +#endif /* __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__ */
Re: Porting xen on rpi4
Hi Vipul, > On 25 Aug 2022, at 08:31, Vipul Suneja wrote: > > Hi Stefano, > > Thanks! > > As suggested, I changed the guest1.cfg file. Below are the contents of config > file > > kernel = "/home/root/Image" > cmdline = "console=hvc0 earlyprintk=xen sync_console root=/dev/xvda" > memory = "1024" > name = "guest1" > vcpus = 1 > serial="pty" > disk = [ > 'file:/home/root/xen-guest-image-minimal-raspberrypi4-64.ext3,xvda,w' ] > vif=[ 'mac=00:11:22:66:88:22,bridge=xenbr0,type=netfront', ] > > Its failing with below logs: > > root@raspberrypi4-64:~# xl create -c guest1.cfg > Parsing config from guest1.cfg > Invalid parameter `type'. > libxl: error: libxl_exec.c:117:libxl_report_child_exitstatus: > /etc/xen/scripts/block add [742] exited with error status 1 > libxl: error: libxl_device.c:1265:device_hotplug_child_death_cb: script: > losetup /dev/loop0 /home/root/xen-guest-image-minimal-raspberrypi4-64.ext3 > failed > libxl: error: libxl_create.c:1643:domcreate_launch_dm: Domain 1:unable to add > disk devices > libxl: error: libxl_exec.c:117:libxl_report_child_exitstatus: > /etc/xen/scripts/block remove [793] exited with error status 1 > libxl: error: libxl_device.c:1265:device_hotplug_child_death_cb: script: > /etc/xen/scripts/block failed; error detected. > libxl: error: libxl_domain.c:1183:libxl__destroy_domid: Domain 1:Non-existant > domain > libxl: error: libxl_domain.c:1137:domain_destroy_callback: Domain 1:Unable to > destroy guest > libxl: error: libxl_domain.c:1064:domain_destroy_cb: Domain 1:Destruction of > domain failed I think you have a loop issue. Could you check if /dev/loop0 exists ? Did you change something on the dom0 linux configuration generated by Yocto ? We are using Yocto on RPI4 here without any issue like that, only difference with your setup is that we generate a wic image to have a real disk image instead of using the ext3/ext4 one. Should be possible to do the same on your side by adding the following in local.conf: IMAGE_FSTYPES:append = " wic.gz” > > Even after removing 'type=netfront' from vif it's failing. This option is only for hvm on x86, so you can remove it from your configuration. > One more doubt here, could this mac address be a dummy or actual here? This is a dummy one you set for the guest network interface and this is the Mac address other devices on your network will see so it must be fully valid (and not conflicting with other devices on your network). Cheers Bertrand > > Regards, > Vipul Kumar > > On Thu, Aug 25, 2022 at 2:36 AM Stefano Stabellini > wrote: > On Wed, 24 Aug 2022, Vipul Suneja wrote: > > Hi Bertrand, > > Thanks for your response! > > > > I builded the guest image on yocto kirkstone source which has FSTYPE ext3. > > Guest image generated is > > xen-guest-image-minimal-raspberrypi4-64.ext3. > > Below is the content of guest.cfg file > > > >kernel = "/home/root/Image" > >cmdline = "console=hvc0 earlyprintk=xen sync_console root=/dev/xvda" > >memory = "256" > >name = "guest1" > >vcpus = 1 > >serial="pty" > >disk = [ 'phy:/dev/loop0,xvda,w' ] > >vif=[ 'mac=00:11:22:66:88:22,bridge=xenbr0,type=netfront', ] > > > > I am trying to mount xen-guest-image-minimal-raspberrypi4-64.ext3 to a > > virtual device & then will run the guest VM by command "xl create -c > > guest.cfg". But facing issue while trying to mount. > > You don't actually need to mount > xen-guest-image-minimal-raspberrypi4-64.ext3 anywhere to use it to run > your guest VM with "xl create". > > It is enough to do this instead, as Bertrand suggested: > > disk=["file:/path/to/file/xen-guest-image-minimal-raspberrypi4-64.ext3,xvda,w"] > > No need to call losetup or mount. Just xl create -c. > > More answers below. > > > > Regards, > > Vipul Kumar > > > > On Wed, Aug 24, 2022 at 8:06 PM Bertrand Marquis > > wrote: > > Hi Vipul, > > > > > On 24 Aug 2022, at 15:16, Vipul Suneja wrote: > > > > > > Hi, > > > > > > I am porting xen hypervisor on rpi4 with yocto kirkstone sources. > > Followed the basic steps to build xen-image-minimal & > > xen-guest-image-minimal. I could flash sd card with xen minimal image > > & could see dom0 up. I copied "Image", > > "xen-guest-image-minimal" .ext3 file & guest.cfg to "/home/root". > > After that created a bridge with below step: > > > > > > killall -SIGUSR2 udhcpc > > > brctl addbr xenbr0 > > > brctl addif xenbr0 eth0 > > > killall udhcpc > > > udhcpc -R -b -p /var/run/udhcpc.xenbr0.pid -i xenbr0 > > > > > > Could see the xenbr0 interface up. > > > After that while mounting the guest file system it shows no such > > file or directory but the file is already there. > > > > > > [23:40:15] root@raspberrypi4-64:~# ls -l > > > [23:40:15] -rw-r--r--1 root root 24652288 > > Mar 9 12:36 Image > > > [23:40:15] -rw-r--r--1 root
Re: [PATCH v1] x86/public: move XEN_ACPI_ in a new header
On 24.08.2022 17:29, Bertrand Marquis wrote:
> When Xen is compiled for x86 on an arm machine, libacpi build is failing
> due to a wrong include path:
> - arch-x86/xen.h includes xen.h
> - xen.h includes arch-arm.h (as __i386__ and __x86_64__ are not defined
> but arm ones are).
>
> To solve this issue move XEN_ACPI_ definitions in a new header
> guest-acpi.h that can be included cleanly by mk_dsdt.c
>
> Previous users needing any of the XEN_ACPI_ definitions will now need to
> include arch-x86/guest-acpi.h instead of arch-x86/xen.h
>
> Fixes: d6ac8e22c7c5 ("acpi/x86: define ACPI IO registers for PVH
> guests")
Nit: Please don't wrap this line.
> Signed-off-by: Bertrand Marquis
> ---
> The x86 header is including ../xen.h before the ifndef/define so that it
> gets included back by xen.h. This is wrongly making the assumption that
> we are using an x86 compiler which is not the case when building the
> tools for x86 on an arm host.
> Moving the definitions to an independent header is making things cleaner
> but some might need to include a new header but the risk is low.
>
> For the release manager:
> - risk: very low, the definitions moved are only used in mk_dsdt and
> external users would just have to include the new header.
> - advantage: we can now compile xen for x86 on arm build machines
You will want to actually Cc him on v2, so he can ack the change (or
not).
> --- /dev/null
> +++ b/xen/include/public/arch-x86/guest-acpi.h
> @@ -0,0 +1,50 @@
> +/**
> + * arch-x86/xen-acpi.h
Stale file name.
> + * XEN ACPI interface to x86 Xen.
Perhaps also here s/XEN/Guest/.
> + * 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.
> + *
> + */
> +
> +#ifndef __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__
> +#define __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__
Please make the guard match the file name.
> +#if defined(__XEN__) || defined(__XEN_TOOLS__)
While separating it out, may I suggest to limit this to just the tool
stack? There's no use of these #define-s in the hypervisor, and none
is to be expected. (Of course this will want justifying this way in
the description.)
Jan
> +/* Location of online VCPU bitmap. */
> +#define XEN_ACPI_CPU_MAP 0xaf00
> +#define XEN_ACPI_CPU_MAP_LEN ((HVM_MAX_VCPUS + 7) / 8)
> +
> +/* GPE0 bit set during CPU hotplug */
> +#define XEN_ACPI_GPE0_CPUHP_BIT 2
> +
> +#endif
> +
> +#endif /* __XEN_PUBLIC_ARCH_X86_XEN_ACPI_H__ */
Re: [PATCH v2 7/7] xen/arm: introduce new xen,enhanced property value
Hi,
> On 25 Aug 2022, at 02:10, Stefano Stabellini wrote:
>
> On Wed, 24 Aug 2022, Julien Grall wrote:
>> On 24/08/2022 22:59, Stefano Stabellini wrote:
>>> On Wed, 24 Aug 2022, Rahul Singh wrote:
> On 24 Aug 2022, at 4:36 pm, Julien Grall wrote:
> On 24/08/2022 15:42, Rahul Singh wrote:
>>> On 24 Aug 2022, at 1:59 pm, Julien Grall wrote:
>>>
>>>
>>>
>>> On 24/08/2022 13:15, Rahul Singh wrote:
Hi Julien,
>>>
>>> Hi Rahul,
>>>
Please let me know your view on this.
diff --git a/xen/arch/arm/domain_build.c
b/xen/arch/arm/domain_build.c
index bfe7bc6b36..a1e23eee59 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -3562,12 +3562,7 @@ static int __init construct_domU(struct
domain *d,
if ( rc == -EILSEQ ||
rc == -ENODATA ||
(rc == 0 && !strcmp(dom0less_enhanced, “enabled”)) )
- {
-if ( hardware_domain )
kinfo.dom0less_enhanced = true;
-else
- panic(“Tried to use xen,enhanced without dom0\n”);
- }
>>>
>>> You can't use "xen,enhanced" without dom0. In fact, you will end up
>>> to dereference NULL in alloc_xenstore_evtchn(). That's because
>>> "xen,enhanced" means the domain will be able to use Xenstored.
>>>
>>> Now if you want to support your feature without a dom0. Then I think
>>> we want to introduce an option which would be the same as
>>> "xen,enhanced" but doesn't expose Xenstored.
>> If we modify the patch as below we can use the "xen,enhanced" for
>> domUs without dom0.
>> I tested the patch and its works fine. Do you see any issue with this
>> approach?
>
> Yes. For two reasons:
> 1) It is muddying the meaning of "xen,enhanced". In particular a user
> may not realize that Xenstore is not available if dom0 is not present.
> 2) It would be more complicated to handle the case where Xenstored lives
> in a non-dom0 domain. I am not aware of anyone wanting this on Arm yet,
> but I don't want to close the door.
>
> So if you want to support create "xen,xen" without all the rest. Then I
> think we need a different property value. I don't have a good suggestion
> for the name.
Is that okay if we use the earlier approach, when user set "xen,enhanced
= evtchn” we will not call alloc_xenstore_evtchn()
but we create hypervisor node with all fields.
>>>
>>> Thinking more about this, today xen,enhanced has the implication that:
>>>
>>> - the guest will get a regular and complete "xen,xen" node in device tree
>>> - xenstore and PV drivers will be available (full Xen interfaces support)
>>>
>>> We don't necessarely imply that dom0 is required (from a domU point of
>>> view) but we do imply that xenstore+evtchn+gnttab will be available to
>>> the domU.
>>>
>>> Now, static event channels are different. They don't require xenstore
>>> and they don't require gnttab.
>>>
>>> It is as if the current xen,enhanced node actually meant:
>>>
>>> xen,enhanced = "xenstore,gnttab,evtchn";
>>
>> Correct.
>>
>>>
>>> and now we are only enabling a subset:
>>>
>>> xen,enhanced = "evtchn";
>>>
>>> Is that a correct understanding?
>>
>> Yes with some cavears (see below).
>>
>>>
>>>
>>> If so, we can clarify that:
>>>
>>> xen,enhanced;
>>>
>>> it is a convenient shortend for:
>>>
>>> xen,enhanced = "xenstore,gnttab,evtchn";
>>>
>>> and that other combinations are also acceptable, e.g.:
>>>
>>> xen,enhanced = "gnttab";
>>> xen,enhanced = "evtchn";
>>> xen,enhanced = "evtchn,gnttab";
>>>
>>> It is OK to panic if the user specifies an option that is currently
>>> unsupported (e.g. "gnttab").
>>
>> So today, if you create the node "xen,xen", the guest will expect to be able
>> to use both grant-table and event channel.
>>
>> Therefore, in the list above, the only configuration we can sensibly support
>> without any major rework is "evtchn,gnttab".
>>
>> If we want to support "evtchn" or "gnttab" only. Then we likely need to
>> define
>> a new binding (or new version) because neither "regs" nor "interrupts" are
>> optional (although a guest OS is free to ignore them).
>
> Yes I think you are right. I also broadly agree with the rest of your
> reply.
>
> Thinking about it and given the above, we only need 2 "levels" of
> enhancement:
>
> 1) everything: xenstore, gnttab, evtchn
> 2) gnttab, evtchn, but not xenstore
>
> Nothing else is really possible because, as Julien pointed out,
> "xen,enhanced" implies the xen,xen node in the domU device tree and in
> turn that node implies both evtchn and gnttab.
So we could say that xen,enhanced always includes gnttab and Xenstore is
optional.
>
> xenstore is separate and is detected using HVM_PARAM_STORE_EVTCHN and
> HVM_PARAM_STORE_PFN anyway.
Ack, not having Xenstore shoul
Re: [PATCH] xen/privcmd: fix error exit of privcmd_ioctl_dm_op()
On 24.08.2022 16:26, Juergen Gross wrote:
> The error exit of privcmd_ioctl_dm_op() is calling unlock_pages()
> potentially with pages being NULL, leading to a NULL dereference.
>
> Fix that by calling unlock_pages only if lock_pages() was at least
> partially successful.
>
> Cc:
> Fixes: ab520be8cd5d ("xen/privcmd: Add IOCTL_PRIVCMD_DM_OP")
> Reported-by: Rustam Subkhankulov
> Signed-off-by: Juergen Gross
Reviewed-by: Jan Beulich
albeit I wonder whether you did consider the variant actually
reducing code size (and avoiding the need for yet another label),
...
> --- a/drivers/xen/privcmd.c
> +++ b/drivers/xen/privcmd.c
> @@ -679,7 +679,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void
> __user *udata)
> rc = lock_pages(kbufs, kdata.num, pages, nr_pages, &pinned);
> if (rc < 0) {
> nr_pages = pinned;
... dropping this line and ...
> - goto out;
> + goto unlock;
> }
>
> for (i = 0; i < kdata.num; i++) {
> @@ -691,8 +691,9 @@ static long privcmd_ioctl_dm_op(struct file *file, void
> __user *udata)
> rc = HYPERVISOR_dm_op(kdata.dom, kdata.num, xbufs);
> xen_preemptible_hcall_end();
>
> -out:
> + unlock:
> unlock_pages(pages, nr_pages);
... passing "pinned" here.
Jan
> + out:
> kfree(xbufs);
> kfree(pages);
> kfree(kbufs);
Re: Porting xen on rpi4
Hi Stefano, Thanks! As suggested, I changed the guest1.cfg file. Below are the contents of config file *kernel = "/home/root/Image"cmdline = "console=hvc0 earlyprintk=xen sync_console root=/dev/xvda"memory = "1024"name = "guest1"vcpus = 1serial="pty"disk = [ 'file:/home/root/xen-guest-image-minimal-raspberrypi4-64.ext3,xvda,w' ]vif=[ 'mac=00:11:22:66:88:22,bridge=xenbr0,type=netfront', ]* Its failing with below logs: *root@raspberrypi4-64:~# xl create -c guest1.cfg Parsing config from guest1.cfgInvalid parameter `type'.libxl: error: libxl_exec.c:117:libxl_report_child_exitstatus: /etc/xen/scripts/block add [742] exited with error status 1libxl: error: libxl_device.c:1265:device_hotplug_child_death_cb: script: losetup /dev/loop0 /home/root/xen-guest-image-minimal-raspberrypi4-64.ext3 failedlibxl: error: libxl_create.c:1643:domcreate_launch_dm: Domain 1:unable to add disk deviceslibxl: error: libxl_exec.c:117:libxl_report_child_exitstatus: /etc/xen/scripts/block remove [793] exited with error status 1libxl: error: libxl_device.c:1265:device_hotplug_child_death_cb: script: /etc/xen/scripts/block failed; error detected.libxl: error: libxl_domain.c:1183:libxl__destroy_domid: Domain 1:Non-existant domainlibxl: error: libxl_domain.c:1137:domain_destroy_callback: Domain 1:Unable to destroy guestlibxl: error: libxl_domain.c:1064:domain_destroy_cb: Domain 1:Destruction of domain failed* Even after removing 'type=netfront' from vif it's failing. One more doubt here, could this mac address be a dummy or actual here? Regards, Vipul Kumar On Thu, Aug 25, 2022 at 2:36 AM Stefano Stabellini wrote: > On Wed, 24 Aug 2022, Vipul Suneja wrote: > > Hi Bertrand, > > Thanks for your response! > > > > I builded the guest image on yocto kirkstone source which has FSTYPE > ext3. Guest image generated is > > xen-guest-image-minimal-raspberrypi4-64.ext3. > > Below is the content of guest.cfg file > > > >kernel = "/home/root/Image" > >cmdline = "console=hvc0 earlyprintk=xen sync_console root=/dev/xvda" > >memory = "256" > >name = "guest1" > >vcpus = 1 > >serial="pty" > >disk = [ 'phy:/dev/loop0,xvda,w' ] > >vif=[ 'mac=00:11:22:66:88:22,bridge=xenbr0,type=netfront', ] > > > > I am trying to mount xen-guest-image-minimal-raspberrypi4-64.ext3 to a > virtual device & then will run the guest VM by command "xl create -c > > guest.cfg". But facing issue while trying to mount. > > You don't actually need to mount > xen-guest-image-minimal-raspberrypi4-64.ext3 anywhere to use it to run > your guest VM with "xl create". > > It is enough to do this instead, as Bertrand suggested: > > > disk=["file:/path/to/file/xen-guest-image-minimal-raspberrypi4-64.ext3,xvda,w"] > > No need to call losetup or mount. Just xl create -c. > > More answers below. > > > > Regards, > > Vipul Kumar > > > > On Wed, Aug 24, 2022 at 8:06 PM Bertrand Marquis < > bertrand.marq...@arm.com> wrote: > > Hi Vipul, > > > > > On 24 Aug 2022, at 15:16, Vipul Suneja > wrote: > > > > > > Hi, > > > > > > I am porting xen hypervisor on rpi4 with yocto kirkstone > sources. Followed the basic steps to build xen-image-minimal & > > xen-guest-image-minimal. I could flash sd card with xen minimal > image & could see dom0 up. I copied "Image", > > "xen-guest-image-minimal" .ext3 file & guest.cfg to "/home/root". > After that created a bridge with below step: > > > > > > killall -SIGUSR2 udhcpc > > > brctl addbr xenbr0 > > > brctl addif xenbr0 eth0 > > > killall udhcpc > > > udhcpc -R -b -p /var/run/udhcpc.xenbr0.pid -i xenbr0 > > > > > > Could see the xenbr0 interface up. > > > After that while mounting the guest file system it shows no such > file or directory but the file is already there. > > > > > > [23:40:15] root@raspberrypi4-64:~# ls -l > > > [23:40:15] -rw-r--r--1 root root > 24652288 Mar 9 12:36 Image > > > [23:40:15] -rw-r--r--1 root root > 247 Mar 9 12:37 guest1.cfg > > > [23:40:15] -rw-r--r--1 root root > 868220928 Mar 9 12:39 xen-guest-image-minimal-raspberrypi4-64.ext3 > > > [23:40:15] root@raspberrypi4-64:~# chmod 0777 > xen-guest-image-minimal-raspberrypi4-64.ext3 > > > [23:40:15] root@raspberrypi4-64:~# ls -l > > > [23:40:15] -rw-r--r--1 root root > 24652288 Mar 9 12:36 Image > > > [23:40:15] -rw-r--r--1 root root > 247 Mar 9 12:37 guest1.cfg > > > [23:40:15] -rwxrwxrwx1 root root > 868220928 Mar 9 12:39 xen-guest-image-minimal-raspberrypi4-64.ext3 > > > [23:40:15] root@raspberrypi4-64:~# losetup > /dev/loop0 xen-guest-image-minimal-raspberrypi4-64.ext3 > > > [23:40:15] losetup: > xen-guest-image-minimal-raspberrypi4-64.ext3: No such file or directory > > > [23:40:15] root@raspberrypi4-64:~# losetup > /dev/loop0 /home/root/xen-guest-image-minimal-raspberrypi4-64.ext3 >
Re: [PATCH 2/4] x86/hvmloader: Don't build as PIC/PIE
On 24.08.2022 12:59, Andrew Cooper wrote: > HVMLoader is not relocatable in memory, and 32bit PIC code has a large > overhead. Build it as non-relocatable. > > Bloat-o-meter reports a net: > add/remove: 0/0 grow/shrink: 3/107 up/down: 14/-3370 (-3356) > > No functional change. > > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Roger Pau Monné > CC: Wei Liu > --- > tools/firmware/hvmloader/Makefile | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/tools/firmware/hvmloader/Makefile > b/tools/firmware/hvmloader/Makefile > index 4f31c881613c..eb757819274b 100644 > --- a/tools/firmware/hvmloader/Makefile > +++ b/tools/firmware/hvmloader/Makefile > @@ -23,7 +23,8 @@ include $(XEN_ROOT)/tools/firmware/Rules.mk > # SMBIOS spec requires format mm/dd/ > SMBIOS_REL_DATE ?= $(shell date +%m/%d/%Y) > > -CFLAGS += $(CFLAGS_xeninclude) > +CFLAGS += $(CFLAGS_xeninclude) -fno-pic > +$(call cc-option-add,CFLAGS,-no-pie) This is supposed to be coming from EMBEDDED_EXTRA_CFLAGS, if only it was spelled correctly there. See the patch just sent. This line (see that other patch) is meaningless anyway, as we don't use $(CFLAGS) for linking here. So with it dropped Reviewed-by: Jan Beulich I do think though that the description could do with some expanding, as I don't think -fpic or -fPIC is the default normally. I suppose it's only specific distros which make this the default. Jan
[PATCH] Config.mk: correct PIE-related option(s) in EMBEDDED_EXTRA_CFLAGS
I haven't been able to find evidence of "-nopie" ever having been a supported compiler option. The correct spelling is "-no-pie". Furthermore like "-pie" this is an option which is solely passed to the linker. The compiler only recognizes "-fpie" / "-fPIE" / "-fno-pie", and it doesn't infer these options from "-pie" / "-no-pie". Add the compiler recognized form, but for the possible case of the variable also being used somewhere for linking keep the linker option as well (with corrected spelling). Signed-off-by: Jan Beulich --- unstable.orig/Config.mk 2022-04-07 12:23:27.0 +0200 +++ unstable/Config.mk 2022-08-25 08:58:00.044287451 +0200 @@ -188,7 +188,7 @@ endif APPEND_LDFLAGS += $(foreach i, $(APPEND_LIB), -L$(i)) APPEND_CFLAGS += $(foreach i, $(APPEND_INCLUDES), -I$(i)) -EMBEDDED_EXTRA_CFLAGS := -nopie -fno-stack-protector -fno-stack-protector-all +EMBEDDED_EXTRA_CFLAGS := -fno-pie -no-pie -fno-stack-protector -fno-stack-protector-all EMBEDDED_EXTRA_CFLAGS += -fno-exceptions -fno-asynchronous-unwind-tables XEN_EXTFILES_URL ?= http://xenbits.xen.org/xen-extfiles
[qemu-mainline test] 172758: regressions - FAIL
flight 172758 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/172758/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 172123 build-i386-libvirt6 libvirt-buildfail REGR. vs. 172123 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-installfail REGR. vs. 172123 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 172123 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 172123 Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-raw 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-qcow2 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-raw 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 172123 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 172123 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 172123 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 172123 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 172123 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 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-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 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-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 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 15 migrate-support-checkfail never pass test-armhf-armhf-xl 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-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-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass version targeted for testing: qemuu1f6a638cee087b1be4f464e44a9311e19a79c50e baseline version: qemuu2480f3bbd03814b0651a1f74959f5c6631ee5819 Last test of basis 172123 2022-08-03 18:10:07 Z 21 days Failing since172148 2022-08-04 21:39:38 Z 20 days 47 attempts Testing same si
