[qemu-mainline test] 168856: tolerable FAIL - PUSHED
flight 168856 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/168856/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168835 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168835 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168835 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168835 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168835 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168835 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168835 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168835 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-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-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-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-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 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-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 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-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-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: qemuuf345abe36527a8b575482bb5a0616f43952bf1f4 baseline version: qemuu9c721291506c037d934900a6167dc3bf4a8f51a6 Last test of basis 168835 2022-03-25 04:11:20 Z0 days Testing same since 168856 2022-03-25 15:08:16 Z0 days1 attempts People who touched revisions under test: Christian Ehrhardt Daniel P. Berrangé luofei
[ovmf test] 168865: regressions - FAIL
flight 168865 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/168865/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-amd64 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 69218d5d2854acaa7a11c777244de4a297d2fbb9 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 25 days Failing since168258 2022-03-01 01:55:31 Z 25 days 256 attempts Testing same since 168832 2022-03-25 01:43:21 Z1 days8 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Bandaru, Purna Chandra Rao Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Patrick Rudolph Purna Chandra Rao Bandaru Sami Mujawar Sean Rhodes Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 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 Not pushing. (No revision log; it would be 904 lines long.)
Re: [PATCH v4 4/6] xen/cpupool: Create different cpupools at boot time
On Thu, 24 Mar 2022, Luca Fancellu wrote:
> Introduce a way to create different cpupools at boot time, this is
> particularly useful on ARM big.LITTLE system where there might be the
> need to have different cpupools for each type of core, but also
> systems using NUMA can have different cpu pools for each node.
>
> The feature on arm relies on a specification of the cpupools from the
> device tree to build pools and assign cpus to them.
>
> Documentation is created to explain the feature.
>
> Signed-off-by: Luca Fancellu
> ---
> Changes in v4:
> - modify Makefile to put in *.init.o, fixed stubs and macro (Jan)
> - fixed docs, fix brakets (Stefano)
> - keep cpu0 in Pool-0 (Julien)
> - moved printk from btcpupools_allocate_pools to
> btcpupools_get_cpupool_id
> - Add to docs constraint about cpu0 and Pool-0
> Changes in v3:
> - Add newline to cpupools.txt and removed "default n" from Kconfig (Jan)
> - Fixed comment, moved defines, used global cpu_online_map, use
> HAS_DEVICE_TREE instead of ARM and place arch specific code in header
> (Juergen)
> - Fix brakets, x86 code only panic, get rid of scheduler dt node, don't
> save pool pointer and look for it from the pool list (Stefano)
> - Changed data structures to allow modification to the code.
> Changes in v2:
> - Move feature to common code (Juergen)
> - Try to decouple dtb parse and cpupool creation to allow
> more way to specify cpupools (for example command line)
> - Created standalone dt node for the scheduler so it can
> be used in future work to set scheduler specific
> parameters
> - Use only auto generated ids for cpupools
> ---
> docs/misc/arm/device-tree/cpupools.txt | 136 ++
> xen/arch/arm/include/asm/smp.h | 3 +
> xen/common/Kconfig | 7 +
> xen/common/Makefile| 1 +
> xen/common/boot_cpupools.c | 190 +
> xen/common/sched/cpupool.c | 9 +-
> xen/include/xen/sched.h| 14 ++
> 7 files changed, 359 insertions(+), 1 deletion(-)
> create mode 100644 docs/misc/arm/device-tree/cpupools.txt
> create mode 100644 xen/common/boot_cpupools.c
>
> diff --git a/docs/misc/arm/device-tree/cpupools.txt
> b/docs/misc/arm/device-tree/cpupools.txt
> new file mode 100644
> index ..5dac2b1384e0
> --- /dev/null
> +++ b/docs/misc/arm/device-tree/cpupools.txt
> @@ -0,0 +1,136 @@
> +Boot time cpupools
> +==
> +
> +When BOOT_TIME_CPUPOOLS is enabled in the Xen configuration, it is possible
> to
> +create cpupools during boot phase by specifying them in the device tree.
> +
> +Cpupools specification nodes shall be direct childs of /chosen node.
> +Each cpupool node contains the following properties:
> +
> +- compatible (mandatory)
> +
> +Must always include the compatiblity string: "xen,cpupool".
> +
> +- cpupool-cpus (mandatory)
> +
> +Must be a list of device tree phandle to nodes describing cpus (e.g.
> having
> +device_type = "cpu"), it can't be empty.
> +
> +- cpupool-sched (optional)
> +
> +Must be a string having the name of a Xen scheduler. Check the
> sched=<...>
> +boot argument for allowed values.
> +
> +
> +Constraints
> +===
> +
> +If no cpupools are specified, all cpus will be assigned to one cpupool
> +implicitly created (Pool-0).
> +
> +If cpupools node are specified, but not every cpu brought up by Xen is
> assigned,
> +all the not assigned cpu will be assigned to an additional cpupool.
> +
> +If a cpu is assigned to a cpupool, but it's not brought up correctly, Xen
> will
> +stop.
> +
> +The boot cpu must be assigned to Pool-0, so the cpupool containing that core
> +will become Pool-0 automatically.
> +
> +
> +Examples
> +
> +
> +A system having two types of core, the following device tree specification
> will
> +instruct Xen to have two cpupools:
> +
> +- The cpupool with id 0 will have 4 cpus assigned.
> +- The cpupool with id 1 will have 2 cpus assigned.
> +
> +The following example can work only if hmp-unsafe=1 is passed to Xen boot
> +arguments, otherwise not all cores will be brought up by Xen and the cpupool
> +creation process will stop Xen.
> +
> +
> +a72_1: cpu@0 {
> +compatible = "arm,cortex-a72";
> +reg = <0x0 0x0>;
> +device_type = "cpu";
> +[...]
> +};
> +
> +a72_2: cpu@1 {
> +compatible = "arm,cortex-a72";
> +reg = <0x0 0x1>;
> +device_type = "cpu";
> +[...]
> +};
> +
> +a53_1: cpu@100 {
> +compatible = "arm,cortex-a53";
> +reg = <0x0 0x100>;
> +device_type = "cpu";
> +[...]
> +};
> +
> +a53_2: cpu@101 {
> +compatible = "arm,cortex-a53";
> +reg = <0x0 0x101>;
> +device_type = "cpu";
> +[...]
> +};
> +
> +a53_3: cpu@102 {
> +compatible = "arm,cortex-a53";
> +reg = <0x0 0x102>;
> +device_type = "cpu";
> +[...]
> +};
> +
> +a53_4: cpu@103 {
> +
Re: Security support status of xnf(4) and xbf(4)
On 25/03/2022 22:42, Chris Cappuccio wrote: > Demi Marie Obenour [d...@invisiblethingslab.com] wrote: >> Linux???s netfront and blkfront drivers recently had a security >> vulnerability (XSA-396) that allowed a malicious backend to potentially >> compromise them. In follow-up audits, I found that OpenBSD???s xnf(4) >> currently trusts the backend domain. I reported this privately to Theo >> de Raadt, who indicated that OpenBSD does not consider this to be a >> security concern. >> > A malicious backend could completely compromise the virtual host in an > infinite number of ways. Xen PV front/back pairs have had far better security properties/guarantees for longer than virtio has existed. Under the Xen architecture, the backend has never had the ability to "DMA" to areas which aren't explicitly permitted by the frontend. If a frontend handles it's grants correctly, then it need only trust Xen but not the backend for any problems beyond "backend refuses to transmit data". The backend can of course cease transmitting data. That's mitigated with market pressures of "OK I'll take my credit card elsewhere". Data integrity issues can be mitigated by using encryption techniques. With the advent of encrypted VM technologies (AMD SEV-SNP, Intel TXT) the VM need not trust Xen any further than "will continue to schedule you" which equally is mitigated with market pressures related to money. ~Andrew
Re: Security support status of xnf(4) and xbf(4)
Demi Marie Obenour [d...@invisiblethingslab.com] wrote: > Linux???s netfront and blkfront drivers recently had a security > vulnerability (XSA-396) that allowed a malicious backend to potentially > compromise them. In follow-up audits, I found that OpenBSD???s xnf(4) > currently trusts the backend domain. I reported this privately to Theo > de Raadt, who indicated that OpenBSD does not consider this to be a > security concern. > A malicious backend could completely compromise the virtual host in an infinite number of ways. Perhaps a small patch to find incorrect values would be of value, but even then, a patch would only be a very slight improvment. If you patch the manual page, should OpenBSD start putting notifications in all manual pages that a compromised virtual machine backend may compromise the integrity of the virtual host? Chris
[xen-unstable test] 168855: tolerable FAIL - PUSHED
flight 168855 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/168855/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168833 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168833 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168833 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168833 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 168833 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 168833 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168833 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168833 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168833 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168833 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168833 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168833 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 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-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-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 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 0e03ff97def12b121b5313094a76e5db7bb5c93c baseline version: xen
Re: Security support status of xnf(4) and xbf(4)
On 3/25/22 18:42, Chris Cappuccio wrote: > Demi Marie Obenour [d...@invisiblethingslab.com] wrote: >> Linux???s netfront and blkfront drivers recently had a security >> vulnerability (XSA-396) that allowed a malicious backend to potentially >> compromise them. In follow-up audits, I found that OpenBSD???s xnf(4) >> currently trusts the backend domain. I reported this privately to Theo >> de Raadt, who indicated that OpenBSD does not consider this to be a >> security concern. >> > > A malicious backend could completely compromise the virtual host in an > infinite number of ways. This is only true if the backend runs in dom0 (the privileged administrative VM) or is otherwise trusted (perhaps because it stores the root filesystem). It is not true in general, and is explicitly false in Qubes OS. In Qubes OS, the only backend that can directly compromise the VM is the disk backend running in dom0 that provides the default volumes. The network backend and other disk backends are explicitly considered to be untrusted. > Perhaps a small patch to find incorrect values > would be of value, but even then, a patch would only be a very slight > improvment. If you patch the manual page, should OpenBSD start putting > notifications in all manual pages that a compromised virtual machine > backend may compromise the integrity of the virtual host? No, because emulated devices are provided by an I/O emulator that is considered trusted. xnf(4) and xbf(4) devices can be provided by a backend that is not dom0 and which has (barring other vulnerabilities) no other way to attack the guest. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab OpenPGP_0xB288B55FFF9C22C1.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v3 2/5] xen: make evtchn_alloc_unbound public
On Fri, 25 Mar 2022, Julien Grall wrote:
> So to me, the idea of switching to a "fake" domain or bypassing the check is
> more appealing. I have a preference for the "fake" domain here.
As a maintainer, I am not opposed to the "fake"/"contructor" domain
concept. It all depends on how many instances of this issue we are
going to have. This is the only one on xen-devel so far. I don't think
it is worth adding a constructor domain for one instance only. But I
agree with you and Daniel that if we end up with several instances, then
the constructor domain approach is probably the best option overall.
As a contributor, sadly I won't be able to spend a lot of time on this
in the following months. If a significant rework is required, I don't
think I'll be able to do it, at least not for this Xen release (and it
would be nice to have dom0less PV drivers in the coming release.) If
Daniel is willing, I could add his "idle_domain is_priv" patch to this
series. Not as clean as a proper constructor domain but it would work
and it would be simple. It could be evolved into a nicer constructor
domain later.
This is not my preference but I could do that if Julien and Jan prefer
this approach and if Daniel is happy to share his patch.
> AFAIU, your proposal is to duplicate code. This brings other risks such as
> duplicated bug and more code to maintain.
Got it. I'll make one last attempt at a proposal that doesn't involve
the fake constructor domain. The goal is to avoid code duplication while
providing a safe way forward to make progress with only a small amount
of changes. What if we:
- rename evtchn_alloc_unbound to _evtchn_alloc_unbound (still static)
- add a skip_xsm parameter to _evtchn_alloc_unbound
- introduce a wrapper evtchn_alloc_unbound that always set skip_xsm to
false (same interface as today's evtchn_alloc_unbound)
- introduce an __init early_evtchn_alloc_unbound public function that
sets skip_xsm to true
This way:
- we have a single implementation in _evtchn_alloc_unbound (no code
duplication)
- the only function exposed that skips the XSM check is __init
- evtchn_alloc_unbound continue to have the XSM check same as today
E.g.:
static int _evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool skip_xsm)
{
...
}
static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
{
return _evtchn_alloc_unbound(alloc, false);
}
int __init early_evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
{
return _evtchn_alloc_unbound(alloc, true);
}
Would this be good enough for now?
[xen-unstable-smoke test] 168860: tolerable all pass - PUSHED
flight 168860 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/168860/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen fc90d75c2b71ae15b75128e7d0d4dbe718164ecb baseline version: xen 0e03ff97def12b121b5313094a76e5db7bb5c93c Last test of basis 168841 2022-03-25 10:00:33 Z0 days Testing same since 168860 2022-03-25 17:00:30 Z0 days1 attempts People who touched revisions under test: Andrew Cooper jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 0e03ff97de..fc90d75c2b fc90d75c2b71ae15b75128e7d0d4dbe718164ecb -> smoke
Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules
On Fri, 25 Mar 2022, Michal Orzel wrote:
> On 25.03.2022 02:32, Stefano Stabellini wrote:
> > On Thu, 24 Mar 2022, Bertrand Marquis wrote:
> >> cppcheck can be used to check Xen code quality.
> >>
> >> To create a report do "make cppcheck" on a built tree adding any options
> >> you added during the process you used to build xen (like CROSS_COMPILE
> >> or XEN_TARGET_ARCH). This will generate an xml report xen-cppcheck.xml.
> >>
> >> To create a html report do "make cppcheck-html" in the same way and a
> >> full report to be seen in a browser will be generated in
> >> cppcheck-htmlreport/index.html.
> >>
> >> For better results it is recommended to build your own cppcheck from the
> >> latest sources that you can find at [1].
> >> Development and result analysis has been done with cppcheck 2.7.
> >>
> >> The Makefile rule is searching for all C files which have been compiled
> >> (ie which have a generated .o file) and is running cppcheck on all of
> >> them using the current configuration of xen so only the code actually
> >> compiled is checked.
> >>
> >> A new tool is introduced to merge all cppcheck reports into one global
> >> report including all findings and removing duplicates.
> >>
> >> Some extra variables can be used to customize the report:
> >> - CPPCHECK can be used to give the full path to the cppcheck binary to
> >> use (default is to use the one from the standard path).
> >> - CPPCHECK_HTMLREPORT can be used to give the full path to
> >> cppcheck-htmlreport (default is to use the one from the standard path).
> >>
> >> This has been tested on several arm configurations (x86 should work but
> >> has not been tested).
> >>
> >> [1] https://cppcheck.sourceforge.io/
> >>
> >> Signed-off-by: Bertrand Marquis
> >> Signed-off-by: Michal Orzel
> >
> > Very cool, I was looking forward to this :-)
> >
> >
> >> diff --git a/xen/tools/merge_cppcheck_reports.py
> >> b/xen/tools/merge_cppcheck_reports.py
> >> new file mode 100755
> >> index 00..ef055f6925
> >> --- /dev/null
> >> +++ b/xen/tools/merge_cppcheck_reports.py
> >> @@ -0,0 +1,73 @@
> >> +#!/usr/bin/env python
> >> +
> >> +"""
> >> +This script acts as a tool to merge XML files created by cppcheck.
> >> +Usage:
> >> +merge_cppcheck_reports.py [FILES] [OUTPUT]
> >> +
> >> +FILES - list of XML files with extension .cppcheck
> >> +OUTPUT - file to store results (with .xml extension).
> >> + If not specified, the script will print results to stdout.
> >> +"""
> >> +
> >> +import sys
> >> +from xml.etree import ElementTree
> >> +
> >> +def elements_equal(el1, el2):
> >> +if type(el1) != type(el2): return False
> >> +
> >> +el1_location = str(el1.find('location').attrib)
> >> +el2_location = str(el2.find('location').attrib)
> >> +
> >> +if el1_location != el2_location: return False
> >> +
> >> +return True
> >> +
> >> +def remove_duplicates(xml_root_element):
> >> +elems_to_remove = []
> >> +index = 0
> >> +elems_list = list(xml_root_element.findall("errors")[0])
> >> +for elem1 in elems_list:
> >> +index += 1
> >> +for elem2 in elems_list[index:]:
> >> +if elements_equal(elem1, elem2) and elem2 not in
> >> elems_to_remove:
> >> +elems_to_remove.append(elem2)
> >> +continue
> >> +
> >> +for elem in elems_to_remove:
> >> +xml_root_element.findall("errors")[0].remove(elem)
> >> +
> >> +def merge(files):
> >> +result_xml_root = None
> >> +for xml_file in files:
> >> +xml_root = ElementTree.parse(xml_file).getroot()
> >
> >
> > Traceback (most recent call last):
> > File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py",
> > line 73, in
> > run()
> > File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py",
> > line 60, in run
> > result = merge(files)
> > File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py",
> > line 43, in merge
> > xml_root = ElementTree.parse(xml_file).getroot()
> > File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1182, in parse
> > tree.parse(source, parser)
> > File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 657, in parse
> > self._root = parser.close()
> > File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1671, in close
> > self._raiseerror(v)
> > File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1523, in
> > _raiseerror
> > raise err
> > xml.etree.ElementTree.ParseError: no element found: line 11, column 0
> > make: *** [Makefile:576: xen-cppcheck.xml] Error 1
> >
> > I think we should catch the xml.etree.ElementTree.ParseError exception and
> > continue?
>
> Well, this is of course something that we might do but this error clearly
> warns us that
> some XML files is not well formatted and therefore is not parsable. This
> could mean that
> you are using some old cppcheck version. Is it correct assumption?
I confirm it was an issue with the
[linux-linus test] 168838: tolerable FAIL - PUSHED
flight 168838 linux-linus real [real] flight 168861 linux-linus real-retest [real] http://logs.test-lab.xenproject.org/osstest/logs/168838/ http://logs.test-lab.xenproject.org/osstest/logs/168861/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-arm64-arm64-libvirt-raw 13 guest-start fail pass in 168861-retest Tests which did not succeed, but are not blocking: test-arm64-arm64-libvirt-raw 14 migrate-support-check fail in 168861 never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-check fail in 168861 never pass test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168830 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168830 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168830 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168830 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168830 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168830 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168830 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168830 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 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 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-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-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-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-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 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: linux34af78c4e616c359ed428d79fe4758a35d2c5473 baseline version: linux7403e6d8263937dea206dd201fed1ceed190ca18 Last test of basis 168830 2022-03-24 20:10:22 Z0 days Testing same since 168838 2022-03-25 06:20:14 Z0 days1 attempts 1025 people touched revisions under test, not listing them all jobs: build-amd64-xsm pass build-arm64-xsm
Re: [PATCH v2] xen: fix is_xen_pmu()
On 3/25/22 10:20 AM, Juergen Gross wrote:
is_xen_pmu() is taking the cpu number as parameter, but it is not using
it. Instead it just tests whether the Xen PMU initialization on the
current cpu did succeed. As this test is done by checking a percpu
pointer, preemption needs to be disabled in order to avoid switching
the cpu while doing the test. While resuming from suspend() this seems
not to be the case:
[ 88.082751] ACPI: PM: Low-level resume complete
[ 88.087933] ACPI: EC: EC started
[ 88.091464] ACPI: PM: Restoring platform NVS memory
[ 88.097166] xen_acpi_processor: Uploading Xen processor PM info
[ 88.103850] Enabling non-boot CPUs ...
[ 88.108128] installing Xen timer for CPU 1
[ 88.112763] BUG: using smp_processor_id() in preemptible [] code:
systemd-sleep/7138
[ 88.122256] caller is is_xen_pmu+0x12/0x30
[ 88.126937] CPU: 0 PID: 7138 Comm: systemd-sleep Tainted: GW
5.16.13-2.fc32.qubes.x86_64 #1
[ 88.137939] Hardware name: Star Labs StarBook/StarBook, BIOS 7.97 03/21/2022
[ 88.145930] Call Trace:
[ 88.148757]
[ 88.151193] dump_stack_lvl+0x48/0x5e
[ 88.155381] check_preemption_disabled+0xde/0xe0
[ 88.160641] is_xen_pmu+0x12/0x30
[ 88.164441] xen_smp_intr_init_pv+0x75/0x100
Fix that by replacing is_xen_pmu() by a simple boolean variable which
reflects the Xen PMU initialization state on cpu 0.
Modify xen_pmu_init() to return early in case it is being called for a
cpu other than cpu 0 and the boolean variable not being set.
Fixes: bf6dfb154d93 ("xen/PMU: PMU emulation code")
Reported-by: Marek Marczykowski-Górecki
Signed-off-by: Juergen Gross
Reviewed-by: Boris Ostrovsky
Re: [PATCH v3 3/5] xen/arm: configure dom0less domain for enabling xenstore after boot
Hi Stefano,
On 23/03/2022 01:18, Stefano Stabellini wrote:
On Sat, 29 Jan 2022, Julien Grall wrote:
On 28/01/2022 21:33, Stefano Stabellini wrote:
+rc = evtchn_alloc_unbound(, true);
+if ( rc )
+{
+printk("Failed allocating event channel for domain\n");
+return rc;
+}
+
+d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN] = alloc.port;
+
+return 0;
+}
+
static int __init construct_domU(struct domain *d,
const struct dt_device_node *node)
{
@@ -3014,7 +3043,19 @@ static int __init construct_domU(struct domain *d,
return rc;
if ( kinfo.vpl011 )
+{
rc = domain_vpl011_init(d, NULL);
+if ( rc < 0 )
+return rc;
+}
+
+if ( kinfo.dom0less_enhanced )
+{
+rc = alloc_xenstore_evtchn(d);
+if ( rc < 0 )
+return rc;
+d->arch.hvm.params[HVM_PARAM_STORE_PFN] = ~0ULL;
I think it would be easy to allocate the page right now. So what prevent us to
do it right now?
Because (as you noted as a comment to the following patch) as soon as
d->arch.hvm.params[HVM_PARAM_STORE_PFN] is set the guest can continue
with the initialization and will expect the right data to be set on the
page.
I think you misunderstood my question. From my understanding, at the
moment, Linux would break with your proposal. So you need to modify the
kernel in order to support what you are doing.
IOW, we have room to decide the approach here.
Xenstore protocol has a way to allow (re)connection (see
docs/mics/xenstore-ring.txt). This feature looks quite suited to what
you are trying to do here (we want to delay the connection).
The main advantage with this approach is the resources allocation for
Xenstore will be done in the place and the work in Linux could be
re-used for non-dom0less domain.
Have you explored it?
In other words: it is not enough to have the pfn allocated, we
also need xenstore to initialize it. At that point, it is better to do
both later from init-dom0less.c.
See above. My main concern with your proposal is the allocation is split
and this making more difficult to understand the initialization. Could
you write some documentation how everything is meant to work?
Cheers,
--
Julien Grall
Re: [PATCH v3 1/5] xen: introduce xen,enhanced dom0less property
Hi Stefano,
On 23/03/2022 00:08, Stefano Stabellini wrote:
On Sat, 29 Jan 2022, Julien Grall wrote:
diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index 6931c022a2..9144d6c0b6 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -2963,6 +2963,7 @@ static int __init construct_domU(struct domain *d,
const struct dt_device_node *node)
{
struct kernel_info kinfo = {};
+const char *dom0less_enhanced;
int rc;
u64 mem;
@@ -2978,6 +2979,12 @@ static int __init construct_domU(struct domain *d,
kinfo.vpl011 = dt_property_read_bool(node, "vpl011");
+rc = dt_property_read_string(node, "xen,enhanced",
_enhanced);
+if ( rc == -EILSEQ ||
I think the use an -EILSEQ wants an explanation. In a previous version, you
wrote that the value would be returned when:
fdt set /chosen/domU0 xen,enhanced
But it is not clear why. Can you print pp->value, pp->length, strnlen(..) when
this happens?
I added in dt_property_read_string:
printk("DEBUG %s %d value=%s value[0]=%d length=%u
len=%lu\n",__func__,__LINE__,(char*)pp->value, *((char*)pp->value),pp->length,
strlen(pp->value));
This is the output:
(XEN) DEBUG dt_property_read_string 205 value= value[0]=0 length=0 len=0
Thanks posting the log!
For convenience, I am copying the comment on top of
dt_property_read_string() prototype:
* Search for a property in a device tree node and retrieve a null
* terminated string value (pointer to data, not a copy). Returns 0 on
* success, -EINVAL if the property does not exist, -ENODATA if property
* doest not have value, and -EILSEQ if the string is not
* null-terminated with the length of the property data.
Per your log, the length is NULL so I would have assumed -ENODATA would
be returned. Looking at the implementation:
const struct dt_property *pp = dt_find_property(np, propname, NULL);
if ( !pp )
return -EINVAL;
if ( !pp->value )
return -ENODATA;
if ( strnlen(pp->value, pp->length) >= pp->length )
return -EILSEQ;
We consider that the property when pp->value is NULL. However, AFAICT,
we never set pp->value to NULL (see unflatten_dt_node()).
So I think there is a bug in the implementation. I would keep the check
!pp->value (for hardening purpose) and also return -ENODATA when
!pp->length.
Most of our device-tree code is from Linux. Looking at v5.17, the bug
seems to be present there too. This would want to be fixed there too.
Cheers,
--
Julien Grall
[ovmf test] 168859: regressions - FAIL
flight 168859 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/168859/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-amd64 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 69218d5d2854acaa7a11c777244de4a297d2fbb9 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 25 days Failing since168258 2022-03-01 01:55:31 Z 24 days 255 attempts Testing same since 168832 2022-03-25 01:43:21 Z0 days7 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Bandaru, Purna Chandra Rao Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Patrick Rudolph Purna Chandra Rao Bandaru Sami Mujawar Sean Rhodes Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 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 Not pushing. (No revision log; it would be 904 lines long.)
Re: [PATCH v3 2/5] xen: make evtchn_alloc_unbound public
Hi Stefano,
On 25/03/2022 00:30, Stefano Stabellini wrote:
On Wed, 23 Mar 2022, Jan Beulich wrote:
On 23.03.2022 01:22, Stefano Stabellini wrote:
On Tue, 15 Mar 2022, Daniel P. Smith wrote:
On 1/28/22 16:33, Stefano Stabellini wrote:
From: Luca Miccio
The xenstore event channel will be allocated for dom0less domains. It is
necessary to have access to the evtchn_alloc_unbound function to do
that, so make evtchn_alloc_unbound public.
Add a skip_xsm parameter to allow disabling the XSM check in
evtchn_alloc_unbound (xsm_evtchn_unbound wouldn't work for a call
originated from Xen before running any domains.)
Signed-off-by: Luca Miccio
Signed-off-by: Stefano Stabellini
CC: Julien Grall
CC: Volodymyr Babchuk
CC: Bertrand Marquis
CC: Andrew Cooper
CC: George Dunlap
CC: Jan Beulich
CC: Wei Liu
---
Changes v3:
- expose evtchn_alloc_unbound, assing a skip_xsm parameter
---
xen/common/event_channel.c | 13 -
xen/include/xen/event.h| 3 +++
2 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index da88ad141a..be57d00a15 100644
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -284,7 +284,7 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
xsm_evtchn_close_post(chn);
}
-static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
+int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool skip_xsm)
{
struct evtchn *chn;
struct domain *d;
@@ -301,9 +301,12 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t
*alloc)
ERROR_EXIT_DOM(port, d);
chn = evtchn_from_port(d, port);
-rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
-if ( rc )
-goto out;
+if ( !skip_xsm )
+{
+rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
+if ( rc )
+goto out;
+}
Please do not subvert the security framework because it causes an
inconvenience. As Jan recommended, work within the XSM check to allow
your access so that we may ensure it is done safely. If you need any
help making modifications to XSM, please do not hesitate to reach out as
I will gladly help.
Thank you!
First let me reply to Jan: this series is only introducing 1 more call
to evtchn_alloc_unbound, which is to allocate the special xenstore event
channel, the one configured via
d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN].
It is not meant to be a generic function, and it is not meant to be
called more than once. It could (should?) be __init.
How that? Its pre-existing use doesn't disappear, and requires it to be
non-__init.
Sorry I meant the new function (calling get_free_port) for the new
use-case could be __init. The new function could be added to
xen/common/event_channel.c or to xen/arch/arm/domain_build.c.
The existing XSM check in evtchn_alloc_unbound cannot work and should
not work: it is based on the current domain having enough privileges to
create the event channel. In this case, we have no current domain at
all. The current domain is Xen itself.
And DOM_XEN cannot be given the appropriate permission, perhaps
explicitly when using a real policy and by default in dummy and SILO
modes?
The issue is that the check is based on "current", not one of the
domains passed as an argument to evtchn_alloc_unbound. Otherwise,
passing DOMID_XEN would be straightforward.
We would need to use a hack (like Daniel wrote in the other email) to
set the idle_domain temporarily as a privileged domain only for the sake
of passing an irrelevant (irrelevant as in "not relevant to this case")
XSM check. That cannot be an improvement. Also, setting current to a
"fake" domain is not great either.
I agree they are not great. But this needs to be compared with the other
proposals.
AFAIU, your proposal is to duplicate code. This brings other risks such
as duplicated bug and more code to maintain.
In your case, you only need one helper. But in some other context (e.g.
Live-Update and it looks like Hyperlaunch), we may need to re-use more
hypercalls code.
So to me, the idea of switching to a "fake" domain or bypassing the
check is more appealing. I have a preference for the "fake" domain here.
In the specific case of dom0less and this patch, this is the only
instance of this issue and could be solved very straightforwardly by
calling get_free_port directly as we discussed [1].
Exporting get_free_port() is a no-go for me. This can be easily mis-used
and in fact you already did it in your proposal by not using the proper
locking (I appreciate this is meant to be boot code only).
I know Julien had some reservations about that. Let's try to find a
technical solution that makes everyone happy.
Maybe, instead of exporting get_free_port, we could create a new
function in xen/common/event_channel.c and mark it as __init? This way:
- we don't need to expose get_free_port
- the new function would only be
Re: [PATCH v3 2/5] xen: make evtchn_alloc_unbound public
Hi Daniel, On 25/03/2022 15:45, Daniel P. Smith wrote: The existing XSM check in evtchn_alloc_unbound cannot work and should not work: it is based on the current domain having enough privileges to create the event channel. In this case, we have no current domain at all. The current domain is Xen itself. And DOM_XEN cannot be given the appropriate permission, perhaps explicitly when using a real policy and by default in dummy and SILO modes? The issue is that the check is based on "current", not one of the domains passed as an argument to evtchn_alloc_unbound. Otherwise, passing DOMID_XEN would be straightforward. We would need to use a hack (like Daniel wrote in the other email) to set the idle_domain temporarily as a privileged domain only for the sake of passing an irrelevant (irrelevant as in "not relevant to this case") XSM check. That cannot be an improvement. Also, setting current to a "fake" domain is not great either. My suggestion was not to intended to be simply a hack but looking at the larger issue instead of simply doing a targeted fix for this one instnace. While I cannot give an example right off hand, the reality is, at least for hyperlaunch, that we cannot say for certain there will not be further resource allocations that is protected by the security framework and will require preliminary handling by the construction logic in the hypervisor. The low-complexity approach is to address each one in a case-by-case fashion using direct calls that go around the security framework. A more security conscience, and higher complexity, approach would be to consider a least-privilege approach and look at introducing the ability to do controlled switching of contexts, i.e. moving `current` from DOMID_IDLE to DOMID_CONSTRUCT, to one that is granted only the necessary privileges to do the resource allocations in which it is limited. This is also not the first time this issue has come up, I don't recall the exact thread but several months ago someone ran into the issue they need to make a call to a resource function and was blocked by XSM because DOMID_IDLE has no privileges. This was in the context of Live-Updating Xen. We are trying to re-use as much as possible the code used by the hypercalls. Those functions may contain xsm check that would fail because current would be an idle vCPU. For the full context: https://lore.kernel.org/xen-devel/bfd645cf42ef7786183be15c222ad04beed362c0.ca...@xen.org/ The reality is that the idea of monolithic high-privileged entities is being dropped in favor of least-privilege, and where possible hardware enforced, constraint. This can be seen with Intel de-privileging SMM and running SMI handlers in constrained ring 3. Arm is gaining capability pointers, CHERI, that will enable the possibility for constrained, least-privileged kernel subsystems. Would it not be advantageous for Xen to start moving in such a direction that would enable it to provide a new level of safety and security for consumers of Xen? Coming back to the short-term, I would advocate for introducing the concept and abstraction of constrained context switching through a set of function calls, which would likely be under XSM to allow policy enforcement. Likely the introductory implementation would just mask the fact that it is just setting `is_privileged` for DOMID_IDLE. Future evolution of the capability could see the introduction of new "contexts", whether they are represented by a domain could be determined then, and the ability to do controlled switching based on policy. +1 with this idea. Cheers, -- Julien Grall
Re: [PATCH v3 2/5] xen: make evtchn_alloc_unbound public
On Fri, Mar 25, 2022 at 11:46 AM Daniel P. Smith wrote: > > On 3/24/22 20:30, Stefano Stabellini wrote: > > On Wed, 23 Mar 2022, Jan Beulich wrote: > >> On 23.03.2022 01:22, Stefano Stabellini wrote: > >>> The existing XSM check in evtchn_alloc_unbound cannot work and should > >>> not work: it is based on the current domain having enough privileges to > >>> create the event channel. In this case, we have no current domain at > >>> all. The current domain is Xen itself. > >> > >> And DOM_XEN cannot be given the appropriate permission, perhaps > >> explicitly when using a real policy and by default in dummy and SILO > >> modes? > > > > The issue is that the check is based on "current", not one of the > > domains passed as an argument to evtchn_alloc_unbound. Otherwise, > > passing DOMID_XEN would be straightforward. > > > > We would need to use a hack (like Daniel wrote in the other email) to > > set the idle_domain temporarily as a privileged domain only for the sake > > of passing an irrelevant (irrelevant as in "not relevant to this case") > > XSM check. That cannot be an improvement. Also, setting current to a > > "fake" domain is not great either. > > My suggestion was not to intended to be simply a hack but looking at the > larger issue instead of simply doing a targeted fix for this one > instnace. While I cannot give an example right off hand, the reality is, > at least for hyperlaunch, that we cannot say for certain there will not > be further resource allocations that is protected by the security > framework and will require preliminary handling by the construction > logic in the hypervisor. The low-complexity approach is to address each > one in a case-by-case fashion using direct calls that go around the > security framework. A more security conscience, and higher complexity, > approach would be to consider a least-privilege approach and look at > introducing the ability to do controlled switching of contexts, i.e. > moving `current` from DOMID_IDLE to DOMID_CONSTRUCT, to one that is > granted only the necessary privileges to do the resource allocations in > which it is limited. > > This is also not the first time this issue has come up, I don't recall > the exact thread but several months ago someone ran into the issue they > need to make a call to a resource function and was blocked by XSM > because DOMID_IDLE has no privileges. The reality is that the idea of > monolithic high-privileged entities is being dropped in favor of > least-privilege, and where possible hardware enforced, constraint. This > can be seen with Intel de-privileging SMM and running SMI handlers in > constrained ring 3. Arm is gaining capability pointers, CHERI, that will > enable the possibility for constrained, least-privileged kernel > subsystems. Would it not be advantageous for Xen to start moving in such > a direction that would enable it to provide a new level of safety and > security for consumers of Xen? > > Coming back to the short-term, I would advocate for introducing the > concept and abstraction of constrained context switching through a set > of function calls, which would likely be under XSM to allow policy > enforcement. Likely the introductory implementation would just mask the > fact that it is just setting `is_privileged` for DOMID_IDLE. Future > evolution of the capability could see the introduction of new > "contexts", whether they are represented by a domain could be determined > then, and the ability to do controlled switching based on policy. For the specific case of evtchn_alloc_unbound, Flask's xsm_evtchn_unbound has a side effect of labeling the event channel. So skipping the hook will have unintended consequences for Flask. xsm_evtchn_unbound could be split in two to have an access piece and a labeling piece. The access piece is run at hypercall entry, and the labeling is still done in evtchn_alloc_unbound. For Flask, labeling depends on the two domain endpoints, but not current. More generally, it seems to me there are too many xsm checks in the middle of functions/operations. They are fine for a normal entry via hypercall, but they interfere with Xen's internal operations. Xen shouldn't be restricted in its own operations. The live update people hit it with domain creation, and I just posted a patch for unmap_domain_pirq. It would be more obvious for auditing if each hypercall entrypoint applied xsm checks. Make the allow/deny decision as early as possible. Then a worker function would be easily callable for the Xen-internal case. The flip side of that is the xsm hook may need sub-op specific data to make its decision, so it fits to put it in the sub-op function. It seems to me the location of hooks was determined by where the data they need is already available. Re-arranging hooks may require some duplication. The xsm controls should clearly apply to the DomUs and other entities managed by Xen. xsm restricting Xen itself seems wrong. Having internal operations get denied by
Re: [PATCH 2/2] Changelog: Add __ro_after_init and CET
On 09/03/2022 13:03, Jan Beulich wrote: > On 09.03.2022 13:39, Andrew Cooper wrote: >> --- a/CHANGELOG.md >> +++ b/CHANGELOG.md >> @@ -6,6 +6,12 @@ The format is based on [Keep a >> Changelog](https://keepachangelog.com/en/1.0.0/) >> >> ## [unstable >> UNRELEASED](https://xenbits.xen.org/gitweb/?p=xen.git;a=shortlog;h=staging) >> - TBD >> >> +### Added >> + - __ro_after_init support on x86, for marking data as immutable after boot. > I'm not sure something like this (being an implementation detail) belongs > here. Having things immutable after boot is not an implementation detail. It is an important security hardening property, and deserves to be here. ~Andrew
[ovmf test] 168858: regressions - FAIL
flight 168858 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/168858/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-amd64 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 69218d5d2854acaa7a11c777244de4a297d2fbb9 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 25 days Failing since168258 2022-03-01 01:55:31 Z 24 days 254 attempts Testing same since 168832 2022-03-25 01:43:21 Z0 days6 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Bandaru, Purna Chandra Rao Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Patrick Rudolph Purna Chandra Rao Bandaru Sami Mujawar Sean Rhodes Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 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 Not pushing. (No revision log; it would be 904 lines long.)
Re: Understanding osdep_xenforeignmemory_map mmap behaviour
(add Arnd to CC)
Juergen Gross writes:
> [[PGP Signed Part:Undecided]]
> On 24.03.22 02:42, Stefano Stabellini wrote:
>> I am pretty sure the reasons have to do with old x86 PV guests, so I am
>> CCing Juergen and Boris.
>>
>>> Hi,
>>>
>>> While we've been working on the rust-vmm virtio backends on Xen we
>>> obviously have to map guest memory info the userspace of the daemon.
>>> However following the logic of what is going on is a little confusing.
>>> For example in the Linux backend we have this:
>>>
>>>void *osdep_xenforeignmemory_map(xenforeignmemory_handle *fmem,
>>> uint32_t dom, void *addr,
>>> int prot, int flags, size_t num,
>>> const xen_pfn_t arr[/*num*/], int
>>> err[/*num*/])
>>>{
>>>int fd = fmem->fd;
>>>privcmd_mmapbatch_v2_t ioctlx;
>>>size_t i;
>>>int rc;
>>>
>>>addr = mmap(addr, num << XC_PAGE_SHIFT, prot, flags | MAP_SHARED,
>>>fd, 0);
>>>if ( addr == MAP_FAILED )
>>>return NULL;
>>>
>>>ioctlx.num = num;
>>>ioctlx.dom = dom;
>>>ioctlx.addr = (unsigned long)addr;
>>>ioctlx.arr = arr;
>>>ioctlx.err = err;
>>>
>>>rc = ioctl(fd, IOCTL_PRIVCMD_MMAPBATCH_V2, );
>>>
>>> Where the fd passed down is associated with the /dev/xen/privcmd device
>>> for issuing hypercalls on userspaces behalf. What is confusing is why
>>> the function does it's own mmap - one would assume the passed addr would
>>> be associated with a anonymous or file backed mmap region already that
>>> the calling code has setup. Applying a mmap to a special device seems a
>>> little odd.
>>>
>>> Looking at the implementation on the kernel side it seems the mmap
>>> handler only sets a few flags:
>>>
>>>static int privcmd_mmap(struct file *file, struct vm_area_struct *vma)
>>>{
>>>/* DONTCOPY is essential for Xen because copy_page_range doesn't
>>> know
>>> * how to recreate these mappings */
>>>vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTCOPY |
>>> VM_DONTEXPAND | VM_DONTDUMP;
>>>vma->vm_ops = _vm_ops;
>>>vma->vm_private_data = NULL;
>>>
>>>return 0;
>>>}
>>>
>>> So can I confirm that the mmap of /dev/xen/privcmd is being called for
>>> side effects? Is it so when the actual ioctl is called the correct flags
>>> are set of the pages associated with the user space virtual address
>>> range?
>>>
>>> Can I confirm there shouldn't be any limitation on where and how the
>>> userspace virtual address space is setup for the mapping in the guest
>>> memory?
>>>
>>> Is there a reason why this isn't done in the ioctl path itself?
>
> For a rather long time we were using "normal" user pages for this purpose,
> which were just locked into memory for doing the hypercall.
Was this using the normal mlock() semantics to stop pages being swapped
out of RAM?
> Unfortunately there have been very rare problems with that approach, as
> the Linux kernel can set a user page related PTE to invalid for short
> periods of time, which led to EFAULT in the hypervisor when trying to
> access the hypercall data.
I must admit I'm not super familiar with the internals of page table
handling with Linux+Xen. Doesn't the kernel need to delegate the
tweaking of page tables to the hypervisor or is it allowed to manipulate
the page tables itself?
> In Linux this can avoided only by using kernel memory, which is the
> reason why the hypercall buffers are allocated and mmap()-ed through the
> privcmd driver.
>
>>>
>>> I'm trying to understand the differences between Xen and KVM in the API
>>> choices here. I think the equivalent is the KVM_SET_USER_MEMORY_REGION
>>> ioctl for KVM which brings a section of the guest physical address space
>>> into the userspaces vaddr range.
>
> The main difference is just that the consumer of the hypercall buffer is
> NOT the kernel, but the hypervisor. In the KVM case both are the same, so
> a brief period of an invalid PTE can be handled just fine in KVM, while
> the Xen hypervisor has no idea that this situation will be over very
> soon.
I still don't follow the details of why we have the separate mmap. Is it
purely because the VM flags of the special file can be changed in a way
that can't be done with a traditional file-backed mmap?
I can see various other devices have their own setting of vm flags but
VM_DONTCOPY for example can be set with the appropriate madvise call:
MADV_DONTFORK (since Linux 2.6.16)
Do not make the pages in this range available to the child after
a fork(2). This is useful to prevent copy-on-write semantics
from changing the physical location of a page if the parent
writes to it after a fork(2). (Such page relocations cause
problems for hardware that
Security support status of xnf(4) and xbf(4)
Linux’s netfront and blkfront drivers recently had a security vulnerability (XSA-396) that allowed a malicious backend to potentially compromise them. In follow-up audits, I found that OpenBSD’s xnf(4) currently trusts the backend domain. I reported this privately to Theo de Raadt, who indicated that OpenBSD does not consider this to be a security concern. This is obviously a valid position for the OpenBSD project to take, but it is surprising to some (such as myself) from the broader Xen ecosystem. Standard practice in the Xen world is that bugs in frontends that allow a malicious backend to cause mischief *are* considered security bugs unless there is explicit documentation to the contrary. As such, I believe this deserves to be noted in xnf(4) and xbf(4)’s man pages. If the OpenBSD project agrees, I am willing to write a patch, but I have no experience with mandoc so it might take a few tries. -- Sincerely, Demi Marie Obenour (she/her/hers) Invisible Things Lab signature.asc Description: PGP signature
Re: [PATCH v3 2/5] xen: make evtchn_alloc_unbound public
On 3/24/22 20:30, Stefano Stabellini wrote:
> On Wed, 23 Mar 2022, Jan Beulich wrote:
>> On 23.03.2022 01:22, Stefano Stabellini wrote:
>>> On Tue, 15 Mar 2022, Daniel P. Smith wrote:
On 1/28/22 16:33, Stefano Stabellini wrote:
> From: Luca Miccio
>
> The xenstore event channel will be allocated for dom0less domains. It is
> necessary to have access to the evtchn_alloc_unbound function to do
> that, so make evtchn_alloc_unbound public.
>
> Add a skip_xsm parameter to allow disabling the XSM check in
> evtchn_alloc_unbound (xsm_evtchn_unbound wouldn't work for a call
> originated from Xen before running any domains.)
>
> Signed-off-by: Luca Miccio
> Signed-off-by: Stefano Stabellini
> CC: Julien Grall
> CC: Volodymyr Babchuk
> CC: Bertrand Marquis
> CC: Andrew Cooper
> CC: George Dunlap
> CC: Jan Beulich
> CC: Wei Liu
> ---
> Changes v3:
> - expose evtchn_alloc_unbound, assing a skip_xsm parameter
> ---
> xen/common/event_channel.c | 13 -
> xen/include/xen/event.h| 3 +++
> 2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
> index da88ad141a..be57d00a15 100644
> --- a/xen/common/event_channel.c
> +++ b/xen/common/event_channel.c
> @@ -284,7 +284,7 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
> xsm_evtchn_close_post(chn);
> }
>
> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> +int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc, bool skip_xsm)
> {
> struct evtchn *chn;
> struct domain *d;
> @@ -301,9 +301,12 @@ static int
> evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> ERROR_EXIT_DOM(port, d);
> chn = evtchn_from_port(d, port);
>
> -rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
> -if ( rc )
> -goto out;
> +if ( !skip_xsm )
> +{
> +rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
> +if ( rc )
> +goto out;
> +}
Please do not subvert the security framework because it causes an
inconvenience. As Jan recommended, work within the XSM check to allow
your access so that we may ensure it is done safely. If you need any
help making modifications to XSM, please do not hesitate to reach out as
I will gladly help.
>>>
>>> Thank you!
>>>
>>> First let me reply to Jan: this series is only introducing 1 more call
>>> to evtchn_alloc_unbound, which is to allocate the special xenstore event
>>> channel, the one configured via
>>> d->arch.hvm.params[HVM_PARAM_STORE_EVTCHN].
>>>
>>> It is not meant to be a generic function, and it is not meant to be
>>> called more than once. It could (should?) be __init.
>>
>> How that? Its pre-existing use doesn't disappear, and requires it to be
>> non-__init.
>
> Sorry I meant the new function (calling get_free_port) for the new
> use-case could be __init. The new function could be added to
> xen/common/event_channel.c or to xen/arch/arm/domain_build.c.
>
>
>>> The existing XSM check in evtchn_alloc_unbound cannot work and should
>>> not work: it is based on the current domain having enough privileges to
>>> create the event channel. In this case, we have no current domain at
>>> all. The current domain is Xen itself.
>>
>> And DOM_XEN cannot be given the appropriate permission, perhaps
>> explicitly when using a real policy and by default in dummy and SILO
>> modes?
>
> The issue is that the check is based on "current", not one of the
> domains passed as an argument to evtchn_alloc_unbound. Otherwise,
> passing DOMID_XEN would be straightforward.
>
> We would need to use a hack (like Daniel wrote in the other email) to
> set the idle_domain temporarily as a privileged domain only for the sake
> of passing an irrelevant (irrelevant as in "not relevant to this case")
> XSM check. That cannot be an improvement. Also, setting current to a
> "fake" domain is not great either.
My suggestion was not to intended to be simply a hack but looking at the
larger issue instead of simply doing a targeted fix for this one
instnace. While I cannot give an example right off hand, the reality is,
at least for hyperlaunch, that we cannot say for certain there will not
be further resource allocations that is protected by the security
framework and will require preliminary handling by the construction
logic in the hypervisor. The low-complexity approach is to address each
one in a case-by-case fashion using direct calls that go around the
security framework. A more security conscience, and higher complexity,
approach would be to consider a least-privilege approach and look at
introducing the ability to do controlled switching of contexts, i.e.
moving `current` from DOMID_IDLE to
[ovmf test] 168854: regressions - FAIL
flight 168854 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/168854/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-amd64 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 69218d5d2854acaa7a11c777244de4a297d2fbb9 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 25 days Failing since168258 2022-03-01 01:55:31 Z 24 days 253 attempts Testing same since 168832 2022-03-25 01:43:21 Z0 days5 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Bandaru, Purna Chandra Rao Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Patrick Rudolph Purna Chandra Rao Bandaru Sami Mujawar Sean Rhodes Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 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 Not pushing. (No revision log; it would be 904 lines long.)
[qemu-mainline test] 168835: tolerable FAIL - PUSHED
flight 168835 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/168835/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168828 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168828 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168828 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168828 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168828 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168828 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168828 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168828 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-i386-libvirt-xsm 15 migrate-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-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-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-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 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-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 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-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-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: qemuu9c721291506c037d934900a6167dc3bf4a8f51a6 baseline version: qemuue309ce90a23bef4f5210a8c49d53441144be293c Last test of basis 168828 2022-03-24 17:07:03 Z0 days Testing same since 168835 2022-03-25 04:11:20 Z0 days1 attempts People who touched revisions under test: Fergus Henderson Laurent Vivier Patrick
Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()
Hi Julien, > On 25 Mar 2022, at 15:42, Julien Grall wrote: > > Hi Bertrand, > > On 25/03/2022 14:35, Bertrand Marquis wrote: >>> On 25 Mar 2022, at 15:24, Julien Grall wrote: >>> On 25/03/2022 13:47, Bertrand Marquis wrote: Hi Julien, >>> >>> Hi Bertrand, >>> > On 9 Mar 2022, at 12:20, Julien Grall wrote: > > From: Julien Grall > > At the moment, switch_ttbr() is switching the TTBR whilst the MMU is > still on. > > Switching TTBR is like replacing existing mappings with new ones. So > we need to follow the break-before-make sequence. > > In this case, it means the MMU needs to be switched off while the > TTBR is updated. In order to disable the MMU, we need to first > jump to an identity mapping. > > Rename switch_ttbr() to switch_ttbr_id() and create an helper on > top to temporary map the identity mapping and call switch_ttbr() > via the identity address. > > switch_ttbr_id() is now reworked to temporarily turn off the MMU > before updating the TTBR. > > We also need to make sure the helper switch_ttbr() is part of the > identity mapping. So move _end_boot past it. > > Take the opportunity to instruction cache flush as the operation is > only necessary when the memory is updated. Your code is actually remove the instruction cache invalidation so this sentence is a bit misleading. >>> >>> I forgot to add the word "remove" in the sentence. >> Ok (my sentence was also wrong by the way) >>> Also an open question: shouldn’t we flush the data cache ? >>> Do you mean clean/invalidate to PoC/PoU? Something else? >> Yes, probably to PoU. >>> As we switch from one TTBR to an other, there might be some data in the cache dependent that could be flushed while the MMU is off >>> >>> I am a bit confused. Those flush could also happen with the MMU on. So how >>> turning off the MMU would result to a problem? Note that the data cache is >>> still enabled during the switch. >> If the first level of cache is VIPT and we turn off the MMU, I am wondering >> if this could not create troubles and could require the cache to be flushed >> before turning the MMU off. > My reading of the Arm Arm (D5.11.1 "Data and unified caches" ARM DDI 0487F.c) > suggests the data cache is always PIPT. You are right, only the instruction cache is VIPT. So the problem most probably does not exist. > >> I have no idea if this is a problem or not, just raising the question. >> I can try to dig on that at Arm when I am back in 10 days. > > Enjoy it! Thanks Bertrand > > Cheers, > > -- > Julien Grall
Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()
Hi Bertrand, On 25/03/2022 14:35, Bertrand Marquis wrote: On 25 Mar 2022, at 15:24, Julien Grall wrote: On 25/03/2022 13:47, Bertrand Marquis wrote: Hi Julien, Hi Bertrand, On 9 Mar 2022, at 12:20, Julien Grall wrote: From: Julien Grall At the moment, switch_ttbr() is switching the TTBR whilst the MMU is still on. Switching TTBR is like replacing existing mappings with new ones. So we need to follow the break-before-make sequence. In this case, it means the MMU needs to be switched off while the TTBR is updated. In order to disable the MMU, we need to first jump to an identity mapping. Rename switch_ttbr() to switch_ttbr_id() and create an helper on top to temporary map the identity mapping and call switch_ttbr() via the identity address. switch_ttbr_id() is now reworked to temporarily turn off the MMU before updating the TTBR. We also need to make sure the helper switch_ttbr() is part of the identity mapping. So move _end_boot past it. Take the opportunity to instruction cache flush as the operation is only necessary when the memory is updated. Your code is actually remove the instruction cache invalidation so this sentence is a bit misleading. I forgot to add the word "remove" in the sentence. Ok (my sentence was also wrong by the way) Also an open question: shouldn’t we flush the data cache ? Do you mean clean/invalidate to PoC/PoU? Something else? Yes, probably to PoU. As we switch from one TTBR to an other, there might be some data in the cache dependent that could be flushed while the MMU is off I am a bit confused. Those flush could also happen with the MMU on. So how turning off the MMU would result to a problem? Note that the data cache is still enabled during the switch. If the first level of cache is VIPT and we turn off the MMU, I am wondering if this could not create troubles and could require the cache to be flushed before turning the MMU off. My reading of the Arm Arm (D5.11.1 "Data and unified caches" ARM DDI 0487F.c) suggests the data cache is always PIPT. I have no idea if this is a problem or not, just raising the question. I can try to dig on that at Arm when I am back in 10 days. Enjoy it! Cheers, -- Julien Grall
Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout
Hi, On 25/03/2022 14:05, Bertrand Marquis wrote: On 25 Mar 2022, at 14:35, Julien Grall wrote: On 25/03/2022 13:17, Bertrand Marquis wrote: Hi Julien, Hi, On 9 Mar 2022, at 12:20, Julien Grall wrote: From: Julien Grall Xen is currently not fully compliant with the Arm because it will I think you wanted to say “arm arm” her. Yes. I will update it. switch the TTBR with the MMU on. In order to be compliant, we need to disable the MMU before switching the TTBR. The implication is the page-tables should contain an identity mapping of the code switching the TTBR. If we don't rework the memory layout, we would need to find a virtual address that matches a physical address and doesn't clash with the static virtual regions. This can be a bit tricky. This sentence is a bit misleading. Even with the rework you need to do that just by moving the Xen virtual address upper you make sure that anything physical memory under 512GB can be mapped 1:1 without clashing with other Xen mappings (unless Xen is loaded in memory at physical address 512GB which would end in the same issue). So the key difference is with the rework, it is trivial to create the 1:1 mapping as we know it doesn't clash. This is not the case without the rework. Agree I think should be rephrased. I am not entirely sure how to rephrase it. Do you have a proposal? Turn it into the positive: Rework the memory layout to put Xen over 512GB. This makes it trivial to create a 1:1 mapping, with the assumption that the physical memory is under 512GB. I will use this wording in the next version. be loaded at (512GB + 2MB). This requires a slight tweak of the boot code as XEN_VIRT_START cannot be used as an immediate. Signed-off-by: Julien Grall --- TODO: - I vaguely recall that one of the early platform we supported add the memory starting in high memory (> 1TB). I need to check whether the new layout will be fine. I think we have some Juno with some memory like that, tell me if you need help here. Would you be able to check the memory layout and confirm? I checked and the Juno we have as the high memory a lot lower than that: RAM: 00088000 - 0009 No idea why it was a lot higher in my mind. I have only encountered one board with the memory over 512GB. I can't remember whether it is AMD Seattle or X-Gene. - Update the documentation to reflect the new layout --- xen/arch/arm/arm64/head.S | 3 ++- xen/arch/arm/include/asm/config.h | 20 ++-- xen/arch/arm/mm.c | 14 +++--- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 66d862fc8137..878649280d73 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -594,7 +594,8 @@ create_page_tables: * need an additional 1:1 mapping, the virtual mapping will * suffice. */ -cmp x19, #XEN_VIRT_START +ldr x0, =XEN_VIRT_START +cmp x19, x0 A comment in the code would be good here to prevent someone reverting this. Anyone trying to revert the change will face a compilation error: CC arch/arm/arm64/head.o arch/arm/arm64/head.S: Assembler messages: arch/arm/arm64/head.S:597: Error: immediate out of range So I don't think a comment is necessary because this is not specific to a compiler/assembler. Right I should have thought of the compilation error. TBH, I would have preferred to keep the single instruction. AFAICT, the immediate should be either between 0 - 4095. Or a number between 4096 and 2^24 that is 4KB aligned. So it would not suit us here. Cheers, -- Julien Grall
Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()
Hi Julien, > On 25 Mar 2022, at 15:24, Julien Grall wrote: > > > > On 25/03/2022 13:47, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 9 Mar 2022, at 12:20, Julien Grall wrote: >>> >>> From: Julien Grall >>> >>> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is >>> still on. >>> >>> Switching TTBR is like replacing existing mappings with new ones. So >>> we need to follow the break-before-make sequence. >>> >>> In this case, it means the MMU needs to be switched off while the >>> TTBR is updated. In order to disable the MMU, we need to first >>> jump to an identity mapping. >>> >>> Rename switch_ttbr() to switch_ttbr_id() and create an helper on >>> top to temporary map the identity mapping and call switch_ttbr() >>> via the identity address. >>> >>> switch_ttbr_id() is now reworked to temporarily turn off the MMU >>> before updating the TTBR. >>> >>> We also need to make sure the helper switch_ttbr() is part of the >>> identity mapping. So move _end_boot past it. >>> >>> Take the opportunity to instruction cache flush as the operation is >>> only necessary when the memory is updated. >> Your code is actually remove the instruction cache invalidation so >> this sentence is a bit misleading. > > I forgot to add the word "remove" in the sentence. Ok (my sentence was also wrong by the way) > >> Also an open question: shouldn’t we flush the data cache ? > Do you mean clean/invalidate to PoC/PoU? Something else? Yes, probably to PoU. > >> As we switch from one TTBR to an other, there might be some data >> in the cache dependent that could be flushed while the MMU is off > > I am a bit confused. Those flush could also happen with the MMU on. So how > turning off the MMU would result to a problem? Note that the data cache is > still enabled during the switch. If the first level of cache is VIPT and we turn off the MMU, I am wondering if this could not create troubles and could require the cache to be flushed before turning the MMU off. I have no idea if this is a problem or not, just raising the question. I can try to dig on that at Arm when I am back in 10 days. > >> or >> that would have no mapping once it is reactivated. > The cache line will be flushed at some point in the future. I would argue if > the caller need it earlier, then it should make sure to issue the flush > before switch_ttbr(). Ok. I will still try to check if there is some kind of recommandation to turn the MMU off. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules
On 25.03.2022 15:28, Bertrand Marquis wrote: >> On 25 Mar 2022, at 15:10, Jan Beulich wrote: >> On 25.03.2022 13:57, Bertrand Marquis wrote: On 25 Mar 2022, at 12:43, Jan Beulich wrote: On 24.03.2022 12:04, Bertrand Marquis wrote: > --- a/xen/include/xen/kconfig.h > +++ b/xen/include/xen/kconfig.h > @@ -8,6 +8,10 @@ > * these only work with boolean option. > */ > > +/* cppcheck is failing to parse the macro so use a dummy one */ > +#ifdef CPPCHECK > +#define IS_ENABLED(option) option > +#else > /* > * Getting something that works in C and CPP for an arg that may or may > * not be defined is tricky. Here, if we have "#define CONFIG_BOOGER 1" > @@ -27,5 +31,6 @@ > * otherwise. > */ > #define IS_ENABLED(option) config_enabled(option) > +#endif What are the consequences of this workaround on the code actually being checked? Does this perhaps lead to certain portions of code being skipped while checking? >>> >>> Cppcheck is not optimising the code but looking at the syntax so the >>> consequence here is that cppcheck is checking some code which might >>> be optimised out by the compiler. This is not optimal but still it should >>> analyse properly the code. >> >> Aren't you saying so merely because all uses of IS_ENABLED() in our >> sources so far are in if()? It would seem to me that when used in #if >> (as can be found in Linux, which hence means could be the case in our >> tree as well sooner or later) sections of code might either be skipped >> or syntax errors may result. Or wait - IS_ENABLED() does itself kind >> of rely on the respective CONFIG_* to expand to a numeric value; when >> expanding to a string, I guess the compiler would also warn about the >> resulting construct when used in if() (and reject any uses with #if). > > I am not quite sure I am following what you are saying (or asking). I first tried to clarify what I'm concerned about, but then said that apparently there is no reason to be concerned. I'm sorry if this didn't come across quite clear enough. Bottom line - no request for any change here. Jan > I say that most use cases are if (IS_ENABLED(x)) so from the syntax point > of view it is ok to not do exactly as IS_ENABLED really does. And > cppcheck is checking the code not the result. > Now it would be better to do it right but the point of the patch is to enable > cppcheck not make it perfect on the first shot. > > Cheers > Bertrand > >> >> Jan >> >
Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules
Hi Jan, > On 25 Mar 2022, at 15:10, Jan Beulich wrote: > > On 25.03.2022 13:57, Bertrand Marquis wrote: >>> On 25 Mar 2022, at 12:43, Jan Beulich wrote: >>> On 24.03.2022 12:04, Bertrand Marquis wrote: --- a/xen/include/xen/kconfig.h +++ b/xen/include/xen/kconfig.h @@ -8,6 +8,10 @@ * these only work with boolean option. */ +/* cppcheck is failing to parse the macro so use a dummy one */ +#ifdef CPPCHECK +#define IS_ENABLED(option) option +#else /* * Getting something that works in C and CPP for an arg that may or may * not be defined is tricky. Here, if we have "#define CONFIG_BOOGER 1" @@ -27,5 +31,6 @@ * otherwise. */ #define IS_ENABLED(option) config_enabled(option) +#endif >>> >>> What are the consequences of this workaround on the code actually >>> being checked? Does this perhaps lead to certain portions of code >>> being skipped while checking? >> >> Cppcheck is not optimising the code but looking at the syntax so the >> consequence here is that cppcheck is checking some code which might >> be optimised out by the compiler. This is not optimal but still it should >> analyse properly the code. > > Aren't you saying so merely because all uses of IS_ENABLED() in our > sources so far are in if()? It would seem to me that when used in #if > (as can be found in Linux, which hence means could be the case in our > tree as well sooner or later) sections of code might either be skipped > or syntax errors may result. Or wait - IS_ENABLED() does itself kind > of rely on the respective CONFIG_* to expand to a numeric value; when > expanding to a string, I guess the compiler would also warn about the > resulting construct when used in if() (and reject any uses with #if). I am not quite sure I am following what you are saying (or asking). I say that most use cases are if (IS_ENABLED(x)) so from the syntax point of view it is ok to not do exactly as IS_ENABLED really does. And cppcheck is checking the code not the result. Now it would be better to do it right but the point of the patch is to enable cppcheck not make it perfect on the first shot. Cheers Bertrand > > Jan >
Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()
On 25/03/2022 13:47, Bertrand Marquis wrote: Hi Julien, Hi Bertrand, On 9 Mar 2022, at 12:20, Julien Grall wrote: From: Julien Grall At the moment, switch_ttbr() is switching the TTBR whilst the MMU is still on. Switching TTBR is like replacing existing mappings with new ones. So we need to follow the break-before-make sequence. In this case, it means the MMU needs to be switched off while the TTBR is updated. In order to disable the MMU, we need to first jump to an identity mapping. Rename switch_ttbr() to switch_ttbr_id() and create an helper on top to temporary map the identity mapping and call switch_ttbr() via the identity address. switch_ttbr_id() is now reworked to temporarily turn off the MMU before updating the TTBR. We also need to make sure the helper switch_ttbr() is part of the identity mapping. So move _end_boot past it. Take the opportunity to instruction cache flush as the operation is only necessary when the memory is updated. Your code is actually remove the instruction cache invalidation so this sentence is a bit misleading. I forgot to add the word "remove" in the sentence. Also an open question: shouldn’t we flush the data cache ? Do you mean clean/invalidate to PoC/PoU? Something else? As we switch from one TTBR to an other, there might be some data in the cache dependent that could be flushed while the MMU is off I am a bit confused. Those flush could also happen with the MMU on. So how turning off the MMU would result to a problem? Note that the data cache is still enabled during the switch. or that would have no mapping once it is reactivated. The cache line will be flushed at some point in the future. I would argue if the caller need it earlier, then it should make sure to issue the flush before switch_ttbr(). Cheers, -- Julien Grall
[xen-unstable-smoke test] 168841: tolerable all pass - PUSHED
flight 168841 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/168841/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 0e03ff97def12b121b5313094a76e5db7bb5c93c baseline version: xen 1c80f13a6efdc832878d7a431e2c216039d063bc Last test of basis 168821 2022-03-24 11:03:05 Z1 days Testing same since 168841 2022-03-25 10:00:33 Z0 days1 attempts People who touched revisions under test: Julien Grall Stefano Stabellini jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 1c80f13a6e..0e03ff97de 0e03ff97def12b121b5313094a76e5db7bb5c93c -> smoke
[PATCH v2] xen: fix is_xen_pmu()
is_xen_pmu() is taking the cpu number as parameter, but it is not using
it. Instead it just tests whether the Xen PMU initialization on the
current cpu did succeed. As this test is done by checking a percpu
pointer, preemption needs to be disabled in order to avoid switching
the cpu while doing the test. While resuming from suspend() this seems
not to be the case:
[ 88.082751] ACPI: PM: Low-level resume complete
[ 88.087933] ACPI: EC: EC started
[ 88.091464] ACPI: PM: Restoring platform NVS memory
[ 88.097166] xen_acpi_processor: Uploading Xen processor PM info
[ 88.103850] Enabling non-boot CPUs ...
[ 88.108128] installing Xen timer for CPU 1
[ 88.112763] BUG: using smp_processor_id() in preemptible [] code:
systemd-sleep/7138
[ 88.122256] caller is is_xen_pmu+0x12/0x30
[ 88.126937] CPU: 0 PID: 7138 Comm: systemd-sleep Tainted: GW
5.16.13-2.fc32.qubes.x86_64 #1
[ 88.137939] Hardware name: Star Labs StarBook/StarBook, BIOS 7.97 03/21/2022
[ 88.145930] Call Trace:
[ 88.148757]
[ 88.151193] dump_stack_lvl+0x48/0x5e
[ 88.155381] check_preemption_disabled+0xde/0xe0
[ 88.160641] is_xen_pmu+0x12/0x30
[ 88.164441] xen_smp_intr_init_pv+0x75/0x100
Fix that by replacing is_xen_pmu() by a simple boolean variable which
reflects the Xen PMU initialization state on cpu 0.
Modify xen_pmu_init() to return early in case it is being called for a
cpu other than cpu 0 and the boolean variable not being set.
Fixes: bf6dfb154d93 ("xen/PMU: PMU emulation code")
Reported-by: Marek Marczykowski-Górecki
Signed-off-by: Juergen Gross
---
V2:
- don't reset is_xen_pmu when suspending (Boris Ostrovsky)
---
arch/x86/xen/pmu.c| 10 --
arch/x86/xen/pmu.h| 3 ++-
arch/x86/xen/smp_pv.c | 2 +-
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/x86/xen/pmu.c b/arch/x86/xen/pmu.c
index 89dd6b1708b0..21ecbe754cb2 100644
--- a/arch/x86/xen/pmu.c
+++ b/arch/x86/xen/pmu.c
@@ -506,10 +506,7 @@ irqreturn_t xen_pmu_irq_handler(int irq, void *dev_id)
return ret;
}
-bool is_xen_pmu(int cpu)
-{
- return (get_xenpmu_data() != NULL);
-}
+bool is_xen_pmu;
void xen_pmu_init(int cpu)
{
@@ -520,7 +517,7 @@ void xen_pmu_init(int cpu)
BUILD_BUG_ON(sizeof(struct xen_pmu_data) > PAGE_SIZE);
- if (xen_hvm_domain())
+ if (xen_hvm_domain() || (cpu != 0 && !is_xen_pmu))
return;
xenpmu_data = (struct xen_pmu_data *)get_zeroed_page(GFP_KERNEL);
@@ -541,7 +538,8 @@ void xen_pmu_init(int cpu)
per_cpu(xenpmu_shared, cpu).xenpmu_data = xenpmu_data;
per_cpu(xenpmu_shared, cpu).flags = 0;
- if (cpu == 0) {
+ if (!is_xen_pmu) {
+ is_xen_pmu = true;
perf_register_guest_info_callbacks(_guest_cbs);
xen_pmu_arch_init();
}
diff --git a/arch/x86/xen/pmu.h b/arch/x86/xen/pmu.h
index 0e83a160589b..65c58894fc79 100644
--- a/arch/x86/xen/pmu.h
+++ b/arch/x86/xen/pmu.h
@@ -4,6 +4,8 @@
#include
+extern bool is_xen_pmu;
+
irqreturn_t xen_pmu_irq_handler(int irq, void *dev_id);
#ifdef CONFIG_XEN_HAVE_VPMU
void xen_pmu_init(int cpu);
@@ -12,7 +14,6 @@ void xen_pmu_finish(int cpu);
static inline void xen_pmu_init(int cpu) {}
static inline void xen_pmu_finish(int cpu) {}
#endif
-bool is_xen_pmu(int cpu);
bool pmu_msr_read(unsigned int msr, uint64_t *val, int *err);
bool pmu_msr_write(unsigned int msr, uint32_t low, uint32_t high, int *err);
int pmu_apic_update(uint32_t reg);
diff --git a/arch/x86/xen/smp_pv.c b/arch/x86/xen/smp_pv.c
index 4a6019238ee7..688aa8b6ae29 100644
--- a/arch/x86/xen/smp_pv.c
+++ b/arch/x86/xen/smp_pv.c
@@ -129,7 +129,7 @@ int xen_smp_intr_init_pv(unsigned int cpu)
per_cpu(xen_irq_work, cpu).irq = rc;
per_cpu(xen_irq_work, cpu).name = callfunc_name;
- if (is_xen_pmu(cpu)) {
+ if (is_xen_pmu) {
pmu_name = kasprintf(GFP_KERNEL, "pmu%d", cpu);
rc = bind_virq_to_irqhandler(VIRQ_XENPMU, cpu,
xen_pmu_irq_handler,
--
2.34.1
[PATCH] x86/physdev: Call xsm_unmap_domain_irq earlier
Pull the XSM check up out of unmap_domain_pirq into physdev_map_pirq.
xsm_unmap_domain_irq was seen denying unmap_domain_pirq when called from
complete_domain_destroy as an RCU callback. The source context was an
unexpected, random domain. Since this is a xen-internal operation,
going through the XSM hook is inapproriate.
Move the XSM hook up into physdev_unmap_pirq, which is the
guest-accessible path. This requires moving some of the sanity check
upwards as well since the hook needs the additional data to make its
decision. Since complete_domain_destroy still calls unmap_domain_pirq,
replace the moved runtime checking with assert. Only valid pirqs should
make their way into unmap_domain_pirq from complete_domain_destroy.
This is mostly code movement, but one style change is to pull `irq =
info->arch.irq` out of the if condition.
Label done is now unused and removed.
Signed-off-by: Jason Andryuk
---
unmap_domain_pirq is also called in vioapic_hwdom_map_gsi and
vpci_msi_disable. vioapic_hwdom_map_gsi is a cleanup path after going
through map_domain_pirq, and I don't think the vpci code is directly
guest-accessible. So I think those are okay, but I not familiar with
that code. Hence, I am highlighting it.
xen/arch/x86/irq.c | 31 +++---
xen/arch/x86/physdev.c | 43 +-
2 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index 285ac399fb..ddd3194fba 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -2310,41 +2310,25 @@ int unmap_domain_pirq(struct domain *d, int pirq)
struct pirq *info;
struct msi_desc *msi_desc = NULL;
-if ( (pirq < 0) || (pirq >= d->nr_pirqs) )
-return -EINVAL;
-
+ASSERT(pirq >= 0);
+ASSERT(pirq < d->nr_pirqs);
ASSERT(pcidevs_locked());
ASSERT(spin_is_locked(>event_lock));
info = pirq_info(d, pirq);
-if ( !info || (irq = info->arch.irq) <= 0 )
-{
-dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
-d->domain_id, pirq);
-ret = -EINVAL;
-goto done;
-}
+ASSERT(info);
+
+irq = info->arch.irq;
+ASSERT(irq > 0);
desc = irq_to_desc(irq);
msi_desc = desc->msi_desc;
if ( msi_desc && msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
{
-if ( msi_desc->msi_attrib.entry_nr )
-{
-printk(XENLOG_G_ERR
- "dom%d: trying to unmap secondary MSI pirq %d\n",
- d->domain_id, pirq);
-ret = -EBUSY;
-goto done;
-}
+ASSERT(msi_desc->msi_attrib.entry_nr == 0);
nr = msi_desc->msi.nvec;
}
-ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
- msi_desc ? msi_desc->dev : NULL);
-if ( ret )
-goto done;
-
forced_unbind = pirq_guest_force_unbind(d, info);
if ( forced_unbind )
dprintk(XENLOG_G_WARNING, "dom%d: forcing unbind of pirq %d\n",
@@ -2405,7 +2389,6 @@ int unmap_domain_pirq(struct domain *d, int pirq)
if (msi_desc)
msi_free_irq(msi_desc);
- done:
return ret;
}
diff --git a/xen/arch/x86/physdev.c b/xen/arch/x86/physdev.c
index 2ddcf44f33..a5ed257dca 100644
--- a/xen/arch/x86/physdev.c
+++ b/xen/arch/x86/physdev.c
@@ -140,8 +140,11 @@ int physdev_map_pirq(domid_t domid, int type, int *index,
int *pirq_p,
int physdev_unmap_pirq(domid_t domid, int pirq)
{
+struct msi_desc *msi_desc;
+struct irq_desc *desc;
+struct pirq *info;
struct domain *d;
-int ret = 0;
+int irq, ret = 0;
d = rcu_lock_domain_by_any_id(domid);
if ( d == NULL )
@@ -162,9 +165,47 @@ int physdev_unmap_pirq(domid_t domid, int pirq)
goto free_domain;
}
+if ( (pirq < 0) || (pirq >= d->nr_pirqs) ) {
+ret = -EINVAL;
+goto free_domain;
+}
+
pcidevs_lock();
spin_lock(>event_lock);
+
+info = pirq_info(d, pirq);
+irq = info ? info->arch.irq : 0;
+if ( !info || irq <= 0 )
+{
+dprintk(XENLOG_G_ERR, "dom%d: pirq %d not mapped\n",
+d->domain_id, pirq);
+ret = -EINVAL;
+goto unlock;
+}
+
+desc = irq_to_desc(irq);
+msi_desc = desc->msi_desc;
+if ( msi_desc && msi_desc->msi_attrib.type == PCI_CAP_ID_MSI )
+{
+if ( msi_desc->msi_attrib.entry_nr )
+{
+printk(XENLOG_G_ERR
+ "dom%d: trying to unmap secondary MSI pirq %d\n",
+ d->domain_id, pirq);
+ret = -EBUSY;
+goto unlock;
+}
+}
+
+ret = xsm_unmap_domain_irq(XSM_HOOK, d, irq,
+ msi_desc ? msi_desc->dev : NULL);
+if ( ret )
+goto unlock;
+
ret = unmap_domain_pirq(d, pirq);
+
+ unlock:
+
spin_unlock(>event_lock);
pcidevs_unlock();
--
2.35.1
Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping
Hi Julien
> On 25 Mar 2022, at 14:48, Julien Grall wrote:
>
>
>
> On 25/03/2022 13:32, Bertrand Marquis wrote:
>> Hi Julien,
>
> Hi,
>
>>> On 9 Mar 2022, at 12:20, Julien Grall wrote:
>>>
>>> From: Julien GralL
>>>
>>> In follow-up patches we will need to have part of Xen identity mapped in
>>> order to safely switch the TTBR.
>>>
>>> On some platform, the identity mapping may have to start at 0. If we always
>>> keep the identity region mapped, NULL pointer ference would lead to access
>>> to valid mapping.
>>>
>>> It would be possible to relocate Xen to avoid clashing with address 0.
>>> However the identity mapping is only meant to be used in very limited
>>> places. Therefore it would be better to keep the identity region invalid
>>> for most of the time.
>>>
>>> Two new helpers are introduced:
>>>- prepare_identity_mapping() will setup the page-tables so it is
>>> easy to create the mapping afterwards.
>>>- update_identity_mapping() will create/remove the identity mapping
>> Nit: Would be better to first say what the patch is doing and then explaining
>> the NULL pointer possible issue.
> The NULL pointer is part of the problem statement. IOW, I would not have
> introduced update_identity_mapping() if we were not concerned that the
> mapping start 0.
>
> So I don't think the commit message would read the same.
Somehow reading your commit message, the link between the first 2 paragraph and
the helpers introduced is not quite clear.
The NULL pointer problem is a consequence of the usage of an identity mapping
and maybe this explanation should be more in documentation or in a code comment
around this functionality.
>
>>> +/*
>>> + * The identity mapping may start at physical address 0. So don't want
>>> + * to keep it mapped longer than necessary.
>>> + *
>>> + * When this is called, we are still using the boot_pgtable.
>>> + *
>>> + * XXX: Handle Arm32 properly.
>>> + */
>>> +static void prepare_identity_mapping(void)
>>> +{
>>> +paddr_t id_addr = virt_to_maddr(_start);
>>> +lpae_t pte;
>>> +DECLARE_OFFSETS(id_offsets, id_addr);
>>> +
>>> +printk("id_addr 0x%lx\n", id_addr);
>> Debug print that should be removed.
>
> Will do. Note the "early-RFC" in the comment. I am not looking for a detailed
> review (I didn't spend too much time cleaning up) but a feedback on the
> approach.
I did read the code and it is easy to forget to remove those, so I just pointed
it :-)
>
>>> +#ifdef CONFIG_ARM_64
>>> +if ( id_offsets[0] != 0 )
>>> +panic("Cannot handled ID mapping above 512GB\n");
>> The error message here might not be really helpful for the user.
>> How about saying that Xen cannot be loaded in memory with
>> a physical address above 512GB ?
>
> Sure.
>
>>> +#endif
>>> +
>>> +/* Link first ID table */
>>> +pte = pte_of_xenaddr((vaddr_t)xen_first_id);
>>> +pte.pt.table = 1;
>>> +pte.pt.xn = 0;
>>> +
>>> +write_pte(_pgtable[id_offsets[0]], pte);
>>> +
>>> +/* Link second ID table */
>>> +pte = pte_of_xenaddr((vaddr_t)xen_second_id);
>>> +pte.pt.table = 1;
>>> +pte.pt.xn = 0;
>>> +
>>> +write_pte(_first_id[id_offsets[1]], pte);
>>> +
>>> +/* Link third ID table */
>>> +pte = pte_of_xenaddr((vaddr_t)xen_third_id);
>>> +pte.pt.table = 1;
>>> +pte.pt.xn = 0;
>>> +
>>> +write_pte(_second_id[id_offsets[2]], pte);
>>> +
>>> +/* The mapping in the third table will be created at a later stage */
>>> +
>>> +/*
>>> + * Link the identity mapping in the runtime Xen page tables. No need to
>>> + * use write_pte here as they are not live yet.
>>> + */
>>> +xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
>>> +}
>>> +
>>> +void update_identity_mapping(bool enable)
>> You probably want an __init attribute here.
> I expect this helper to be necessary after boot (e.g. CPU bring-up,
> suspend/resume). So I decided to keep it without _init.
Ok
>
>>> +{
>>> +paddr_t id_addr = virt_to_maddr(_start);
>>> +int rc;
>>> +
>>> +if ( enable )
>>> +rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
>>> + PAGE_HYPERVISOR_RX);
>>> +else
>>> +rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
>>> +
>>> +BUG_ON(rc);
>>> +}
>>> +
>>> /*
>>> * After boot, Xen page-tables should not contain mapping that are both
>>> * Writable and eXecutables.
>>> @@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long
>>> boot_phys_offset)
>>>
>>> phys_offset = boot_phys_offset;
>>>
>>> +/* XXX: Find a better place to call it */
>> Why do you think this place is not right ?
> Because the use in setup_pagetables() will soon disappear (my plan it to
> completely remove setup_pagetables) and this will used in other subsystem
> (CPU bring-up, suspend/resume).
Ok
Cheers
Bertrand
>
> Cheers,
>
> --
> Julien Grall
Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules
On 25.03.2022 13:57, Bertrand Marquis wrote: >> On 25 Mar 2022, at 12:43, Jan Beulich wrote: >> On 24.03.2022 12:04, Bertrand Marquis wrote: >>> --- a/xen/include/xen/kconfig.h >>> +++ b/xen/include/xen/kconfig.h >>> @@ -8,6 +8,10 @@ >>> * these only work with boolean option. >>> */ >>> >>> +/* cppcheck is failing to parse the macro so use a dummy one */ >>> +#ifdef CPPCHECK >>> +#define IS_ENABLED(option) option >>> +#else >>> /* >>> * Getting something that works in C and CPP for an arg that may or may >>> * not be defined is tricky. Here, if we have "#define CONFIG_BOOGER 1" >>> @@ -27,5 +31,6 @@ >>> * otherwise. >>> */ >>> #define IS_ENABLED(option) config_enabled(option) >>> +#endif >> >> What are the consequences of this workaround on the code actually >> being checked? Does this perhaps lead to certain portions of code >> being skipped while checking? > > Cppcheck is not optimising the code but looking at the syntax so the > consequence here is that cppcheck is checking some code which might > be optimised out by the compiler. This is not optimal but still it should > analyse properly the code. Aren't you saying so merely because all uses of IS_ENABLED() in our sources so far are in if()? It would seem to me that when used in #if (as can be found in Linux, which hence means could be the case in our tree as well sooner or later) sections of code might either be skipped or syntax errors may result. Or wait - IS_ENABLED() does itself kind of rely on the respective CONFIG_* to expand to a numeric value; when expanding to a string, I guess the compiler would also warn about the resulting construct when used in if() (and reject any uses with #if). Jan
Re: [PATCH 1/2] livepatch: do not ignore sections with 0 size
Ping?
There was some discussion on whether we need to handle such empty
sections, but I think we settled that it's necessary.
Thanks, Roger.
On Thu, Mar 17, 2022 at 12:08:53PM +0100, Roger Pau Monne wrote:
> A side effect of ignoring such sections is that symbols belonging to
> them won't be resolved, and that could make relocations belonging to
> other sections that reference those symbols fail.
>
> For example it's likely to have an empty .altinstr_replacement with
> symbols pointing to it, and marking the section as ignored will
> prevent the symbols from being resolved, which in turn will cause any
> relocations against them to fail.
>
> In order to solve this do not ignore sections with 0 size, only ignore
> sections that don't have the SHF_ALLOC flag set.
>
> Special case such empty sections in move_payload so they are not taken
> into account in order to decide whether a livepatch can be safely
> re-applied after a revert.
>
> Fixes: 98b728a7b2 ('livepatch: Disallow applying after an revert')
> Signed-off-by: Roger Pau Monné
> ---
> xen/common/livepatch.c | 16 +++-
> xen/include/xen/livepatch_elf.h | 2 +-
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index be2cf75c2d..abc1cae136 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -300,9 +300,6 @@ static int move_payload(struct payload *payload, struct
> livepatch_elf *elf)
> * and .shstrtab. For the non-relocate we allocate and copy these
> * via other means - and the .rel we can ignore as we only use it
> * once during loading.
> - *
> - * Also ignore sections with zero size. Those can be for example:
> - * data, or .bss.
> */
> if ( livepatch_elf_ignore_section(elf->sec[i].sec) )
> offset[i] = UINT_MAX;
> @@ -361,8 +358,17 @@ static int move_payload(struct payload *payload, struct
> livepatch_elf *elf)
> else if ( elf->sec[i].sec->sh_flags & SHF_WRITE )
> {
> buf = rw_buf;
> -rw_buf_sec = i;
> -rw_buf_cnt++;
> +if ( elf->sec[i].sec->sh_size )
> +{
> +/*
> + * Special handling of RW empty regions: do not account
> for
> + * them in order to decide whether a patch can safely be
> + * re-applied, but assign them a load address so symbol
> + * resolution and relocations work.
> + */
> +rw_buf_sec = i;
> +rw_buf_cnt++;
> +}
> }
> else
> buf = ro_buf;
> diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
> index 9ad499ee8b..5b1ec469da 100644
> --- a/xen/include/xen/livepatch_elf.h
> +++ b/xen/include/xen/livepatch_elf.h
> @@ -48,7 +48,7 @@ int livepatch_elf_perform_relocs(struct livepatch_elf *elf);
>
> static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
> {
> -return !(sec->sh_flags & SHF_ALLOC) || sec->sh_size == 0;
> +return !(sec->sh_flags & SHF_ALLOC);
> }
> #endif /* __XEN_LIVEPATCH_ELF_H__ */
>
> --
> 2.34.1
>
Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout
Hi Julien, > On 25 Mar 2022, at 14:35, Julien Grall wrote: > > > > On 25/03/2022 13:17, Bertrand Marquis wrote: >> Hi Julien, > > Hi, > >>> On 9 Mar 2022, at 12:20, Julien Grall wrote: >>> >>> From: Julien Grall >>> >>> Xen is currently not fully compliant with the Arm because it will >> I think you wanted to say “arm arm” her. > > Yes. I will update it. > >>> switch the TTBR with the MMU on. >>> >>> In order to be compliant, we need to disable the MMU before >>> switching the TTBR. The implication is the page-tables should >>> contain an identity mapping of the code switching the TTBR. >>> >>> If we don't rework the memory layout, we would need to find a >>> virtual address that matches a physical address and doesn't clash >>> with the static virtual regions. This can be a bit tricky. >> This sentence is a bit misleading. Even with the rework you need >> to do that just by moving the Xen virtual address upper you make >> sure that anything physical memory under 512GB can be mapped >> 1:1 without clashing with other Xen mappings (unless Xen is loaded >> in memory at physical address 512GB which would end in the same issue). > > So the key difference is with the rework, it is trivial to create the 1:1 > mapping as we know it doesn't clash. This is not the case without the rework. Agree > >> I think should be rephrased. > > I am not entirely sure how to rephrase it. Do you have a proposal? Turn it into the positive: Rework the memory layout to put Xen over 512GB. This makes it trivial to create a 1:1 mapping, with the assumption that the physical memory is under 512GB. Something in this area, telling what we do and not what we don't > >>> >>> On arm64, the memory layout has plenty of unused space. In most of >>> the case we expect Xen to be loaded in low memory. >>> >>> The memory layout is reshuffled to keep the 0th slot free. Xen will now >> 0th slot of first level of page table. > > We want to keep the first 512GB free. So did you intend to write "zero level"? Yes sorry. > >>> be loaded at (512GB + 2MB). This requires a slight tweak of the boot >>> code as XEN_VIRT_START cannot be used as an immediate. >>> >>> Signed-off-by: Julien Grall >>> >>> --- >>> >>>TODO: >>>- I vaguely recall that one of the early platform we supported add >>> the memory starting in high memory (> 1TB). I need to check >>> whether the new layout will be fine. >> I think we have some Juno with some memory like that, tell me if you need >> help here. > > Would you be able to check the memory layout and confirm? I checked and the Juno we have as the high memory a lot lower than that: RAM: 00088000 - 0009 No idea why it was a lot higher in my mind. > >>>- Update the documentation to reflect the new layout >>> --- >>> xen/arch/arm/arm64/head.S | 3 ++- >>> xen/arch/arm/include/asm/config.h | 20 ++-- >>> xen/arch/arm/mm.c | 14 +++--- >>> 3 files changed, 23 insertions(+), 14 deletions(-) >>> >>> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S >>> index 66d862fc8137..878649280d73 100644 >>> --- a/xen/arch/arm/arm64/head.S >>> +++ b/xen/arch/arm/arm64/head.S >>> @@ -594,7 +594,8 @@ create_page_tables: >>> * need an additional 1:1 mapping, the virtual mapping will >>> * suffice. >>> */ >>> -cmp x19, #XEN_VIRT_START >>> +ldr x0, =XEN_VIRT_START >>> +cmp x19, x0 >> A comment in the code would be good here to prevent someone reverting this. > > Anyone trying to revert the change will face a compilation error: > > CC arch/arm/arm64/head.o > arch/arm/arm64/head.S: Assembler messages: > arch/arm/arm64/head.S:597: Error: immediate out of range > > So I don't think a comment is necessary because this is not specific to a > compiler/assembler. Right I should have thought of the compilation error. >>> -#define SLOT0_ENTRY_BITS 39 >>> -#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS) >>> -#define SLOT0_ENTRY_SIZE SLOT0(1) >>> - >>> -#define VMAP_VIRT_START GB(1) >>> +#define VMAP_VIRT_START (SLOT0(1) + GB(1)) >>> #define VMAP_VIRT_END(VMAP_VIRT_START + GB(1)) >>> >>> -#define FRAMETABLE_VIRT_START GB(32) >>> +#define FRAMETABLE_VIRT_START (SLOT0(1) + GB(32)) >>> #define FRAMETABLE_SIZEGB(32) >>> #define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) >>> #define FRAMETABLE_VIRT_END(FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1) >>> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c >>> index 6b7c41d827ca..75ed9a3ce249 100644 >>> --- a/xen/arch/arm/mm.c >>> +++ b/xen/arch/arm/mm.c >>> @@ -187,11 +187,10 @@ static void __init __maybe_unused >>> build_assertions(void) >>> BUILD_BUG_ON(DIRECTMAP_VIRT_START & ~FIRST_MASK); >>> #endif >>> /* Page table structure constraints */ >>> -#ifdef CONFIG_ARM_64 >>> -BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START)); >>>
[ovmf test] 168844: regressions - FAIL
flight 168844 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/168844/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-amd64 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 69218d5d2854acaa7a11c777244de4a297d2fbb9 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 25 days Failing since168258 2022-03-01 01:55:31 Z 24 days 252 attempts Testing same since 168832 2022-03-25 01:43:21 Z0 days4 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Bandaru, Purna Chandra Rao Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Patrick Rudolph Purna Chandra Rao Bandaru Sami Mujawar Sean Rhodes Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 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 Not pushing. (No revision log; it would be 904 lines long.)
Re: [PATCH 3/3] x86/mem_sharing: make fork_reset more configurable
On Fri, Mar 25, 2022 at 9:42 AM Roger Pau Monné wrote:
>
> On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote:
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index a21c781452..bfa6082f13 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1892,15 +1892,19 @@ static int fork(struct domain *cd, struct domain
> > *d, uint16_t flags)
> > * footprints the hypercall continuation should be implemented (or if this
> > * feature needs to be become "stable").
> > */
> > -static int mem_sharing_fork_reset(struct domain *d)
> > +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> > + bool reset_memory)
> > {
> > -int rc;
> > +int rc = 0;
> > struct domain *pd = d->parent;
> > struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > struct page_info *page, *tmp;
> >
> > domain_pause(d);
>
> I would assert that at least one of reset_sate or reset_memory is set
> here, as callers already do the checks.
Sure.
>
> >
> > +if ( !reset_memory )
> > +goto state;
>
> I don't like using labels and goto like this as I think it makes the
> code harder to follow, and so more likely to introduce bugs. I would
> rather place the memory reset parts inside of an if ( reset_memory ) {
> ... }, but that's my taste.
I did that first but because it requires shifting everything to the
right it requires odd line breaks.
>
> > +
> > /* need recursive lock because we will free pages */
> > spin_lock_recursive(>page_alloc_lock);
> > page_list_for_each_safe(page, tmp, >page_list)
> > @@ -1933,7 +1937,9 @@ static int mem_sharing_fork_reset(struct domain *d)
> > }
> > spin_unlock_recursive(>page_alloc_lock);
> >
> > -rc = copy_settings(d, pd, d->arch.hvm.mem_sharing.skip_special_pages);
> > + state:
> > +if ( reset_state )
> > +rc = copy_settings(d, pd,
> > d->arch.hvm.mem_sharing.skip_special_pages);
> >
> > domain_unpause(d);
> >
> > @@ -2239,15 +2245,21 @@ int
> > mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
> >
> > case XENMEM_sharing_op_fork_reset:
> > {
> > +bool reset_state = mso.u.fork.flags & XENMEM_FORK_RESET_STATE;
> > +bool reset_memory = mso.u.fork.flags & XENMEM_FORK_RESET_MEMORY;
> > +
> > rc = -EINVAL;
> > -if ( mso.u.fork.pad || mso.u.fork.flags )
> > +if ( mso.u.fork.pad || (!reset_state && !reset_memory) )
> > +goto out;
> > +if ( mso.u.fork.flags &
> > + ~(XENMEM_FORK_RESET_STATE | XENMEM_FORK_RESET_MEMORY) )
> > goto out;
> >
> > rc = -ENOSYS;
> > if ( !d->parent )
> > goto out;
> >
> > -rc = mem_sharing_fork_reset(d);
> > +rc = mem_sharing_fork_reset(d, reset_state, reset_memory);
> > break;
> > }
> >
> > diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> > index 84cf52636b..a7b192be0d 100644
> > --- a/xen/common/vm_event.c
> > +++ b/xen/common/vm_event.c
> > @@ -28,6 +28,11 @@
> > #include
> > #include
> > #include
> > +
> > +#ifdef CONFIG_MEM_SHARING
> > +#include
> > +#endif
> > +
> > #include
> > #include
> >
> > @@ -394,6 +399,15 @@ static int vm_event_resume(struct domain *d, struct
> > vm_event_domain *ved)
> > if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
> > p2m_mem_paging_resume(d, );
> > #endif
> > +#ifdef CONFIG_MEM_SHARING
> > +do {
> > +bool reset_state = rsp.flags &
> > VM_EVENT_FLAG_RESET_FORK_STATE;
> > +bool reset_mem = rsp.flags &
> > VM_EVENT_FLAG_RESET_FORK_MEMORY;
> > +
> > +if ( reset_state || reset_mem )
> > +mem_sharing_fork_reset(d, reset_state, reset_mem);
>
> You seem to drop the error code returned by mem_sharing_fork_reset.
Yes, there is no response that could be sent to the toolstack from
here. I could add an ASSERT that rc is 0 though. If the fork() op
successfully finished then fork_reset() couldn't reasonably end up in
a path where it fails.
>
> > +} while(0);
> > +#endif
>
> I think you can avoid the do {} while(0); just using the braces will
> allow you to define local variables in the inner block.
Sure.
>
> > /*
> > * Check emulation flags in the arch-specific handler only, as
> > it
> > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > index 208d8dcbd9..30ce23c5a7 100644
> > --- a/xen/include/public/memory.h
> > +++ b/xen/include/public/memory.h
> > @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
> > uint32_t gref; /* IN: gref to debug */
> > } u;
> > } debug;
> > -struct mem_sharing_op_fork { /* OP_FORK */
> > +struct mem_sharing_op_fork { /* OP_FORK/_RESET */
> > domid_t parent_domain;/*
Re: [PATCH early-RFC 4/5] xen/arm: mm: Rework switch_ttbr()
Hi Julien,
> On 9 Mar 2022, at 12:20, Julien Grall wrote:
>
> From: Julien Grall
>
> At the moment, switch_ttbr() is switching the TTBR whilst the MMU is
> still on.
>
> Switching TTBR is like replacing existing mappings with new ones. So
> we need to follow the break-before-make sequence.
>
> In this case, it means the MMU needs to be switched off while the
> TTBR is updated. In order to disable the MMU, we need to first
> jump to an identity mapping.
>
> Rename switch_ttbr() to switch_ttbr_id() and create an helper on
> top to temporary map the identity mapping and call switch_ttbr()
> via the identity address.
>
> switch_ttbr_id() is now reworked to temporarily turn off the MMU
> before updating the TTBR.
>
> We also need to make sure the helper switch_ttbr() is part of the
> identity mapping. So move _end_boot past it.
>
> Take the opportunity to instruction cache flush as the operation is
> only necessary when the memory is updated.
Your code is actually remove the instruction cache invalidation so
this sentence is a bit misleading.
Also an open question: shouldn’t we flush the data cache ?
As we switch from one TTBR to an other, there might be some data
in the cache dependent that could be flushed while the MMU is off or
that would have no mapping once it is reactivated.
>
> Signed-off-by: Julien Grall
>
> ---
>
>TODO:
>* Rename _end_boot to _end_id_mapping or similar
>* Check the memory barriers
>* I suspect the instruction cache flush will be necessary
> for cache coloring.
> ---
> xen/arch/arm/arm64/head.S | 31 ---
> xen/arch/arm/mm.c | 14 +-
> 2 files changed, 33 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S
> index 878649280d73..c5cc72b8fe6f 100644
> --- a/xen/arch/arm/arm64/head.S
> +++ b/xen/arch/arm/arm64/head.S
> @@ -803,36 +803,45 @@ fail: PRINT("- Boot failed -\r\n")
> b 1b
> ENDPROC(fail)
>
> -GLOBAL(_end_boot)
> -
> /*
> * Switch TTBR
> *
> * x0ttbr
> *
> - * TODO: This code does not comply with break-before-make.
> + * XXX: Check the barriers
> */
> -ENTRY(switch_ttbr)
> +ENTRY(switch_ttbr_id)
> dsb sy /* Ensure the flushes happen before
> * continuing */
> isb /* Ensure synchronization with previous
> * changes to text */
> +
> +/* Turn off MMU */
> +mrsx1, SCTLR_EL2
> +bicx1, x1, #SCTLR_Axx_ELx_M
> +msrSCTLR_EL2, x1
> +dsbsy
> +isb
> +
> tlbi alle2 /* Flush hypervisor TLB */
> -ic iallu /* Flush I-cache */
> dsbsy/* Ensure completion of TLB flush */
> isb
>
> -msrTTBR0_EL2, x0
> +msr TTBR0_EL2, x0
> +
> +mrs x1, SCTLR_EL2
> +orr x1, x1, #SCTLR_Axx_ELx_M /* Enable MMU */
> +msr SCTLR_EL2, x1
>
> isb /* Ensure synchronization with previous
> * changes to text */
> -tlbi alle2 /* Flush hypervisor TLB */
> -ic iallu /* Flush I-cache */
> -dsbsy/* Ensure completion of TLB flush */
> -isb
> +/* Turn on the MMU */
> +
>
> ret
> -ENDPROC(switch_ttbr)
> +ENDPROC(switch_ttbr_id)
> +
> +GLOBAL(_end_boot)
>
> #ifdef CONFIG_EARLY_PRINTK
> /*
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 5c4dece16f7f..a53760af7af0 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -660,7 +660,19 @@ static void xen_pt_enforce_wnx(void)
> flush_xen_tlb_local();
> }
>
> -extern void switch_ttbr(uint64_t ttbr);
> +extern void switch_ttbr_id(uint64_t ttbr);
> +
> +typedef void (switch_ttbr_fn)(uint64_t ttbr);
> +
> +static void switch_ttbr(uint64_t ttbr)
> +{
> +vaddr_t id_addr = virt_to_maddr(switch_ttbr_id);
> +switch_ttbr_fn *fn = (switch_ttbr_fn *)id_addr;
> +
> +update_identity_mapping(true);
> +fn(ttbr);
> +update_identity_mapping(false);
> +}
>
> /* Clear a translation table and clean & invalidate the cache */
> static void clear_table(void *table)
> --
> 2.32.0
>
Cheers
Bertrand
Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping
On 25/03/2022 13:32, Bertrand Marquis wrote:
Hi Julien,
Hi,
On 9 Mar 2022, at 12:20, Julien Grall wrote:
From: Julien GralL
In follow-up patches we will need to have part of Xen identity mapped in
order to safely switch the TTBR.
On some platform, the identity mapping may have to start at 0. If we always
keep the identity region mapped, NULL pointer ference would lead to access
to valid mapping.
It would be possible to relocate Xen to avoid clashing with address 0.
However the identity mapping is only meant to be used in very limited
places. Therefore it would be better to keep the identity region invalid
for most of the time.
Two new helpers are introduced:
- prepare_identity_mapping() will setup the page-tables so it is
easy to create the mapping afterwards.
- update_identity_mapping() will create/remove the identity mapping
Nit: Would be better to first say what the patch is doing and then explaining
the NULL pointer possible issue.
The NULL pointer is part of the problem statement. IOW, I would not have
introduced update_identity_mapping() if we were not concerned that the
mapping start 0.
So I don't think the commit message would read the same.
+/*
+ * The identity mapping may start at physical address 0. So don't want
+ * to keep it mapped longer than necessary.
+ *
+ * When this is called, we are still using the boot_pgtable.
+ *
+ * XXX: Handle Arm32 properly.
+ */
+static void prepare_identity_mapping(void)
+{
+paddr_t id_addr = virt_to_maddr(_start);
+lpae_t pte;
+DECLARE_OFFSETS(id_offsets, id_addr);
+
+printk("id_addr 0x%lx\n", id_addr);
Debug print that should be removed.
Will do. Note the "early-RFC" in the comment. I am not looking for a
detailed review (I didn't spend too much time cleaning up) but a
feedback on the approach.
+#ifdef CONFIG_ARM_64
+if ( id_offsets[0] != 0 )
+panic("Cannot handled ID mapping above 512GB\n");
The error message here might not be really helpful for the user.
How about saying that Xen cannot be loaded in memory with
a physical address above 512GB ?
Sure.
+#endif
+
+/* Link first ID table */
+pte = pte_of_xenaddr((vaddr_t)xen_first_id);
+pte.pt.table = 1;
+pte.pt.xn = 0;
+
+write_pte(_pgtable[id_offsets[0]], pte);
+
+/* Link second ID table */
+pte = pte_of_xenaddr((vaddr_t)xen_second_id);
+pte.pt.table = 1;
+pte.pt.xn = 0;
+
+write_pte(_first_id[id_offsets[1]], pte);
+
+/* Link third ID table */
+pte = pte_of_xenaddr((vaddr_t)xen_third_id);
+pte.pt.table = 1;
+pte.pt.xn = 0;
+
+write_pte(_second_id[id_offsets[2]], pte);
+
+/* The mapping in the third table will be created at a later stage */
+
+/*
+ * Link the identity mapping in the runtime Xen page tables. No need to
+ * use write_pte here as they are not live yet.
+ */
+xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
+}
+
+void update_identity_mapping(bool enable)
You probably want an __init attribute here.
I expect this helper to be necessary after boot (e.g. CPU bring-up,
suspend/resume). So I decided to keep it without _init.
+{
+paddr_t id_addr = virt_to_maddr(_start);
+int rc;
+
+if ( enable )
+rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
+ PAGE_HYPERVISOR_RX);
+else
+rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
+
+BUG_ON(rc);
+}
+
/*
* After boot, Xen page-tables should not contain mapping that are both
* Writable and eXecutables.
@@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long boot_phys_offset)
phys_offset = boot_phys_offset;
+/* XXX: Find a better place to call it */
Why do you think this place is not right ?
Because the use in setup_pagetables() will soon disappear (my plan it to
completely remove setup_pagetables) and this will used in other
subsystem (CPU bring-up, suspend/resume).
Cheers,
--
Julien Grall
Re: [PATCH 3/3] x86/mem_sharing: make fork_reset more configurable
On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index a21c781452..bfa6082f13 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1892,15 +1892,19 @@ static int fork(struct domain *cd, struct domain *d,
> uint16_t flags)
> * footprints the hypercall continuation should be implemented (or if this
> * feature needs to be become "stable").
> */
> -static int mem_sharing_fork_reset(struct domain *d)
> +int mem_sharing_fork_reset(struct domain *d, bool reset_state,
> + bool reset_memory)
> {
> -int rc;
> +int rc = 0;
> struct domain *pd = d->parent;
> struct p2m_domain *p2m = p2m_get_hostp2m(d);
> struct page_info *page, *tmp;
>
> domain_pause(d);
I would assert that at least one of reset_sate or reset_memory is set
here, as callers already do the checks.
>
> +if ( !reset_memory )
> +goto state;
I don't like using labels and goto like this as I think it makes the
code harder to follow, and so more likely to introduce bugs. I would
rather place the memory reset parts inside of an if ( reset_memory ) {
... }, but that's my taste.
> +
> /* need recursive lock because we will free pages */
> spin_lock_recursive(>page_alloc_lock);
> page_list_for_each_safe(page, tmp, >page_list)
> @@ -1933,7 +1937,9 @@ static int mem_sharing_fork_reset(struct domain *d)
> }
> spin_unlock_recursive(>page_alloc_lock);
>
> -rc = copy_settings(d, pd, d->arch.hvm.mem_sharing.skip_special_pages);
> + state:
> +if ( reset_state )
> +rc = copy_settings(d, pd,
> d->arch.hvm.mem_sharing.skip_special_pages);
>
> domain_unpause(d);
>
> @@ -2239,15 +2245,21 @@ int
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>
> case XENMEM_sharing_op_fork_reset:
> {
> +bool reset_state = mso.u.fork.flags & XENMEM_FORK_RESET_STATE;
> +bool reset_memory = mso.u.fork.flags & XENMEM_FORK_RESET_MEMORY;
> +
> rc = -EINVAL;
> -if ( mso.u.fork.pad || mso.u.fork.flags )
> +if ( mso.u.fork.pad || (!reset_state && !reset_memory) )
> +goto out;
> +if ( mso.u.fork.flags &
> + ~(XENMEM_FORK_RESET_STATE | XENMEM_FORK_RESET_MEMORY) )
> goto out;
>
> rc = -ENOSYS;
> if ( !d->parent )
> goto out;
>
> -rc = mem_sharing_fork_reset(d);
> +rc = mem_sharing_fork_reset(d, reset_state, reset_memory);
> break;
> }
>
> diff --git a/xen/common/vm_event.c b/xen/common/vm_event.c
> index 84cf52636b..a7b192be0d 100644
> --- a/xen/common/vm_event.c
> +++ b/xen/common/vm_event.c
> @@ -28,6 +28,11 @@
> #include
> #include
> #include
> +
> +#ifdef CONFIG_MEM_SHARING
> +#include
> +#endif
> +
> #include
> #include
>
> @@ -394,6 +399,15 @@ static int vm_event_resume(struct domain *d, struct
> vm_event_domain *ved)
> if ( rsp.reason == VM_EVENT_REASON_MEM_PAGING )
> p2m_mem_paging_resume(d, );
> #endif
> +#ifdef CONFIG_MEM_SHARING
> +do {
> +bool reset_state = rsp.flags &
> VM_EVENT_FLAG_RESET_FORK_STATE;
> +bool reset_mem = rsp.flags & VM_EVENT_FLAG_RESET_FORK_MEMORY;
> +
> +if ( reset_state || reset_mem )
> +mem_sharing_fork_reset(d, reset_state, reset_mem);
You seem to drop the error code returned by mem_sharing_fork_reset.
> +} while(0);
> +#endif
I think you can avoid the do {} while(0); just using the braces will
allow you to define local variables in the inner block.
> /*
> * Check emulation flags in the arch-specific handler only, as it
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 208d8dcbd9..30ce23c5a7 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
> uint32_t gref; /* IN: gref to debug */
> } u;
> } debug;
> -struct mem_sharing_op_fork { /* OP_FORK */
> +struct mem_sharing_op_fork { /* OP_FORK/_RESET */
> domid_t parent_domain;/* IN: parent's domain id */
> /* These flags only makes sense for short-lived forks */
> #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
> #define XENMEM_FORK_BLOCK_INTERRUPTS (1u << 1)
> #define XENMEM_FORK_SKIP_SPECIAL_PAGES (1u << 2)
> +#define XENMEM_FORK_RESET_STATE(1u << 3)
> +#define XENMEM_FORK_RESET_MEMORY (1u << 4)
For backward compatibility purposes should the flags be added
backwards, ie:
#define XENMEM_FORK_KEEP_STATE(1u << 3)
#define XENMEM_FORK_KEEP_MEMORY (1u << 4)
So that existing callers of XENMEM_sharing_op_fork_reset will continue
working as expected?
Or we don't care about that
Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout
On 25/03/2022 13:17, Bertrand Marquis wrote: Hi Julien, Hi, On 9 Mar 2022, at 12:20, Julien Grall wrote: From: Julien Grall Xen is currently not fully compliant with the Arm because it will I think you wanted to say “arm arm” her. Yes. I will update it. switch the TTBR with the MMU on. In order to be compliant, we need to disable the MMU before switching the TTBR. The implication is the page-tables should contain an identity mapping of the code switching the TTBR. If we don't rework the memory layout, we would need to find a virtual address that matches a physical address and doesn't clash with the static virtual regions. This can be a bit tricky. This sentence is a bit misleading. Even with the rework you need to do that just by moving the Xen virtual address upper you make sure that anything physical memory under 512GB can be mapped 1:1 without clashing with other Xen mappings (unless Xen is loaded in memory at physical address 512GB which would end in the same issue). So the key difference is with the rework, it is trivial to create the 1:1 mapping as we know it doesn't clash. This is not the case without the rework. I think should be rephrased. I am not entirely sure how to rephrase it. Do you have a proposal? On arm64, the memory layout has plenty of unused space. In most of the case we expect Xen to be loaded in low memory. The memory layout is reshuffled to keep the 0th slot free. Xen will now 0th slot of first level of page table. We want to keep the first 512GB free. So did you intend to write "zero level"? be loaded at (512GB + 2MB). This requires a slight tweak of the boot code as XEN_VIRT_START cannot be used as an immediate. Signed-off-by: Julien Grall --- TODO: - I vaguely recall that one of the early platform we supported add the memory starting in high memory (> 1TB). I need to check whether the new layout will be fine. I think we have some Juno with some memory like that, tell me if you need help here. Would you be able to check the memory layout and confirm? - Update the documentation to reflect the new layout --- xen/arch/arm/arm64/head.S | 3 ++- xen/arch/arm/include/asm/config.h | 20 ++-- xen/arch/arm/mm.c | 14 +++--- 3 files changed, 23 insertions(+), 14 deletions(-) diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S index 66d862fc8137..878649280d73 100644 --- a/xen/arch/arm/arm64/head.S +++ b/xen/arch/arm/arm64/head.S @@ -594,7 +594,8 @@ create_page_tables: * need an additional 1:1 mapping, the virtual mapping will * suffice. */ -cmp x19, #XEN_VIRT_START +ldr x0, =XEN_VIRT_START +cmp x19, x0 A comment in the code would be good here to prevent someone reverting this. Anyone trying to revert the change will face a compilation error: CC arch/arm/arm64/head.o arch/arm/arm64/head.S: Assembler messages: arch/arm/arm64/head.S:597: Error: immediate out of range So I don't think a comment is necessary because this is not specific to a compiler/assembler. -#define SLOT0_ENTRY_BITS 39 -#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS) -#define SLOT0_ENTRY_SIZE SLOT0(1) - -#define VMAP_VIRT_START GB(1) +#define VMAP_VIRT_START (SLOT0(1) + GB(1)) #define VMAP_VIRT_END(VMAP_VIRT_START + GB(1)) -#define FRAMETABLE_VIRT_START GB(32) +#define FRAMETABLE_VIRT_START (SLOT0(1) + GB(32)) #define FRAMETABLE_SIZEGB(32) #define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) #define FRAMETABLE_VIRT_END(FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1) diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c index 6b7c41d827ca..75ed9a3ce249 100644 --- a/xen/arch/arm/mm.c +++ b/xen/arch/arm/mm.c @@ -187,11 +187,10 @@ static void __init __maybe_unused build_assertions(void) BUILD_BUG_ON(DIRECTMAP_VIRT_START & ~FIRST_MASK); #endif /* Page table structure constraints */ -#ifdef CONFIG_ARM_64 -BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START)); -#endif Don’t you want to enforce the opposite now ? Check that it is not on slot 0 ? I can. But this is not going to guarantee us the first 512GB is going to be free. Cheers, -- Julien Grall
Re: [PATCH early-RFC 3/5] xen/arm: mm: Introduce helpers to prepare/enable/disable the identity mapping
Hi Julien,
> On 9 Mar 2022, at 12:20, Julien Grall wrote:
>
> From: Julien GralL
>
> In follow-up patches we will need to have part of Xen identity mapped in
> order to safely switch the TTBR.
>
> On some platform, the identity mapping may have to start at 0. If we always
> keep the identity region mapped, NULL pointer ference would lead to access
> to valid mapping.
>
> It would be possible to relocate Xen to avoid clashing with address 0.
> However the identity mapping is only meant to be used in very limited
> places. Therefore it would be better to keep the identity region invalid
> for most of the time.
>
> Two new helpers are introduced:
>- prepare_identity_mapping() will setup the page-tables so it is
> easy to create the mapping afterwards.
>- update_identity_mapping() will create/remove the identity mapping
Nit: Would be better to first say what the patch is doing and then explaining
the NULL pointer possible issue.
>
> Signed-off-by: Julien Grall
> ---
> xen/arch/arm/include/asm/mm.h | 2 +
> xen/arch/arm/mm.c | 73 +++
> 2 files changed, 75 insertions(+)
>
> diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h
> index 045a8ba4bb63..76973ea9a0ff 100644
> --- a/xen/arch/arm/include/asm/mm.h
> +++ b/xen/arch/arm/include/asm/mm.h
> @@ -177,6 +177,8 @@ extern unsigned long total_pages;
>
> /* Boot-time pagetable setup */
> extern void setup_pagetables(unsigned long boot_phys_offset);
> +/* Enable/disable the identity mapping */
> +extern void update_identity_mapping(bool enable);
> /* Map FDT in boot pagetable */
> extern void *early_fdt_map(paddr_t fdt_paddr);
> /* Remove early mappings */
> diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
> index 75ed9a3ce249..5c4dece16f7f 100644
> --- a/xen/arch/arm/mm.c
> +++ b/xen/arch/arm/mm.c
> @@ -138,6 +138,12 @@ static DEFINE_PAGE_TABLE(cpu0_pgtable);
> static DEFINE_PAGE_TABLES(cpu0_dommap, DOMHEAP_SECOND_PAGES);
> #endif
>
> +#ifdef CONFIG_ARM_64
> +static DEFINE_PAGE_TABLE(xen_first_id);
> +static DEFINE_PAGE_TABLE(xen_second_id);
> +static DEFINE_PAGE_TABLE(xen_third_id);
> +#endif
> +
> /* Common pagetable leaves */
> /* Second level page tables.
> *
> @@ -573,6 +579,70 @@ void __init remove_early_mappings(void)
> BUG_ON(rc);
> }
>
> +/*
> + * The identity mapping may start at physical address 0. So don't want
> + * to keep it mapped longer than necessary.
> + *
> + * When this is called, we are still using the boot_pgtable.
> + *
> + * XXX: Handle Arm32 properly.
> + */
> +static void prepare_identity_mapping(void)
> +{
> +paddr_t id_addr = virt_to_maddr(_start);
> +lpae_t pte;
> +DECLARE_OFFSETS(id_offsets, id_addr);
> +
> +printk("id_addr 0x%lx\n", id_addr);
Debug print that should be removed.
> +#ifdef CONFIG_ARM_64
> +if ( id_offsets[0] != 0 )
> +panic("Cannot handled ID mapping above 512GB\n");
The error message here might not be really helpful for the user.
How about saying that Xen cannot be loaded in memory with
a physical address above 512GB ?
> +#endif
> +
> +/* Link first ID table */
> +pte = pte_of_xenaddr((vaddr_t)xen_first_id);
> +pte.pt.table = 1;
> +pte.pt.xn = 0;
> +
> +write_pte(_pgtable[id_offsets[0]], pte);
> +
> +/* Link second ID table */
> +pte = pte_of_xenaddr((vaddr_t)xen_second_id);
> +pte.pt.table = 1;
> +pte.pt.xn = 0;
> +
> +write_pte(_first_id[id_offsets[1]], pte);
> +
> +/* Link third ID table */
> +pte = pte_of_xenaddr((vaddr_t)xen_third_id);
> +pte.pt.table = 1;
> +pte.pt.xn = 0;
> +
> +write_pte(_second_id[id_offsets[2]], pte);
> +
> +/* The mapping in the third table will be created at a later stage */
> +
> +/*
> + * Link the identity mapping in the runtime Xen page tables. No need to
> + * use write_pte here as they are not live yet.
> + */
> +xen_pgtable[id_offsets[0]] = boot_pgtable[id_offsets[0]];
> +}
> +
> +void update_identity_mapping(bool enable)
You probably want an __init attribute here.
> +{
> +paddr_t id_addr = virt_to_maddr(_start);
> +int rc;
> +
> +if ( enable )
> +rc = map_pages_to_xen(id_addr, maddr_to_mfn(id_addr), 1,
> + PAGE_HYPERVISOR_RX);
> +else
> +rc = destroy_xen_mappings(id_addr, id_addr + PAGE_SIZE);
> +
> +BUG_ON(rc);
> +}
> +
> /*
> * After boot, Xen page-tables should not contain mapping that are both
> * Writable and eXecutables.
> @@ -609,6 +679,9 @@ void __init setup_pagetables(unsigned long
> boot_phys_offset)
>
> phys_offset = boot_phys_offset;
>
> +/* XXX: Find a better place to call it */
Why do you think this place is not right ?
> +prepare_identity_mapping();
> +
> #ifdef CONFIG_ARM_64
> pte = pte_of_xenaddr((uintptr_t)xen_first);
> pte.pt.table = 1;
> --
> 2.32.0
Cheers
Bertrand
[PATCH v5] x86/vmx: add hvm functions to get/set non-register state
During VM forking and resetting a failed vmentry has been observed due
to the guest non-register state going out-of-sync with the guest register
state. For example, a VM fork reset right after a STI instruction can trigger
the failed entry. This is due to the guest non-register state not being saved
from the parent VM, thus the reset operation only copies the register state.
Fix this by adding a new pair of hvm functions to get/set the guest
non-register state so that the overall vCPU state remains in sync.
Signed-off-by: Tamas K Lengyel
---
v5: Switch to internal-only hvm funcs instead of adding to hvm_hw_cpu
---
xen/arch/x86/hvm/vmx/vmx.c | 32
xen/arch/x86/include/asm/hvm/hvm.h | 40 ++
xen/arch/x86/mm/mem_sharing.c | 11 +++-
3 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
index c075370f64..2685da16c8 100644
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -1334,6 +1334,36 @@ static void cf_check vmx_set_interrupt_shadow(
__vmwrite(GUEST_INTERRUPTIBILITY_INFO, intr_shadow);
}
+static void cf_check vmx_get_nonreg_state(struct vcpu *v,
+struct hvm_vcpu_nonreg_state *nrs)
+{
+vmx_vmcs_enter(v);
+
+__vmread(GUEST_ACTIVITY_STATE, >vmx.activity_state);
+__vmread(GUEST_INTERRUPTIBILITY_INFO, >vmx.interruptibility_info);
+__vmread(GUEST_PENDING_DBG_EXCEPTIONS, >vmx.pending_dbg);
+
+if ( cpu_has_vmx_virtual_intr_delivery )
+__vmread(GUEST_INTR_STATUS, >vmx.interrupt_status);
+
+vmx_vmcs_exit(v);
+}
+
+static void cf_check vmx_set_nonreg_state(struct vcpu *v,
+struct hvm_vcpu_nonreg_state *nrs)
+{
+vmx_vmcs_enter(v);
+
+__vmwrite(GUEST_ACTIVITY_STATE, nrs->vmx.activity_state);
+__vmwrite(GUEST_INTERRUPTIBILITY_INFO, nrs->vmx.interruptibility_info);
+__vmwrite(GUEST_PENDING_DBG_EXCEPTIONS, nrs->vmx.pending_dbg);
+
+if ( cpu_has_vmx_virtual_intr_delivery )
+__vmwrite(GUEST_INTR_STATUS, nrs->vmx.interrupt_status);
+
+vmx_vmcs_exit(v);
+}
+
static void vmx_load_pdptrs(struct vcpu *v)
{
uint32_t cr3 = v->arch.hvm.guest_cr[3];
@@ -2487,6 +2517,8 @@ static struct hvm_function_table __initdata_cf_clobber
vmx_function_table = {
.load_cpu_ctxt= vmx_load_vmcs_ctxt,
.get_interrupt_shadow = vmx_get_interrupt_shadow,
.set_interrupt_shadow = vmx_set_interrupt_shadow,
+.get_nonreg_state = vmx_get_nonreg_state,
+.set_nonreg_state = vmx_set_nonreg_state,
.guest_x86_mode = vmx_guest_x86_mode,
.get_cpl = _vmx_get_cpl,
.get_segment_register = vmx_get_segment_register,
diff --git a/xen/arch/x86/include/asm/hvm/hvm.h
b/xen/arch/x86/include/asm/hvm/hvm.h
index 5b7ec0cf69..9dee0f87a3 100644
--- a/xen/arch/x86/include/asm/hvm/hvm.h
+++ b/xen/arch/x86/include/asm/hvm/hvm.h
@@ -84,6 +84,17 @@ enum hvm_intblk {
/* update_guest_cr() flags. */
#define HVM_UPDATE_GUEST_CR3_NOFLUSH 0x0001
+struct hvm_vcpu_nonreg_state {
+union {
+struct {
+uint64_t activity_state;
+uint64_t interruptibility_info;
+uint64_t pending_dbg;
+uint64_t interrupt_status;
+} vmx;
+};
+};
+
/*
* The hardware virtual machine (HVM) interface abstracts away from the
* x86/x86_64 CPU virtualization assist specifics. Currently this interface
@@ -122,6 +133,10 @@ struct hvm_function_table {
/* Examine specifics of the guest state. */
unsigned int (*get_interrupt_shadow)(struct vcpu *v);
void (*set_interrupt_shadow)(struct vcpu *v, unsigned int intr_shadow);
+void (*get_nonreg_state)(struct vcpu *v,
+ struct hvm_vcpu_nonreg_state *nrs);
+void (*set_nonreg_state)(struct vcpu *v,
+ struct hvm_vcpu_nonreg_state *nrs);
int (*guest_x86_mode)(struct vcpu *v);
unsigned int (*get_cpl)(struct vcpu *v);
void (*get_segment_register)(struct vcpu *v, enum x86_segment seg,
@@ -744,6 +759,20 @@ void hvm_set_reg(struct vcpu *v, unsigned int reg,
uint64_t val);
d_->arch.hvm.pi_ops.vcpu_block(v_); \
})
+static inline void hvm_get_nonreg_state(struct vcpu *v,
+struct hvm_vcpu_nonreg_state *nrs)
+{
+if ( hvm_funcs.get_nonreg_state )
+alternative_vcall(hvm_funcs.get_nonreg_state, v, nrs);
+}
+
+static inline void hvm_set_nonreg_state(struct vcpu *v,
+struct hvm_vcpu_nonreg_state *nrs)
+{
+if ( hvm_funcs.set_nonreg_state )
+alternative_vcall(hvm_funcs.set_nonreg_state, v, nrs);
+}
+
#else /* CONFIG_HVM */
#define hvm_enabled false
@@ -863,6 +892,17 @@ static inline void hvm_set_reg(struct vcpu *v, unsigned
int reg, uint64_t val)
ASSERT_UNREACHABLE();
}
+static inline void hvm_get_nonreg_state(struct vcpu *v,
+
Re: [PATCH early-RFC 2/5] xen/arm64: Rework the memory layout
Hi Julien, > On 9 Mar 2022, at 12:20, Julien Grall wrote: > > From: Julien Grall > > Xen is currently not fully compliant with the Arm because it will I think you wanted to say “arm arm” her. > switch the TTBR with the MMU on. > > In order to be compliant, we need to disable the MMU before > switching the TTBR. The implication is the page-tables should > contain an identity mapping of the code switching the TTBR. > > If we don't rework the memory layout, we would need to find a > virtual address that matches a physical address and doesn't clash > with the static virtual regions. This can be a bit tricky. This sentence is a bit misleading. Even with the rework you need to do that just by moving the Xen virtual address upper you make sure that anything physical memory under 512GB can be mapped 1:1 without clashing with other Xen mappings (unless Xen is loaded in memory at physical address 512GB which would end in the same issue). I think should be rephrased. > > On arm64, the memory layout has plenty of unused space. In most of > the case we expect Xen to be loaded in low memory. > > The memory layout is reshuffled to keep the 0th slot free. Xen will now 0th slot of first level of page table. > be loaded at (512GB + 2MB). This requires a slight tweak of the boot > code as XEN_VIRT_START cannot be used as an immediate. > > Signed-off-by: Julien Grall > > --- > >TODO: >- I vaguely recall that one of the early platform we supported add > the memory starting in high memory (> 1TB). I need to check > whether the new layout will be fine. I think we have some Juno with some memory like that, tell me if you need help here. >- Update the documentation to reflect the new layout > --- > xen/arch/arm/arm64/head.S | 3 ++- > xen/arch/arm/include/asm/config.h | 20 ++-- > xen/arch/arm/mm.c | 14 +++--- > 3 files changed, 23 insertions(+), 14 deletions(-) > > diff --git a/xen/arch/arm/arm64/head.S b/xen/arch/arm/arm64/head.S > index 66d862fc8137..878649280d73 100644 > --- a/xen/arch/arm/arm64/head.S > +++ b/xen/arch/arm/arm64/head.S > @@ -594,7 +594,8 @@ create_page_tables: > * need an additional 1:1 mapping, the virtual mapping will > * suffice. > */ > -cmp x19, #XEN_VIRT_START > +ldr x0, =XEN_VIRT_START > +cmp x19, x0 A comment in the code would be good here to prevent someone reverting this. > bne 1f > ret > 1: > diff --git a/xen/arch/arm/include/asm/config.h > b/xen/arch/arm/include/asm/config.h > index 5db28a8dbd56..b2f31a914103 100644 > --- a/xen/arch/arm/include/asm/config.h > +++ b/xen/arch/arm/include/asm/config.h > @@ -107,8 +107,20 @@ > * Unused > */ > > +#ifdef CONFIG_ARM_32 > + > #define COMMON_VIRT_START _AT(vaddr_t, 0) > > +#else > + > +#define SLOT0_ENTRY_BITS 39 > +#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS) > +#define SLOT0_ENTRY_SIZE SLOT0(1) > + > +#define COMMON_VIRT_START SLOT(1) > + > +#endif > + > #define XEN_VIRT_START (COMMON_VIRT_START + MB(2)) > #define XEN_SLOT_SIZE MB(2) > #define XEN_VIRT_END(XEN_VIRT_START + XEN_SLOT_SIZE) > @@ -161,14 +173,10 @@ > > #else /* ARM_64 */ > > -#define SLOT0_ENTRY_BITS 39 > -#define SLOT0(slot) (_AT(vaddr_t,slot) << SLOT0_ENTRY_BITS) > -#define SLOT0_ENTRY_SIZE SLOT0(1) > - > -#define VMAP_VIRT_START GB(1) > +#define VMAP_VIRT_START (SLOT0(1) + GB(1)) > #define VMAP_VIRT_END(VMAP_VIRT_START + GB(1)) > > -#define FRAMETABLE_VIRT_START GB(32) > +#define FRAMETABLE_VIRT_START (SLOT0(1) + GB(32)) > #define FRAMETABLE_SIZEGB(32) > #define FRAMETABLE_NR (FRAMETABLE_SIZE / sizeof(*frame_table)) > #define FRAMETABLE_VIRT_END(FRAMETABLE_VIRT_START + FRAMETABLE_SIZE - 1) > diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c > index 6b7c41d827ca..75ed9a3ce249 100644 > --- a/xen/arch/arm/mm.c > +++ b/xen/arch/arm/mm.c > @@ -187,11 +187,10 @@ static void __init __maybe_unused build_assertions(void) > BUILD_BUG_ON(DIRECTMAP_VIRT_START & ~FIRST_MASK); > #endif > /* Page table structure constraints */ > -#ifdef CONFIG_ARM_64 > -BUILD_BUG_ON(zeroeth_table_offset(XEN_VIRT_START)); > -#endif Don’t you want to enforce the opposite now ? Check that it is not on slot 0 ? > BUILD_BUG_ON(first_table_offset(XEN_VIRT_START)); > +#ifdef CONFIG_ARM_32 > BUILD_BUG_ON(second_linear_offset(XEN_VIRT_START) >= XEN_PT_LPAE_ENTRIES); > +#endif > #ifdef CONFIG_DOMAIN_PAGE > BUILD_BUG_ON(DOMHEAP_VIRT_START & ~FIRST_MASK); > #endif > @@ -611,10 +610,11 @@ void __init setup_pagetables(unsigned long > boot_phys_offset) > phys_offset = boot_phys_offset; > > #ifdef CONFIG_ARM_64 > -p = (void *) xen_pgtable; > -p[0] = pte_of_xenaddr((uintptr_t)xen_first); > -p[0].pt.table = 1; > -p[0].pt.xn = 0; > +pte = pte_of_xenaddr((uintptr_t)xen_first); > +pte.pt.table = 1; > +
Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules
Hi Jan, > On 25 Mar 2022, at 12:43, Jan Beulich wrote: > > On 24.03.2022 12:04, Bertrand Marquis wrote: >> cppcheck can be used to check Xen code quality. >> >> To create a report do "make cppcheck" on a built tree adding any options >> you added during the process you used to build xen (like CROSS_COMPILE >> or XEN_TARGET_ARCH). This will generate an xml report xen-cppcheck.xml. >> >> To create a html report do "make cppcheck-html" in the same way and a >> full report to be seen in a browser will be generated in >> cppcheck-htmlreport/index.html. >> >> For better results it is recommended to build your own cppcheck from the >> latest sources that you can find at [1]. >> Development and result analysis has been done with cppcheck 2.7. >> >> The Makefile rule is searching for all C files which have been compiled >> (ie which have a generated .o file) and is running cppcheck on all of >> them using the current configuration of xen so only the code actually >> compiled is checked. > > Why this restriction? It means there are multiple runs needed in case > files are touched by a patch which can't both be built at the same time > (e.g. ones under multiple xen/arch/*/). In addition, by going from .o > files, you also require (and yes, you say so) that the tree has been > built before. I think you would instead want to go from the collective > set of $(obj-y), $(obj-n), and $(obj-), traversing the tree suitably. Cppcheck is running on the preprocessed files. It has a mode to try to do all combinations of CONFIG_ but we have far to much of them and this is ending up in lots of wrong findings doing combinations which are not possible. I tried at the beginning to use obj- but failed. Now this is a lot cleaner in Makefiles thanks to Anthony’s job and I will give it an other try. > >> @@ -511,6 +513,75 @@ cloc: >> done; \ >> done | cloc --list-file=- >> >> +# What cppcheck command to use. >> +# To get proper results, it is recommended to build cppcheck manually from >> the >> +# latest source and use CPPCHECK to give the full path to the built version. >> +CPPCHECK ?= cppcheck >> + >> +# What cppcheck-htmlreport to use. >> +# If you give the full path to a self compiled cppcheck, this should be set >> +# to the full path to cppcheck-html in the htmlreport directory of cppcheck. >> +# On recent distribution, this is available in the standard path. >> +CPPCHECK_HTMLREPORT ?= cppcheck-htmlreport >> + >> +# By default we generate the report in cppcheck-htmlreport directory in the >> +# build directory. This can be changed by giving a directory in this >> variable. >> +CPPCHECK_HTMLREPORT_OUTDIR ?= cppcheck-htmlreport >> + >> +# Compile flags to pass to cppcheck: >> +# - include directories and defines Xen Makefile is passing (from CFLAGS) >> +# - include config.h as this is passed directly to the compiler. >> +# - define CPPCHECK as we use to disable or enable some specific part of the >> +# code to solve some cppcheck issues. >> +# - explicitely enable some cppcheck checks as we do not want to use "all" >> +# which includes unusedFunction which gives wrong positives as we check >> file >> +# per file. >> +# >> +# Compiler defines are in compiler-def.h which is included in config.h >> +# >> +CPPCHECKFLAGS := -DCPPCHECK --max-ctu-depth=10 \ >> + --enable=style,information,missingInclude \ >> + --include=$(BASEDIR)/include/xen/config.h \ >> + -I $(BASEDIR)/xsm/flask/include \ >> + -I $(BASEDIR)/include/xen/libfdt \ > > Why ware these two -I necessary? Shouldn't they derive cleanly ... Those are not in the standard CFLAGS but are added to CFLAGS in sub-makefiles so I have to add them explicitly. > >> + $(filter -D% -I%,$(CFLAGS)) > > ... here? > > As to style (also applicable further down) I think it would help if you > didn't use tabs and if you aligned things, e.g. > > CPPCHECKFLAGS := -DCPPCHECK --max-ctu-depth=10 \ > --enable=style,information,missingInclude \ > --include=$(BASEDIR)/include/xen/config.h \ > -I $(BASEDIR)/xsm/flask/include \ > -I $(BASEDIR)/include/xen/libfdt \ > $(filter -D% -I%,$(CFLAGS)) Ok > >> +# We need to find all C files (as we are not checking assembly files) so >> +# we find all generated .o files which have a .c corresponding file. >> +CPPCHECKFILES := $(wildcard $(patsubst %.o,%.c, $(filter-out >> $(BASEDIR)/tools/%,$(shell find $(BASEDIR) -name "*.o" >> + >> +quiet_cmd_cppcheck_xml = CPPCHECK $(patsubst $(BASEDIR)/%,%,$<) >> +cmd_cppcheck_xml = $(CPPCHECK) -v -q --xml $(CPPCHECKFLAGS) \ >> + --output-file=$@ $< > > As per the earlier comment (just to give another example) I think > this would want to be > > cmd_cppcheck_xml = $(CPPCHECK) -v -q --xml $(CPPCHECKFLAGS) \ >
Re: [PATCH 3/3] x86/mem_sharing: make fork_reset more configurable
On Fri, Mar 25, 2022 at 06:48:42AM -0400, Tamas K Lengyel wrote:
> On Fri, Mar 25, 2022, 5:04 AM Jan Beulich wrote:
>
> > On 24.03.2022 18:02, Tamas K Lengyel wrote:
> > > On Thu, Mar 24, 2022 at 12:44 PM Roger Pau Monné
> > wrote:
> > >>
> > >> On Thu, Mar 24, 2022 at 12:22:49PM -0400, Tamas K Lengyel wrote:
> > >>> On Thu, Mar 24, 2022 at 12:04 PM Roger Pau Monné
> > wrote:
> >
> > On Thu, Mar 24, 2022 at 11:52:38AM -0400, Tamas K Lengyel wrote:
> > > On Thu, Mar 24, 2022 at 11:46 AM Roger Pau Monné <
> > roger@citrix.com> wrote:
> > >>
> > >> On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote:
> > >>> diff --git a/xen/include/public/memory.h
> > b/xen/include/public/memory.h
> > >>> index 208d8dcbd9..30ce23c5a7 100644
> > >>> --- a/xen/include/public/memory.h
> > >>> +++ b/xen/include/public/memory.h
> > >>> @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
> > >>> uint32_t gref; /* IN: gref to debug */
> > >>> } u;
> > >>> } debug;
> > >>> -struct mem_sharing_op_fork { /* OP_FORK */
> > >>> +struct mem_sharing_op_fork { /* OP_FORK/_RESET */
> > >>> domid_t parent_domain;/* IN: parent's domain
> > id */
> > >>> /* These flags only makes sense for short-lived forks */
> > >>> #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
> > >>> #define XENMEM_FORK_BLOCK_INTERRUPTS (1u << 1)
> > >>> #define XENMEM_FORK_SKIP_SPECIAL_PAGES (1u << 2)
> > >>> +#define XENMEM_FORK_RESET_STATE(1u << 3)
> > >>> +#define XENMEM_FORK_RESET_MEMORY (1u << 4)
> > >>> uint16_t flags; /* IN: optional
> > settings */
> > >>> uint32_t pad; /* Must be set to 0 */
> > >>> } fork;
> > >>> diff --git a/xen/include/public/vm_event.h
> > b/xen/include/public/vm_event.h
> > >>> index bb003d21d0..81c2ee28cc 100644
> > >>> --- a/xen/include/public/vm_event.h
> > >>> +++ b/xen/include/public/vm_event.h
> > >>> @@ -127,6 +127,14 @@
> > >>> * Reset the vmtrace buffer (if vmtrace is enabled)
> > >>> */
> > >>> #define VM_EVENT_FLAG_RESET_VMTRACE (1 << 13)
> > >>> +/*
> > >>> + * Reset the VM state (if VM is fork)
> > >>> + */
> > >>> +#define VM_EVENT_FLAG_RESET_FORK_STATE (1 << 14)
> > >>> +/*
> > >>> + * Remove unshared entried from physmap (if VM is fork)
> > >>> + */
> > >>> +#define VM_EVENT_FLAG_RESET_FORK_MEMORY (1 << 15)
> > >>
> > >> I'm confused about why two different interfaces are added to do this
> > >> kind of selective resets, one to vm_event and one to xenmem_fork?
> > >>
> > >> I thin k the natural place for the option to live would be
> > >> XENMEM_FORK?
> > >
> > > Yes, that's the natural place for it. But we are adding it to both
> > for
> > > a reason. In our use-case the reset operation will happen after a
> > > vm_event is received to which we already must send a reply. Setting
> > > the flag on the vm_event reply saves us having to issue an extra
> > memop
> > > hypercall afterwards.
> >
> > Can you do a multicall and batch both operations in a single
> > hypercall?
> >
> > That would seem more natural than adding duplicated interfaces.
> > >>>
> > >>> Not in a straight forward way, no. There is no exposed API in libxc to
> > >>> do a multicall. Even if that was an option it is still easier for me
> > >>> to just flip a bit in the response field than having to construct a
> > >>> whole standalone hypercall structure to be sent as part of a
> > >>> multicall.
> > >>
> > >> Right, I can see it being easier, but it seems like a bad choice from
> > >> an interface PoV. You are the maintainer of both subsystems, but it
> > >> would seem to me it's in your best interest to try to keep the
> > >> interfaces separated and clean.
> > >>
> > >> Would it be possible for the reset XENMEM_FORK op to have the side
> > >> effect of performing what you would instead do with the vm_event
> > >> hypercall?
> > >
> > > Yes, the event response is really just an event channel signal to Xen,
> > > so the memop hypercall could similarly encode the "now check the
> > > vm_event response" as an optional field. But why is that any better
> > > than the current event channel route processing the vm_response
> > > encoding the "now do these ops on the fork"?
> >
> > Well, as Roger said: Less duplication in the interface.
> >
>
> No, you would just duplicate something else instead, ie. the event channel
> hypercall.
>
>
> > > We already have a bunch of different operations you can encode in the
> > > vm_event response field, so it reduces the complexity on the toolstack
> > > side since I don't have to switch around which hypercall I need to
> > > issue depending on what extra ops I want to put into
Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork
On Fri, Mar 25, 2022, 7:31 AM Roger Pau Monné wrote: > On Fri, Mar 25, 2022 at 07:15:59AM -0400, Tamas K Lengyel wrote: > > On Fri, Mar 25, 2022, 6:59 AM Roger Pau Monné > wrote: > > > > > On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote: > > > > Add option to the fork memop to skip populating the fork with special > > > pages. > > > > These special pages are only necessary when setting up forks to be > fully > > > > functional with a toolstack. For short-lived forks where no > toolstack is > > > active > > > > these pages are uneccesary. > > > > > > Replying here because there's no cover letter AFAICT. > > > > > > For this kind of performance related changes it would be better if you > > > could provide some figures about the performance impact. It would help > > > if we knew whether avoiding mapping the vAPIC page means you can > > > create 0.1% more forks per-minute or 20%. > > > > > > If you really want to speed up the forking path it might be good to > start > > > by perf sampling Xen in order to find the bottlenecks? > > > > > > > Sure but for experiment systems I don't think its necessary to collect > that > > data. > > It helps weight whether the extra logic is worth the performance > benefit IMO. Here it might not matter that much since you say there's > also a non-performance reason for the change. > > > There is also a non-performance reason why we want to keep special pages > > from being populated, in cases we really want the forks physmap to start > > empty for better control over its state. There was already a case where > > having special pages mapped in ended up triggering unexpected Xen > behaviors > > leading to chain of events not easy to follow. For example if page 0 gets > > brought in while the vCPU is being created it ends up as a misconfigured > > ept entry if nested virtualization is enabled. That leads to ept > > misconfiguration exits instead of epf faults. Simply enforcing no entry > in > > the physmap until forking is complete eliminates the chance of something > > like that happening again and makes reasoning about the VM's behavior > from > > the start easier. > > Could we have this added to the commit message then, and the option > renamed to 'empty_p2m' or some such. Then you should also ASSERT that > at the end of the fork process the p2m is indeed empty, not sure if > checking d->arch.paging.hap.p2m_pages == 0 would accomplish that? > Sure, I can do that. Thanks, Tamas >
Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules
On 24.03.2022 12:04, Bertrand Marquis wrote: > cppcheck can be used to check Xen code quality. > > To create a report do "make cppcheck" on a built tree adding any options > you added during the process you used to build xen (like CROSS_COMPILE > or XEN_TARGET_ARCH). This will generate an xml report xen-cppcheck.xml. > > To create a html report do "make cppcheck-html" in the same way and a > full report to be seen in a browser will be generated in > cppcheck-htmlreport/index.html. > > For better results it is recommended to build your own cppcheck from the > latest sources that you can find at [1]. > Development and result analysis has been done with cppcheck 2.7. > > The Makefile rule is searching for all C files which have been compiled > (ie which have a generated .o file) and is running cppcheck on all of > them using the current configuration of xen so only the code actually > compiled is checked. Why this restriction? It means there are multiple runs needed in case files are touched by a patch which can't both be built at the same time (e.g. ones under multiple xen/arch/*/). In addition, by going from .o files, you also require (and yes, you say so) that the tree has been built before. I think you would instead want to go from the collective set of $(obj-y), $(obj-n), and $(obj-), traversing the tree suitably. > @@ -511,6 +513,75 @@ cloc: > done; \ > done | cloc --list-file=- > > +# What cppcheck command to use. > +# To get proper results, it is recommended to build cppcheck manually from > the > +# latest source and use CPPCHECK to give the full path to the built version. > +CPPCHECK ?= cppcheck > + > +# What cppcheck-htmlreport to use. > +# If you give the full path to a self compiled cppcheck, this should be set > +# to the full path to cppcheck-html in the htmlreport directory of cppcheck. > +# On recent distribution, this is available in the standard path. > +CPPCHECK_HTMLREPORT ?= cppcheck-htmlreport > + > +# By default we generate the report in cppcheck-htmlreport directory in the > +# build directory. This can be changed by giving a directory in this > variable. > +CPPCHECK_HTMLREPORT_OUTDIR ?= cppcheck-htmlreport > + > +# Compile flags to pass to cppcheck: > +# - include directories and defines Xen Makefile is passing (from CFLAGS) > +# - include config.h as this is passed directly to the compiler. > +# - define CPPCHECK as we use to disable or enable some specific part of the > +# code to solve some cppcheck issues. > +# - explicitely enable some cppcheck checks as we do not want to use "all" > +# which includes unusedFunction which gives wrong positives as we check > file > +# per file. > +# > +# Compiler defines are in compiler-def.h which is included in config.h > +# > +CPPCHECKFLAGS := -DCPPCHECK --max-ctu-depth=10 \ > + --enable=style,information,missingInclude \ > + --include=$(BASEDIR)/include/xen/config.h \ > + -I $(BASEDIR)/xsm/flask/include \ > + -I $(BASEDIR)/include/xen/libfdt \ Why ware these two -I necessary? Shouldn't they derive cleanly ... > + $(filter -D% -I%,$(CFLAGS)) ... here? As to style (also applicable further down) I think it would help if you didn't use tabs and if you aligned things, e.g. CPPCHECKFLAGS := -DCPPCHECK --max-ctu-depth=10 \ --enable=style,information,missingInclude \ --include=$(BASEDIR)/include/xen/config.h \ -I $(BASEDIR)/xsm/flask/include \ -I $(BASEDIR)/include/xen/libfdt \ $(filter -D% -I%,$(CFLAGS)) > +# We need to find all C files (as we are not checking assembly files) so > +# we find all generated .o files which have a .c corresponding file. > +CPPCHECKFILES := $(wildcard $(patsubst %.o,%.c, $(filter-out > $(BASEDIR)/tools/%,$(shell find $(BASEDIR) -name "*.o" > + > +quiet_cmd_cppcheck_xml = CPPCHECK $(patsubst $(BASEDIR)/%,%,$<) > +cmd_cppcheck_xml = $(CPPCHECK) -v -q --xml $(CPPCHECKFLAGS) \ > +--output-file=$@ $< As per the earlier comment (just to give another example) I think this would want to be cmd_cppcheck_xml = $(CPPCHECK) -v -q --xml $(CPPCHECKFLAGS) \ --output-file=$@ $< (i.e. with continue options aligned with the first one). This is even more noticable with ... > +quiet_cmd_merge_cppcheck_reports = CPPCHECK-MERGE $@ > +cmd_merge_cppcheck_reports = $(BASEDIR)/tools/merge_cppcheck_reports.py $^ $@ > + > +quiet_cmd_cppcheck_html = CPPCHECK-HTML $< > +cmd_cppcheck_html = $(CPPCHECK_HTMLREPORT) --file=$< --source-dir=$(BASEDIR) > \ > + >--report-dir=$(CPPCHECK_HTMLREPORT_OUTDIR) \ > + >--title=Xen ... needlessly long lines
Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork
On Fri, Mar 25, 2022 at 07:15:59AM -0400, Tamas K Lengyel wrote: > On Fri, Mar 25, 2022, 6:59 AM Roger Pau Monné wrote: > > > On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote: > > > Add option to the fork memop to skip populating the fork with special > > pages. > > > These special pages are only necessary when setting up forks to be fully > > > functional with a toolstack. For short-lived forks where no toolstack is > > active > > > these pages are uneccesary. > > > > Replying here because there's no cover letter AFAICT. > > > > For this kind of performance related changes it would be better if you > > could provide some figures about the performance impact. It would help > > if we knew whether avoiding mapping the vAPIC page means you can > > create 0.1% more forks per-minute or 20%. > > > > If you really want to speed up the forking path it might be good to start > > by perf sampling Xen in order to find the bottlenecks? > > > > Sure but for experiment systems I don't think its necessary to collect that > data. It helps weight whether the extra logic is worth the performance benefit IMO. Here it might not matter that much since you say there's also a non-performance reason for the change. > There is also a non-performance reason why we want to keep special pages > from being populated, in cases we really want the forks physmap to start > empty for better control over its state. There was already a case where > having special pages mapped in ended up triggering unexpected Xen behaviors > leading to chain of events not easy to follow. For example if page 0 gets > brought in while the vCPU is being created it ends up as a misconfigured > ept entry if nested virtualization is enabled. That leads to ept > misconfiguration exits instead of epf faults. Simply enforcing no entry in > the physmap until forking is complete eliminates the chance of something > like that happening again and makes reasoning about the VM's behavior from > the start easier. Could we have this added to the commit message then, and the option renamed to 'empty_p2m' or some such. Then you should also ASSERT that at the end of the fork process the p2m is indeed empty, not sure if checking d->arch.paging.hap.p2m_pages == 0 would accomplish that? Thanks, Roger.
Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork
On Fri, Mar 25, 2022, 6:59 AM Roger Pau Monné wrote: > On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote: > > Add option to the fork memop to skip populating the fork with special > pages. > > These special pages are only necessary when setting up forks to be fully > > functional with a toolstack. For short-lived forks where no toolstack is > active > > these pages are uneccesary. > > Replying here because there's no cover letter AFAICT. > > For this kind of performance related changes it would be better if you > could provide some figures about the performance impact. It would help > if we knew whether avoiding mapping the vAPIC page means you can > create 0.1% more forks per-minute or 20%. > > If you really want to speed up the forking path it might be good to start > by perf sampling Xen in order to find the bottlenecks? > Sure but for experiment systems I don't think its necessary to collect that data. There is also a non-performance reason why we want to keep special pages from being populated, in cases we really want the forks physmap to start empty for better control over its state. There was already a case where having special pages mapped in ended up triggering unexpected Xen behaviors leading to chain of events not easy to follow. For example if page 0 gets brought in while the vCPU is being created it ends up as a misconfigured ept entry if nested virtualization is enabled. That leads to ept misconfiguration exits instead of epf faults. Simply enforcing no entry in the physmap until forking is complete eliminates the chance of something like that happening again and makes reasoning about the VM's behavior from the start easier. Tamas >
Re: [PATCH] xen: don't hang when resuming PCI device
On 23.03.22 02:21, Jakub Kądziołka wrote: If a xen domain with at least two VCPUs has a PCI device attached which enters the D3hot state during suspend, the kernel may hang while resuming, depending on the core on which an async resume task gets scheduled. The bug occurs because xen's do_suspend calls dpm_resume_start while only the timer of the boot CPU has been resumed (when xen_suspend called syscore_resume), before calling xen_arch_suspend to resume the timers of the other CPUs. This breaks pci_dev_d3_sleep. Thus this patch moves the call to xen_arch_resume before the call to dpm_resume_start, eliminating the hangs and restoring the stack-like structure of the suspend/restore procedure. Signed-off-by: Jakub Kądziołka Reviewed-by: Juergen Gross Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork
On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote: > Add option to the fork memop to skip populating the fork with special pages. > These special pages are only necessary when setting up forks to be fully > functional with a toolstack. For short-lived forks where no toolstack is > active > these pages are uneccesary. Replying here because there's no cover letter AFAICT. For this kind of performance related changes it would be better if you could provide some figures about the performance impact. It would help if we knew whether avoiding mapping the vAPIC page means you can create 0.1% more forks per-minute or 20%. If you really want to speed up the forking path it might be good to start by perf sampling Xen in order to find the bottlenecks? Thanks, Roger.
[xen-unstable test] 168833: tolerable FAIL
flight 168833 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/168833/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail pass in 168825 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168825 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168825 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168825 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168825 test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail like 168825 test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail like 168825 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168825 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168825 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168825 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 168825 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 168825 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168825 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-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-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 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-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-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 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
Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork
On Fri, Mar 25, 2022, 6:25 AM Roger Pau Monné wrote:
> On Thu, Mar 24, 2022 at 12:27:02PM -0400, Tamas K Lengyel wrote:
> > On Thu, Mar 24, 2022 at 11:51 AM Roger Pau Monné
> wrote:
> > >
> > > On Thu, Mar 24, 2022 at 11:15:08AM -0400, Tamas K Lengyel wrote:
> > > > On Thu, Mar 24, 2022 at 10:50 AM Roger Pau Monné <
> roger@citrix.com> wrote:
> > > > >
> > > > > On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote:
> > > > > > +{
> > > > > > +cd->arch.hvm.mem_sharing.block_interrupts =
> block_interrupts;
> > > > > > +cd->arch.hvm.mem_sharing.skip_special_pages =
> skip_special_pages;
> > > > > > +/* skip mapping the vAPIC page on unpause if skipping
> special pages */
> > > > > > +cd->creation_finished = skip_special_pages;
> > > > >
> > > > > I think this is dangerous. While it might be true at the moment
> that
> > > > > the arch_domain_creation_finished only maps the vAPIC page,
> there's no
> > > > > guarantee it couldn't do other stuff in the future that could be
> > > > > required for the VM to be started.
> > > >
> > > > I understand this domain_creation_finished route could be expanded in
> > > > the future to include other stuff, but IMHO we can evaluate what to
> do
> > > > about that when and if it becomes necessary. This is all experimental
> > > > to begin with.
> > >
> > > Maybe you could check the skip_special_pages field from
> > > domain_creation_finished in order to decide whether the vAPIC page
> > > needs to be mapped?
> >
> > Could certainly do that but it means adding experimental code in an
> > #ifdef to the vAPIC mapping logic. Technically nothing wrong with that
> > but I would prefer keeping all this code in a single-place if
> > possible.
>
> I see, while I agree it's best to keep the code contained when
> possible, I think in this case it's better to modify the hook,
> specially because further changes to domain_creation_finished will
> easily spot that this path is special cases for VM forks.
>
> While the code is experimental it doesn't mean it shouldn't be
> properly integrated with the rest of the code base.
>
Sure, I'm fine with moving it there.
Tamas
>
Re: [PATCH 3/3] x86/mem_sharing: make fork_reset more configurable
On Fri, Mar 25, 2022, 5:04 AM Jan Beulich wrote:
> On 24.03.2022 18:02, Tamas K Lengyel wrote:
> > On Thu, Mar 24, 2022 at 12:44 PM Roger Pau Monné
> wrote:
> >>
> >> On Thu, Mar 24, 2022 at 12:22:49PM -0400, Tamas K Lengyel wrote:
> >>> On Thu, Mar 24, 2022 at 12:04 PM Roger Pau Monné
> wrote:
>
> On Thu, Mar 24, 2022 at 11:52:38AM -0400, Tamas K Lengyel wrote:
> > On Thu, Mar 24, 2022 at 11:46 AM Roger Pau Monné <
> roger@citrix.com> wrote:
> >>
> >> On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote:
> >>> diff --git a/xen/include/public/memory.h
> b/xen/include/public/memory.h
> >>> index 208d8dcbd9..30ce23c5a7 100644
> >>> --- a/xen/include/public/memory.h
> >>> +++ b/xen/include/public/memory.h
> >>> @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
> >>> uint32_t gref; /* IN: gref to debug */
> >>> } u;
> >>> } debug;
> >>> -struct mem_sharing_op_fork { /* OP_FORK */
> >>> +struct mem_sharing_op_fork { /* OP_FORK/_RESET */
> >>> domid_t parent_domain;/* IN: parent's domain
> id */
> >>> /* These flags only makes sense for short-lived forks */
> >>> #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
> >>> #define XENMEM_FORK_BLOCK_INTERRUPTS (1u << 1)
> >>> #define XENMEM_FORK_SKIP_SPECIAL_PAGES (1u << 2)
> >>> +#define XENMEM_FORK_RESET_STATE(1u << 3)
> >>> +#define XENMEM_FORK_RESET_MEMORY (1u << 4)
> >>> uint16_t flags; /* IN: optional
> settings */
> >>> uint32_t pad; /* Must be set to 0 */
> >>> } fork;
> >>> diff --git a/xen/include/public/vm_event.h
> b/xen/include/public/vm_event.h
> >>> index bb003d21d0..81c2ee28cc 100644
> >>> --- a/xen/include/public/vm_event.h
> >>> +++ b/xen/include/public/vm_event.h
> >>> @@ -127,6 +127,14 @@
> >>> * Reset the vmtrace buffer (if vmtrace is enabled)
> >>> */
> >>> #define VM_EVENT_FLAG_RESET_VMTRACE (1 << 13)
> >>> +/*
> >>> + * Reset the VM state (if VM is fork)
> >>> + */
> >>> +#define VM_EVENT_FLAG_RESET_FORK_STATE (1 << 14)
> >>> +/*
> >>> + * Remove unshared entried from physmap (if VM is fork)
> >>> + */
> >>> +#define VM_EVENT_FLAG_RESET_FORK_MEMORY (1 << 15)
> >>
> >> I'm confused about why two different interfaces are added to do this
> >> kind of selective resets, one to vm_event and one to xenmem_fork?
> >>
> >> I thin k the natural place for the option to live would be
> >> XENMEM_FORK?
> >
> > Yes, that's the natural place for it. But we are adding it to both
> for
> > a reason. In our use-case the reset operation will happen after a
> > vm_event is received to which we already must send a reply. Setting
> > the flag on the vm_event reply saves us having to issue an extra
> memop
> > hypercall afterwards.
>
> Can you do a multicall and batch both operations in a single
> hypercall?
>
> That would seem more natural than adding duplicated interfaces.
> >>>
> >>> Not in a straight forward way, no. There is no exposed API in libxc to
> >>> do a multicall. Even if that was an option it is still easier for me
> >>> to just flip a bit in the response field than having to construct a
> >>> whole standalone hypercall structure to be sent as part of a
> >>> multicall.
> >>
> >> Right, I can see it being easier, but it seems like a bad choice from
> >> an interface PoV. You are the maintainer of both subsystems, but it
> >> would seem to me it's in your best interest to try to keep the
> >> interfaces separated and clean.
> >>
> >> Would it be possible for the reset XENMEM_FORK op to have the side
> >> effect of performing what you would instead do with the vm_event
> >> hypercall?
> >
> > Yes, the event response is really just an event channel signal to Xen,
> > so the memop hypercall could similarly encode the "now check the
> > vm_event response" as an optional field. But why is that any better
> > than the current event channel route processing the vm_response
> > encoding the "now do these ops on the fork"?
>
> Well, as Roger said: Less duplication in the interface.
>
No, you would just duplicate something else instead, ie. the event channel
hypercall.
> > We already have a bunch of different operations you can encode in the
> > vm_event response field, so it reduces the complexity on the toolstack
> > side since I don't have to switch around which hypercall I need to
> > issue depending on what extra ops I want to put into a single
> > hypercall.
>
> The two goals need to be weighed against one another; for the moment
> I think I'm with Roger aiming at a clean interface.
>
It may look like that from the Xen side but from the toolstack side this is
actually the cleanest way to achieve what we
Re: [PATCH 1/3] x86/mem_sharing: option to skip populating special pages during fork
On Thu, Mar 24, 2022 at 12:27:02PM -0400, Tamas K Lengyel wrote:
> On Thu, Mar 24, 2022 at 11:51 AM Roger Pau Monné wrote:
> >
> > On Thu, Mar 24, 2022 at 11:15:08AM -0400, Tamas K Lengyel wrote:
> > > On Thu, Mar 24, 2022 at 10:50 AM Roger Pau Monné
> > > wrote:
> > > >
> > > > On Tue, Mar 22, 2022 at 01:41:37PM -0400, Tamas K Lengyel wrote:
> > > > > +{
> > > > > +cd->arch.hvm.mem_sharing.block_interrupts = block_interrupts;
> > > > > +cd->arch.hvm.mem_sharing.skip_special_pages =
> > > > > skip_special_pages;
> > > > > +/* skip mapping the vAPIC page on unpause if skipping
> > > > > special pages */
> > > > > +cd->creation_finished = skip_special_pages;
> > > >
> > > > I think this is dangerous. While it might be true at the moment that
> > > > the arch_domain_creation_finished only maps the vAPIC page, there's no
> > > > guarantee it couldn't do other stuff in the future that could be
> > > > required for the VM to be started.
> > >
> > > I understand this domain_creation_finished route could be expanded in
> > > the future to include other stuff, but IMHO we can evaluate what to do
> > > about that when and if it becomes necessary. This is all experimental
> > > to begin with.
> >
> > Maybe you could check the skip_special_pages field from
> > domain_creation_finished in order to decide whether the vAPIC page
> > needs to be mapped?
>
> Could certainly do that but it means adding experimental code in an
> #ifdef to the vAPIC mapping logic. Technically nothing wrong with that
> but I would prefer keeping all this code in a single-place if
> possible.
I see, while I agree it's best to keep the code contained when
possible, I think in this case it's better to modify the hook,
specially because further changes to domain_creation_finished will
easily spot that this path is special cases for VM forks.
While the code is experimental it doesn't mean it shouldn't be
properly integrated with the rest of the code base.
Thanks, Roger.
[ovmf test] 168837: regressions - FAIL
flight 168837 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/168837/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 168254 build-amd64 6 xen-buildfail REGR. vs. 168254 build-i3866 xen-buildfail REGR. vs. 168254 build-i386-xsm6 xen-buildfail REGR. vs. 168254 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 69218d5d2854acaa7a11c777244de4a297d2fbb9 baseline version: ovmf b1b89f9009f2390652e0061bd7b24fc40732bc70 Last test of basis 168254 2022-02-28 10:41:46 Z 24 days Failing since168258 2022-03-01 01:55:31 Z 24 days 251 attempts Testing same since 168832 2022-03-25 01:43:21 Z0 days3 attempts People who touched revisions under test: Abdul Lateef Attar Abdul Lateef Attar via groups.io Abner Chang Bandaru, Purna Chandra Rao Gerd Hoffmann Guo Dong Guomin Jiang Hao A Wu Hua Ma Huang, Li-Xia Jagadeesh Ujja Jason Jason Lou Ken Lautner Kenneth Lautner Kuo, Ted Li, Zhihao Lixia Huang Lou, Yun Ma, Hua Mara Sophie Grosch Mara Sophie Grosch via groups.io Matt DeVillier Michael Kubacki Patrick Rudolph Purna Chandra Rao Bandaru Sami Mujawar Sean Rhodes Sebastien Boeuf Sunny Wang Ted Kuo Wenyi Xie wenyi,xie via groups.io Xiaolu.Jiang Zhihao Li jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 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 Not pushing. (No revision log; it would be 904 lines long.)
[libvirt test] 168836: regressions - FAIL
flight 168836 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/168836/ 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 b8d6ecc70c8a8e9c90bab48b6829b42d8b77c748 baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 623 days Failing since151818 2020-07-11 04:18:52 Z 622 days 604 attempts Testing same since 168836 2022-03-25 04:19:00 Z0 days1 attempts People who touched revisions under test: Adolfo Jayme Barrientos Aleksandr Alekseev Aleksei Zakharov Andika Triwidada Andrea Bolognani 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 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é Didik Supriadi dinglimin Divya Garg Dmitrii Shcherbakov Dmytro Linkin Eiichi Tsukata Emilio Herrera Eric Farman Erik Skultety Fabian Affolter Fabian Freyer Fabiano Fidêncio Fangge Jin Farhan Ali Fedora Weblate Translation 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 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 Liao Pingfang Lin Ma Lin Ma Lin Ma Liu Yiding Lubomir Rintel Luke Yue Luyao Zhong Marc Hartmayer Marc-André Lureau Marek Marczykowski-Górecki Markus Schade Martin Kletzander Martin Pitt Masayoshi Mizuma Matej Cepl Matt Coleman Matt Coleman Mauro Matteo Cascella Meina Li Michal Privoznik Michał Smyk Milo Casagrande Moshe Levi Muha Aliss Nathan Neal Gompa Nick Chevsky Nick Shyrokovskiy Nickys Music Group Nico Pache Nicolas Lécureuil Nicolas Lécureuil Nikolay Shirokovskiy Olaf Hering Olesya Gerasimenko Or Ozeri Orion Poplawski Pany Patrick Magauran Paulo de Rezende Pinatti Pavel Hrdina Peng Liang Peter Krempa Pino Toscano Pino Toscano Piotr Drąg Prathamesh Chavan Praveen K Paladugu Richard W.M. Jones Ricky Tigg Robin Lee Rohit Kumar Roman Bogorodskiy Roman Bolshakov Ryan Gahagan Ryan Schmidt Sam Hartman Scott Shambarger Sebastian Mitterle
Re: [PATCH] xen/arm: set CPSR Z bit when creating aarch32 guests
Hi Wei, On 25/03/2022 02:51, Wei Chen wrote: -Original Message- From: Xen-devel On Behalf Of Stefano Stabellini Sent: 2022年3月25日 9:01 To: xen-devel@lists.xenproject.org Cc: jul...@xen.org; sstabell...@kernel.org; Bertrand Marquis ; volodymyr_babc...@epam.com; Stefano Stabellini Subject: [PATCH] xen/arm: set CPSR Z bit when creating aarch32 guests From: Stefano Stabellini The first 32 bytes of zImage are NOPs. When CONFIG_EFI is enabled in the kernel, certain versions of Linux will use an UNPREDICATABLE NOP encoding, sometimes resulting in an unbootable kernel. Whether the resulting kernel is bootable or not depends on the processor. See commit a92882a4d270 in the Linux kernel for all the details. All kernel releases starting from Linux 4.9 without commit a92882a4d270 are affected. Fortunately there is a simple workaround: setting the "Z" bit in CPSR make it so those invalid NOP instructions are never executed. That is because the instruction is conditional (not equal). So, on QEMU at least, the instruction will end up to be ignored and not generate an exception. Setting the "Z" bit makes those kernel versions bootable again and it is harmless in the other cases. Note that both U-Boot and QEMU -kernel set the "Z" bit in CPSR when booting a zImage kernel on aarch32. Signed-off-by: Stefano Stabellini --- Changes in v3: - improve commit message - improve in-code comment Changes in v2: - improve commit message - add in-code comment - move PSR_Z to the beginning --- xen/include/public/arch-arm.h | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h index 94b31511dd..c0c1149e27 100644 --- a/xen/include/public/arch-arm.h +++ b/xen/include/public/arch-arm.h @@ -361,6 +361,7 @@ typedef uint64_t xen_callback_t; #define PSR_DBG_MASK(1<<9)/* arm64: Debug Exception mask */ #define PSR_IT_MASK (0x0600fc00) /* Thumb If-Then Mask */ #define PSR_JAZELLE (1<<24) /* Jazelle Mode */ +#define PSR_Z (1<<30) /* Zero condition flag */ /* 32 bit modes */ #define PSR_MODE_USR 0x10 @@ -383,7 +384,15 @@ typedef uint64_t xen_callback_t; #define PSR_MODE_EL1t 0x04 #define PSR_MODE_EL0t 0x00 -#define PSR_GUEST32_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC) +/* + * We set PSR_Z to be able to boot Linux kernel versions with an invalid + * encoding of the first 8 NOP instructions. See commit a92882a4d270 in + * Linux. + * + * Note that PSR_Z is also set by U-Boot and QEMU -kernel when loading + * zImage kernels on aarch32. + */ +#define PSR_GUEST32_INIT (PSR_Z|PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC) #define PSR_GUEST64_INIT (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h) Maybe this is a good opportunity to fix the alignment of the two macros : ) I have dropped one space for PSR_GUEST32_INIT and committed. Cheers, -- Julien Grall
Re: [PATCH] xen/arm: set CPSR Z bit when creating aarch32 guests
Hi Stefano, On 25/03/2022 01:00, Stefano Stabellini wrote: From: Stefano Stabellini The first 32 bytes of zImage are NOPs. When CONFIG_EFI is enabled in the kernel, certain versions of Linux will use an UNPREDICATABLE NOP typo: s/UNPREDICATABLE/UNPREDICTABLE/ I will fix it on commit. Acked-by: Julien Grall Cheers, -- Julien Grall
Re: [PATCH] xen/arm: set CPSR Z bit when creating aarch32 guests
Hi Stefano, > On 25 Mar 2022, at 02:00, Stefano Stabellini wrote: > > From: Stefano Stabellini > > The first 32 bytes of zImage are NOPs. When CONFIG_EFI is enabled in the > kernel, certain versions of Linux will use an UNPREDICATABLE NOP > encoding, sometimes resulting in an unbootable kernel. Whether the > resulting kernel is bootable or not depends on the processor. See commit > a92882a4d270 in the Linux kernel for all the details. > > All kernel releases starting from Linux 4.9 without commit a92882a4d270 > are affected. > > Fortunately there is a simple workaround: setting the "Z" bit in CPSR > make it so those invalid NOP instructions are never executed. That is > because the instruction is conditional (not equal). So, on QEMU at > least, the instruction will end up to be ignored and not generate an > exception. Setting the "Z" bit makes those kernel versions bootable > again and it is harmless in the other cases. > > Note that both U-Boot and QEMU -kernel set the "Z" bit in CPSR when > booting a zImage kernel on aarch32. > > Signed-off-by: Stefano Stabellini Thanks for the comment and commit message fixes. Reviewed-by: Bertrand Marquis Cheers Bertrand > --- > Changes in v3: > - improve commit message > - improve in-code comment > > Changes in v2: > - improve commit message > - add in-code comment > - move PSR_Z to the beginning > --- > xen/include/public/arch-arm.h | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/xen/include/public/arch-arm.h b/xen/include/public/arch-arm.h > index 94b31511dd..c0c1149e27 100644 > --- a/xen/include/public/arch-arm.h > +++ b/xen/include/public/arch-arm.h > @@ -361,6 +361,7 @@ typedef uint64_t xen_callback_t; > #define PSR_DBG_MASK(1<<9)/* arm64: Debug Exception mask */ > #define PSR_IT_MASK (0x0600fc00) /* Thumb If-Then Mask */ > #define PSR_JAZELLE (1<<24) /* Jazelle Mode */ > +#define PSR_Z (1<<30) /* Zero condition flag */ > > /* 32 bit modes */ > #define PSR_MODE_USR 0x10 > @@ -383,7 +384,15 @@ typedef uint64_t xen_callback_t; > #define PSR_MODE_EL1t 0x04 > #define PSR_MODE_EL0t 0x00 > > -#define PSR_GUEST32_INIT > (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC) > +/* > + * We set PSR_Z to be able to boot Linux kernel versions with an invalid > + * encoding of the first 8 NOP instructions. See commit a92882a4d270 in > + * Linux. > + * > + * Note that PSR_Z is also set by U-Boot and QEMU -kernel when loading > + * zImage kernels on aarch32. > + */ > +#define PSR_GUEST32_INIT > (PSR_Z|PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_SVC) > #define PSR_GUEST64_INIT > (PSR_ABT_MASK|PSR_FIQ_MASK|PSR_IRQ_MASK|PSR_MODE_EL1h) > > #define SCTLR_GUEST_INITxen_mk_ullong(0x00c50078) > -- > 2.25.1 >
Re: [PATCH 3/3] x86/mem_sharing: make fork_reset more configurable
On 24.03.2022 18:02, Tamas K Lengyel wrote:
> On Thu, Mar 24, 2022 at 12:44 PM Roger Pau Monné wrote:
>>
>> On Thu, Mar 24, 2022 at 12:22:49PM -0400, Tamas K Lengyel wrote:
>>> On Thu, Mar 24, 2022 at 12:04 PM Roger Pau Monné
>>> wrote:
On Thu, Mar 24, 2022 at 11:52:38AM -0400, Tamas K Lengyel wrote:
> On Thu, Mar 24, 2022 at 11:46 AM Roger Pau Monné
> wrote:
>>
>> On Tue, Mar 22, 2022 at 01:41:39PM -0400, Tamas K Lengyel wrote:
>>> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
>>> index 208d8dcbd9..30ce23c5a7 100644
>>> --- a/xen/include/public/memory.h
>>> +++ b/xen/include/public/memory.h
>>> @@ -541,12 +541,14 @@ struct xen_mem_sharing_op {
>>> uint32_t gref; /* IN: gref to debug */
>>> } u;
>>> } debug;
>>> -struct mem_sharing_op_fork { /* OP_FORK */
>>> +struct mem_sharing_op_fork { /* OP_FORK/_RESET */
>>> domid_t parent_domain;/* IN: parent's domain id */
>>> /* These flags only makes sense for short-lived forks */
>>> #define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
>>> #define XENMEM_FORK_BLOCK_INTERRUPTS (1u << 1)
>>> #define XENMEM_FORK_SKIP_SPECIAL_PAGES (1u << 2)
>>> +#define XENMEM_FORK_RESET_STATE(1u << 3)
>>> +#define XENMEM_FORK_RESET_MEMORY (1u << 4)
>>> uint16_t flags; /* IN: optional settings */
>>> uint32_t pad; /* Must be set to 0 */
>>> } fork;
>>> diff --git a/xen/include/public/vm_event.h
>>> b/xen/include/public/vm_event.h
>>> index bb003d21d0..81c2ee28cc 100644
>>> --- a/xen/include/public/vm_event.h
>>> +++ b/xen/include/public/vm_event.h
>>> @@ -127,6 +127,14 @@
>>> * Reset the vmtrace buffer (if vmtrace is enabled)
>>> */
>>> #define VM_EVENT_FLAG_RESET_VMTRACE (1 << 13)
>>> +/*
>>> + * Reset the VM state (if VM is fork)
>>> + */
>>> +#define VM_EVENT_FLAG_RESET_FORK_STATE (1 << 14)
>>> +/*
>>> + * Remove unshared entried from physmap (if VM is fork)
>>> + */
>>> +#define VM_EVENT_FLAG_RESET_FORK_MEMORY (1 << 15)
>>
>> I'm confused about why two different interfaces are added to do this
>> kind of selective resets, one to vm_event and one to xenmem_fork?
>>
>> I thin k the natural place for the option to live would be
>> XENMEM_FORK?
>
> Yes, that's the natural place for it. But we are adding it to both for
> a reason. In our use-case the reset operation will happen after a
> vm_event is received to which we already must send a reply. Setting
> the flag on the vm_event reply saves us having to issue an extra memop
> hypercall afterwards.
Can you do a multicall and batch both operations in a single
hypercall?
That would seem more natural than adding duplicated interfaces.
>>>
>>> Not in a straight forward way, no. There is no exposed API in libxc to
>>> do a multicall. Even if that was an option it is still easier for me
>>> to just flip a bit in the response field than having to construct a
>>> whole standalone hypercall structure to be sent as part of a
>>> multicall.
>>
>> Right, I can see it being easier, but it seems like a bad choice from
>> an interface PoV. You are the maintainer of both subsystems, but it
>> would seem to me it's in your best interest to try to keep the
>> interfaces separated and clean.
>>
>> Would it be possible for the reset XENMEM_FORK op to have the side
>> effect of performing what you would instead do with the vm_event
>> hypercall?
>
> Yes, the event response is really just an event channel signal to Xen,
> so the memop hypercall could similarly encode the "now check the
> vm_event response" as an optional field. But why is that any better
> than the current event channel route processing the vm_response
> encoding the "now do these ops on the fork"?
Well, as Roger said: Less duplication in the interface.
> We already have a bunch of different operations you can encode in the
> vm_event response field, so it reduces the complexity on the toolstack
> side since I don't have to switch around which hypercall I need to
> issue depending on what extra ops I want to put into a single
> hypercall.
The two goals need to be weighed against one another; for the moment
I think I'm with Roger aiming at a clean interface.
Jan
Re: Support status of OpenBSD frontend drivers
On Thu, Mar 24, 2022 at 09:10:57PM -0400, Demi Marie Obenour wrote: > On 3/24/22 18:21, Marek Marczykowski-Górecki wrote: > > On Thu, Mar 24, 2022 at 11:49:14AM -0400, Demi Marie Obenour wrote: > >> On 3/24/22 10:11, Roger Pau Monné wrote: > >>> On Thu, Mar 24, 2022 at 09:56:29AM -0400, Demi Marie Obenour wrote: > As per private discussion with Theo de Raadt, OpenBSD does not consider > bugs in its xnf(4) that allow a backend to cause mischief to be security > issues. I believe the same applies to its xbf(4). Should the support > document be updated? > >>> > >>> I think that's already reflected in the support document: > >>> > >>> 'Status, OpenBSD: Supported, Security support external' > >>> > >>> Since the security support is external it's my understanding OpenBSD > >>> security team gets to decide what's a security issue and what is not. > >>> > >>> That however creates differences in the level of support offered by > >>> the different OSes, but I think that's unavoidable. It's also hard to > >>> track the status here because those are external components in > >>> separate code bases. > >>> > >>> Could be added as a mention together with the Windows note about > >>> frontends trusting backends, but then I would fear this is likely to > >>> get out of sync if OpenBSD ever changes their frontends to support > >>> untrusted backends (even if not considered as a security issue). > >> > >> As a Qubes OS developer, I still think this is useful information and > >> should be documented. For instance, if I choose to add proper OpenBSD > >> guest support to Qubes OS (as opposed to the current “you can run > >> anything in an HVM” situation), I might decide to have OpenBSD > >> guests use devices emulated by a Linux-based stubdomain, since the > >> stubdomain’s netfront and blkfront drivers *are* security-supported > >> against malicious backends. I might also choose to have a warning in > >> the GUI when switching the NetVM of an OpenBSD guest to something other > >> than the empty string (meaning no network access) or the (normally > >> fairly trusted) sys-firewall or sys-whonix qubes. > > > > I'm with Roger on this - when security support is external, such > > information in xen.git could easily become stale. If anything, there > > could be a link to OpenBSD security status info, maintained by whoever > > such support provides. > > This ought to be on https://man.openbsd.org/xnf.4 and > https://man.openbsd.org/xbf.4, but it is not. Should I send a patch? You should discuss with the OpenBSD people I think, I really have no idea where those limitations should be listed. Introducing a man page 'Caveats' or 'Limitations' sections would seem suitable to me, but it's ultimately up to them. Thanks, Roger.
Re: [PATCH v3 2/5] xen: make evtchn_alloc_unbound public
On 25.03.2022 01:30, Stefano Stabellini wrote: > Maybe, instead of exporting get_free_port, we could create a new > function in xen/common/event_channel.c and mark it as __init? This way: > - we don't need to expose get_free_port > - the new function would only be __init anyway, so no risk of runtime > misuse > > What do you think? Maybe. Such a function would want to serve both your an Daniel's purpose then. Jan
Re: [RFC PATCH] xen/build: Add cppcheck and cppcheck-html make rules
Hi Stefano,
On 25.03.2022 02:32, Stefano Stabellini wrote:
> On Thu, 24 Mar 2022, Bertrand Marquis wrote:
>> cppcheck can be used to check Xen code quality.
>>
>> To create a report do "make cppcheck" on a built tree adding any options
>> you added during the process you used to build xen (like CROSS_COMPILE
>> or XEN_TARGET_ARCH). This will generate an xml report xen-cppcheck.xml.
>>
>> To create a html report do "make cppcheck-html" in the same way and a
>> full report to be seen in a browser will be generated in
>> cppcheck-htmlreport/index.html.
>>
>> For better results it is recommended to build your own cppcheck from the
>> latest sources that you can find at [1].
>> Development and result analysis has been done with cppcheck 2.7.
>>
>> The Makefile rule is searching for all C files which have been compiled
>> (ie which have a generated .o file) and is running cppcheck on all of
>> them using the current configuration of xen so only the code actually
>> compiled is checked.
>>
>> A new tool is introduced to merge all cppcheck reports into one global
>> report including all findings and removing duplicates.
>>
>> Some extra variables can be used to customize the report:
>> - CPPCHECK can be used to give the full path to the cppcheck binary to
>> use (default is to use the one from the standard path).
>> - CPPCHECK_HTMLREPORT can be used to give the full path to
>> cppcheck-htmlreport (default is to use the one from the standard path).
>>
>> This has been tested on several arm configurations (x86 should work but
>> has not been tested).
>>
>> [1] https://cppcheck.sourceforge.io/
>>
>> Signed-off-by: Bertrand Marquis
>> Signed-off-by: Michal Orzel
>
> Very cool, I was looking forward to this :-)
>
>
>> diff --git a/xen/tools/merge_cppcheck_reports.py
>> b/xen/tools/merge_cppcheck_reports.py
>> new file mode 100755
>> index 00..ef055f6925
>> --- /dev/null
>> +++ b/xen/tools/merge_cppcheck_reports.py
>> @@ -0,0 +1,73 @@
>> +#!/usr/bin/env python
>> +
>> +"""
>> +This script acts as a tool to merge XML files created by cppcheck.
>> +Usage:
>> +merge_cppcheck_reports.py [FILES] [OUTPUT]
>> +
>> +FILES - list of XML files with extension .cppcheck
>> +OUTPUT - file to store results (with .xml extension).
>> + If not specified, the script will print results to stdout.
>> +"""
>> +
>> +import sys
>> +from xml.etree import ElementTree
>> +
>> +def elements_equal(el1, el2):
>> +if type(el1) != type(el2): return False
>> +
>> +el1_location = str(el1.find('location').attrib)
>> +el2_location = str(el2.find('location').attrib)
>> +
>> +if el1_location != el2_location: return False
>> +
>> +return True
>> +
>> +def remove_duplicates(xml_root_element):
>> +elems_to_remove = []
>> +index = 0
>> +elems_list = list(xml_root_element.findall("errors")[0])
>> +for elem1 in elems_list:
>> +index += 1
>> +for elem2 in elems_list[index:]:
>> +if elements_equal(elem1, elem2) and elem2 not in
>> elems_to_remove:
>> +elems_to_remove.append(elem2)
>> +continue
>> +
>> +for elem in elems_to_remove:
>> +xml_root_element.findall("errors")[0].remove(elem)
>> +
>> +def merge(files):
>> +result_xml_root = None
>> +for xml_file in files:
>> +xml_root = ElementTree.parse(xml_file).getroot()
>
>
> Traceback (most recent call last):
> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line
> 73, in
> run()
> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line
> 60, in run
> result = merge(files)
> File "/local/repos/xen-upstream/xen/tools/merge_cppcheck_reports.py", line
> 43, in merge
> xml_root = ElementTree.parse(xml_file).getroot()
> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1182, in parse
> tree.parse(source, parser)
> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 657, in parse
> self._root = parser.close()
> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1671, in close
> self._raiseerror(v)
> File "/usr/lib/python2.7/xml/etree/ElementTree.py", line 1523, in
> _raiseerror
> raise err
> xml.etree.ElementTree.ParseError: no element found: line 11, column 0
> make: *** [Makefile:576: xen-cppcheck.xml] Error 1
>
> I think we should catch the xml.etree.ElementTree.ParseError exception and
> continue?
Well, this is of course something that we might do but this error clearly warns
us that
some XML files is not well formatted and therefore is not parsable. This could
mean that
you are using some old cppcheck version. Is it correct assumption?
Cheers,
Michal
[linux-linus test] 168830: tolerable FAIL - PUSHED
flight 168830 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/168830/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds18 guest-start/debian.repeat fail REGR. vs. 168816 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 168816 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 168816 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 168816 test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail like 168816 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 168816 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 168816 test-armhf-armhf-libvirt-qcow2 15 saverestore-support-check fail like 168816 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 168816 test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-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-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 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 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qcow2 14 migrate-support-checkfail never pass test-amd64-amd64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 14 migrate-support-checkfail never pass test-arm64-arm64-libvirt-raw 15 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-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-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 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-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-qcow2 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 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: linux7403e6d8263937dea206dd201fed1ceed190ca18 baseline version: linuxed4643521e6af8ab8ed1e467630a85884d2696cf Last test of basis 168816 2022-03-24 03:18:14 Z1 days Testing same since 168830 2022-03-24 20:10:22 Z0 days1 attempts People who touched revisions under test: Abhishek Sahu Al Viro Alex Williamson Alexandru Elisei Anssi Hannula Anup Patel Anup Patel Barret Rhoden Bjorn Helgaas Bjorn Helgaas# pci_ids.h Catalin Marinas Changcheng Deng Christian Borntraeger
