[Xen-devel] [qemu-mainline test] 144331: tolerable FAIL - PUSHED
flight 144331 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/144331/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-xsm 7 xen-boot fail in 144316 pass in 144331 test-amd64-amd64-xl-rtds 16 guest-localmigrate fail pass in 144316 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail in 144316 like 144305 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeat fail in 144316 like 144305 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 144305 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144305 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144305 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144305 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 144305 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass version targeted for testing: qemuu1a61a081ac33ae6cb7dd2e38d119a572f416c7f7 baseline version: qemuu65e05c82bdc6d348155e301c9d87dba7a08a5701 Last test of basis 144305 2019-11-26 05:17:32 Z2 days Testing same since 144316 2019-11-27 00:06:47 Z1 days2 attempts People who touched revisions under test: Alex Bennée Alex Williamson Ariadne Conill Cameron Esfahani David Gibson Dr. David Alan Gilbert Edgar E. Iglesias Eduardo Habkost Greg Kurz Jason Wang Jean-Hugues Deschenes Jean-Hugues Deschênes
Re: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames and max_maptrack_frames handling
On 27.11.19 23:32, Hans van Kranenburg wrote:
Hi all,
On 11/27/19 12:13 PM, Durrant, Paul wrote:
-Original Message-
From: Ian Jackson
Sent: 27 November 2019 11:10
[...]
Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
and max_maptrack_frames handling
Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
max_grant_frames and max_maptrack_frames handling"):
-Original Message-
From: Xen-devel On Behalf Of
Ian
Jackson
I have seen reports of users who ran out of grant/maptrack frames
because of updates to use multiring protocols etc. The error messages
are not very good and the recommended workaround has been to increase
the default limit on the hypervisor command line.
It is important that we don't break that workaround!
Alas it has apparently been broken for several releases now :-(
I guess at least in Debian (where I have seen this) we haven't
released with any affected versions yet...
I believe the problem was introduce in 4.10, so I think it would be prudent to
also back-port the final fix to stable trees from then on.
Yes, the max grant frame issue has historically always been a painful
experience for end users, and Xen 4.11 which we now have in the current
Debian stable has made it worse compared to previous versions indeed.
Changing the hypervisor command line worked in the past, and now that
value is overwritten again by a lower value in the toolstack, which
requires setting per-domU settings, or, what I did, just additionally
also setting max_grant_frames in /etc/xen/xl.conf to the same value as
the hypervisor command line.
This change is very welcome, even to 4.11-stable if possible, since it
will not break existing configuration of users.
If changing only the value of the hypervisor command line works again,
then old information that shows up when the users searches the web will
be useful again, which is good.
Hans
P.S. Now I'm curious to figure out what a maptrack frame is, didn't hear
about that one before.
The maptrack frames are used by the hypervisor for keeping track which
grants are mapped by a specific domain. So they are necessary for driver
domains (including dom0), and max_maptrack_frames limits how many
mappings of other domain's pages can be active simultaneously in a
domain.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-4.10-testing test] 144324: tolerable trouble: fail/pass/starved - PUSHED
flight 144324 xen-4.10-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/144324/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail like 139091 test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail like 139091 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail never pass test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-arm64-arm64-xl-thunderx 2 hosts-allocate starved n/a version targeted for testing: xen e4899550ff7834e1ea5dfbbfb1c618f64e247761 baseline version: xen 3131bf956ab159295ecdde0c5ad003d0c5af4695 Last test of basis 139091 2019-07-17 14:01:39 Z 133 days Failing since143729 2019-11-04 14:27:14 Z 23 days4 attempts Testing same since 144315 2019-11-26 22:06:14 Z1 days2 attempts People who touched revisions under test: Andrew Cooper George Dunlap Ian Jackson Jan Beulich Julien Grall Paul Durrant jobs: build-amd64-xsm pass build-arm64-xsm
[Xen-devel] [PATCH] tools/arm: include xen-tools/libs.h from libxl_arm_acpi.c
libxl_arm_acpi.c is using BUILD_BUG_ON but it is not including xen-tools/libs.h that defines it. Signed-off-by: Stefano Stabellini --- tools/libxl/libxl_arm_acpi.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c index ba874c3d32..52c476ff65 100644 --- a/tools/libxl/libxl_arm_acpi.c +++ b/tools/libxl/libxl_arm_acpi.c @@ -19,6 +19,7 @@ #include "libxl_arm.h" #include +#include /* Below typedefs are useful for the headers under acpi/ */ typedef uint8_t u8; -- 2.17.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable test] 144323: tolerable FAIL
flight 144323 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/144323/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail blocked in 144313 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 144313 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144313 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 144313 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144313 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 144313 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144313 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 144313 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 144313 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 144313 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 144313 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: xen 5530782cfe70ed22fe44358f6a10c38916443b42 baseline version: xen 5530782cfe70ed22fe44358f6a10c38916443b42 Last test of basis 144323 2019-11-27 12:02:43 Z0 days Testing same since (not found) 0 attempts jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64-xtf
Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER range
On Wed, 27 Nov 2019, Jürgen Groß wrote: > On 27.11.19 10:31, Peng Fan wrote: > > > Subject: Re: [Xen-devel] [PATCH V2] arch: arm: vgic-v3: fix GICD_ISACTIVER > > > range > > > > > > On 27.11.19 01:01, Julien Grall wrote: > > > > Hi, > > > > > > > > On 26/11/2019 23:17, Stefano Stabellini wrote: > > > > > On Tue, 26 Nov 2019, Julien Grall wrote: > > > > > > Hi, > > > > > > > > > > > > On 26/11/2019 20:43, Stefano Stabellini wrote: > > > > > > > + Juergen > > > > > > > > > > > > > > I missed that you weren't in CC to the original patch, sorry. > > > > > > > I think this patch should go in, as otherwise Linux 5.4 could run > > > > > > > into problems. It is also a pretty straightforward 4 lines patch. > > > > > > > > > > > > 5.5 (or 5.6) is not going to run on Xen for other reasons (still in > > > > > > the vGIC)... So I would not view this as critical. > > > > > > > > > > 5.5 is not out yet, in fact, the dev window has just opened. Isn't > > > > > your statement a bit premature? > > > > > > > > The GICv4.1 work [1] is going to prevent Linux booting on all current > > > > versions of Xen. While I can't confirm this is going to be merged in > > > > 5.5, I can tell you this will break. > > > > > > > > > > > > > > In any case, even if potential future Linux releases could have other > > > > > additional issues, I don't think it should change our current view on > > > > > this specific issue which affects 5.4, just released. > > > > > > > > The patch is definitely not as straightforward as you may think. > > > > Please refer to the discussion we had on the first version. I voiced > > > > concern about this approach and gave point what could go wrong with > > > happen. > > > > > > > > This patch may be better than the current state (i.e crashing), but > > > > this wasn't tested enough to confirm this is the correct things to do > > > > and no other bug will appear (I don't believe reading I*ACTIVER was > > > > ever tested before). > > > > > > > > It is an annoying bug, but this is only affecting 5.4 which has just > > > > been released. It feels to me this is a fairly risky choice to merge > > > > it qutie late in the release without a good graps of the problem (see > > > > above). > > > > > > > > So I would definitly, prefer if this patch is getting through backport > > > > once we get more testing. > > > > > > > > We can still document the bug in the release note and point people to > > > > the patch. > > > > > > > > Anyway, this is Juergen choice here. But at least now he has the full > > > > picture... > > > > > > > > Cheers, > > > > > > > > [1] > > > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flwn. > > > > > > > net%2FArticles%2F800494%2Fdata=02%7C01%7Cpeng.fan%40nxp.co > > > m%7Cdca > > > > > > > dfb39240749ee675e08d772fcd3ba%7C686ea1d3bc2b4c6fa92cd99c5c30163 > > > 5%7C0%7 > > > > > > > C0%7C637104302519996592sdata=7Jv2IhI8HZgBTSuYzkEplFyhX1lzmv > > > d73xb5 > > > > 2d6ERVQ%3Dreserved=0 > > > > > > > > > > Thanks, Julien, for sharing your opinion. > > > > > > With that statement I'd like to defer this patch to 4.14. > > > > But without this patch, 5.4 kernel will crash. So you prefer > > we develop the solution as Julien suggested for 4.13? > > I certainly won't take a patch for 4.13 when a maintainer of the > related code has reservations against it. > > I think the best thing to do is to develop a proper patch the > maintainers are happy with and don't try to force it into 4.13 now. > Such a patch can still be backported to 4.13 later. I chatted with Juergen and he explained to me something I didn't know before. The release manager can only *block* a patch from being committed, he/she cannot actually decide if the patch should be committed or not for a given release. He/she cannot overrule a maintainer either. In this case, Juergen cannot make the decision on whether the patch should go in 4.13 or not. Although I couldn't reproduce the problem on Xilinx boards, I have to take the community angle on this, and I would like to make sure our releases work properly on any hardware, including NXP. Thus, I'll make the case one more time, hoping that Julien might change his mind :-) We know that the bug fix won't introduce any regressions because, as Julien wrote, this code path was never used before. Also because of that, waiting for the backport and more OSSTest runs won't make much of a difference because OSSTest won't exercise this code path. It is true that the original code handling GICD_ISACTIVER was never spec compliant, and it should be fixed properly. However, that is not what this patch addresses. That code, in addition from not being spec compliant by design, it also happens to have a typo. Fixing the typo at this stage of the release is appropriate at least to get a consistent behavior in the handling of GICD_ISACTIVER*, and also for Linux 5.4 as guest. Not to give a false impression to users, the warning ensures that the underlying Xen behavior
Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests
On Wed, 27 Nov 2019, Julien Grall wrote:
> On Tue, 26 Nov 2019, 23:18 Stefano Stabellini, wrote:
> On Fri, 15 Nov 2019, Stewart Hildebrand wrote:
> > Allow vgic_get_hw_irq_desc to be called with a vcpu argument.
> >
> > Use vcpu argument in vgic_connect_hw_irq.
> >
> > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce
> with
> > ASSERTs.
> >
> > Signed-off-by: Stewart Hildebrand
> >
> > ---
> > v3: new patch
> >
> > ---
> > Note: I have only modified the old vgic to allow delivery of PPIs.
> > ---
> > xen/arch/arm/gic-vgic.c | 24
> > xen/arch/arm/vgic.c | 6 +++---
> > 2 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> > index 98c021f1a8..2c66a8fa92 100644
> > --- a/xen/arch/arm/gic-vgic.c
> > +++ b/xen/arch/arm/gic-vgic.c
> > @@ -418,7 +418,7 @@ struct irq_desc *vgic_get_hw_irq_desc(struct
> domain *d, struct vcpu *v,
> > {
> > struct pending_irq *p;
> >
> > - ASSERT(!v && virq >= 32);
> > + ASSERT((!v && (virq >= 32)) || (!d && v && (virq >= 16) && (virq
> < 32)));
>
> I don't think !d is necessary for this to work as intended so I would
> limit the ASSERT to
>
> ASSERT((!v && (virq >= 32)) || (v && (virq >= 16) && (virq < 32)));
>
> the caller can always pass v->domain
>
> But then you have the risk to run into d != v->domain. So at least with the
> ASSERT you document the expectation.
Yes, that was not my intention.
It makes sense in certain scenarios for v to be NULL. What I was trying
to say is that when v is not-NULL, then also d should be not-NULL for
consistency. I don't think it makes sense to pass v corresponding to
vcpu1 of domain2 and d == NULL, right?
I don't know if you want to add a (d == v->domain) check to the ASSERT
as it is pretty busy already. I am OK either way.___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
On Fri, 27 Sep 2019, Jan Beulich wrote: > On 26.09.2019 21:39, Lars Kurth wrote: > > +### Verbose vs. terse > > +Due to the time it takes to review and compose code reviewer, reviewers > > often adopt a > > +terse style. It is not unusual to see review comments such as > > +> typo > > +> s/resions/regions/ > > +> coding style > > +> coding style: brackets not needed > > +etc. > > + > > +Terse code review style has its place and can be productive for both the > > reviewer and > > +the author. However, overuse can come across as unfriendly, lacking > > empathy and > > +can thus create a negative impression with the author of a patch. This is > > in particular > > +true, when you do not know the author or the author is a newcomer. Terse > > +communication styles can also be perceived as rude in some cultures. > > And another remark here: Not being terse in situations like the ones > enumerated as examples above is a double waste of the reviewer's time: > They shouldn't even need to make such comments, especially not many > times for a single patch (see your mention of "overuse"). I realize > we still have no automated mechanism to check style aspects, but > anybody can easily look over their patches before submitting them. > And for an occasional issue I think a terse reply is quite reasonable > to have. > > Overall I'm seeing the good intentions of this document, yet I'd still > vote at least -1 on it if it came to a vote. Following even just a > fair part of it is a considerable extra amount of time to invest in > reviews, when we already have a severe reviewing bottleneck. If I have > to judge between doing a bad (stylistically according to this doc, not > technically) review or none at all (because of time constraints), I'd > favor the former. Unless of course I'm asked to stop doing so, in > which case I'd expect whoever asks to arrange for the reviews to be > done by someone else in due course. Reading the document, I think Jan has a point that it gives the impression that following the suggestions would take significant efforts, while actually I don't think Lars meant it that way at all, and I don't think it should be the case either. Maybe we should highlight and encourage "clarity" instead of "verbosity" of the communication, and encourage "expressing appreciation" to newcomers, not necessarily to seasoned contributors. The ultimate goal of this document is actually to *reduce* our overall efforts by making our communication more efficient, not to increase efforts. Maybe it is worth saying this too. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 5/6] Add guide on Communication Best Practice
On Thu, 26 Sep 2019, Lars Kurth wrote: > From: Lars Kurth > > This guide covers the bulk on Best Practice related to code review > It primarily focusses on code review interactions > It also covers how to deal with Misunderstandings and Cultural > Differences > > Signed-off-by: Lars Kurth > --- > Cc: minios-de...@lists.xenproject.org > Cc: xen-...@lists.xenproject.org > Cc: win-pv-de...@lists.xenproject.org > Cc: mirageos-de...@lists.xenproject.org > Cc: committ...@xenproject.org > --- > communication-practice.md | 410 > ++ > 1 file changed, 410 insertions(+) > create mode 100644 communication-practice.md > > diff --git a/communication-practice.md b/communication-practice.md > new file mode 100644 > index 000..db9a5ef > --- /dev/null > +++ b/communication-practice.md > @@ -0,0 +1,410 @@ > +# Communication Best Practice > + > +This guide provides communication Best Practice that helps you in > +* Using welcoming and inclusive language > +* Keeping discussions technical and actionable > +* Being respectful of differing viewpoints and experiences > +* Being aware of your own and counterpart’s communication style and culture > +* Show empathy towards other community members > + > +## Code reviews for **reviewers** and **patch authors** > + > +Before embarking on a code review, it is important to remember that > +* A poorly executed code review can hurt the contributors feeling, even when > a reviewer > + did not intend to do so. Feeling defensive is a normal reaction to a > critique or feedback. > + A reviewer should be aware of how the pitch, tone, or sentiment of their > comments > + could be interpreted by the contributor. The same applies to responses of > an author > + to the reviewer. > +* When reviewing someone's code, you are ultimately looking for issues. A > good code > + reviewer is able to mentally separate finding issues from articulating > code review > + comments in a constructive and positive manner: depending on your > personality this > + can be **difficult** and you may need to develop a technique that works > for you. > +* As software engineers we like to be proud of the solutions we came up > with. This can > + make it easy to take another people’s criticism personally. Always > remember that it is > + the code that is being reviewed, not you as a person. > +* When you receive code review feedback, please be aware that we have > reviewers > + from different backgrounds, communication styles and cultures. Although we > all trying > + to create a productive, welcoming and agile environment, we do not always > succeed. > + > +### Express appreciation > +As the nature of code review to find bugs and possible issues, it is very > easy for > +reviewers to get into a mode of operation where the patch review ends up > being a list > +of issues, not mentioning what is right and well done. This can lead to the > code > +submitter interpreting your feedback in a negative way. > + > +The opening of a code review provides an opportunity to address this and > also sets the > +tone for the rest of the code review. Starting **every** review on a > positive note, helps > +set the tone for the rest of the review. > + > +For an initial patch, you can use phrases such as > +> Thanks for the patch > +> Thanks for doing this > + > +For further revisions within a review, phrases such as > +> Thank you for addressing the last set of changes > + > +If you believe the code was good, it is good practice to highlight this by > using phrases > +such as > +> Looks good, just a few comments > +> The changes you have made since the last version look good > + > +If you think there were issues too many with the code to use one of the > phrases, > +you can still start on a positive note, by for example saying > +> I think this is a good change > +> I think this is a good feature proposal > + > +It is also entirely fine to highlight specific changes as good. The best > place to > +do this, is at top of a patch, as addressing code review comments typically > requires ^ the top > +a contributor to go through the list of things to address and an in-lined > positive > +comment is likely to break that workflow. > + > +You should also consider, that if you review a patch of an experienced > +contributor phrases such as *Thanks for the patch* could come across as > +patronizing, while using *Thanks for doing this* is less likely to be > interpreted > +as such. > + > +Appreciation should also be expressed by patch authors when asking for > clarifications > +to a review or responding to questions. A simple > +> Thank you for your feedback > +> Thank you for your reply > +> Thank you XXX! > + > +is normally sufficient. > + > +### Avoid opinion: stick to the facts > +The way how a reviewer expresses feedback, has a big impact on how the author > +perceives the feedback. Key to this is what we call **stick to the facts**. >
Re: [Xen-devel] [PATCH v2 6/6] Added Resolving Disagreement
On Thu, 26 Sep 2019, Lars Kurth wrote: > From: Lars Kurth > > This guide provides Best Practice on identifying and resolving > common classes of disagreement > > Signed-off-by: Lars Kurth > -- > Cc: minios-de...@lists.xenproject.org > Cc: xen-...@lists.xenproject.org > Cc: win-pv-de...@lists.xenproject.org > Cc: mirageos-de...@lists.xenproject.org > Cc: committ...@xenproject.org > --- > resolving-disagreement.md | 146 > ++ > 1 file changed, 146 insertions(+) > create mode 100644 resolving-disagreement.md > > diff --git a/resolving-disagreement.md b/resolving-disagreement.md > new file mode 100644 > index 000..19aedbe > --- /dev/null > +++ b/resolving-disagreement.md > @@ -0,0 +1,146 @@ > +# Resolving Disagreement > + > +This guide provides Best Practice on resolving disagreement, such as > +* Gracefully accept constructive criticism > +* Focus on what is best for the community > +* Resolve differences in opinion effectively > + > +## Theory: Paul Graham's hierarchy of disagreement > +Paul Graham proposed a **disagreement hierarchy** in a 2008 essay > +**[How to Disagree](http://www.paulgraham.com/disagree.html)**, putting > types of > +arguments into a seven-point hierarchy and observing that *moving up the > +disagreement hierarchy makes people less mean, and will make most of them > happier*. > +Graham also suggested that the hierarchy can be thought of as a pyramid, as > the > +highest forms of disagreement are rarer. > + > +|  > | ^ Disagreement This is a NIT but in a few places in this series you go over the original line length. > +| *A representation of Graham's hierarchy of disagreement from > [Loudacris](http://www.createdebate.com/user/viewprofile/Loudacris) modified > by [Rocket000](https://en.wikipedia.org/wiki/User:Rocket000)* | > + > +In the context of the Xen Project we strive to **only use the top half** of > the hierarchy. > +**Name-calling** and **Ad hominem** arguments are not acceptable within the > Xen > +Project. > + > +## Issue: Scope creep > + > +One thing which occasionally happens during code review is that a code > reviewer > +asks or appears to ask the author of patch to implement additional > functionality. ^ a patch ^ functionalities > +This could take for example the form of > +> Do you think it would be useful for the code to do XXX? > +> I can imagine a user wanting to do YYY (and XXX would enable this) > + > +That potentially adds additional work for the code author, which they may > not have > +the time to perform. It is good practice for authors to consider such a > request in terms of > +* Usefulness to the user > +* Code churn, complexity or impact on other system properties > +* Extra time to implement and report back to the reviewer > + > +If you believe that the impact/cost is too high, report back to the > reviewer. To resolve > +this, it is advisable to > +* Report your findings > +* And then check whether this was merely an interesting suggestion, or > something the > +reviewer feels more strongly about > + > +In the latter case, there are typically several common outcomes > +* The **author and reviewer agree** that the suggestion should be implemented > +* The **author and reviewer agree** that it may make sense to defer > implementation > +* The **author and reviewer agree** that it makes no sense to implement the > suggestion > + > +The author of a patch would typically suggest their preferred outcome, for > example > +> I am not sure it is worth to implement XXX > +> Do you think this could be done as a separate patch in future? > + > +In cases, where no agreement can be found, the best approach would be to get > an > +independent opinion from another maintainer or the project's leadership team. I think we should mention somewhere here that it is recommended for reviewers to be explicit about whether a request is optional or whether it is a requirement. For instance: "I think it would be good if X also did Y" doesn't say if it is optional (future work) or it is actually required as part of this series. More explicit word choices are preferable, such as: "I think it would be good if X also did Y, not a requirement but good to have." "I think it would be good if X also did Y and it should be part of this series." It helps make the communication with the author more effective, especially in this kind of situations. > +## Issue: [Bikeshedding](https://en.wiktionary.org/wiki/bikeshedding) > + > +Occasionally discussions about unimportant but easy-to-grasp issues can lead > to > +prolonged and unproductive discussion. The best way to approach this is to ^ discussions > +try and **anticipate** bikeshedding and highlight
Re: [Xen-devel] [PATCH v2 4/6] Add Code Review Guide
On Thu, 26 Sep 2019, Lars Kurth wrote: > From: Lars Kurth > > This document highlights what reviewers such as maintainers and committers > look > for when reviewing code. It sets expectations for code authors and provides > a framework for code reviewers. I think the document is missing a couple of things: - a simple one line statement that possibly the most important thing in a code review is to indentify any bugs in the code - an explanation that requests for major changes to the series should be made early on (i.e. let's not change the architecture of a feature at v9 if possible) I also made this comment in reply to patch #5. I'll let you decide where is the best place for it. > Signed-off-by: Lars Kurth > --- > Cc: minios-de...@lists.xenproject.org > Cc: xen-...@lists.xenproject.org > Cc: win-pv-de...@lists.xenproject.org > Cc: mirageos-de...@lists.xenproject.org > Cc: committ...@xenproject.org > --- > code-review-guide.md | 125 > +++ > 1 file changed, 125 insertions(+) > create mode 100644 code-review-guide.md > > diff --git a/code-review-guide.md b/code-review-guide.md > new file mode 100644 > index 000..8639431 > --- /dev/null > +++ b/code-review-guide.md > @@ -0,0 +1,125 @@ > +# Code Review Guide > + > +This document highlights what reviewers such as maintainers and committers > look > +for when reviewing your code. It sets expectations for code authors and > provides > +a framework for code reviewers. > + > +This document does **not cover** the following topics: > +* [Communication Best Practice](communication-practice.md) > +* [Resolving Disagreement](resolving-disagreement.md) > +* [Patch Submission > Workflow](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches) > +* [Managing Patch Submission with > Git](https://wiki.xenproject.org/wiki/Managing_Xen_Patches_with_Git) > + > +## What we look for in Code Reviews > +When performing a code review, reviewers typically look for the following > things > + > +### Is the change necessary to accomplish the goals? > +* Is it clear what the goals are? > +* Do we need to make a change, or can the goals be met with existing > + functionality? > + > +### Architecture / Interface > +* Is this the best way to solve the problem? > +* Is this the right part of the code to modify? > +* Is this the right level of abstraction? > +* Is the interface general enough? Too general? Forward compatible? > + > +### Functionality > +* Does it do what it’s trying to do? > +* Is it doing it in the most efficient way? > +* Does it handle all the corner / error cases correctly? > + > +### Maintainability / Robustness > +* Is the code clear? Appropriately commented? > +* Does it duplicate another piece of code? > +* Does the code make hidden assumptions? > +* Does it introduce sections which need to be kept **in sync** with other > sections? > +* Are there other **traps** someone modifying this code might fall into? > + > +**Note:** Sometimes you will work in areas which have identified > maintainability > +and/or robustness issues. In such cases, maintainers may ask you to make > additional > +changes, such that your submitted code does not make things worse or point > you > +to other patches are already being worked on. > + > +### System properties > +In some areas of the code, system properties such as > +* Code size > +* Performance > +* Scalability > +* Latency > +* Complexity > +* > +are also important during code reviews. > + > +### Style > +* Comments, carriage returns, **snuggly braces**, > +* See > [CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=CODING_STYLE) > + and > [tools/libxl/CODING_STYLE](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob;f=tools/libxl/CODING_STYLE) > +* No extraneous whitespace changes > + > +### Documentation and testing > +* If there is pre-existing documentation in the tree, such as man pages, > design > + documents, etc. a contributor may be asked to update the documentation > alongside > + the change. Documentation is typically present in the > + [docs](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) folder. > +* When adding new features that have an impact on the end-user, > + a contributor should include an update to the > + [SUPPORT.md](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) file. > + Typically, more complex features require several patch series before it is > ready to be > + advertised in SUPPORT.md > +* When adding new features, a contributor may be asked to provide tests or > + ensure that existing tests pass > + > + Testing for the Xen Project Hypervisor > +Tests are typically located in one of the following directories > +* **Unit tests**: > [tools/tests](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=tools/tests) > +or > [xen/test](https://xenbits.xenproject.org/gitweb/?p=xen.git;a=tree;f=xen/test) > + Unit testing is hard for a system like Xen and
Re: [Xen-devel] getting 4.11.3 ready
On 29/10/2019, 12:41, "Stefano Stabellini" wrote: On Tue, 29 Oct 2019, Julien Grall wrote: > On 28/10/2019 21:43, Stefano Stabellini wrote: > > On Mon, 28 Oct 2019, Julien Grall wrote: > >> Hi, > >> > >> On 25/10/2019 11:31, Jan Beulich wrote: > >>> All, > >>> > >>> the 4.11.3 stable release is due. I intend to wait for the XSA fixes > >>> going public on the 31st, but not (much) longer. Please point out > >>> backporting candidates that you find missing from the respective > >>> stable trees. I have three ones queued which haven't passed the push > >>> gate to the master branch yet: > >>> > >>> 9257c218e5x86/vvmx: Fix the use of RDTSCP when it is intercepted at L0 > >>> 7eee9c16d6x86/tsc: update vcpu time info on guest TSC adjustments > >>> 9633929824x86: fix off-by-one in is_xen_fixed_mfn() > >> > >> We don't seem to have backported patches for quite a while on Arm. Some of my > >> patches have been marked as to be backported from the beginning [1]. I am > >> specifically thinking to: > >> > >> e04818b46d xen/arm: traps: Avoid using BUG_ON() to check guest state in > >> advance_pc() > > Urgh, I gave the correct title but the wrong commit sha1. It should be > > 72615f2e6b98e861c08abb1d2b194126013d54fe > > > > > I have e04818b46d, plus the following marked for backport: > > > > e98edccb944a80db782e551f3090628e66c7fb52 xen/arm: SCTLR_EL1 is a 64-bit register on Arm64 > > There are more than that to backport: > > 30f5047b2c4e577436b505ba7627f34c3be02014xen/arm: gic: Make sure the number of interrupt lines is valid before using it [4.11] > 8aa276235b93eeb4f81095c638970900e19b31e5xen/arm: irq: End cleanly spurious interrupt[4.11] > b4df73de493954c44f240f78779c9bd3782e1572xen/arm: gic-v2: deactivate interrupts during initialization[4.11] > 0322e0db5b29a0d1ce4b452885e34023e3a4b00earm: gic-v3: deactivate interrupts during initialization[4.11] > > 5ba1c5d0641cf63086b3058e547fcd28c3c4a011xen/arm: memaccess: Initialize correctly *access in __p2m_get_mem_access[4.12] > 07e44b3d1be32fa2165c2367ae3ef9c6c8b39e1exen/arm: Implement workaround for Cortex A-57 and Cortex A72 AT speculate [4.12] > > 08e2059facd78d5ffaf206ba06ac2017c4adeed4xen/arm: setup: Calculate correctly the size of Xen [4.11+] > 8dba9a81e7c62b8a7dbe023fffecd2e16cc20486xen/arm: Don't use _end in is_xen_fixed_mfn() [4.11+] > 671878779741b38c5f2363adceef8de2ce0b3945xen/arm: p2m: Free the p2m entry after flushing the IOMMU TLBs [4.11+] > 7f4217cc60574866cb90d67d9750228c6b86c91exen/arm: vsmc: The function identifier is always 32-bit [4.11+] > 612d476e74a314be514ee6a9744eea8db09d32e5xen/arm64: Correctly compute the virtual address in maddr_to_virt() [4.11+] > f51027be0688540aaab61513b06a8693a37e4c00xen/arm: fix nr_pdxs calculation[4.11+] > a189ef027dbb7a3c0dfe566137f05c06d6685fb9xen/arm: mm: Flush the TLBs even if a mapping failed in create_xen_entries [4.11+] They all make sense, I did the backports, building each commit individually. Jan, AFAICT this is not yet ready to run the XSA checking tools. Let me know when you think I should run them Lars ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] RFC disable GCC 9 -Waddress-of-packed-member
On Wed, 27 Nov 2019, Andrew Cooper wrote: > On 27/11/2019 22:44, Stefano Stabellini wrote: > > Hi all, > > > > GCC 9 introduced a new warning: address-of-packed-member. It warns when > > a pointer points to a member of a packed struct, leading to a build > > failure in Xen (cross compiling Xen on Arm with GCC 9.2): > > > > 556 trace.c: In function '__trace_hypercall': > > 557 trace.c:826:19: error: taking address of packed member of 'struct > > ' may result in an unaligned pointer value > > [-Werror=address-of-packed-member] > > 558 826 | uint32_t *a = d.args; > > > > Looking at the code, I cannot see anything wrong with what we are doing. > > At least on Arm, it looks OK? Anything I am missing? > > c/s 3fd3b266d4 at a guess. Thank you! I missed it :-/ ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] RFC disable GCC 9 -Waddress-of-packed-member
On 27/11/2019 22:44, Stefano Stabellini wrote: > Hi all, > > GCC 9 introduced a new warning: address-of-packed-member. It warns when > a pointer points to a member of a packed struct, leading to a build > failure in Xen (cross compiling Xen on Arm with GCC 9.2): > > 556 trace.c: In function '__trace_hypercall': > 557 trace.c:826:19: error: taking address of packed member of 'struct > ' may result in an unaligned pointer value > [-Werror=address-of-packed-member] > 558 826 | uint32_t *a = d.args; > > Looking at the code, I cannot see anything wrong with what we are doing. > At least on Arm, it looks OK? Anything I am missing? c/s 3fd3b266d4 at a guess. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] RFC disable GCC 9 -Waddress-of-packed-member
Hi all,
GCC 9 introduced a new warning: address-of-packed-member. It warns when
a pointer points to a member of a packed struct, leading to a build
failure in Xen (cross compiling Xen on Arm with GCC 9.2):
556 trace.c: In function '__trace_hypercall':
557 trace.c:826:19: error: taking address of packed member of 'struct
' may result in an unaligned pointer value
[-Werror=address-of-packed-member]
558 826 | uint32_t *a = d.args;
Looking at the code, I cannot see anything wrong with what we are doing.
At least on Arm, it looks OK? Anything I am missing?
If you can spot anything wrong with the Xen code, do let me know.
Otherwise, I am thinking of disabling the warning:
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 5337e20..8d5c77c 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -86,6 +86,9 @@ CFLAGS += $(CFLAGS-y)
# allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE
CFLAGS += $(EXTRA_CFLAGS_XEN_CORE)
+# Disable GCC 9 warning about pointers to members of a packed struct
+CFLAGS += -Wno-address-of-packed-member
+
# Most CFLAGS are safe for assembly files:
# -std=gnu{89,99} gets confused by #-prefixed end-of-line comments
# -flto makes no sense and annoys clang
Cheers,
Stefano
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] getting 4.12.2 ready
On Wed, 27 Nov 2019, Julien Grall wrote: > On 25/11/2019 15:10, Jan Beulich wrote: > > All, > > Hi, > > > > > the 4.12.2 stable release is due in about 2 weeks time. Please point > > out backporting candidates that you find missing from the respective > > stable trees. > > Most of the series "xen/arm: XSA-201 and XSA-263 fixes" [1] should be > backported to at least Xen 4.12 (this is already in staging). > > This would error issues with SError and SSBD. But I haven't had the chance to > check this is applying cleanly to Xen 4.12. Maybe Stefano can take a look? > > Cheers > > [1] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg59283.html It applied without any issues ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames and max_maptrack_frames handling
Hi all,
On 11/27/19 12:13 PM, Durrant, Paul wrote:
>> -Original Message-
>> From: Ian Jackson
>> Sent: 27 November 2019 11:10
>> [...]
>> Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
>> and max_maptrack_frames handling
>>
>> Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
>> max_grant_frames and max_maptrack_frames handling"):
-Original Message-
From: Xen-devel On Behalf Of
>> Ian
Jackson
I have seen reports of users who ran out of grant/maptrack frames
because of updates to use multiring protocols etc. The error messages
are not very good and the recommended workaround has been to increase
the default limit on the hypervisor command line.
It is important that we don't break that workaround!
>>>
>>> Alas it has apparently been broken for several releases now :-(
>>
>> I guess at least in Debian (where I have seen this) we haven't
>> released with any affected versions yet...
>
> I believe the problem was introduce in 4.10, so I think it would be prudent
> to also back-port the final fix to stable trees from then on.
Yes, the max grant frame issue has historically always been a painful
experience for end users, and Xen 4.11 which we now have in the current
Debian stable has made it worse compared to previous versions indeed.
Changing the hypervisor command line worked in the past, and now that
value is overwritten again by a lower value in the toolstack, which
requires setting per-domU settings, or, what I did, just additionally
also setting max_grant_frames in /etc/xen/xl.conf to the same value as
the hypervisor command line.
This change is very welcome, even to 4.11-stable if possible, since it
will not break existing configuration of users.
If changing only the value of the hypervisor command line works again,
then old information that shows up when the users searches the web will
be useful again, which is good.
Hans
P.S. Now I'm curious to figure out what a maptrack frame is, didn't hear
about that one before.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-4.12-testing test] 144319: tolerable FAIL - PUSHED
flight 144319 xen-4.12-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/144319/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qcow217 guest-localmigrate/x10 fail like 144299 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: xen 875879a7b8c1d561e6ea2a20958a1e61242ffef1 baseline version: xen a0084358978b3aab1b9c2722d7bfa4e7f4dcf580 Last test of basis 144299 2019-11-25 16:36:34 Z2 days Testing same since 144308 2019-11-26 13:36:45 Z1 days2 attempts People who touched revisions under test: Jan Beulich jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm
[Xen-devel] [xen-unstable-smoke test] 144335: tolerable all pass - PUSHED
flight 144335 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/144335/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 9a400d1797ec7f77ffefeb5c4e17a8c2e8b91a12 baseline version: xen 34c11725483beb45499f934c7e06e00b55f04ef4 Last test of basis 144322 2019-11-27 11:01:00 Z0 days Testing same since 144328 2019-11-27 14:00:31 Z0 days2 attempts People who touched revisions under test: Igor Druzhinin Jan Beulich Julien Grall Sergey Dyasli 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 34c1172548..9a400d1797 9a400d1797ec7f77ffefeb5c4e17a8c2e8b91a12 -> smoke ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed
Hi Paul, On 27/11/2019 12:00, Paul Durrant wrote: From: Julien Grall A guest will setup a shared page with the hypervisor for each vCPU via XENPMU_init. The page will then get mapped in the hypervisor and only released when XENPMU_finish is called. This means that if the guest fails to invoke XENPMU_finish, e.g if it is destroyed rather than cleanly shut down, the page will stay mapped in the hypervisor. One of the consequences is the domain can never be fully destroyed as a page reference is still held. As Xen should never rely on the guest to correctly clean-up any allocation in the hypervisor, we should also unmap such pages during the domain destruction if there are any left. We can re-use the same logic as in pvpmu_finish(). To avoid duplication, move the logic in a new function that can also be called from vpmu_destroy(). NOTE: The call to vpmu_destroy() must also be moved from arch_vcpu_destroy() into domain_relinquish_resources() such that the reference on the mapped page does not prevent domain_destroy() (which calls arch_vcpu_destroy()) from being called. Also, whils it appears that vpmu_arch_destroy() is idempotent it is by no means obvious. Hence move manipulation of the VPMU_CONTEXT_ALLOCATED flag out of implementation specific code and make sure it is cleared at the end of vpmu_arch_destroy(). If you resend the patch, it might be worth to add a line about the lack of XSA. Something like: There is no associated XSA because vPMU is not security supported (see XSA-163). Cheers, ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests
Hi, On 27/11/2019 18:48, Stefano Stabellini wrote: Yes, I think that is a good suggestion. I take that you mean that in vgic_disable_irqs for PPIs we would only clear GIC_IRQ_GUEST_ENABLED then return basically, right? Not really, I am only suggesting to remove the part if ( desc != NULL ) ... I think we are saying the same thing The function is doing a bit more, hence why I wasn't not sure :). But this change alone is not enough. It would require some modification in the rest of the vGIC (see my previous e-mail) and likely some investigation to understand the implication of keeping the interrupt enabled from the HW (I am a bit worry we may have backed this assumption into other part of the vGIC :(). I can see that at least save_and_mask_hwppi and restore_hwppi would need to be modified to account for the fact that GICD_ISENABLER would say "it is enabled" but actually GIC_IRQ_GUEST_ENABLED is unset. It depends how we decide to implement the two functions. We may want to decouple the GIC completely the GIC state from the vGIC state. For instance, you may still want to mask the interrupt regardless of the vGIC state when the vCPU is scheduled out. This would prevent a non-quiescent device to generate interrupt while we can't deal with them. But as we seem to consider the device will be quiescent and also clear the pending bit, then I think we can completely avoid to mask/unmask the interrupt. This would save a couple of access to the GIC interface. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH v3 07/11] xen: arm: vgic: allow delivery of PPIs to guests
On Tue, 26 Nov 2019, Julien Grall wrote: > On 26/11/2019 22:36, Stefano Stabellini wrote: > > On Mon, 25 Nov 2019, Julien Grall wrote: > > > On 23/11/2019 20:35, Julien Grall wrote: > > > > Hi, > > > > > > > > On 15/11/2019 20:10, Stewart Hildebrand wrote: > > > > > Allow vgic_get_hw_irq_desc to be called with a vcpu argument. > > > > > > > > > > Use vcpu argument in vgic_connect_hw_irq. > > > > > > > > > > vgic_connect_hw_irq is called for PPIs and SPIs, not SGIs. Enforce > > > > > with > > > > > ASSERTs. > > > > > > > > > > Signed-off-by: Stewart Hildebrand > > > > > > > > > > --- > > > > > v3: new patch > > > > > > > > > > --- > > > > > Note: I have only modified the old vgic to allow delivery of PPIs. > > > > > > > > The new vGIC should also be modified to support delivery of PPIs. > > > > > > > > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > > > > > index 82f524a35c..c3933c2687 100644 > > > > > --- a/xen/arch/arm/vgic.c > > > > > +++ b/xen/arch/arm/vgic.c > > > > > @@ -410,10 +410,10 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t > > > > > r, > > > > > int n) > > > > > irq_set_affinity(p->desc, > > > > > cpumask_of(v_target->processor)); > > > > > spin_lock_irqsave(>desc->lock, flags); > > > > > /* > > > > > - * The irq cannot be a PPI, we only support delivery of > > > > > SPIs > > > > > - * to guests. > > > > > + * The irq cannot be a SGI, we only support delivery of > > > > > SPIs > > > > > + * and PPIs to guests. > > > > > */ > > > > > - ASSERT(irq >= 32); > > > > > + ASSERT(irq >= NR_SGIS); > > > > > > > > We usually put ASSERT() in place we know that code wouldn't be able to > > > > work > > > > correctly if there ASSERT were hit. In this particular case: > > > > > > > > > if ( irq_type_set_by_domain(d) ) > > > > > gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, > > > > > i)); > > > > > > > > 1) We don't want to allow any domain (including Dom0) to modify the > > > > interrupt type (i.e. level/edge) for PPIs as this is shared. You will > > > > also > > > > most likely need to modify the counterpart in setup_guest_irq(). > > > > > > > > > p->desc->handler->enable(p->desc); > > > > > > > > 2) On GICv3, the re-distributor of vCPU A is accessible by vCPU B. So > > > > vCPU B > > > > could enable the SGI for vCPU A. But this would be called on the wrong > > > > pCPU > > > > leading to inconsistency between the hardware state of the internal vGIC > > > > state. > > > > > > I thought a bit more of the issue over the week-end. The current vGIC is > > > fairly messy. I can see two solutions on how to solve this: > > > 1) Send an IPI to the pCPU where the vCPU A is running and > > > disable/enable > > > the interrupt. The other side would need to the vCPU was actually running > > > to > > > avoid disabling the PPI for the wrong pCPU > > > 2) Keep the HW interrupt always enabled > > > > > > We propagated the enable/disable because of some messy part in the vGIC: > > > - vgic_inject_irq() will not queue any pending interrupt if the vCPU > > > is > > > offline. While interrupt cannot be delivered, we still need to keep them > > > pending as they will never occur again otherwise. This is because they are > > > active on the host side and the guest has no way to deactivate them. > > > - Our implementation of PSCI CPU will remove all pending interrupts > > > (see > > > vgic_clear_pending_irqs()). I am not entirely sure the implication here > > > because of the previous. > > > > > > There are a probably more. Aside the issues with it, I don't really see > > > good > > > advantage to propagate the interrupt state as the interrupts (PPIs, SPIs) > > > have > > > active state. So they can only be received once until the guest actually > > > handles it. > > > > > > So my preference would still be 2) because this makes the code simpler, > > > avoid > > > IPI and other potential locking trouble. > > > > Yes, I think that is a good suggestion. I take that you mean that in > > vgic_disable_irqs for PPIs we would only clear GIC_IRQ_GUEST_ENABLED > > then return basically, right? > Not really, I am only suggesting to remove the part > > if ( desc != NULL ) > ... I think we are saying the same thing > But this change alone is not enough. It would require some modification in the > rest of the vGIC (see my previous e-mail) and likely some investigation to > understand the implication of keeping the interrupt enabled from the HW (I am > a bit worry we may have backed this assumption into other part of the vGIC > :(). I can see that at least save_and_mask_hwppi and restore_hwppi would need to be modified to account for the fact that GICD_ISENABLER would say "it is enabled" but actually GIC_IRQ_GUEST_ENABLED is unset.___ Xen-devel mailing list
Re: [Xen-devel] [PATCH 0/3] Use C inlines for uaccess
Sorry, forgot to set the subject prefix correctly. It should be: [PATCH v3 0/3]. On Wed, Nov 27, 2019 at 1:44 PM Pavel Tatashin wrote: > > Changelog > v3: > - Added Acked-by from Stefano Stabellini > - Addressed comments from Mark Rutland > v2: > - Addressed Russell King's concern by not adding > uaccess_* to ARM. > - Removed the accidental change to xtensa > > Convert the remaining uaccess_* calls from ASM macros to C inlines. > > These patches apply against linux-next. I boot tested ARM64, and > compile tested ARM change > Pavel Tatashin (3): > arm/arm64/xen: use C inlines for privcmd_call > arm64: remove uaccess_ttbr0 asm macros from cache functions > arm64: remove the rest of asm-uaccess.h > > arch/arm/include/asm/assembler.h | 2 +- > arch/arm/include/asm/xen/hypercall.h | 10 + > arch/arm/xen/enlighten.c | 2 +- > arch/arm/xen/hypercall.S | 4 +- > arch/arm64/include/asm/asm-uaccess.h | 61 -- > arch/arm64/include/asm/cacheflush.h| 39 ++-- > arch/arm64/include/asm/xen/hypercall.h | 28 > arch/arm64/kernel/entry.S | 27 +++- > arch/arm64/lib/clear_user.S| 2 +- > arch/arm64/lib/copy_from_user.S| 2 +- > arch/arm64/lib/copy_in_user.S | 2 +- > arch/arm64/lib/copy_to_user.S | 2 +- > arch/arm64/mm/cache.S | 42 ++ > arch/arm64/mm/flush.c | 2 +- > arch/arm64/xen/hypercall.S | 19 +--- > include/xen/arm/hypercall.h| 12 ++--- > 16 files changed, 130 insertions(+), 126 deletions(-) > delete mode 100644 arch/arm64/include/asm/asm-uaccess.h > > -- > 2.24.0 > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/3] arm/arm64/xen: use C inlines for privcmd_call
privcmd_call requires to enable access to userspace for the
duration of the hypercall.
Currently, this is done via assembly macros. Change it to C
inlines instead.
Signed-off-by: Pavel Tatashin
Acked-by: Stefano Stabellini
---
arch/arm/include/asm/assembler.h | 2 +-
arch/arm/include/asm/xen/hypercall.h | 10 +
arch/arm/xen/enlighten.c | 2 +-
arch/arm/xen/hypercall.S | 4 ++--
arch/arm64/include/asm/xen/hypercall.h | 28 ++
arch/arm64/xen/hypercall.S | 19 ++---
include/xen/arm/hypercall.h| 12 +--
7 files changed, 50 insertions(+), 27 deletions(-)
diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 99929122dad7..8e9262a0f016 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -480,7 +480,7 @@ THUMB( orr \reg , \reg , #PSR_T_BIT)
.macro uaccess_disable, tmp, isb=1
#ifdef CONFIG_CPU_SW_DOMAIN_PAN
/*
-* Whenever we re-enter userspace, the domains should always be
+* Whenever we re-enter kernel, the domains should always be
* set appropriately.
*/
mov \tmp, #DACR_UACCESS_DISABLE
diff --git a/arch/arm/include/asm/xen/hypercall.h
b/arch/arm/include/asm/xen/hypercall.h
index 3522cbaed316..cac5bd9ef519 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -1 +1,11 @@
+#ifndef _ASM_ARM_XEN_HYPERCALL_H
+#define _ASM_ARM_XEN_HYPERCALL_H
#include
+
+static inline long privcmd_call(unsigned int call, unsigned long a1,
+ unsigned long a2, unsigned long a3,
+ unsigned long a4, unsigned long a5)
+{
+ return arch_privcmd_call(call, a1, a2, a3, a4, a5);
+}
+#endif /* _ASM_ARM_XEN_HYPERCALL_H */
diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
index dd6804a64f1a..e87280c6d25d 100644
--- a/arch/arm/xen/enlighten.c
+++ b/arch/arm/xen/enlighten.c
@@ -440,4 +440,4 @@ EXPORT_SYMBOL_GPL(HYPERVISOR_platform_op_raw);
EXPORT_SYMBOL_GPL(HYPERVISOR_multicall);
EXPORT_SYMBOL_GPL(HYPERVISOR_vm_assist);
EXPORT_SYMBOL_GPL(HYPERVISOR_dm_op);
-EXPORT_SYMBOL_GPL(privcmd_call);
+EXPORT_SYMBOL_GPL(arch_privcmd_call);
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index b11bba542fac..277078c7da49 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -94,7 +94,7 @@ HYPERCALL2(multicall);
HYPERCALL2(vm_assist);
HYPERCALL3(dm_op);
-ENTRY(privcmd_call)
+ENTRY(arch_privcmd_call)
stmdb sp!, {r4}
mov r12, r0
mov r0, r1
@@ -119,4 +119,4 @@ ENTRY(privcmd_call)
ldm sp!, {r4}
ret lr
-ENDPROC(privcmd_call);
+ENDPROC(arch_privcmd_call);
diff --git a/arch/arm64/include/asm/xen/hypercall.h
b/arch/arm64/include/asm/xen/hypercall.h
index 3522cbaed316..1a74fb28607f 100644
--- a/arch/arm64/include/asm/xen/hypercall.h
+++ b/arch/arm64/include/asm/xen/hypercall.h
@@ -1 +1,29 @@
+#ifndef _ASM_ARM64_XEN_HYPERCALL_H
+#define _ASM_ARM64_XEN_HYPERCALL_H
#include
+#include
+
+static inline long privcmd_call(unsigned int call, unsigned long a1,
+ unsigned long a2, unsigned long a3,
+ unsigned long a4, unsigned long a5)
+{
+ long rv;
+
+ /*
+* Privcmd calls are issued by the userspace. The kernel needs to
+* enable access to TTBR0_EL1 as the hypervisor would issue stage 1
+* translations to user memory via AT instructions. Since AT
+* instructions are not affected by the PAN bit (ARMv8.1), we only
+* need the explicit uaccess_enable/disable if the TTBR0 PAN emulation
+* is enabled (it implies that hardware UAO and PAN disabled).
+*/
+ uaccess_ttbr0_enable();
+ rv = arch_privcmd_call(call, a1, a2, a3, a4, a5);
+ /*
+* Disable userspace access from kernel once the hyp call completed.
+*/
+ uaccess_ttbr0_disable();
+
+ return rv;
+}
+#endif /* _ASM_ARM64_XEN_HYPERCALL_H */
diff --git a/arch/arm64/xen/hypercall.S b/arch/arm64/xen/hypercall.S
index c5f05c4a4d00..921611778d2a 100644
--- a/arch/arm64/xen/hypercall.S
+++ b/arch/arm64/xen/hypercall.S
@@ -49,7 +49,6 @@
#include
#include
-#include
#include
@@ -86,27 +85,13 @@ HYPERCALL2(multicall);
HYPERCALL2(vm_assist);
HYPERCALL3(dm_op);
-ENTRY(privcmd_call)
+ENTRY(arch_privcmd_call)
mov x16, x0
mov x0, x1
mov x1, x2
mov x2, x3
mov x3, x4
mov x4, x5
- /*
-* Privcmd calls are issued by the userspace. The kernel needs to
-* enable access to TTBR0_EL1 as the hypervisor would issue stage 1
-* translations to user memory via AT instructions. Since AT
-* instructions are not affected by the PAN bit (ARMv8.1), we only
-* need the explicit uaccess_enable/disable if the TTBR0 PAN
[Xen-devel] [PATCH 3/3] arm64: remove the rest of asm-uaccess.h
The __uaccess_ttbr0_disable and __uaccess_ttbr0_enable, are the last two macros defined in asm-uaccess.h. For now move them to entry.S where they are used. Eventually, these macros should be replaced with C wrappers to reduce the maintenance burden. Also, once these macros are unified with the C counterparts, it is a good idea to check that PAN is in correct state on every enable/disable calls. Signed-off-by: Pavel Tatashin --- arch/arm64/include/asm/asm-uaccess.h | 39 arch/arm64/kernel/entry.S| 27 ++- arch/arm64/lib/clear_user.S | 2 +- arch/arm64/lib/copy_from_user.S | 2 +- arch/arm64/lib/copy_in_user.S| 2 +- arch/arm64/lib/copy_to_user.S| 2 +- arch/arm64/mm/cache.S| 1 - 7 files changed, 30 insertions(+), 45 deletions(-) delete mode 100644 arch/arm64/include/asm/asm-uaccess.h diff --git a/arch/arm64/include/asm/asm-uaccess.h b/arch/arm64/include/asm/asm-uaccess.h deleted file mode 100644 index fba2a69f7fef.. --- a/arch/arm64/include/asm/asm-uaccess.h +++ /dev/null @@ -1,39 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0 */ -#ifndef __ASM_ASM_UACCESS_H -#define __ASM_ASM_UACCESS_H - -#include -#include -#include -#include -#include - -/* - * User access enabling/disabling macros. - */ -#ifdef CONFIG_ARM64_SW_TTBR0_PAN - .macro __uaccess_ttbr0_disable, tmp1 - mrs \tmp1, ttbr1_el1// swapper_pg_dir - bic \tmp1, \tmp1, #TTBR_ASID_MASK - sub \tmp1, \tmp1, #RESERVED_TTBR0_SIZE // reserved_ttbr0 just before swapper_pg_dir - msr ttbr0_el1, \tmp1// set reserved TTBR0_EL1 - isb - add \tmp1, \tmp1, #RESERVED_TTBR0_SIZE - msr ttbr1_el1, \tmp1// set reserved ASID - isb - .endm - - .macro __uaccess_ttbr0_enable, tmp1, tmp2 - get_current_task \tmp1 - ldr \tmp1, [\tmp1, #TSK_TI_TTBR0] // load saved TTBR0_EL1 - mrs \tmp2, ttbr1_el1 - extr\tmp2, \tmp2, \tmp1, #48 - ror \tmp2, \tmp2, #16 - msr ttbr1_el1, \tmp2// set the active ASID - isb - msr ttbr0_el1, \tmp1// set the non-PAN TTBR0_EL1 - isb - .endm -#endif - -#endif diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index 583f71abbe98..446d90ab31af 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -22,8 +22,8 @@ #include #include #include +#include #include -#include #include /* @@ -143,6 +143,31 @@ alternative_cb_end #endif .endm +#ifdef CONFIG_ARM64_SW_TTBR0_PAN + .macro __uaccess_ttbr0_disable, tmp1 + mrs \tmp1, ttbr1_el1// swapper_pg_dir + bic \tmp1, \tmp1, #TTBR_ASID_MASK + sub \tmp1, \tmp1, #RESERVED_TTBR0_SIZE // reserved_ttbr0 just before swapper_pg_dir + msr ttbr0_el1, \tmp1// set reserved TTBR0_EL1 + isb + add \tmp1, \tmp1, #RESERVED_TTBR0_SIZE + msr ttbr1_el1, \tmp1// set reserved ASID + isb + .endm + + .macro __uaccess_ttbr0_enable, tmp1, tmp2 + get_current_task \tmp1 + ldr \tmp1, [\tmp1, #TSK_TI_TTBR0] // load saved TTBR0_EL1 + mrs \tmp2, ttbr1_el1 + extr\tmp2, \tmp2, \tmp1, #48 + ror \tmp2, \tmp2, #16 + msr ttbr1_el1, \tmp2// set the active ASID + isb + msr ttbr0_el1, \tmp1// set the non-PAN TTBR0_EL1 + isb + .endm +#endif + .macro kernel_entry, el, regsize = 64 .if \regsize == 32 mov w0, w0 // zero upper 32 bits of x0 diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S index aeafc03e961a..b0b4a86a09e2 100644 --- a/arch/arm64/lib/clear_user.S +++ b/arch/arm64/lib/clear_user.S @@ -6,7 +6,7 @@ */ #include -#include +#include #include .text diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S index ebb3c06cbb5d..142bc7505518 100644 --- a/arch/arm64/lib/copy_from_user.S +++ b/arch/arm64/lib/copy_from_user.S @@ -5,7 +5,7 @@ #include -#include +#include #include #include diff --git a/arch/arm64/lib/copy_in_user.S b/arch/arm64/lib/copy_in_user.S index 3d8153a1ebce..04dc48ca26f7 100644 --- a/arch/arm64/lib/copy_in_user.S +++ b/arch/arm64/lib/copy_in_user.S @@ -7,7 +7,7 @@ #include -#include +#include #include #include diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S index 357eae2c18eb..8f3218ae88ab 100644 --- a/arch/arm64/lib/copy_to_user.S +++ b/arch/arm64/lib/copy_to_user.S @@ -5,7 +5,7 @@ #include -#include +#include #include #include diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S index a48b6dba304e..f7130c30d6e3 100644 ---
[Xen-devel] [PATCH 0/3] Use C inlines for uaccess
Changelog v3: - Added Acked-by from Stefano Stabellini - Addressed comments from Mark Rutland v2: - Addressed Russell King's concern by not adding uaccess_* to ARM. - Removed the accidental change to xtensa Convert the remaining uaccess_* calls from ASM macros to C inlines. These patches apply against linux-next. I boot tested ARM64, and compile tested ARM change Pavel Tatashin (3): arm/arm64/xen: use C inlines for privcmd_call arm64: remove uaccess_ttbr0 asm macros from cache functions arm64: remove the rest of asm-uaccess.h arch/arm/include/asm/assembler.h | 2 +- arch/arm/include/asm/xen/hypercall.h | 10 + arch/arm/xen/enlighten.c | 2 +- arch/arm/xen/hypercall.S | 4 +- arch/arm64/include/asm/asm-uaccess.h | 61 -- arch/arm64/include/asm/cacheflush.h| 39 ++-- arch/arm64/include/asm/xen/hypercall.h | 28 arch/arm64/kernel/entry.S | 27 +++- arch/arm64/lib/clear_user.S| 2 +- arch/arm64/lib/copy_from_user.S| 2 +- arch/arm64/lib/copy_in_user.S | 2 +- arch/arm64/lib/copy_to_user.S | 2 +- arch/arm64/mm/cache.S | 42 ++ arch/arm64/mm/flush.c | 2 +- arch/arm64/xen/hypercall.S | 19 +--- include/xen/arm/hypercall.h| 12 ++--- 16 files changed, 130 insertions(+), 126 deletions(-) delete mode 100644 arch/arm64/include/asm/asm-uaccess.h -- 2.24.0 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions
We currently duplicate the logic to enable/disable uaccess via TTBR0,
with C functions and assembly macros. This is a maintenenace burden
and is liable to lead to subtle bugs, so let's get rid of the assembly
macros, and always use the C functions. This requires refactoring
some assembly functions to have a C wrapper.
Signed-off-by: Pavel Tatashin
---
arch/arm64/include/asm/asm-uaccess.h | 22 ---
arch/arm64/include/asm/cacheflush.h | 39 --
arch/arm64/mm/cache.S| 41 +---
arch/arm64/mm/flush.c| 2 +-
4 files changed, 50 insertions(+), 54 deletions(-)
diff --git a/arch/arm64/include/asm/asm-uaccess.h
b/arch/arm64/include/asm/asm-uaccess.h
index f68a0e64482a..fba2a69f7fef 100644
--- a/arch/arm64/include/asm/asm-uaccess.h
+++ b/arch/arm64/include/asm/asm-uaccess.h
@@ -34,28 +34,6 @@
msr ttbr0_el1, \tmp1// set the non-PAN TTBR0_EL1
isb
.endm
-
- .macro uaccess_ttbr0_disable, tmp1, tmp2
-alternative_if_not ARM64_HAS_PAN
- save_and_disable_irq \tmp2 // avoid preemption
- __uaccess_ttbr0_disable \tmp1
- restore_irq \tmp2
-alternative_else_nop_endif
- .endm
-
- .macro uaccess_ttbr0_enable, tmp1, tmp2, tmp3
-alternative_if_not ARM64_HAS_PAN
- save_and_disable_irq \tmp3 // avoid preemption
- __uaccess_ttbr0_enable \tmp1, \tmp2
- restore_irq \tmp3
-alternative_else_nop_endif
- .endm
-#else
- .macro uaccess_ttbr0_disable, tmp1, tmp2
- .endm
-
- .macro uaccess_ttbr0_enable, tmp1, tmp2, tmp3
- .endm
#endif
#endif
diff --git a/arch/arm64/include/asm/cacheflush.h
b/arch/arm64/include/asm/cacheflush.h
index 665c78e0665a..c0b265e12d9d 100644
--- a/arch/arm64/include/asm/cacheflush.h
+++ b/arch/arm64/include/asm/cacheflush.h
@@ -61,16 +61,49 @@
* - kaddr - page address
* - size - region size
*/
-extern void __flush_icache_range(unsigned long start, unsigned long end);
-extern int invalidate_icache_range(unsigned long start, unsigned long end);
+extern void __asm_flush_icache_range(unsigned long start, unsigned long end);
+extern long __asm_flush_cache_user_range(unsigned long start,
+unsigned long end);
+extern int asm_invalidate_icache_range(unsigned long start,
+ unsigned long end);
extern void __flush_dcache_area(void *addr, size_t len);
extern void __inval_dcache_area(void *addr, size_t len);
extern void __clean_dcache_area_poc(void *addr, size_t len);
extern void __clean_dcache_area_pop(void *addr, size_t len);
extern void __clean_dcache_area_pou(void *addr, size_t len);
-extern long __flush_cache_user_range(unsigned long start, unsigned long end);
extern void sync_icache_aliases(void *kaddr, unsigned long len);
+static inline void __flush_icache_range(unsigned long start, unsigned long end)
+{
+ uaccess_ttbr0_enable();
+ __asm_flush_icache_range(start, end);
+ uaccess_ttbr0_disable();
+}
+
+static inline void __flush_cache_user_range(unsigned long start,
+ unsigned long end)
+{
+ uaccess_ttbr0_enable();
+ __asm_flush_cache_user_range(start, end);
+ uaccess_ttbr0_disable();
+}
+
+static inline int invalidate_icache_range(unsigned long start,
+ unsigned long end)
+{
+ int rv;
+
+ if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
+ isb();
+ return 0;
+ }
+ uaccess_ttbr0_enable();
+ rv = asm_invalidate_icache_range(start, end);
+ uaccess_ttbr0_disable();
+
+ return rv;
+}
+
static inline void flush_icache_range(unsigned long start, unsigned long end)
{
__flush_icache_range(start, end);
diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
index db767b072601..a48b6dba304e 100644
--- a/arch/arm64/mm/cache.S
+++ b/arch/arm64/mm/cache.S
@@ -15,7 +15,7 @@
#include
/*
- * flush_icache_range(start,end)
+ * __asm_flush_icache_range(start,end)
*
* Ensure that the I and D caches are coherent within specified region.
* This is typically used when code has been written to a memory region,
@@ -24,11 +24,11 @@
* - start - virtual start address of region
* - end - virtual end address of region
*/
-ENTRY(__flush_icache_range)
+ENTRY(__asm_flush_icache_range)
/* FALLTHROUGH */
/*
- * __flush_cache_user_range(start,end)
+ * __asm_flush_cache_user_range(start,end)
*
* Ensure that the I and D caches are coherent within specified region.
* This is typically used when code has been written to a memory region,
@@ -37,8 +37,7 @@ ENTRY(__flush_icache_range)
* - start - virtual start address of region
* - end - virtual end address of region
*/
-ENTRY(__flush_cache_user_range)
Re: [Xen-devel] Ping: [PATCH v2] build: provide option to disambiguate symbol names
On 21/11/2019 08:34, Jan Beulich wrote: > On 20.11.2019 18:13, Andrew Cooper wrote: >> On 20/11/2019 16:40, Jürgen Groß wrote: >>> On 20.11.19 17:30, Jan Beulich wrote: On 08.11.2019 12:18, Jan Beulich wrote: > The .file assembler directives generated by the compiler do not include > any path components (gcc) or just the ones specified on the command > line > (clang, at least version 5), and hence multiple identically named > source > files (in different directories) may produce identically named static > symbols (in their kallsyms representation). The binary diffing > algorithm > used by xen-livepatch, however, depends on having unique symbols. > > Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build) > behavior, and if enabled use objcopy to prepend the (relative to the > xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note > that this build option is made no longer depend on LIVEPATCH, but > merely > defaults to its setting now. > > Conditionalize explicit .file directive insertion in C files where it > exists just to disambiguate names in a less generic manner; note that > at the same time the redundant emission of STT_FILE symbols gets > suppressed for clang. Assembler files as well as multiply compiled C > ones using __OBJECT_FILE__ are left alone for the time being. > > Since we now expect there not to be any duplicates anymore, also don't > force the selection of the option to 'n' anymore in allrandom.config. > Similarly COVERAGE no longer suppresses duplicate symbol warnings if > enforcement is in effect, which in turn allows > SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on > !ENFORCE_UNIQUE_SYMBOLS. > > Signed-off-by: Jan Beulich I've got acks from Konrad and Wei, but still need an x86 and a release one here. Andrew? Or alternatively - Jürgen, would you rather not see this go in anymore? >>> In case the needed x86 Ack is coming in before RC3 I'm fine to give my >>> Release-ack, but I'm hesitant to take it later. >> Has anyone actually tried building a livepatch with this change in place? > Actually - what is your concern here? The exact spelling of symbols > names should be of no interest to the tool. After all the compiler is > free to invent all sorts of names for its local symbols, including > the ones we would produce with this change in place. All the tool > cares about is that the names be unambiguous. Hence any (theoretical) > regression here would be a bug in the tools, which imo is no reason > to delay this change any further. (Granted I should have got to it > earlier, but it had been continuing to get deferred.) This might all be true (theoretically). The livepatch build tools are fragile and very sensitive to how the object files are laid out. There is a very real risk that this change accidentally breaks livepatching totally, even on GCC builds. Were this to happen, it would be yet another 4.13 regression. This is a change to fix a concrete livepatch issue with Clang. Sure - it resolves the symbol uniqueness failures for the in-tree build, but considering the risks to the area you are modifying, the fact you haven't even done a dev test of a livepatch build on GCC means that the patch as a whole has not had what I would consider a reasonable amount of testing. Luckily for you, Ross and Sergey have agreed to smoke test this with some livepatches. They will report on this thread with their findings. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v4] x86/vmx: always sync PIR to IRR before vmentry
On 11/27/19 7:48 AM, Roger Pau Monne wrote: > When using posted interrupts on Intel hardware it's possible that the > vCPU resumes execution with a stale local APIC IRR register because > depending on the interrupts to be injected vlapic_has_pending_irq > might not be called, and thus PIR won't be synced into IRR. > > Fix this by making sure PIR is always synced to IRR in > hvm_vcpu_has_pending_irq regardless of what interrupts are pending. > > Reported-by: Joe Jin > Signed-off-by: Roger Pau Monné > --- > Cc: Juergen Gross Patch works for me. Tested-by: Joe Jin Thanks, Joe ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [XEN PATCH v3 06/11] Add NR_SGIS and NR_PPIS definitions to irq.h
Hi,
On 15/11/2019 20:10, Stewart Hildebrand wrote:
These will be used in a follow-up patch.
Signed-off-by: Stewart Hildebrand
---
v3: new patch
---
xen/include/asm-arm/irq.h | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/xen/include/asm-arm/irq.h b/xen/include/asm-arm/irq.h
index 3b37a21c06..367fe6269c 100644
--- a/xen/include/asm-arm/irq.h
+++ b/xen/include/asm-arm/irq.h
@@ -33,7 +33,9 @@ struct arch_irq_desc {
unsigned int type;
};
-#define NR_LOCAL_IRQS 32
+#define NR_SGIS 16
+#define NR_PPIS 16
+#define NR_LOCAL_IRQS (NR_SGIS + NR_PPIS)
We have already NR_GIC_SGI (see include/asm-arm/gic.h) and VGIC_NR_SGIS
(see include/asm-arm/new_vgic.h).
So I would rather not want to define the same value (with the same
meaning) a third time.
Note that I am ok if the two existing one are dropped in favor of NR_SGIS.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] AMD/IOMMU: Render IO_PAGE_FAULT errors in a more useful manner
On 27/11/2019 09:40, Roger Pau Monné wrote: > On Tue, Nov 26, 2019 at 03:01:12PM +, Andrew Cooper wrote: >> Print the PCI coordinates in its common format and use d%u notation for the >> domain. As well as printing flags, decode them. IO_PAGE_FAULT is used for >> interrupt remapping errors as well as DMA remapping errors. >> >> Before: >> (XEN) AMD-Vi: IO_PAGE_FAULT: domain = 0, device id = 0xa1, fault address = >> 0xbf695000, flags = 0x10 >> (XEN) AMD-Vi: IO_PAGE_FAULT: domain = 0, device id = 0xa1, fault address = >> 0xbf695040, flags = 0x10 >> (XEN) AMD-Vi: IO_PAGE_FAULT: domain = 0, device id = 0xa1, fault address = >> 0xfff0, flags = 0x30 >> (XEN) AMD-Vi: IO_PAGE_FAULT: domain = 0, device id = 0xa1, fault address = >> 0x1, flags = 0x30 >> (XEN) AMD-Vi: IO_PAGE_FAULT: domain = 0, device id = 0xa1, fault address = >> 0x10040, flags = 0x30 >> >> After: >> (XEN) AMD-Vi: IO_PAGE_FAULT: :00:14.1 d0 addr bf5fc000 flags >> 0x10 PR >> (XEN) AMD-Vi: IO_PAGE_FAULT: :00:14.1 d0 addr bf5fc040 flags >> 0x10 PR >> (XEN) AMD-Vi: IO_PAGE_FAULT: :00:14.1 d0 addr fff0 flags >> 0x30 RW PR >> (XEN) AMD-Vi: IO_PAGE_FAULT: :00:14.1 d0 addr 0001 flags >> 0x30 RW PR >> (XEN) AMD-Vi: IO_PAGE_FAULT: :00:14.1 d0 addr 00010040 flags >> 0x30 RW PR > Nit: I would place the domain id information at the beginning (since > that's more similar to gprintk format), and maybe drop the AMD-Vi > prefix, it's not very useful IMO: > > (XEN) d0 IO_PAGE_FAULT :00:14.1 addr 00010040 flags 0x30 RW PR > > But I'm not specially concerned. So I debated not using d%d format. This is the DTE's "domain_id" (a.k.a. Tag in the IO-TLB) field which by convention we set to the domid of the owning device, but isn't necessarily the best option. In particular, it might be wise to use domid + 1 and choke if we ever find 0 in use. > >> +uint64_t addr = *(uint64_t *)(entry + 2); >> + >> +printk(XENLOG_ERR "AMD-Vi: %s: %04x:%02x:%02x.%u d%d addr >> %016"PRIx64 >> + " flags %#x%s%s%s%s%s%s%s%s%s%s\n", >> + code_str, iommu->seg, PCI_BUS(device_id), >> PCI_SLOT(device_id), >> + PCI_FUNC(device_id), domain_id, addr, flags, >> + (flags & 0xe00) ? " ??" : "", >> + (flags & 0x100) ? " TR" : "", >> + (flags & 0x080) ? " RZ" : "", >> + (flags & 0x040) ? " PE" : "", >> + (flags & 0x020) ? " RW" : "", >> + (flags & 0x010) ? " PR" : "", >> + (flags & 0x008) ? " I" : "", >> + (flags & 0x004) ? " US" : "", >> + (flags & 0x002) ? " NX" : "", >> + (flags & 0x001) ? " GN" : ""); > I wold rather have those added with proper defined names to > amd-iommu-defs.h. All of this is in desperate need of turning into real C structs, rather than being opencoded in terms of u32[] and offsets/shifts/masks, but such a change definitely isn't appropriate for backport. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] arm64: remove the rest of asm-uaccess.h
On Wed, Nov 27, 2019 at 12:01 PM Mark Rutland wrote: > > On Wed, Nov 27, 2019 at 11:09:35AM -0500, Pavel Tatashin wrote: > > On Wed, Nov 27, 2019 at 11:03 AM Mark Rutland wrote: > > > > > > On Wed, Nov 27, 2019 at 10:31:54AM -0500, Pavel Tatashin wrote: > > > > On Wed, Nov 27, 2019 at 10:12 AM Mark Rutland > > > > wrote: > > > > > > > > > > On Thu, Nov 21, 2019 at 09:24:06PM -0500, Pavel Tatashin wrote: > > > > > > The __uaccess_ttbr0_disable and __uaccess_ttbr0_enable, > > > > > > are the last two macros defined in asm-uaccess.h. > > > > > > > > > > > > Replace them with C wrappers and call C functions from > > > > > > kernel_entry and kernel_exit. > > > > > > > > > > For now, please leave those as-is. > > > > > > > > > > I don't think we want to have out-of-line C wrappers in the middle of > > > > > the entry assembly where we don't have a complete kernel environment. > > > > > The use in entry code can also assume non-preemptibility, while the C > > > > > functions have to explcitily disable that. > > > > > > > > I do not understand, if C function is called form non-preemptible > > > > context it stays non-preemptible. kernel_exit already may call C > > > > functions around the time __uaccess_ttbr0_enable is called (it may > > > > call post_ttbr_update_workaround), and that C functions does not do > > > > explicit preempt disable: > > > > > > Sorry, I meant that IRQs are disabled here. > > > > > > The C wrapper calls __uaccess_ttbr0_enable(), which calls > > > local_irq_save() and local_irq_restore(). Those are pointless in the > > > bowels of the entry code, and potentially expensive if IRQ prio masking > > > is in use. > > > > > > I'd rather not add more out-of-line C code calls here right now as I'd > > > prefer to factor out the logic to C in a better way. > > > > Ah, yes, this makes sense. I could certainly factor out C calls in a > > better way, or is this something you want to work on? > > I'm hoping to do that as part of ongoing entry-deasm work, now that a > lot of the prerequisite work was merged in v5.4. OK, I will send new patches with what we agreed on, and your comments addressed. > > > Without removing these assembly macros I do not think we want to > > address this suggestion from Kees Cook: > > https://lore.kernel.org/lkml/ca+ck2bcbs2fkotmtfm13iv3u5tbpwpocsyeep352dve-gs9...@mail.gmail.com/ > > In the mean time, we could add checks around addr_limit_user_check(), > and in the context-switch path. I have some preparatory cleanup to allow > for the context-switch check, which I'll send out at -rc1. That was what > I used to detect the case you reported previously. Sounds good. Thank you, Pasha ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] x86 / iommu: set up a scratch page in the quarantine domain
This patch introduces a new iommu_op to facilitate a per-implementation
quarantine set up, and then further code for x86 implementations
(amd and vtd) to set up a read-only scratch page to serve as the source
for DMA reads whilst a device is assigned to dom_io. DMA writes will
continue to fault as before.
The reason for doing this is that some hardware may continue to re-try
DMA (despite FLR) in the event of an error, or even BME being cleared, and
will fail to deal with DMA read faults gracefully. Having a scratch page
mapped will allow pending DMA reads to complete and thus such buggy
hardware will eventually be quiesced.
NOTE: These modifications are restricted to x86 implementations only as
the buggy h/w I am aware of is only used with Xen in an x86
environment. ARM may require similar code but, since I am not
aware of the need, this patch does not modify any ARM implementation.
Signed-off-by: Paul Durrant
---
Cc: Jan Beulich
Cc: Andrew Cooper
Cc: Kevin Tian
Cc: Wei Liu
Cc: "Roger Pau Monné"
v2:
- Addressed comments from Jan
There is still the open question of whether use of a scratch page ought
to be gated on something, either are run-time or compile-time.
---
xen/drivers/passthrough/amd/iommu_map.c | 62
xen/drivers/passthrough/amd/pci_amd_iommu.c | 14 ++--
xen/drivers/passthrough/iommu.c | 20 +-
xen/drivers/passthrough/vtd/iommu.c | 72 +++
xen/include/asm-x86/hvm/svm/amd-iommu-proto.h | 3 +
xen/include/xen/iommu.h | 1 +
6 files changed, 148 insertions(+), 24 deletions(-)
diff --git a/xen/drivers/passthrough/amd/iommu_map.c
b/xen/drivers/passthrough/amd/iommu_map.c
index cd5c7de7c5..54e1d132d9 100644
--- a/xen/drivers/passthrough/amd/iommu_map.c
+++ b/xen/drivers/passthrough/amd/iommu_map.c
@@ -560,6 +560,68 @@ int amd_iommu_reserve_domain_unity_map(struct domain
*domain,
return rt;
}
+int __init amd_iommu_quarantine_init(struct domain *d)
+{
+struct domain_iommu *hd = dom_iommu(d);
+unsigned long max_gfn =
+PFN_DOWN((1ul << DEFAULT_DOMAIN_ADDRESS_WIDTH) - 1);
+unsigned int level = amd_iommu_get_paging_mode(max_gfn);
+struct amd_iommu_pte *table;
+
+if ( hd->arch.root_table )
+{
+ASSERT_UNREACHABLE();
+return 0;
+}
+
+spin_lock(>arch.mapping_lock);
+
+hd->arch.root_table = alloc_amd_iommu_pgtable();
+if ( !hd->arch.root_table )
+goto out;
+
+table = __map_domain_page(hd->arch.root_table);
+while ( level )
+{
+struct page_info *pg;
+unsigned int i;
+
+/*
+ * The pgtable allocator is fine for the leaf page, as well as
+ * page table pages, and the resulting allocations are always
+ * zeroed.
+ */
+pg = alloc_amd_iommu_pgtable();
+if ( !pg )
+break;
+
+for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ )
+{
+struct amd_iommu_pte *pde = [i];
+
+/*
+ * PDEs are essentially a subset of PTEs, so this function
+ * is fine to use even at the leaf.
+ */
+set_iommu_pde_present(pde, mfn_x(page_to_mfn(pg)), level - 1,
+ false, true);
+}
+
+unmap_domain_page(table);
+table = __map_domain_page(pg);
+level--;
+}
+unmap_domain_page(table);
+
+ out:
+spin_unlock(>arch.mapping_lock);
+
+amd_iommu_flush_all_pages(d);
+
+/* Pages leaked in failure case */
+return level ? -ENOMEM : 0;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c
b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 75a0f1b4ab..4da6518773 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -95,10 +95,6 @@ static void amd_iommu_setup_domain_device(
u8 bus = pdev->bus;
const struct domain_iommu *hd = dom_iommu(domain);
-/* dom_io is used as a sentinel for quarantined devices */
-if ( domain == dom_io )
-return;
-
BUG_ON( !hd->arch.root_table || !hd->arch.paging_mode ||
!iommu->dev_table.buffer );
@@ -235,7 +231,7 @@ static int __must_check allocate_domain_resources(struct
domain_iommu *hd)
return rc;
}
-static int get_paging_mode(unsigned long entries)
+int amd_iommu_get_paging_mode(unsigned long entries)
{
int level = 1;
@@ -257,7 +253,8 @@ static int amd_iommu_domain_init(struct domain *d)
/* For pv and dom0, stick with get_paging_mode(max_page)
* For HVM dom0, use 2 level page table at first */
-hd->arch.paging_mode = is_hvm_domain(d) ? 2 : get_paging_mode(max_page);
+hd->arch.paging_mode = is_hvm_domain(d) ?
+2 : amd_iommu_get_paging_mode(max_page);
return 0;
}
@@ -290,10 +287,6 @@ static void amd_iommu_disable_domain_device(const struct
domain *domain,
int req_id;
u8
Re: [Xen-devel] [PATCH 2/2] AMD/IOMMU: Render IO_PAGE_FAULT errors in a more useful manner
On 27/11/2019 17:02, Jan Beulich wrote:
> On 26.11.2019 16:01, Andrew Cooper wrote:
>> @@ -560,18 +557,26 @@ static void parse_event_log_entry(struct amd_iommu
>> *iommu, u32 entry[])
>>
>> if ( code == IOMMU_EVENT_IO_PAGE_FAULT )
>> {
>> -device_id = iommu_get_devid_from_event(entry[0]);
>> -domain_id = get_field_from_reg_u32(entry[1],
>> - IOMMU_EVENT_DOMAIN_ID_MASK,
>> - IOMMU_EVENT_DOMAIN_ID_SHIFT);
>> -flags = get_field_from_reg_u32(entry[1],
>> - IOMMU_EVENT_FLAGS_MASK,
>> - IOMMU_EVENT_FLAGS_SHIFT);
>> -addr= (u64*) (entry + 2);
>> -printk(XENLOG_ERR "AMD-Vi: "
>> - "%s: domain = %d, device id = %#x, "
>> - "fault address = %#"PRIx64", flags = %#x\n",
>> - code_str, domain_id, device_id, *addr, flags);
>> +unsigned int bdf;
>> +uint16_t device_id = MASK_EXTR(entry[0], IOMMU_CMD_DEVICE_ID_MASK);
> s/CMD/EVENT/ and then
> Acked-by: Jan Beulich
Oops yes. That was a consequence of following
#define iommu_get_devid_from_event iommu_get_devid_from_cmd
to get the mask to use.
These really need turning into structs, but that is a job for a
different day.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] build: provide option to disambiguate symbol names
On Fri, Nov 08, 2019 at 12:18:40PM +0100, Jan Beulich wrote: > The .file assembler directives generated by the compiler do not include > any path components (gcc) or just the ones specified on the command line > (clang, at least version 5), and hence multiple identically named source > files (in different directories) may produce identically named static > symbols (in their kallsyms representation). The binary diffing algorithm > used by xen-livepatch, however, depends on having unique symbols. > > Make the ENFORCE_UNIQUE_SYMBOLS Kconfig option control the (build) > behavior, and if enabled use objcopy to prepend the (relative to the > xen/ subdirectory) path to the compiler invoked STT_FILE symbols. Note > that this build option is made no longer depend on LIVEPATCH, but merely > defaults to its setting now. > > Conditionalize explicit .file directive insertion in C files where it > exists just to disambiguate names in a less generic manner; note that > at the same time the redundant emission of STT_FILE symbols gets > suppressed for clang. Assembler files as well as multiply compiled C > ones using __OBJECT_FILE__ are left alone for the time being. > > Since we now expect there not to be any duplicates anymore, also don't > force the selection of the option to 'n' anymore in allrandom.config. > Similarly COVERAGE no longer suppresses duplicate symbol warnings if > enforcement is in effect, which in turn allows > SUPPRESS_DUPLICATE_SYMBOL_WARNINGS to simply depend on > !ENFORCE_UNIQUE_SYMBOLS. > > Signed-off-by: Jan Beulich Reviewed-by: Roger Pau Monné And tested on FreeBSD with clang 8 and elftoolchain objcopy. Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] arm64: remove the rest of asm-uaccess.h
On Wed, Nov 27, 2019 at 11:09:35AM -0500, Pavel Tatashin wrote: > On Wed, Nov 27, 2019 at 11:03 AM Mark Rutland wrote: > > > > On Wed, Nov 27, 2019 at 10:31:54AM -0500, Pavel Tatashin wrote: > > > On Wed, Nov 27, 2019 at 10:12 AM Mark Rutland > > > wrote: > > > > > > > > On Thu, Nov 21, 2019 at 09:24:06PM -0500, Pavel Tatashin wrote: > > > > > The __uaccess_ttbr0_disable and __uaccess_ttbr0_enable, > > > > > are the last two macros defined in asm-uaccess.h. > > > > > > > > > > Replace them with C wrappers and call C functions from > > > > > kernel_entry and kernel_exit. > > > > > > > > For now, please leave those as-is. > > > > > > > > I don't think we want to have out-of-line C wrappers in the middle of > > > > the entry assembly where we don't have a complete kernel environment. > > > > The use in entry code can also assume non-preemptibility, while the C > > > > functions have to explcitily disable that. > > > > > > I do not understand, if C function is called form non-preemptible > > > context it stays non-preemptible. kernel_exit already may call C > > > functions around the time __uaccess_ttbr0_enable is called (it may > > > call post_ttbr_update_workaround), and that C functions does not do > > > explicit preempt disable: > > > > Sorry, I meant that IRQs are disabled here. > > > > The C wrapper calls __uaccess_ttbr0_enable(), which calls > > local_irq_save() and local_irq_restore(). Those are pointless in the > > bowels of the entry code, and potentially expensive if IRQ prio masking > > is in use. > > > > I'd rather not add more out-of-line C code calls here right now as I'd > > prefer to factor out the logic to C in a better way. > > Ah, yes, this makes sense. I could certainly factor out C calls in a > better way, or is this something you want to work on? I'm hoping to do that as part of ongoing entry-deasm work, now that a lot of the prerequisite work was merged in v5.4. > Without removing these assembly macros I do not think we want to > address this suggestion from Kees Cook: > https://lore.kernel.org/lkml/ca+ck2bcbs2fkotmtfm13iv3u5tbpwpocsyeep352dve-gs9...@mail.gmail.com/ In the mean time, we could add checks around addr_limit_user_check(), and in the context-switch path. I have some preparatory cleanup to allow for the context-switch check, which I'll send out at -rc1. That was what I used to detect the case you reported previously. Thanks, Mark. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/2] AMD/IOMMU: Render IO_PAGE_FAULT errors in a more useful manner
On 26.11.2019 16:01, Andrew Cooper wrote:
> @@ -560,18 +557,26 @@ static void parse_event_log_entry(struct amd_iommu
> *iommu, u32 entry[])
>
> if ( code == IOMMU_EVENT_IO_PAGE_FAULT )
> {
> -device_id = iommu_get_devid_from_event(entry[0]);
> -domain_id = get_field_from_reg_u32(entry[1],
> - IOMMU_EVENT_DOMAIN_ID_MASK,
> - IOMMU_EVENT_DOMAIN_ID_SHIFT);
> -flags = get_field_from_reg_u32(entry[1],
> - IOMMU_EVENT_FLAGS_MASK,
> - IOMMU_EVENT_FLAGS_SHIFT);
> -addr= (u64*) (entry + 2);
> -printk(XENLOG_ERR "AMD-Vi: "
> - "%s: domain = %d, device id = %#x, "
> - "fault address = %#"PRIx64", flags = %#x\n",
> - code_str, domain_id, device_id, *addr, flags);
> +unsigned int bdf;
> +uint16_t device_id = MASK_EXTR(entry[0], IOMMU_CMD_DEVICE_ID_MASK);
s/CMD/EVENT/ and then
Acked-by: Jan Beulich
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/2] AMD/IOMMU: Always print IOMMU errors
On 27.11.2019 10:19, Roger Pau Monné wrote: > On Tue, Nov 26, 2019 at 03:01:11PM +, Andrew Cooper wrote: >> Unhandled IOMMU errors (i.e. not IO_PAGE_FAULT) should still be printed, and >> not hidden behind iommu=debug. >> >> While adjusting this, factor out the symbolic name handling to just one >> location exposing its off-by-one nature. >> >> Signed-off-by: Andrew Cooper > > LGTM: > > Reviewed-by: Roger Pau Monné Acked-by: Jan Beulich > I wonder however whether XENLOG_G_ERR should be used instead of > XENLOG_ERR in order to rate limit IOMMU faults triggered by guests. IO_PAGE_FAULT uses XENLOG_ERR as well, so I'd stick to it. If there are really massive amounts of faults, log spam won't be our only problem, I think. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling
On 11/27/19 4:43 PM, Durrant, Paul wrote:
>> -Original Message-
>> From: George Dunlap
>> Sent: 27 November 2019 16:34
>> To: Jan Beulich ; Durrant, Paul
>> Cc: AndrewCooper ; Anthony PERARD
>> ; Roger Pau Monné ;
>> Volodymyr Babchuk ; George Dunlap
>> ; Ian Jackson ;
>> Marek Marczykowski-Górecki ; Stefano
>> Stabellini ; xen-devel@lists.xenproject.org;
>> Konrad Rzeszutek Wilk ; Julien Grall
>> ; Wei Liu
>> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
>> max_maptrack_frames handling
>>
>> On 11/27/19 4:20 PM, Jan Beulich wrote:
>>> On 27.11.2019 17:14, Durrant, Paul wrote:
> From: Jan Beulich
> Sent: 27 November 2019 15:56
>
> On 27.11.2019 15:37, Paul Durrant wrote:
>> --- a/xen/arch/arm/setup.c
>> +++ b/xen/arch/arm/setup.c
>> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
> boot_phys_offset,
>> .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>> .max_evtchn_port = -1,
>> .max_grant_frames = gnttab_dom0_frames(),
>> -.max_maptrack_frames = opt_max_maptrack_frames,
>> +.max_maptrack_frames = -1,
>> };
>> int rc;
>>
>> --- a/xen/arch/x86/setup.c
>> +++ b/xen/arch/x86/setup.c
>> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
>> struct xen_domctl_createdomain dom0_cfg = {
>> .flags = IS_ENABLED(CONFIG_TBOOT) ?
>> XEN_DOMCTL_CDF_s3_integrity
> : 0,
>> .max_evtchn_port = -1,
>> -.max_grant_frames = opt_max_grant_frames,
>> -.max_maptrack_frames = opt_max_maptrack_frames,
>> +.max_grant_frames = -1,
>> +.max_maptrack_frames = -1,
>> };
>
> With these there's no need anymore for opt_max_maptrack_frames to
> be non-static. Sadly Arm still wants opt_max_grant_frames
> accessible in gnttab_dom0_frames().
Yes, I was about to make them static until I saw what the ARM code did.
>>>
>>> But the one that Arm doesn't need should become static now.
>>>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -1837,12 +1837,18 @@ active_alloc_failed:
>> return -ENOMEM;
>> }
>>
>> -int grant_table_init(struct domain *d, unsigned int
>> max_grant_frames,
>> - unsigned int max_maptrack_frames)
>> +int grant_table_init(struct domain *d, int max_grant_frames,
>> + int max_maptrack_frames)
>> {
>> struct grant_table *gt;
>> int ret = -ENOMEM;
>>
>> +/* Default to maximum value if no value was specified */
>> +if ( max_grant_frames < 0 )
>> +max_grant_frames = opt_max_grant_frames;
>> +if ( max_maptrack_frames < 0 )
>> +max_maptrack_frames = opt_max_maptrack_frames;
>> +
>> if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>
> I take it we don't expect people to specify 2^^31 or more
> frames for either option. It looks like almost everything
> here would cope, except for this very comparison. Nevertheless
> I wonder whether you wouldn't better confine both values to
> [0, INT_MAX] now, including when adjusted at runtime.
I can certainly remove the 'U' from the definition of
INITIAL_NR_GRANT_FRAMES,
>>>
>>> Oh, I didn't pay attention that is has a U on it - in this case
>>> the comparison above is fine.
>>>
but do you want me to make opt_max_grant_frames and
opt_max_maptrack_frames into signed ints and add signed parser
code too?
>>>
>>> Definitely not. They should remain unsigned quantities, but their
>>> values may need sanity checking now.
>>>
I also don't understand the 'adjusted at runtime' part.
>>>
>>> Well, for a command line drive value you could adjust an out of
>>> bounds value in some __init function. But for runtime modifiable
>>> settings you won't get away this easily.
>>
>> TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
>> long)(-1) or something, and explicitly compare to that. That leaves
>> open the possibility of having more sentinel values if we decided we
>> wanted them.
>
> I'm extremely confused now. What do you want me to compare and where?
>
> I assume we're talking about the opt_XXX values. Am I supposed to stop
> >INT_MAX being assigned to them? Or should I define local unsigned values for
> max_maptrack/grant_frames and simply initialize them to the passed-in arg (if
> >= 0) or the opt_XXX value otherwise.
In this version of the patch, you change the domctl arguments from
uint32_t to int32_t. I would leave them uint32_t, and if (
max_grant_frames == XENSOMETHING_MAX_DEFAULT ) max_grant_frames = opt_
Then the only invalid value we have to worry about is checking for
XENSOMETHING_MAX_DEFAULT.
This is a suggestion, and I wouldn't argue strongly if someone thought
it was a bad idea, but it seems like the most
Re: [Xen-devel] [PATCH v2] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed
> -Original Message- > From: Boris Ostrovsky > Sent: 27 November 2019 16:32 > To: Jan Beulich ; Durrant, Paul > Cc: Grall, Julien ; Andrew Cooper > ; Roger Pau Monné ; Jun > Nakajima ; Kevin Tian ; Wei > Liu ; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v2] xen/x86: vpmu: Unmap per-vCPU PMU page when the > domain is destroyed > > On 11/27/19 10:44 AM, Jan Beulich wrote: > > On 27.11.2019 13:00, Paul Durrant wrote: > >> --- a/xen/arch/x86/cpu/vpmu.c > >> +++ b/xen/arch/x86/cpu/vpmu.c > >> @@ -479,6 +479,8 @@ static int vpmu_arch_initialise(struct vcpu *v) > >> > >> if ( ret ) > >> printk(XENLOG_G_WARNING "VPMU: Initialization failed for > %pv\n", v); > >> +else > >> +vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED); > > That won't work I think. > > On Intel the context is allocated lazily for HVM/PVH guests during the > first MSR access. For example: > > core2_vpmu_do_wrmsr() -> > core2_vpmu_msr_common_check()): > if ( unlikely(!vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED)) && > !core2_vpmu_alloc_resource(current) ) > return 0; > > For PV guests the context *is* allocated from vmx_vpmu_initialise(). > > I don't remember why only PV does eager allocation but I think doing it > for all guests would make code much simpler and then this patch will be > correct. > Ok. Simpler if I leave setting the flag in the implementation code. I think clearing it in vcpu_arch_destroy() would still be correct in all cases. Paul > -boris > > > >> > >> return ret; > >> } > >> @@ -576,11 +578,36 @@ static void vpmu_arch_destroy(struct vcpu *v) > >> > >> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > >> } > >> + > >> +vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); > >> } > > Boris, > > > > I'd like to ask that you comment on this part of the change at > > least, as I seem to vaguely recall that things were intentionally > > not done this way originally. > > > > Paul, > > > > everything else looks god to me now. > > > > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable-smoke test] 144328: regressions - FAIL
flight 144328 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/144328/
Regressions :-(
Tests which did not succeed and are blocking,
including tests which could not be run:
test-armhf-armhf-xl 16 guest-start/debian.repeat fail REGR. vs. 144322
Tests which did not succeed, but are not blocking:
test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass
test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass
test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass
test-armhf-armhf-xl 13 migrate-support-checkfail never pass
test-armhf-armhf-xl 14 saverestore-support-checkfail never pass
version targeted for testing:
xen 9a400d1797ec7f77ffefeb5c4e17a8c2e8b91a12
baseline version:
xen 34c11725483beb45499f934c7e06e00b55f04ef4
Last test of basis 144322 2019-11-27 11:01:00 Z0 days
Testing same since 144328 2019-11-27 14:00:31 Z0 days1 attempts
People who touched revisions under test:
Igor Druzhinin
Jan Beulich
Julien Grall
Sergey Dyasli
jobs:
build-arm64-xsm pass
build-amd64 pass
build-armhf pass
build-amd64-libvirt pass
test-armhf-armhf-xl fail
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
Not pushing.
commit 9a400d1797ec7f77ffefeb5c4e17a8c2e8b91a12
Author: Julien Grall
Date: Tue Nov 26 13:30:23 2019 +
MAINTAINERS: Update path to the livepatch documentation
Commit d661611d08 "docs/markdown: Switch to using pandoc, and fix
underscore escaping" converted the livepatch documentation from markdown
to pandoc.
Update MAINTAINERS to reflect the change so the correct maintainers are
CCed to the patches.
Fixes: d661611d08 ("docs/markdown: Switch to using pandoc, and fix
underscore escaping")
Signed-off-by: Julien Grall
Acked-by: Jan Beulich
Release-acked-by: Juergen Gross
commit 72580a8d3c7ac70859437b69570de67dab668d9f
Author: Sergey Dyasli
Date: Wed Nov 27 10:04:30 2019 +
x86/microcode: refuse to load the same revision ucode
Currently if a user tries to live-load the same or older ucode revision
than CPU already has, he will get a single message in Xen log like:
(XEN) 128 cores are to update their microcode
No actual ucode loading will happen and this situation can be quite
confusing. Fix this by starting ucode update only when the provided
ucode revision is higher than the currently cached one (if any).
This is based on the property that if microcode_cache exists, all CPUs
in the system should have at least that ucode revision.
Additionally, print a user friendly message if no matching or newer
ucode can be found in the provided blob. This also requires ignoring
-ENODATA in AMD-side code, otherwise the message given to the user is:
(XEN) Parsing microcode blob error -61
Which actually means that a ucode blob was parsed fine, but no matching
ucode was found.
Signed-off-by: Sergey Dyasli
Reviewed-by: Chao Gao
Acked-by: Jan Beulich
Release-acked-by: Juergen Gross
commit 195b79a97e6721ba8830036f47d2454545f32e44
Author: Igor Druzhinin
Date: Tue Nov 26 17:08:19 2019 +
AMD/IOMMU: honour IR setting while pre-filling DTEs
IV bit shouldn't be set in DTE if interrupt remapping is not
enabled. It's a regression in behavior of "iommu=no-intremap"
option which otherwise would keep interrupt requests untranslated
for all of the devices in the system regardless of wether it's
described as valid in IVRS or not.
Signed-off-by: Igor Druzhinin
Reviewed-by: Andrew Cooper
Release-acked-by: Juergen Gross
(qemu changes not included)
___
Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling
> -Original Message-
> From: George Dunlap
> Sent: 27 November 2019 16:34
> To: Jan Beulich ; Durrant, Paul
> Cc: AndrewCooper ; Anthony PERARD
> ; Roger Pau Monné ;
> Volodymyr Babchuk ; George Dunlap
> ; Ian Jackson ;
> Marek Marczykowski-Górecki ; Stefano
> Stabellini ; xen-devel@lists.xenproject.org;
> Konrad Rzeszutek Wilk ; Julien Grall
> ; Wei Liu
> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
> max_maptrack_frames handling
>
> On 11/27/19 4:20 PM, Jan Beulich wrote:
> > On 27.11.2019 17:14, Durrant, Paul wrote:
> >>> From: Jan Beulich
> >>> Sent: 27 November 2019 15:56
> >>>
> >>> On 27.11.2019 15:37, Paul Durrant wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
> >>> boot_phys_offset,
> .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> .max_evtchn_port = -1,
> .max_grant_frames = gnttab_dom0_frames(),
> -.max_maptrack_frames = opt_max_maptrack_frames,
> +.max_maptrack_frames = -1,
> };
> int rc;
>
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
> >>> mbi_p)
> struct xen_domctl_createdomain dom0_cfg = {
> .flags = IS_ENABLED(CONFIG_TBOOT) ?
> XEN_DOMCTL_CDF_s3_integrity
> >>> : 0,
> .max_evtchn_port = -1,
> -.max_grant_frames = opt_max_grant_frames,
> -.max_maptrack_frames = opt_max_maptrack_frames,
> +.max_grant_frames = -1,
> +.max_maptrack_frames = -1,
> };
> >>>
> >>> With these there's no need anymore for opt_max_maptrack_frames to
> >>> be non-static. Sadly Arm still wants opt_max_grant_frames
> >>> accessible in gnttab_dom0_frames().
> >>
> >> Yes, I was about to make them static until I saw what the ARM code did.
> >
> > But the one that Arm doesn't need should become static now.
> >
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1837,12 +1837,18 @@ active_alloc_failed:
> return -ENOMEM;
> }
>
> -int grant_table_init(struct domain *d, unsigned int
> max_grant_frames,
> - unsigned int max_maptrack_frames)
> +int grant_table_init(struct domain *d, int max_grant_frames,
> + int max_maptrack_frames)
> {
> struct grant_table *gt;
> int ret = -ENOMEM;
>
> +/* Default to maximum value if no value was specified */
> +if ( max_grant_frames < 0 )
> +max_grant_frames = opt_max_grant_frames;
> +if ( max_maptrack_frames < 0 )
> +max_maptrack_frames = opt_max_maptrack_frames;
> +
> if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
> >>>
> >>> I take it we don't expect people to specify 2^^31 or more
> >>> frames for either option. It looks like almost everything
> >>> here would cope, except for this very comparison. Nevertheless
> >>> I wonder whether you wouldn't better confine both values to
> >>> [0, INT_MAX] now, including when adjusted at runtime.
> >>
> >> I can certainly remove the 'U' from the definition of
> >> INITIAL_NR_GRANT_FRAMES,
> >
> > Oh, I didn't pay attention that is has a U on it - in this case
> > the comparison above is fine.
> >
> >> but do you want me to make opt_max_grant_frames and
> >> opt_max_maptrack_frames into signed ints and add signed parser
> >> code too?
> >
> > Definitely not. They should remain unsigned quantities, but their
> > values may need sanity checking now.
> >
> >> I also don't understand the 'adjusted at runtime' part.
> >
> > Well, for a command line drive value you could adjust an out of
> > bounds value in some __init function. But for runtime modifiable
> > settings you won't get away this easily.
>
> TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
> long)(-1) or something, and explicitly compare to that. That leaves
> open the possibility of having more sentinel values if we decided we
> wanted them.
I'm extremely confused now. What do you want me to compare and where?
I assume we're talking about the opt_XXX values. Am I supposed to stop >INT_MAX
being assigned to them? Or should I define local unsigned values for
max_maptrack/grant_frames and simply initialize them to the passed-in arg (if
>= 0) or the opt_XXX value otherwise.
Paul
>
> -George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] clang: do not enable live-patching support
On 11/27/19 4:35 PM, Jürgen Groß wrote: > On 27.11.19 17:25, Jan Beulich wrote: >> On 27.11.2019 17:21, George Dunlap wrote: >>> On 11/27/19 4:14 PM, Jan Beulich wrote: On 27.11.2019 17:01, Roger Pau Monne wrote: > Live-patching requires unique symbols, and sadly the clang build > generates a lot of duplicate symbols: > > Duplicate symbol 'asid.c#get_cpu_info' (82d0803032c0 != > 82d0802e0f50) > Duplicate symbol 'asid.c#get_cpu_info_from_stack' (82d0802e1080 > != 82d0803032f0) > Duplicate symbol 'ats.c#__list_add' (82d080260a00 != > 82d080267c70) > Duplicate symbol 'boot.c#constant_test_bit' (82d08040ea60 != > 82d0804372f0) > Duplicate symbol 'common.c#clear_bit' (82d080332440 != > 82d0802d33b0) > Duplicate symbol 'common.c#constant_test_bit' (82d080332340 != > 82d0802d2220) > Duplicate symbol 'common.c#cpumask_check' (82d0802d3370 != > 82d080337b60) > Duplicate symbol 'common.c#get_cpu_info' (82d0802d22b0 != > 82d080331590) > Duplicate symbol 'common.c#get_cpu_info_from_stack' > (82d0802d31c0 != 82d0803374b0) > Duplicate symbol 'common.c#pfn_to_pdx' (82d0802d3270 != > 82d080331e00) > Duplicate symbol 'common.c#test_and_set_bit' (82d0802d3360 != > 82d080332250) > Duplicate symbol 'common.c#variable_clear_bit' (82d0802d2270 != > 82d080337b50) > Duplicate symbol 'compat.c#get_cpu_info' (82d08026eab0 != > 82d080200460) > Duplicate symbol 'compat.c#get_cpu_info_from_stack' > (82d08026ebd0 != 82d080200f70) > Duplicate symbol 'cpu_idle.c#get_cpu_info' (82d0802ccb00 != > 82d08035fcc0) > [...] > > For the time being disable live-patching when building with clang, > since duplicate symbols will trigger a build failure because > ENFORCE_UNIQUE_SYMBOLS is now also enabled by default in conjunction > with live-patching. > > Signed-off-by: Roger Pau Monné To be honest, as indicated before I'm inclined to nak this patch on the basis that a proper solution has been posted almost 3 weeks ago (and this was already v2). >>> >>> What's that patch waiting on? >> >> x86 and release acks. > > I plan to release ack the patch in case the missing maintainer's acks > are not coming in too late. I think Andy's objection was that there has been zero testing of livepatching on gcc. Maybe we can find someone to do a smoke-test. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [libvirt test] 144318: regressions - FAIL
flight 144318 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/144318/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-libvirt 19 leak-check/check fail REGR. vs. 144304 Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144304 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144304 test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 12 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass version targeted for testing: libvirt c8579871a980e7cd41df50afad62e9c3183534c6 baseline version: libvirt 9d6920bd7de3f92be1894790adeb689060ab25eb Last test of basis 144304 2019-11-26 04:19:14 Z1 days Testing same since 144318 2019-11-27 04:19:28 Z0 days1 attempts People who touched revisions under test: Michal Privoznik Peter Krempa jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt fail test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images 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. commit c8579871a980e7cd41df50afad62e9c3183534c6 Author: Michal Privoznik Date: Tue Nov 26 15:28:22 2019 +0100
Re: [Xen-devel] [PATCH for-4.13] clang: do not enable live-patching support
On 27.11.19 17:25, Jan Beulich wrote: On 27.11.2019 17:21, George Dunlap wrote: On 11/27/19 4:14 PM, Jan Beulich wrote: On 27.11.2019 17:01, Roger Pau Monne wrote: Live-patching requires unique symbols, and sadly the clang build generates a lot of duplicate symbols: Duplicate symbol 'asid.c#get_cpu_info' (82d0803032c0 != 82d0802e0f50) Duplicate symbol 'asid.c#get_cpu_info_from_stack' (82d0802e1080 != 82d0803032f0) Duplicate symbol 'ats.c#__list_add' (82d080260a00 != 82d080267c70) Duplicate symbol 'boot.c#constant_test_bit' (82d08040ea60 != 82d0804372f0) Duplicate symbol 'common.c#clear_bit' (82d080332440 != 82d0802d33b0) Duplicate symbol 'common.c#constant_test_bit' (82d080332340 != 82d0802d2220) Duplicate symbol 'common.c#cpumask_check' (82d0802d3370 != 82d080337b60) Duplicate symbol 'common.c#get_cpu_info' (82d0802d22b0 != 82d080331590) Duplicate symbol 'common.c#get_cpu_info_from_stack' (82d0802d31c0 != 82d0803374b0) Duplicate symbol 'common.c#pfn_to_pdx' (82d0802d3270 != 82d080331e00) Duplicate symbol 'common.c#test_and_set_bit' (82d0802d3360 != 82d080332250) Duplicate symbol 'common.c#variable_clear_bit' (82d0802d2270 != 82d080337b50) Duplicate symbol 'compat.c#get_cpu_info' (82d08026eab0 != 82d080200460) Duplicate symbol 'compat.c#get_cpu_info_from_stack' (82d08026ebd0 != 82d080200f70) Duplicate symbol 'cpu_idle.c#get_cpu_info' (82d0802ccb00 != 82d08035fcc0) [...] For the time being disable live-patching when building with clang, since duplicate symbols will trigger a build failure because ENFORCE_UNIQUE_SYMBOLS is now also enabled by default in conjunction with live-patching. Signed-off-by: Roger Pau Monné To be honest, as indicated before I'm inclined to nak this patch on the basis that a proper solution has been posted almost 3 weeks ago (and this was already v2). What's that patch waiting on? x86 and release acks. I plan to release ack the patch in case the missing maintainer's acks are not coming in too late. Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling
On 11/27/19 4:20 PM, Jan Beulich wrote:
> On 27.11.2019 17:14, Durrant, Paul wrote:
>>> From: Jan Beulich
>>> Sent: 27 November 2019 15:56
>>>
>>> On 27.11.2019 15:37, Paul Durrant wrote:
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -789,7 +789,7 @@ void __init start_xen(unsigned long
>>> boot_phys_offset,
.flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
.max_evtchn_port = -1,
.max_grant_frames = gnttab_dom0_frames(),
-.max_maptrack_frames = opt_max_maptrack_frames,
+.max_maptrack_frames = -1,
};
int rc;
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
>>> mbi_p)
struct xen_domctl_createdomain dom0_cfg = {
.flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity
>>> : 0,
.max_evtchn_port = -1,
-.max_grant_frames = opt_max_grant_frames,
-.max_maptrack_frames = opt_max_maptrack_frames,
+.max_grant_frames = -1,
+.max_maptrack_frames = -1,
};
>>>
>>> With these there's no need anymore for opt_max_maptrack_frames to
>>> be non-static. Sadly Arm still wants opt_max_grant_frames
>>> accessible in gnttab_dom0_frames().
>>
>> Yes, I was about to make them static until I saw what the ARM code did.
>
> But the one that Arm doesn't need should become static now.
>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1837,12 +1837,18 @@ active_alloc_failed:
return -ENOMEM;
}
-int grant_table_init(struct domain *d, unsigned int max_grant_frames,
- unsigned int max_maptrack_frames)
+int grant_table_init(struct domain *d, int max_grant_frames,
+ int max_maptrack_frames)
{
struct grant_table *gt;
int ret = -ENOMEM;
+/* Default to maximum value if no value was specified */
+if ( max_grant_frames < 0 )
+max_grant_frames = opt_max_grant_frames;
+if ( max_maptrack_frames < 0 )
+max_maptrack_frames = opt_max_maptrack_frames;
+
if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>>>
>>> I take it we don't expect people to specify 2^^31 or more
>>> frames for either option. It looks like almost everything
>>> here would cope, except for this very comparison. Nevertheless
>>> I wonder whether you wouldn't better confine both values to
>>> [0, INT_MAX] now, including when adjusted at runtime.
>>
>> I can certainly remove the 'U' from the definition of
>> INITIAL_NR_GRANT_FRAMES,
>
> Oh, I didn't pay attention that is has a U on it - in this case
> the comparison above is fine.
>
>> but do you want me to make opt_max_grant_frames and
>> opt_max_maptrack_frames into signed ints and add signed parser
>> code too?
>
> Definitely not. They should remain unsigned quantities, but their
> values may need sanity checking now.
>
>> I also don't understand the 'adjusted at runtime' part.
>
> Well, for a command line drive value you could adjust an out of
> bounds value in some __init function. But for runtime modifiable
> settings you won't get away this easily.
TBH I'd be tempted to define XENSOMETHING_MAX_DEFAULT as (unsigned
long)(-1) or something, and explicitly compare to that. That leaves
open the possibility of having more sentinel values if we decided we
wanted them.
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed
On 11/27/19 10:44 AM, Jan Beulich wrote: > On 27.11.2019 13:00, Paul Durrant wrote: >> --- a/xen/arch/x86/cpu/vpmu.c >> +++ b/xen/arch/x86/cpu/vpmu.c >> @@ -479,6 +479,8 @@ static int vpmu_arch_initialise(struct vcpu *v) >> >> if ( ret ) >> printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v); >> +else >> +vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED); That won't work I think. On Intel the context is allocated lazily for HVM/PVH guests during the first MSR access. For example: core2_vpmu_do_wrmsr() -> core2_vpmu_msr_common_check()): if ( unlikely(!vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED)) && !core2_vpmu_alloc_resource(current) ) return 0; For PV guests the context *is* allocated from vmx_vpmu_initialise(). I don't remember why only PV does eager allocation but I think doing it for all guests would make code much simpler and then this patch will be correct. -boris >> >> return ret; >> } >> @@ -576,11 +578,36 @@ static void vpmu_arch_destroy(struct vcpu *v) >> >> vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); >> } >> + >> +vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); >> } > Boris, > > I'd like to ask that you comment on this part of the change at > least, as I seem to vaguely recall that things were intentionally > not done this way originally. > > Paul, > > everything else looks god to me now. > > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] clang: do not enable live-patching support
On 27.11.2019 17:21, George Dunlap wrote: > On 11/27/19 4:14 PM, Jan Beulich wrote: >> On 27.11.2019 17:01, Roger Pau Monne wrote: >>> Live-patching requires unique symbols, and sadly the clang build >>> generates a lot of duplicate symbols: >>> >>> Duplicate symbol 'asid.c#get_cpu_info' (82d0803032c0 != >>> 82d0802e0f50) >>> Duplicate symbol 'asid.c#get_cpu_info_from_stack' (82d0802e1080 != >>> 82d0803032f0) >>> Duplicate symbol 'ats.c#__list_add' (82d080260a00 != 82d080267c70) >>> Duplicate symbol 'boot.c#constant_test_bit' (82d08040ea60 != >>> 82d0804372f0) >>> Duplicate symbol 'common.c#clear_bit' (82d080332440 != 82d0802d33b0) >>> Duplicate symbol 'common.c#constant_test_bit' (82d080332340 != >>> 82d0802d2220) >>> Duplicate symbol 'common.c#cpumask_check' (82d0802d3370 != >>> 82d080337b60) >>> Duplicate symbol 'common.c#get_cpu_info' (82d0802d22b0 != >>> 82d080331590) >>> Duplicate symbol 'common.c#get_cpu_info_from_stack' (82d0802d31c0 != >>> 82d0803374b0) >>> Duplicate symbol 'common.c#pfn_to_pdx' (82d0802d3270 != >>> 82d080331e00) >>> Duplicate symbol 'common.c#test_and_set_bit' (82d0802d3360 != >>> 82d080332250) >>> Duplicate symbol 'common.c#variable_clear_bit' (82d0802d2270 != >>> 82d080337b50) >>> Duplicate symbol 'compat.c#get_cpu_info' (82d08026eab0 != >>> 82d080200460) >>> Duplicate symbol 'compat.c#get_cpu_info_from_stack' (82d08026ebd0 != >>> 82d080200f70) >>> Duplicate symbol 'cpu_idle.c#get_cpu_info' (82d0802ccb00 != >>> 82d08035fcc0) >>> [...] >>> >>> For the time being disable live-patching when building with clang, >>> since duplicate symbols will trigger a build failure because >>> ENFORCE_UNIQUE_SYMBOLS is now also enabled by default in conjunction >>> with live-patching. >>> >>> Signed-off-by: Roger Pau Monné >> >> To be honest, as indicated before I'm inclined to nak this patch >> on the basis that a proper solution has been posted almost 3 weeks >> ago (and this was already v2). > > What's that patch waiting on? x86 and release acks. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] clang: do not enable live-patching support
On 11/27/19 4:14 PM, Jan Beulich wrote: > On 27.11.2019 17:01, Roger Pau Monne wrote: >> Live-patching requires unique symbols, and sadly the clang build >> generates a lot of duplicate symbols: >> >> Duplicate symbol 'asid.c#get_cpu_info' (82d0803032c0 != 82d0802e0f50) >> Duplicate symbol 'asid.c#get_cpu_info_from_stack' (82d0802e1080 != >> 82d0803032f0) >> Duplicate symbol 'ats.c#__list_add' (82d080260a00 != 82d080267c70) >> Duplicate symbol 'boot.c#constant_test_bit' (82d08040ea60 != >> 82d0804372f0) >> Duplicate symbol 'common.c#clear_bit' (82d080332440 != 82d0802d33b0) >> Duplicate symbol 'common.c#constant_test_bit' (82d080332340 != >> 82d0802d2220) >> Duplicate symbol 'common.c#cpumask_check' (82d0802d3370 != >> 82d080337b60) >> Duplicate symbol 'common.c#get_cpu_info' (82d0802d22b0 != >> 82d080331590) >> Duplicate symbol 'common.c#get_cpu_info_from_stack' (82d0802d31c0 != >> 82d0803374b0) >> Duplicate symbol 'common.c#pfn_to_pdx' (82d0802d3270 != 82d080331e00) >> Duplicate symbol 'common.c#test_and_set_bit' (82d0802d3360 != >> 82d080332250) >> Duplicate symbol 'common.c#variable_clear_bit' (82d0802d2270 != >> 82d080337b50) >> Duplicate symbol 'compat.c#get_cpu_info' (82d08026eab0 != >> 82d080200460) >> Duplicate symbol 'compat.c#get_cpu_info_from_stack' (82d08026ebd0 != >> 82d080200f70) >> Duplicate symbol 'cpu_idle.c#get_cpu_info' (82d0802ccb00 != >> 82d08035fcc0) >> [...] >> >> For the time being disable live-patching when building with clang, >> since duplicate symbols will trigger a build failure because >> ENFORCE_UNIQUE_SYMBOLS is now also enabled by default in conjunction >> with live-patching. >> >> Signed-off-by: Roger Pau Monné > > To be honest, as indicated before I'm inclined to nak this patch > on the basis that a proper solution has been posted almost 3 weeks > ago (and this was already v2). What's that patch waiting on? -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling
On 27.11.2019 17:14, Durrant, Paul wrote:
>> From: Jan Beulich
>> Sent: 27 November 2019 15:56
>>
>> On 27.11.2019 15:37, Paul Durrant wrote:
>>> --- a/xen/arch/arm/setup.c
>>> +++ b/xen/arch/arm/setup.c
>>> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
>> boot_phys_offset,
>>> .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
>>> .max_evtchn_port = -1,
>>> .max_grant_frames = gnttab_dom0_frames(),
>>> -.max_maptrack_frames = opt_max_maptrack_frames,
>>> +.max_maptrack_frames = -1,
>>> };
>>> int rc;
>>>
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>>> struct xen_domctl_createdomain dom0_cfg = {
>>> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity
>> : 0,
>>> .max_evtchn_port = -1,
>>> -.max_grant_frames = opt_max_grant_frames,
>>> -.max_maptrack_frames = opt_max_maptrack_frames,
>>> +.max_grant_frames = -1,
>>> +.max_maptrack_frames = -1,
>>> };
>>
>> With these there's no need anymore for opt_max_maptrack_frames to
>> be non-static. Sadly Arm still wants opt_max_grant_frames
>> accessible in gnttab_dom0_frames().
>
> Yes, I was about to make them static until I saw what the ARM code did.
But the one that Arm doesn't need should become static now.
>>> --- a/xen/common/grant_table.c
>>> +++ b/xen/common/grant_table.c
>>> @@ -1837,12 +1837,18 @@ active_alloc_failed:
>>> return -ENOMEM;
>>> }
>>>
>>> -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
>>> - unsigned int max_maptrack_frames)
>>> +int grant_table_init(struct domain *d, int max_grant_frames,
>>> + int max_maptrack_frames)
>>> {
>>> struct grant_table *gt;
>>> int ret = -ENOMEM;
>>>
>>> +/* Default to maximum value if no value was specified */
>>> +if ( max_grant_frames < 0 )
>>> +max_grant_frames = opt_max_grant_frames;
>>> +if ( max_maptrack_frames < 0 )
>>> +max_maptrack_frames = opt_max_maptrack_frames;
>>> +
>>> if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>>
>> I take it we don't expect people to specify 2^^31 or more
>> frames for either option. It looks like almost everything
>> here would cope, except for this very comparison. Nevertheless
>> I wonder whether you wouldn't better confine both values to
>> [0, INT_MAX] now, including when adjusted at runtime.
>
> I can certainly remove the 'U' from the definition of
> INITIAL_NR_GRANT_FRAMES,
Oh, I didn't pay attention that is has a U on it - in this case
the comparison above is fine.
> but do you want me to make opt_max_grant_frames and
> opt_max_maptrack_frames into signed ints and add signed parser
> code too?
Definitely not. They should remain unsigned quantities, but their
values may need sanity checking now.
> I also don't understand the 'adjusted at runtime' part.
Well, for a command line drive value you could adjust an out of
bounds value in some __init function. But for runtime modifiable
settings you won't get away this easily.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling
> -Original Message-
> From: Jan Beulich
> Sent: 27 November 2019 15:56
> To: Durrant, Paul ; George Dunlap
>
> Cc: xen-devel@lists.xenproject.org; Andrew Cooper
> ; Anthony PERARD ;
> Roger Pau Monné ; Volodymyr Babchuk
> ; George Dunlap ;
> Ian Jackson ; Marek Marczykowski-Górecki
> ; Stefano Stabellini
> ; Konrad Rzeszutek Wilk ;
> Julien Grall ; Wei Liu
> Subject: Re: [PATCH v2] Rationalize max_grant_frames and
> max_maptrack_frames handling
>
> On 27.11.2019 15:37, Paul Durrant wrote:
> > --- a/xen/arch/arm/setup.c
> > +++ b/xen/arch/arm/setup.c
> > @@ -789,7 +789,7 @@ void __init start_xen(unsigned long
> boot_phys_offset,
> > .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> > .max_evtchn_port = -1,
> > .max_grant_frames = gnttab_dom0_frames(),
> > -.max_maptrack_frames = opt_max_maptrack_frames,
> > +.max_maptrack_frames = -1,
> > };
> > int rc;
> >
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
> > struct xen_domctl_createdomain dom0_cfg = {
> > .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity
> : 0,
> > .max_evtchn_port = -1,
> > -.max_grant_frames = opt_max_grant_frames,
> > -.max_maptrack_frames = opt_max_maptrack_frames,
> > +.max_grant_frames = -1,
> > +.max_maptrack_frames = -1,
> > };
>
> With these there's no need anymore for opt_max_maptrack_frames to
> be non-static. Sadly Arm still wants opt_max_grant_frames
> accessible in gnttab_dom0_frames().
>
Yes, I was about to make them static until I saw what the ARM code did.
> > --- a/xen/common/grant_table.c
> > +++ b/xen/common/grant_table.c
> > @@ -1837,12 +1837,18 @@ active_alloc_failed:
> > return -ENOMEM;
> > }
> >
> > -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
> > - unsigned int max_maptrack_frames)
> > +int grant_table_init(struct domain *d, int max_grant_frames,
> > + int max_maptrack_frames)
> > {
> > struct grant_table *gt;
> > int ret = -ENOMEM;
> >
> > +/* Default to maximum value if no value was specified */
> > +if ( max_grant_frames < 0 )
> > +max_grant_frames = opt_max_grant_frames;
> > +if ( max_maptrack_frames < 0 )
> > +max_maptrack_frames = opt_max_maptrack_frames;
> > +
> > if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
>
> I take it we don't expect people to specify 2^^31 or more
> frames for either option. It looks like almost everything
> here would cope, except for this very comparison. Nevertheless
> I wonder whether you wouldn't better confine both values to
> [0, INT_MAX] now, including when adjusted at runtime.
I can certainly remove the 'U' from the definition of INITIAL_NR_GRANT_FRAMES,
but do you want me to make opt_max_grant_frames and opt_max_maptrack_frames
into signed ints and add signed parser code too? I also don't understand the
'adjusted at runtime' part.
Paul
>
> Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] clang: do not enable live-patching support
On 27.11.2019 17:01, Roger Pau Monne wrote: > Live-patching requires unique symbols, and sadly the clang build > generates a lot of duplicate symbols: > > Duplicate symbol 'asid.c#get_cpu_info' (82d0803032c0 != 82d0802e0f50) > Duplicate symbol 'asid.c#get_cpu_info_from_stack' (82d0802e1080 != > 82d0803032f0) > Duplicate symbol 'ats.c#__list_add' (82d080260a00 != 82d080267c70) > Duplicate symbol 'boot.c#constant_test_bit' (82d08040ea60 != > 82d0804372f0) > Duplicate symbol 'common.c#clear_bit' (82d080332440 != 82d0802d33b0) > Duplicate symbol 'common.c#constant_test_bit' (82d080332340 != > 82d0802d2220) > Duplicate symbol 'common.c#cpumask_check' (82d0802d3370 != > 82d080337b60) > Duplicate symbol 'common.c#get_cpu_info' (82d0802d22b0 != > 82d080331590) > Duplicate symbol 'common.c#get_cpu_info_from_stack' (82d0802d31c0 != > 82d0803374b0) > Duplicate symbol 'common.c#pfn_to_pdx' (82d0802d3270 != 82d080331e00) > Duplicate symbol 'common.c#test_and_set_bit' (82d0802d3360 != > 82d080332250) > Duplicate symbol 'common.c#variable_clear_bit' (82d0802d2270 != > 82d080337b50) > Duplicate symbol 'compat.c#get_cpu_info' (82d08026eab0 != > 82d080200460) > Duplicate symbol 'compat.c#get_cpu_info_from_stack' (82d08026ebd0 != > 82d080200f70) > Duplicate symbol 'cpu_idle.c#get_cpu_info' (82d0802ccb00 != > 82d08035fcc0) > [...] > > For the time being disable live-patching when building with clang, > since duplicate symbols will trigger a build failure because > ENFORCE_UNIQUE_SYMBOLS is now also enabled by default in conjunction > with live-patching. > > Signed-off-by: Roger Pau Monné To be honest, as indicated before I'm inclined to nak this patch on the basis that a proper solution has been posted almost 3 weeks ago (and this was already v2). Nevertheless a remark here: > --- a/xen/common/Kconfig > +++ b/xen/common/Kconfig > @@ -80,6 +80,10 @@ config HAS_CHECKPOLICY > string > option env="XEN_HAS_CHECKPOLICY" > > +config BUILD_WITH_CLANG > + string > + option env="XEN_BUILD_WITH_CLANG" Instead of introducing a new option here, ... > @@ -350,7 +354,7 @@ config CRYPTO > config LIVEPATCH > bool "Live patching support" > default X86 > - depends on HAS_BUILD_ID = "y" > + depends on HAS_BUILD_ID = "y" && BUILD_WITH_CLANG != "y" ... seeing this, why don't you simply suppress HAS_BUILD_ID acquiring a value of y in ./Config.mk (accompanied by a suitable comment)? Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [qemu-mainline test] 144316: regressions - FAIL
flight 144316 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/144316/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-xl-xsm 7 xen-boot fail REGR. vs. 144305 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail like 144305 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 144305 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144305 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144305 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 144305 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144305 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 144305 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass version targeted for testing: qemuu1a61a081ac33ae6cb7dd2e38d119a572f416c7f7 baseline version: qemuu65e05c82bdc6d348155e301c9d87dba7a08a5701 Last test of basis 144305 2019-11-26 05:17:32 Z1 days Testing same since 144316 2019-11-27 00:06:47 Z0 days1 attempts People who touched revisions under test: Alex Bennée Alex Williamson Ariadne Conill Cameron Esfahani David Gibson Dr. David Alan Gilbert Edgar E. Iglesias Eduardo Habkost Greg Kurz Jason Wang Jean-Hugues Deschenes Jean-Hugues Deschênes Jens Freimann Laurent Vivier Laurent Vivier Marc Zyngier
Re: [Xen-devel] [PATCH v2 3/3] arm64: remove the rest of asm-uaccess.h
On Wed, Nov 27, 2019 at 11:03 AM Mark Rutland wrote: > > On Wed, Nov 27, 2019 at 10:31:54AM -0500, Pavel Tatashin wrote: > > On Wed, Nov 27, 2019 at 10:12 AM Mark Rutland wrote: > > > > > > On Thu, Nov 21, 2019 at 09:24:06PM -0500, Pavel Tatashin wrote: > > > > The __uaccess_ttbr0_disable and __uaccess_ttbr0_enable, > > > > are the last two macros defined in asm-uaccess.h. > > > > > > > > Replace them with C wrappers and call C functions from > > > > kernel_entry and kernel_exit. > > > > > > For now, please leave those as-is. > > > > > > I don't think we want to have out-of-line C wrappers in the middle of > > > the entry assembly where we don't have a complete kernel environment. > > > The use in entry code can also assume non-preemptibility, while the C > > > functions have to explcitily disable that. > > > > I do not understand, if C function is called form non-preemptible > > context it stays non-preemptible. kernel_exit already may call C > > functions around the time __uaccess_ttbr0_enable is called (it may > > call post_ttbr_update_workaround), and that C functions does not do > > explicit preempt disable: > > Sorry, I meant that IRQs are disabled here. > > The C wrapper calls __uaccess_ttbr0_enable(), which calls > local_irq_save() and local_irq_restore(). Those are pointless in the > bowels of the entry code, and potentially expensive if IRQ prio masking > is in use. > > I'd rather not add more out-of-line C code calls here right now as I'd > prefer to factor out the logic to C in a better way. Ah, yes, this makes sense. I could certainly factor out C calls in a better way, or is this something you want to work on? Without removing these assembly macros I do not think we want to address this suggestion from Kees Cook: https://lore.kernel.org/lkml/ca+ck2bcbs2fkotmtfm13iv3u5tbpwpocsyeep352dve-gs9...@mail.gmail.com/ Thank you, Pasha ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v4] x86/vmx: always sync PIR to IRR before vmentry
On 27.11.19 16:48, Roger Pau Monne wrote: When using posted interrupts on Intel hardware it's possible that the vCPU resumes execution with a stale local APIC IRR register because depending on the interrupts to be injected vlapic_has_pending_irq might not be called, and thus PIR won't be synced into IRR. Fix this by making sure PIR is always synced to IRR in hvm_vcpu_has_pending_irq regardless of what interrupts are pending. Reported-by: Joe Jin Signed-off-by: Roger Pau Monné Release-acked-by: Juergen Gross Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] arm64: remove the rest of asm-uaccess.h
On Wed, Nov 27, 2019 at 10:31:54AM -0500, Pavel Tatashin wrote: > On Wed, Nov 27, 2019 at 10:12 AM Mark Rutland wrote: > > > > On Thu, Nov 21, 2019 at 09:24:06PM -0500, Pavel Tatashin wrote: > > > The __uaccess_ttbr0_disable and __uaccess_ttbr0_enable, > > > are the last two macros defined in asm-uaccess.h. > > > > > > Replace them with C wrappers and call C functions from > > > kernel_entry and kernel_exit. > > > > For now, please leave those as-is. > > > > I don't think we want to have out-of-line C wrappers in the middle of > > the entry assembly where we don't have a complete kernel environment. > > The use in entry code can also assume non-preemptibility, while the C > > functions have to explcitily disable that. > > I do not understand, if C function is called form non-preemptible > context it stays non-preemptible. kernel_exit already may call C > functions around the time __uaccess_ttbr0_enable is called (it may > call post_ttbr_update_workaround), and that C functions does not do > explicit preempt disable: Sorry, I meant that IRQs are disabled here. The C wrapper calls __uaccess_ttbr0_enable(), which calls local_irq_save() and local_irq_restore(). Those are pointless in the bowels of the entry code, and potentially expensive if IRQ prio masking is in use. I'd rather not add more out-of-line C code calls here right now as I'd prefer to factor out the logic to C in a better way. > > We can certainly remove the includes of elsewhere, > > and maybe fold the macros into entry.S if it's not too crowded. > > I can do this as a separate patch. That sounds fine to me, Thanks, Mark. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH for-4.13] clang: do not enable live-patching support
Live-patching requires unique symbols, and sadly the clang build
generates a lot of duplicate symbols:
Duplicate symbol 'asid.c#get_cpu_info' (82d0803032c0 != 82d0802e0f50)
Duplicate symbol 'asid.c#get_cpu_info_from_stack' (82d0802e1080 !=
82d0803032f0)
Duplicate symbol 'ats.c#__list_add' (82d080260a00 != 82d080267c70)
Duplicate symbol 'boot.c#constant_test_bit' (82d08040ea60 !=
82d0804372f0)
Duplicate symbol 'common.c#clear_bit' (82d080332440 != 82d0802d33b0)
Duplicate symbol 'common.c#constant_test_bit' (82d080332340 !=
82d0802d2220)
Duplicate symbol 'common.c#cpumask_check' (82d0802d3370 != 82d080337b60)
Duplicate symbol 'common.c#get_cpu_info' (82d0802d22b0 != 82d080331590)
Duplicate symbol 'common.c#get_cpu_info_from_stack' (82d0802d31c0 !=
82d0803374b0)
Duplicate symbol 'common.c#pfn_to_pdx' (82d0802d3270 != 82d080331e00)
Duplicate symbol 'common.c#test_and_set_bit' (82d0802d3360 !=
82d080332250)
Duplicate symbol 'common.c#variable_clear_bit' (82d0802d2270 !=
82d080337b50)
Duplicate symbol 'compat.c#get_cpu_info' (82d08026eab0 != 82d080200460)
Duplicate symbol 'compat.c#get_cpu_info_from_stack' (82d08026ebd0 !=
82d080200f70)
Duplicate symbol 'cpu_idle.c#get_cpu_info' (82d0802ccb00 !=
82d08035fcc0)
[...]
For the time being disable live-patching when building with clang,
since duplicate symbols will trigger a build failure because
ENFORCE_UNIQUE_SYMBOLS is now also enabled by default in conjunction
with live-patching.
Signed-off-by: Roger Pau Monné
---
Cc: Jürgen Groß
---
Config.mk | 2 ++
xen/common/Kconfig | 6 +-
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/Config.mk b/Config.mk
index d8f90d75b3..009abda225 100644
--- a/Config.mk
+++ b/Config.mk
@@ -157,6 +157,8 @@ ifndef XEN_HAS_CHECKPOLICY
export XEN_HAS_CHECKPOLICY
endif
+export XEN_BUILD_WITH_CLANG = $(clang)
+
# as-insn: Check whether assembler supports an instruction.
# Usage: cflags-y += $(call as-insn,CC FLAGS,"insn",option-yes,option-no)
as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index f754741972..097996fc6c 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -80,6 +80,10 @@ config HAS_CHECKPOLICY
string
option env="XEN_HAS_CHECKPOLICY"
+config BUILD_WITH_CLANG
+ string
+ option env="XEN_BUILD_WITH_CLANG"
+
menu "Speculative hardening"
config SPECULATIVE_HARDEN_ARRAY
@@ -350,7 +354,7 @@ config CRYPTO
config LIVEPATCH
bool "Live patching support"
default X86
- depends on HAS_BUILD_ID = "y"
+ depends on HAS_BUILD_ID = "y" && BUILD_WITH_CLANG != "y"
---help---
Allows a running Xen hypervisor to be dynamically patched using
binary patches without rebooting. This is primarily used to binarily
--
2.24.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v4] x86/vmx: always sync PIR to IRR before vmentry
On 27.11.2019 16:48, Roger Pau Monne wrote: > When using posted interrupts on Intel hardware it's possible that the > vCPU resumes execution with a stale local APIC IRR register because > depending on the interrupts to be injected vlapic_has_pending_irq > might not be called, and thus PIR won't be synced into IRR. > > Fix this by making sure PIR is always synced to IRR in > hvm_vcpu_has_pending_irq regardless of what interrupts are pending. > > Reported-by: Joe Jin > Signed-off-by: Roger Pau Monné On the assumption that this will work for Joe as well, Acked-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] livepatch-build-tools regression
On 27/11/2019 15:22, Wieczorkiewicz, Pawel wrote:
>
>
>> On 27. Nov 2019, at 12:16, Sergey Dyasli wrote:
>>
>> On 26/11/2019 18:37, Wieczorkiewicz, Pawel wrote:
>>> It looks like gcc plays the usual dirty tricks with local variables
>>> renaming:
>>>
>>> - xen-syms
>>> 7529: 82d0805fed50 8 OBJECT LOCAL DEFAULT 4230 lastpage.22857
>>> - livepatch
>>> 289: 8 OBJECT GLOBAL DEFAULT UND
>>> hvm.c#lastpage.22856
>>>
>>> Then, symbols resolution by name fails..
>>>
>>> Can you please try to build the livepatch module with additional option
>>> '—prelink' and give it a try ?
>>
>> My LP loading error is:
>>
>>(XEN) livepatch: lp: Unknown symbol: .LC7
>>
>> When I pass --prelink to livepatch-build, it complains in a similar way:
>>
>>livepatch-build-tools/prelink: ERROR: output.o:
>> livepatch_resolve_symbols: 80: lookup_local_symbol .LC7 (p2m.c)
>>
>
> Could you give this testing patch a try?
>
> diff --git a/create-diff-object.c b/create-diff-object.c
> index 8d63940..10807d2 100644
> --- a/create-diff-object.c
> +++ b/create-diff-object.c
> @@ -839,8 +839,10 @@ static void kpatch_compare_symbols(struct list_head
> *symlist)
> list_for_each_entry(sym, symlist, list) {
> if (sym->twin)
> kpatch_compare_correlated_symbol(sym);
> - else
> + else {
> sym->status = NEW;
> + sym->include = 1;
> + }
>
> log_debug("symbol %s is %s\n", sym->name,
> status_str(sym->status));
> }
>
Looks like this change fixed the issue for me!
One thing to notice is that the size of a stripped LP binary increased
from 45K to 60K.
--
Thanks,
Sergey
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling
On 27.11.2019 15:37, Paul Durrant wrote:
> --- a/xen/arch/arm/setup.c
> +++ b/xen/arch/arm/setup.c
> @@ -789,7 +789,7 @@ void __init start_xen(unsigned long boot_phys_offset,
> .flags = XEN_DOMCTL_CDF_hvm | XEN_DOMCTL_CDF_hap,
> .max_evtchn_port = -1,
> .max_grant_frames = gnttab_dom0_frames(),
> -.max_maptrack_frames = opt_max_maptrack_frames,
> +.max_maptrack_frames = -1,
> };
> int rc;
>
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -697,8 +697,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> struct xen_domctl_createdomain dom0_cfg = {
> .flags = IS_ENABLED(CONFIG_TBOOT) ? XEN_DOMCTL_CDF_s3_integrity : 0,
> .max_evtchn_port = -1,
> -.max_grant_frames = opt_max_grant_frames,
> -.max_maptrack_frames = opt_max_maptrack_frames,
> +.max_grant_frames = -1,
> +.max_maptrack_frames = -1,
> };
With these there's no need anymore for opt_max_maptrack_frames to
be non-static. Sadly Arm still wants opt_max_grant_frames
accessible in gnttab_dom0_frames().
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1837,12 +1837,18 @@ active_alloc_failed:
> return -ENOMEM;
> }
>
> -int grant_table_init(struct domain *d, unsigned int max_grant_frames,
> - unsigned int max_maptrack_frames)
> +int grant_table_init(struct domain *d, int max_grant_frames,
> + int max_maptrack_frames)
> {
> struct grant_table *gt;
> int ret = -ENOMEM;
>
> +/* Default to maximum value if no value was specified */
> +if ( max_grant_frames < 0 )
> +max_grant_frames = opt_max_grant_frames;
> +if ( max_maptrack_frames < 0 )
> +max_maptrack_frames = opt_max_maptrack_frames;
> +
> if ( max_grant_frames < INITIAL_NR_GRANT_FRAMES ||
I take it we don't expect people to specify 2^^31 or more
frames for either option. It looks like almost everything
here would cope, except for this very comparison. Nevertheless
I wonder whether you wouldn't better confine both values to
[0, INT_MAX] now, including when adjusted at runtime.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH for-4.13 v4] x86/vmx: always sync PIR to IRR before vmentry
When using posted interrupts on Intel hardware it's possible that the
vCPU resumes execution with a stale local APIC IRR register because
depending on the interrupts to be injected vlapic_has_pending_irq
might not be called, and thus PIR won't be synced into IRR.
Fix this by making sure PIR is always synced to IRR in
hvm_vcpu_has_pending_irq regardless of what interrupts are pending.
Reported-by: Joe Jin
Signed-off-by: Roger Pau Monné
---
Cc: Juergen Gross
---
Changes since v3:
- Introduce and use vlapic_sync_pir_to_irr in order to sync PIR with
IRR.
- Do not move the call to vlapic_has_pending_irq in
hvm_vcpu_has_pending_irq.
- Remove the changes done to __vmx_deliver_posted_interrupt.
Changes since v2:
- Raise a softirq if in interrupt context and the vCPU is the current
one.
- Use is_running instead of runnable.
- Remove the call to vmx_sync_pir_to_irr in vmx_intr_assist and
instead always call vlapic_has_pending_irq in
hvm_vcpu_has_pending_irq.
---
xen/arch/x86/hvm/irq.c | 9 +
xen/arch/x86/hvm/vlapic.c| 10 ++
xen/include/asm-x86/hvm/vlapic.h | 6 ++
3 files changed, 17 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/hvm/irq.c b/xen/arch/x86/hvm/irq.c
index e03a87ad50..c684422b24 100644
--- a/xen/arch/x86/hvm/irq.c
+++ b/xen/arch/x86/hvm/irq.c
@@ -517,6 +517,15 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
struct hvm_domain *plat = >domain->arch.hvm;
int vector;
+/*
+ * Always call vlapic_sync_pir_to_irr so that PIR is synced into IRR when
+ * using posted interrupts. Note this is also done by
+ * vlapic_has_pending_irq but depending on which interrupts are pending
+ * hvm_vcpu_has_pending_irq will return early without calling
+ * vlapic_has_pending_irq.
+ */
+vlapic_sync_pir_to_irr(v);
+
if ( unlikely(v->nmi_pending) )
return hvm_intack_nmi;
diff --git a/xen/arch/x86/hvm/vlapic.c b/xen/arch/x86/hvm/vlapic.c
index 9466258d6f..6fcce95713 100644
--- a/xen/arch/x86/hvm/vlapic.c
+++ b/xen/arch/x86/hvm/vlapic.c
@@ -106,15 +106,9 @@ static void vlapic_clear_irr(int vector, struct vlapic
*vlapic)
vlapic_clear_vector(vector, >regs->data[APIC_IRR]);
}
-static void sync_pir_to_irr(struct vcpu *v)
-{
-if ( hvm_funcs.sync_pir_to_irr )
-alternative_vcall(hvm_funcs.sync_pir_to_irr, v);
-}
-
static int vlapic_find_highest_irr(struct vlapic *vlapic)
{
-sync_pir_to_irr(vlapic_vcpu(vlapic));
+vlapic_sync_pir_to_irr(vlapic_vcpu(vlapic));
return vlapic_find_highest_vector(>regs->data[APIC_IRR]);
}
@@ -1493,7 +1487,7 @@ static int lapic_save_regs(struct vcpu *v,
hvm_domain_context_t *h)
if ( !has_vlapic(v->domain) )
return 0;
-sync_pir_to_irr(v);
+vlapic_sync_pir_to_irr(v);
return hvm_save_entry(LAPIC_REGS, v->vcpu_id, h, vcpu_vlapic(v)->regs);
}
diff --git a/xen/include/asm-x86/hvm/vlapic.h b/xen/include/asm-x86/hvm/vlapic.h
index dde66b4f0f..f0d5e3fbc9 100644
--- a/xen/include/asm-x86/hvm/vlapic.h
+++ b/xen/include/asm-x86/hvm/vlapic.h
@@ -150,4 +150,10 @@ bool_t vlapic_match_dest(
const struct vlapic *target, const struct vlapic *source,
int short_hand, uint32_t dest, bool_t dest_mode);
+static inline void vlapic_sync_pir_to_irr(struct vcpu *v)
+{
+if ( hvm_funcs.sync_pir_to_irr )
+alternative_vcall(hvm_funcs.sync_pir_to_irr, v);
+}
+
#endif /* __ASM_X86_HVM_VLAPIC_H__ */
--
2.24.0
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed
On 27.11.2019 13:00, Paul Durrant wrote: > --- a/xen/arch/x86/cpu/vpmu.c > +++ b/xen/arch/x86/cpu/vpmu.c > @@ -479,6 +479,8 @@ static int vpmu_arch_initialise(struct vcpu *v) > > if ( ret ) > printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v); > +else > +vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED); > > return ret; > } > @@ -576,11 +578,36 @@ static void vpmu_arch_destroy(struct vcpu *v) > > vpmu->arch_vpmu_ops->arch_vpmu_destroy(v); > } > + > +vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED); > } Boris, I'd like to ask that you comment on this part of the change at least, as I seem to vaguely recall that things were intentionally not done this way originally. Paul, everything else looks god to me now. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On 27.11.2019 12:56, Roger Pau Monné wrote:
> On Wed, Nov 27, 2019 at 12:30:06PM +0100, Jan Beulich wrote:
>> On 27.11.2019 12:22, Roger Pau Monné wrote:
>>> On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
On 26.11.2019 14:26, Roger Pau Monne wrote:
> --- a/xen/arch/x86/hvm/irq.c
> +++ b/xen/arch/x86/hvm/irq.c
> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
> via)
> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> {
> struct hvm_domain *plat = >domain->arch.hvm;
> -int vector;
> +/*
> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
> when
> + * using posted interrupts.
> + */
> +int vector = vlapic_has_pending_irq(v);
Did you consider doing this conditionally either here ...
> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct
> vcpu *v)
> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> return hvm_intack_pic(0);
>
> -vector = vlapic_has_pending_irq(v);
> if ( vector != -1 )
> return hvm_intack_lapic(vector);
... or here?
>>>
>>> I'm afraid I don't follow. The whole point of this change is to ensure
>>> vlapic_has_pending_irq is unconditionally called in
>>> hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
>>> conditionally...".
>>
>> Do it early when using interrupt posting, and keep it in its
>> current place otherwise.
>>
I ask not only because the function isn't exactly
cheap to call (as iirc you did also mention during the v2
discussion), but also because of its interaction with Viridian
and nested mode. In case of problems there, avoiding the use
of interrupt posting would be a workaround in such cases then.
>>>
>>> Would you like me to export something like vlapic_sync_pir_to_irr and
>>> call it unconditionally instead of calling vlapic_has_pending_irq?
>>
>> This looks to be another option, yes. Albeit instead of making
>> non-static (which I assume is what you mean by "export"), maybe
>> simply make this a static inline in vlapic.h then.
>
> Yes, that would work and IMO is better than moving the call to
> vlapic_has_pending_irq around. Are you OK with this approach?
I think so, yes.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86 / iommu: set up a scratch page in the quarantine domain
> -Original Message- > From: Jan Beulich > Sent: 27 November 2019 15:26 > To: Durrant, Paul > Cc: Kevin Tian ; xen-devel@lists.xenproject.org; > Andrew Cooper ; Roger Pau Monné > ; Wei Liu > Subject: Re: [Xen-devel] [PATCH] x86 / iommu: set up a scratch page in the > quarantine domain > > On 27.11.2019 16:18, Durrant, Paul wrote: > >> -Original Message- > >> From: Tian, Kevin > >> Sent: 25 November 2019 08:22 > >> To: Durrant, Paul ; xen-devel@lists.xenproject.org > >> Cc: Jan Beulich ; Andrew Cooper > >> ; Wei Liu ; Roger Pau Monné > >> > >> Subject: RE: [PATCH] x86 / iommu: set up a scratch page in the > quarantine > >> domain > >> > >>> From: Paul Durrant [mailto:pdurr...@amazon.com] > >>> Sent: Wednesday, November 20, 2019 8:09 PM > >>> > >>> This patch introduces a new iommu_op to facilitate a per- > implementation > >>> quarantine set up, and then further code for x86 implementations > >>> (amd and vtd) to set up a read/wrote scratch page to serve as the > >> source/ > >>> target for all DMA whilst a device is assigned to dom_io. > >>> > >>> The reason for doing this is that some hardware may continue to re-try > >>> DMA, despite FLR, in the event of an error. Having a scratch page > mapped > >>> will allow pending DMA to drain and thus quiesce such buggy hardware. > >> > >> then there is no diagnostics at all since all faults are quiescent > now... > >> why do we want to support such buggy hardware? Is it better to make > >> it an default-off option since buggy is supposed to niche case? > > > > I guess it could be a command line option... perhaps making the new > > 'iommu=quarantine' boolean into something more complex, but I'm not > > sure it's really worth it. Perhaps a compile time option would be > > better? > > Yet another option: How about installing the scratch page mappings > only after a (handful of) IOMMU faults? But of course there was the > related earlier question of whether indeed our turning off of bus > mastering doesn't already help silencing the faults. No. Unfortunately the h/w has zero tolerance for some faults. Paul > > Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86 / iommu: set up a scratch page in the quarantine domain
> -Original Message-
> From: Xen-devel On Behalf Of Jan
> Beulich
> Sent: 20 November 2019 13:52
> To: Durrant, Paul
> Cc: xen-devel@lists.xenproject.org; Kevin Tian ;
> Roger Pau Monné ; Wei Liu ; Andrew
> Cooper
> Subject: Re: [Xen-devel] [PATCH] x86 / iommu: set up a scratch page in the
> quarantine domain
>
> On 20.11.2019 13:08, Paul Durrant wrote:
> > This patch introduces a new iommu_op to facilitate a per-implementation
> > quarantine set up, and then further code for x86 implementations
> > (amd and vtd) to set up a read/wrote scratch page to serve as the
> source/
> > target for all DMA whilst a device is assigned to dom_io.
>
> A single page in the system won't do, I'm afraid. If one guest's
> (prior) device is retrying reads with data containing secrets of that
> guest, another guest's (prior) device could end up writing this data
> to e.g. storage where after a guest restart it is then available to
> the wrong guest.
>
True. I was unsure whether this was a concern in the scenarios we had to deal
with but I'm informed it is, and in the general case it is too.
> Also nit: s/wrote/write/ .
>
Yep. Will fix.
> > The reason for doing this is that some hardware may continue to re-try
> > DMA, despite FLR, in the event of an error. Having a scratch page mapped
> > will allow pending DMA to drain and thus quiesce such buggy hardware.
>
> Without a "sink" page mapped, this would result in IOMMU faults aiui.
> What's the problem with having these faults surface and get handled,
> eventually leading to the device getting bus-mastering disabled? Is
> it that devices continue DMAing even when bus-mastering is off? If
> so, is it even safe to pass through any such device? In any event
> the description needs to be extended here.
>
The devices in question ignore both FLR and BME and some IOMMU faults are
fatal. I believe, however, write faults are not and so I think a single
read-only 'source' page will be sufficient.
> > Signed-off-by: Paul Durrant
>
> What about Arm? Can devices which Arm allows to assign to guests
> also "babble" like this after de-assignment? If not, this should be
> said in the description. If so, obviously that side would also want
> fixing.
>
> > --- a/xen/drivers/passthrough/amd/iommu_map.c
> > +++ b/xen/drivers/passthrough/amd/iommu_map.c
> > @@ -560,6 +560,63 @@ int amd_iommu_reserve_domain_unity_map(struct
> domain *domain,
> > return rt;
> > }
> >
> > +int amd_iommu_quarantine_init(struct domain *d)
>
> __init
>
Ok.
> > +{
> > +struct domain_iommu *hd = dom_iommu(d);
> > +unsigned int level;
> > +struct amd_iommu_pte *table;
> > +
> > +if ( hd->arch.root_table )
> > +{
> > +ASSERT_UNREACHABLE();
> > +return 0;
> > +}
> > +
> > +spin_lock(>arch.mapping_lock);
> > +
> > +level = hd->arch.paging_mode;
>
> With DomIO being PV in principle, this is going to be the
> fixed value PV domains get set, merely depending on RAM size at
> boot time (i.e. not accounting for memory hotplug). This could
> be easily too little for HVM guests, which are free to extend
> their GFN (and hence DFN) space. Therefore I think you need to
> set the maximum possible level here.
>
Ok. I'd not considered memory hotplug. I'll use a static maximum value instead,
as VT-d does in general.
> > +hd->arch.root_table = alloc_amd_iommu_pgtable();
> > +if ( !hd->arch.root_table )
> > +goto out;
> > +
> > +table = __map_domain_page(hd->arch.root_table);
> > +while ( level )
> > +{
> > +struct page_info *pg;
> > +unsigned int i;
> > +
> > +/*
> > + * The pgtable allocator is fine for the leaf page, as well as
> > + * page table pages.
> > + */
> > +pg = alloc_amd_iommu_pgtable();
> > +if ( !pg )
> > +break;
> > +
> > +for ( i = 0; i < PTE_PER_TABLE_SIZE; i++ )
> > +{
> > +struct amd_iommu_pte *pde = [i];
> > +
> > +set_iommu_pde_present(pde, mfn_x(page_to_mfn(pg)), level -
> 1,
> > + true, true);
>
> This would also benefit from a comment indicating that it's fine
> for the leaf level as well, despite the "pde" in the name (and
> its sibling set_iommu_pte_present() actually existing). Of course
> you could as well extend the comment a few lines up.
The AMD IOMMU conflates PDE and PTE all over the place but I'll add a comment
here to that effect.
>
> What you do need to do though is pre-fill the leaf page ...
>
> > +}
> > +
> > +unmap_domain_page(table);
> > +table = __map_domain_page(pg);
> > +level--;
> > +}
>
> ... here, such that possible left over secrets can't end up
> getting written to e.g. persistent storage or over a network.
>
That's actually one reason for using the pgtable allocator... It already does
that.
> > @@ -2683,9 +2671,68 @@ static void vtd_dump_p2m_table(struct domain *d)
> >
Re: [Xen-devel] [PATCH v2 3/3] arm64: remove the rest of asm-uaccess.h
On Wed, Nov 27, 2019 at 10:12 AM Mark Rutland wrote: > > On Thu, Nov 21, 2019 at 09:24:06PM -0500, Pavel Tatashin wrote: > > The __uaccess_ttbr0_disable and __uaccess_ttbr0_enable, > > are the last two macros defined in asm-uaccess.h. > > > > Replace them with C wrappers and call C functions from > > kernel_entry and kernel_exit. > > For now, please leave those as-is. > > I don't think we want to have out-of-line C wrappers in the middle of > the entry assembly where we don't have a complete kernel environment. > The use in entry code can also assume non-preemptibility, while the C > functions have to explcitily disable that. I do not understand, if C function is called form non-preemptible context it stays non-preemptible. kernel_exit already may call C functions around the time __uaccess_ttbr0_enable is called (it may call post_ttbr_update_workaround), and that C functions does not do explicit preempt disable: > We can certainly remove the includes of elsewhere, > and maybe fold the macros into entry.S if it's not too crowded. I can do this as a separate patch. Thank you, Pasha ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86 / iommu: set up a scratch page in the quarantine domain
On 27.11.2019 16:18, Durrant, Paul wrote: >> -Original Message- >> From: Tian, Kevin >> Sent: 25 November 2019 08:22 >> To: Durrant, Paul ; xen-devel@lists.xenproject.org >> Cc: Jan Beulich ; Andrew Cooper >> ; Wei Liu ; Roger Pau Monné >> >> Subject: RE: [PATCH] x86 / iommu: set up a scratch page in the quarantine >> domain >> >>> From: Paul Durrant [mailto:pdurr...@amazon.com] >>> Sent: Wednesday, November 20, 2019 8:09 PM >>> >>> This patch introduces a new iommu_op to facilitate a per-implementation >>> quarantine set up, and then further code for x86 implementations >>> (amd and vtd) to set up a read/wrote scratch page to serve as the >> source/ >>> target for all DMA whilst a device is assigned to dom_io. >>> >>> The reason for doing this is that some hardware may continue to re-try >>> DMA, despite FLR, in the event of an error. Having a scratch page mapped >>> will allow pending DMA to drain and thus quiesce such buggy hardware. >> >> then there is no diagnostics at all since all faults are quiescent now... >> why do we want to support such buggy hardware? Is it better to make >> it an default-off option since buggy is supposed to niche case? > > I guess it could be a command line option... perhaps making the new > 'iommu=quarantine' boolean into something more complex, but I'm not > sure it's really worth it. Perhaps a compile time option would be > better? Yet another option: How about installing the scratch page mappings only after a (handful of) IOMMU faults? But of course there was the related earlier question of whether indeed our turning off of bus mastering doesn't already help silencing the faults. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] livepatch-build-tools regression
> On 27. Nov 2019, at 12:16, Sergey Dyasli wrote:
>
> On 26/11/2019 18:37, Wieczorkiewicz, Pawel wrote:
>> It looks like gcc plays the usual dirty tricks with local variables renaming:
>>
>> - xen-syms
>> 7529: 82d0805fed50 8 OBJECT LOCAL DEFAULT 4230 lastpage.22857
>> - livepatch
>> 289: 8 OBJECT GLOBAL DEFAULT UND
>> hvm.c#lastpage.22856
>>
>> Then, symbols resolution by name fails..
>>
>> Can you please try to build the livepatch module with additional option
>> '—prelink' and give it a try ?
>
> My LP loading error is:
>
>(XEN) livepatch: lp: Unknown symbol: .LC7
>
> When I pass --prelink to livepatch-build, it complains in a similar way:
>
>livepatch-build-tools/prelink: ERROR: output.o: livepatch_resolve_symbols:
> 80: lookup_local_symbol .LC7 (p2m.c)
>
Could you give this testing patch a try?
diff --git a/create-diff-object.c b/create-diff-object.c
index 8d63940..10807d2 100644
--- a/create-diff-object.c
+++ b/create-diff-object.c
@@ -839,8 +839,10 @@ static void kpatch_compare_symbols(struct list_head
*symlist)
list_for_each_entry(sym, symlist, list) {
if (sym->twin)
kpatch_compare_correlated_symbol(sym);
- else
+ else {
sym->status = NEW;
+ sym->include = 1;
+ }
log_debug("symbol %s is %s\n", sym->name,
status_str(sym->status));
}
> --
> Thanks,
> Sergey
Best Regards,
Pawel Wieczorkiewicz
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Ralf Herbrich
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86 / iommu: set up a scratch page in the quarantine domain
> -Original Message- > From: Tian, Kevin > Sent: 25 November 2019 08:22 > To: Durrant, Paul ; xen-devel@lists.xenproject.org > Cc: Jan Beulich ; Andrew Cooper > ; Wei Liu ; Roger Pau Monné > > Subject: RE: [PATCH] x86 / iommu: set up a scratch page in the quarantine > domain > > > From: Paul Durrant [mailto:pdurr...@amazon.com] > > Sent: Wednesday, November 20, 2019 8:09 PM > > > > This patch introduces a new iommu_op to facilitate a per-implementation > > quarantine set up, and then further code for x86 implementations > > (amd and vtd) to set up a read/wrote scratch page to serve as the > source/ > > target for all DMA whilst a device is assigned to dom_io. > > > > The reason for doing this is that some hardware may continue to re-try > > DMA, despite FLR, in the event of an error. Having a scratch page mapped > > will allow pending DMA to drain and thus quiesce such buggy hardware. > > then there is no diagnostics at all since all faults are quiescent now... > why do we want to support such buggy hardware? Is it better to make > it an default-off option since buggy is supposed to niche case? I guess it could be a command line option... perhaps making the new 'iommu=quarantine' boolean into something more complex, but I'm not sure it's really worth it. Perhaps a compile time option would be better? Paul ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions
On Wed, Nov 27, 2019 at 10:10:07AM -0500, Pavel Tatashin wrote: > Hi Mark, > > Thank you for reviewing this work. > > The 'arch_' prefix should probably be 'asm_' (or have an '_asm' suffix), > > since this is entirely local to the arch code, and even then should only > > be called from the C wrappers. > > Sure, I can change it to asm_*, I was using arch_* to be consistent > with __arch_copy_from_user() and friends. FWIW, that naming was from before the common uaccess code took on the raw_* anming for the arch functions, and I was expecting that the arch_* functions would end up being called from core code. For now it's probably too churny to change that existing case. Thanks, Mark. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] arm64: remove the rest of asm-uaccess.h
On Thu, Nov 21, 2019 at 09:24:06PM -0500, Pavel Tatashin wrote: > The __uaccess_ttbr0_disable and __uaccess_ttbr0_enable, > are the last two macros defined in asm-uaccess.h. > > Replace them with C wrappers and call C functions from > kernel_entry and kernel_exit. For now, please leave those as-is. I don't think we want to have out-of-line C wrappers in the middle of the entry assembly where we don't have a complete kernel environment. The use in entry code can also assume non-preemptibility, while the C functions have to explcitily disable that. We can certainly remove the includes of elsewhere, and maybe fold the macros into entry.S if it's not too crowded. Thanks, Mark. > > Signed-off-by: Pavel Tatashin > --- > arch/arm64/include/asm/asm-uaccess.h | 38 > arch/arm64/kernel/entry.S| 6 ++--- > arch/arm64/lib/clear_user.S | 2 +- > arch/arm64/lib/copy_from_user.S | 2 +- > arch/arm64/lib/copy_in_user.S| 2 +- > arch/arm64/lib/copy_to_user.S| 2 +- > arch/arm64/mm/cache.S| 1 - > arch/arm64/mm/context.c | 12 + > 8 files changed, 19 insertions(+), 46 deletions(-) > delete mode 100644 arch/arm64/include/asm/asm-uaccess.h > > diff --git a/arch/arm64/include/asm/asm-uaccess.h > b/arch/arm64/include/asm/asm-uaccess.h > deleted file mode 100644 > index 8f763e5b41b1.. > --- a/arch/arm64/include/asm/asm-uaccess.h > +++ /dev/null > @@ -1,38 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0 */ > -#ifndef __ASM_ASM_UACCESS_H > -#define __ASM_ASM_UACCESS_H > - > -#include > -#include > -#include > -#include > -#include > - > -/* > - * User access enabling/disabling macros. > - */ > -#ifdef CONFIG_ARM64_SW_TTBR0_PAN > - .macro __uaccess_ttbr0_disable, tmp1 > - mrs \tmp1, ttbr1_el1// swapper_pg_dir > - bic \tmp1, \tmp1, #TTBR_ASID_MASK > - sub \tmp1, \tmp1, #RESERVED_TTBR0_SIZE // reserved_ttbr0 just > before swapper_pg_dir > - msr ttbr0_el1, \tmp1// set reserved > TTBR0_EL1 > - isb > - add \tmp1, \tmp1, #RESERVED_TTBR0_SIZE > - msr ttbr1_el1, \tmp1// set reserved ASID > - isb > - .endm > - > - .macro __uaccess_ttbr0_enable, tmp1, tmp2 > - get_current_task \tmp1 > - ldr \tmp1, [\tmp1, #TSK_TI_TTBR0] // load saved TTBR0_EL1 > - mrs \tmp2, ttbr1_el1 > - extr\tmp2, \tmp2, \tmp1, #48 > - ror \tmp2, \tmp2, #16 > - msr ttbr1_el1, \tmp2// set the active ASID > - isb > - msr ttbr0_el1, \tmp1// set the non-PAN TTBR0_EL1 > - isb > - .endm > -#endif > -#endif > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S > index 583f71abbe98..c7b571e6d0f2 100644 > --- a/arch/arm64/kernel/entry.S > +++ b/arch/arm64/kernel/entry.S > @@ -22,8 +22,8 @@ > #include > #include > #include > +#include > #include > -#include > #include > > /* > @@ -219,7 +219,7 @@ alternative_else_nop_endif > and x23, x23, #~PSR_PAN_BIT // Clear the emulated PAN in > the saved SPSR > .endif > > - __uaccess_ttbr0_disable x21 > + bl __uaccess_ttbr0_disable_c > 1: > #endif > > @@ -293,7 +293,7 @@ alternative_else_nop_endif > tbnzx22, #22, 1f// Skip re-enabling TTBR0 > access if the PSR_PAN_BIT is set > .endif > > - __uaccess_ttbr0_enable x0, x1 > + bl __uaccess_ttbr0_enable_c > > .if \el == 0 > /* > diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S > index aeafc03e961a..b0b4a86a09e2 100644 > --- a/arch/arm64/lib/clear_user.S > +++ b/arch/arm64/lib/clear_user.S > @@ -6,7 +6,7 @@ > */ > #include > > -#include > +#include > #include > > .text > diff --git a/arch/arm64/lib/copy_from_user.S b/arch/arm64/lib/copy_from_user.S > index ebb3c06cbb5d..142bc7505518 100644 > --- a/arch/arm64/lib/copy_from_user.S > +++ b/arch/arm64/lib/copy_from_user.S > @@ -5,7 +5,7 @@ > > #include > > -#include > +#include > #include > #include > > diff --git a/arch/arm64/lib/copy_in_user.S b/arch/arm64/lib/copy_in_user.S > index 3d8153a1ebce..04dc48ca26f7 100644 > --- a/arch/arm64/lib/copy_in_user.S > +++ b/arch/arm64/lib/copy_in_user.S > @@ -7,7 +7,7 @@ > > #include > > -#include > +#include > #include > #include > > diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S > index 357eae2c18eb..8f3218ae88ab 100644 > --- a/arch/arm64/lib/copy_to_user.S > +++ b/arch/arm64/lib/copy_to_user.S > @@ -5,7 +5,7 @@ > > #include > > -#include > +#include > #include > #include > > diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S > index 408d317a47d2..7940d6ef5da5 100644 > --- a/arch/arm64/mm/cache.S > +++ b/arch/arm64/mm/cache.S > @@ -12,7 +12,6 @@ > #include > #include > #include >
Re: [Xen-devel] [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions
Hi Mark,
Thank you for reviewing this work.
> A commit message should provide rationale, rather than just a
> description of the patch. Something like:
>
> | We currently duplicate the logic to enable/disable uaccess via TTBR0,
> | with C functions and assembly macros. This is a maintenenace burden
> | and is liable to lead to subtle bugs, so let's get rid of the assembly
> | macros, and always use the C functions. This requires refactoring
> | some assembly functions to have a C wrapper.
Thank you for suggestion, I will fix my commit log.
>
> [...]
>
> > +static inline int invalidate_icache_range(unsigned long start,
> > + unsigned long end)
> > +{
> > + int rv;
> > +#if ARM64_HAS_CACHE_DIC
> > + rv = arch_invalidate_icache_range(start, end);
> > +#else
> > + uaccess_ttbr0_enable();
> > + rv = arch_invalidate_icache_range(start, end);
> > + uaccess_ttbr0_disable();
> > +#endif
> > + return rv;
> > +}
>
> This ifdeffery is not the same as an alternative_if, and even if it were
> the ARM64_HAS_CACHE_DIC behaviour is not the same as the existing
> assembly.
>
> This should be:
>
> static inline int invalidate_icache_range(unsigned long start,
> unsigned long end)
> {
> int ret;
>
> if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
> isb();
> return 0;
> }
>
> uaccess_ttbr0_enable();
> ret = arch_invalidate_icache_range(start, end);
> uaccess_ttbr0_disable();
>
> return ret;
> }
I will fix it, thanks.
>
> The 'arch_' prefix should probably be 'asm_' (or have an '_asm' suffix),
> since this is entirely local to the arch code, and even then should only
> be called from the C wrappers.
Sure, I can change it to asm_*, I was using arch_* to be consistent
with __arch_copy_from_user() and friends.
Thank you,
Pasha
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions
Hi Pavel,
On Thu, Nov 21, 2019 at 09:24:05PM -0500, Pavel Tatashin wrote:
> Replace the uaccess_ttbr0_disable/uaccess_ttbr0_enable via
> inline variants, and remove asm macros.
A commit message should provide rationale, rather than just a
description of the patch. Something like:
| We currently duplicate the logic to enable/disable uaccess via TTBR0,
| with C functions and assembly macros. This is a maintenenace burden
| and is liable to lead to subtle bugs, so let's get rid of the assembly
| macros, and always use the C functions. This requires refactoring
| some assembly functions to have a C wrapper.
[...]
> +static inline int invalidate_icache_range(unsigned long start,
> + unsigned long end)
> +{
> + int rv;
> +#if ARM64_HAS_CACHE_DIC
> + rv = arch_invalidate_icache_range(start, end);
> +#else
> + uaccess_ttbr0_enable();
> + rv = arch_invalidate_icache_range(start, end);
> + uaccess_ttbr0_disable();
> +#endif
> + return rv;
> +}
This ifdeffery is not the same as an alternative_if, and even if it were
the ARM64_HAS_CACHE_DIC behaviour is not the same as the existing
assembly.
This should be:
static inline int invalidate_icache_range(unsigned long start,
unsigned long end)
{
int ret;
if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
isb();
return 0;
}
uaccess_ttbr0_enable();
ret = arch_invalidate_icache_range(start, end);
uaccess_ttbr0_disable();
return ret;
}
The 'arch_' prefix should probably be 'asm_' (or have an '_asm' suffix),
since this is entirely local to the arch code, and even then should only
be called from the C wrappers.
Thanks,
Mark.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] Rationalize max_grant_frames and max_maptrack_frames handling
From: George Dunlap
Xen used to have single, system-wide limits for the number of grant
frames and maptrack frames a guest was allowed to create. Increasing
or decreasing this single limit on the Xen command-line would change
the limit for all guests on the system.
Later, per-domain limits for these values was created. The system-wide
limits became strict limits: domains could not be created with higher
limits, but could be created with lower limits. However, that change
also introduced a range of different "default" values into various
places in the toolstack:
- The python libxc bindings hard-coded these values to 32 and 1024,
respectively
- The libxl default values are 32 and 1024 respectively.
- xl will use the libxl default for maptrack, but does its own default
calculation for grant frames: either 32 or 64, based on the max
possible mfn.
These defaults interact poorly with the hypervisor command-line limit:
- The hypervisor command-line limit cannot be used to raise the limit
for all guests anymore, as the default in the toolstack will
effectively override this.
- If you use the hypervisor command-line limit to *reduce* the limit,
then the "default" values generated by the toolstack are too high,
and all guest creations will fail.
In other words, the toolstack defaults require any change to be
effected by having the admin explicitly specify a new value in every
guest.
In order to address this, have grant_table_init treat negative values
for max_grant_frames and max_maptrack_frames as instructions to use the
system-wide default, and have all the above toolstacks default to passing
-1 unless a different value is explicitly configured.
This restores the old behavior in that changing the hypervisor command-line
option can change the behavior for all guests, while retaining the ability
to set per-guest values. It also removes the bug that reducing the
system-wide max will cause all domains without explicit limits to fail.
NOTE: The Ocaml bindings require the caller to always specify a value, and
the code to start a xenstored stubdomain hard-codes these to 4 and
128 respectively; this behavour will not be modified.
Signed-off-by: George Dunlap
Signed-off-by: Paul Durrant
---
Cc: Ian Jackson
Cc: Wei Liu
Cc: Andrew Cooper
Cc: George Dunlap
Cc: Jan Beulich
Cc: Julien Grall
Cc: Konrad Rzeszutek Wilk
Cc: Stefano Stabellini
Cc: Anthony PERARD
Cc: "Marek Marczykowski-Górecki"
Cc: Volodymyr Babchuk
Cc: "Roger Pau Monné"
v2:
- re-worked George's original commit massage a little
- fixed the text in xl.conf.5.pod
- use -1 as the sentinel value for 'default' and < 0 for checking it
---
docs/man/xl.conf.5.pod| 6 --
tools/libxl/libxl.h | 4 ++--
tools/libxl/libxl_types.idl | 4 ++--
tools/python/xen/lowlevel/xc/xc.c | 4 ++--
tools/xl/xl.c | 8
tools/xl/xl_parse.c | 3 ++-
xen/arch/arm/setup.c | 2 +-
xen/arch/x86/setup.c | 4 ++--
xen/common/grant_table.c | 10 --
xen/include/public/domctl.h | 10 ++
xen/include/xen/grant_table.h | 8
11 files changed, 33 insertions(+), 30 deletions(-)
diff --git a/docs/man/xl.conf.5.pod b/docs/man/xl.conf.5.pod
index 962144e38e..207ab3e77a 100644
--- a/docs/man/xl.conf.5.pod
+++ b/docs/man/xl.conf.5.pod
@@ -81,13 +81,15 @@ Default: C
Sets the default value for the C domain config value.
-Default: C<32> on hosts up to 16TB of memory, C<64> on hosts larger than 16TB
+Default: value of Xen command line B parameter (or its
+default value if unspecified).
=item B
Sets the default value for the C domain config value.
-Default: C<1024>
+Default: value of Xen command line B
+parameter (or its default value if unspecified).
=item B
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 49b56fa1a3..a2a5d321c5 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -364,8 +364,8 @@
*/
#define LIBXL_HAVE_BUILDINFO_GRANT_LIMITS 1
-#define LIBXL_MAX_GRANT_FRAMES_DEFAULT 32
-#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT 1024
+#define LIBXL_MAX_GRANT_FRAMES_DEFAULT -1
+#define LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT -1
/*
* LIBXL_HAVE_BUILDINFO_* indicates that libxl_domain_build_info has
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0546d7865a..63e29bb2fb 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -511,8 +511,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
("vnuma_nodes", Array(libxl_vnode_info, "num_vnuma_nodes")),
-("max_grant_frames",uint32, {'init_val':
'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
-("max_maptrack_frames", uint32, {'init_val':
'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
+("max_grant_frames",integer, {'init_val':
'LIBXL_MAX_GRANT_FRAMES_DEFAULT'}),
+("max_maptrack_frames", integer, {'init_val':
'LIBXL_MAX_MAPTRACK_FRAMES_DEFAULT'}),
[Xen-devel] [xen-unstable-smoke test] 144322: tolerable all pass - PUSHED
flight 144322 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/144322/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 34c11725483beb45499f934c7e06e00b55f04ef4 baseline version: xen 5530782cfe70ed22fe44358f6a10c38916443b42 Last test of basis 144310 2019-11-26 15:01:24 Z0 days Testing same since 144322 2019-11-27 11:01:00 Z0 days1 attempts People who touched revisions under test: George Dunlap Ian Jackson Wei Liu 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 5530782cfe..34c1172548 34c11725483beb45499f934c7e06e00b55f04ef4 -> smoke ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [Xen-users] 4.13RC3 and PVHVM makes drive drops just after boot
Thanks George, I have the system setup for testing, so happy to test any patches that may come out. Best Regards, Andrew. On 27/11/19 20:38, George Dunlap wrote: Andrew, thanks for the report. Redirecting to xen-devel, as it looks like a bug in a development version of Xen. -George On Wed, Nov 27, 2019 at 4:27 AM Andrew wrote: Hi Everyone, We have been trying to get Xen + QEMU 4.x working with Ceph/rbd. A like-for-like build process works with Xen 4.11 & 4.12 + QEMU 3.1. So we think it is a QEMU 4.x + Xen issue. The guest starts the boot process in a full hvm guest (ie, gets to grub, then disk drops, and we end up in busy box and not being able to boot the guest). So the ceph/rbd config starts to be read, then stops/drops. These entries are in the logs many times: qemu-system-i386: failed to create 'qdisk' device '768': failed to create drive: Could not open 'rbd:testvms/testvm-disk0:id=admin:key=AQB20M1dsjYmKRAAV7vhmyH/jFhfP22vaEQAvw==:conf=/etc/ceph/ceph.conf': No such file or directory If we set: xen_platform_pci=0 Then it boots fine (all other config is like for like). Qemu was compiled with rbd support, and this is confirmed as not working when not using xen_platform_pci. The problem is that xen_platform_pci=0 is a massive hit on network performance. Is anyone aware of a way to enable the above without the drive dropping? Many thanks for any input/advice/directions. Best Regards, Andrew. Other notes: Within the guest, we see: xenbus_probe_frontend: Waiting for devices to initialise: (then a time-out/count in seconds) xenbus_probe_frontend: Timeout connecting to device: device/vbd/768 (local state 1, remote state 1) [20191127T04:19:27.998Z] A85 newconn [20191127T04:19:28.000Z] A85.1rm/local/domain/7 [20191127T04:19:28.000Z] A85.1write /local/domain/7 [20191127T04:19:28.001Z] A85.1setperms /local/domain/7 n0 r7 [20191127T04:19:28.001Z] A85.1rm /vm/4f17921c-5198-44f9-89c1-e67188586ba4 [20191127T04:19:28.001Z] A85.1write /vm/4f17921c-5198-44f9-89c1-e67188586ba4 [20191127T04:19:28.001Z] A85.1setperms /vm/4f17921c-5198-44f9-89c1-e67188586ba4 n0 r7 [20191127T04:19:28.002Z] A85.1rm/libxl/7 [20191127T04:19:28.002Z] A85.1write /libxl/7 [20191127T04:19:28.002Z] A85.1setperms /libxl/7 n0 [20191127T04:19:28.002Z] A85.1write /libxl/7/device [20191127T04:19:28.003Z] A85.1setperms /libxl/7/device n0 [20191127T04:19:28.003Z] A85.1write /local/domain/7/vm /vm/4f17921c-5198-44f9-89c1-e67188586ba4 [20191127T04:19:28.005Z] A85.1write /local/domain/7/name test [20191127T04:19:28.005Z] A85.1write /vm/4f17921c-5198-44f9-89c1-e67188586ba4/name test [20191127T04:19:28.005Z] A85.1write /local/domain/7/cpu [20191127T04:19:28.006Z] A85.1setperms /local/domain/7/cpu n0 r7 [20191127T04:19:28.006Z] A85.1write /local/domain/7/memory [20191127T04:19:28.006Z] A85.1setperms /local/domain/7/memory n0 r7 [20191127T04:19:28.006Z] A85.1write /local/domain/7/device [20191127T04:19:28.006Z] A85.1setperms /local/domain/7/device n0 r7 [20191127T04:19:28.007Z] A85.1write /local/domain/7/control [20191127T04:19:28.007Z] A85.1setperms /local/domain/7/control n0 r7 [20191127T04:19:28.007Z] A85.1write /local/domain/7/hvmloader [20191127T04:19:28.007Z] A85.1setperms /local/domain/7/hvmloader n0 r7 [20191127T04:19:28.007Z] A85.1write /local/domain/7/control/shutdown [20191127T04:19:28.008Z] A85.1setperms /local/domain/7/control/shutdown n7 [20191127T04:19:28.008Z] A85.1write /local/domain/7/control/feature-poweroff [20191127T04:19:28.008Z] A85.1setperms /local/domain/7/control/feature-poweroff n7 [20191127T04:19:28.008Z] A85.1write /local/domain/7/control/feature-reboot [20191127T04:19:28.008Z] A85.1setperms /local/domain/7/control/feature-reboot n7 [20191127T04:19:28.009Z] A85.1write /local/domain/7/control/feature-suspend [20191127T04:19:28.009Z] A85.1setperms /local/domain/7/control/feature-suspend n7 [20191127T04:19:28.009Z] A85.1write /local/domain/7/control/feature-s3 [20191127T04:19:28.009Z] A85.1setperms /local/domain/7/control/feature-s3 n7 [20191127T04:19:28.010Z] A85.1write /local/domain/7/control/feature-s4 [20191127T04:19:28.010Z] A85.1setperms /local/domain/7/control/feature-s4 n7 [20191127T04:19:28.010Z] A85.1write /local/domain/7/control/sysrq [20191127T04:19:28.010Z] A85.1setperms /local/domain/7/control/sysrq n7 [20191127T04:19:28.010Z] A85.1write /local/domain/7/device/suspend/event-channel [20191127T04:19:28.011Z] A85.1setperms /local/domain/7/device/suspend/event-channel n7
Re: [Xen-devel] [PATCH] MAINTAINERS: Update path to the livepatch documentation
On 27.11.19 13:50, Julien Grall wrote:
(+Juergen)
Hi,
On 26/11/2019 14:05, Jan Beulich wrote:
On 26.11.2019 14:30, Julien Grall wrote:
Commit d661611d08 "docs/markdown: Switch to using pandoc, and fix
underscore escaping" converted the livepatch documentation from markdown
to pandoc.
Update MAINTAINERS to reflect the change so the correct maintainers are
CCed to the patches.
Fixes: d661611d08 ("docs/markdown: Switch to using pandoc, and fix
underscore escaping")
Signed-off-by: Julien Grall
Acked-by: Jan Beulich
@Juergen: This is just an update to MAINTAINERS file. Would you be happy
to take it for Xen 4.13?
Release-acked-by: Juergen Gross
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] MAINTAINERS: Update path to the livepatch documentation
(+Juergen)
Hi,
On 26/11/2019 14:05, Jan Beulich wrote:
On 26.11.2019 14:30, Julien Grall wrote:
Commit d661611d08 "docs/markdown: Switch to using pandoc, and fix
underscore escaping" converted the livepatch documentation from markdown
to pandoc.
Update MAINTAINERS to reflect the change so the correct maintainers are
CCed to the patches.
Fixes: d661611d08 ("docs/markdown: Switch to using pandoc, and fix underscore
escaping")
Signed-off-by: Julien Grall
Acked-by: Jan Beulich
@Juergen: This is just an update to MAINTAINERS file. Would you be happy
to take it for Xen 4.13?
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 for 4.13] x86/microcode: refuse to load the same revision ucode
On 27.11.19 11:04, Sergey Dyasli wrote: Currently if a user tries to live-load the same or older ucode revision than CPU already has, he will get a single message in Xen log like: (XEN) 128 cores are to update their microcode No actual ucode loading will happen and this situation can be quite confusing. Fix this by starting ucode update only when the provided ucode revision is higher than the currently cached one (if any). This is based on the property that if microcode_cache exists, all CPUs in the system should have at least that ucode revision. Additionally, print a user friendly message if no matching or newer ucode can be found in the provided blob. This also requires ignoring -ENODATA in AMD-side code, otherwise the message given to the user is: (XEN) Parsing microcode blob error -61 Which actually means that a ucode blob was parsed fine, but no matching ucode was found. Signed-off-by: Sergey Dyasli Reviewed-by: Chao Gao Acked-by: Jan Beulich Release-acked-by: Juergen Gross Juergen ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] getting 4.12.2 ready
On 25/11/2019 15:10, Jan Beulich wrote: All, Hi, the 4.12.2 stable release is due in about 2 weeks time. Please point out backporting candidates that you find missing from the respective stable trees. Most of the series "xen/arm: XSA-201 and XSA-263 fixes" [1] should be backported to at least Xen 4.12 (this is already in staging). This would error issues with SError and SSBD. But I haven't had the chance to check this is applying cleanly to Xen 4.12. Maybe Stefano can take a look? Cheers [1] https://www.mail-archive.com/xen-devel@lists.xenproject.org/msg59283.html -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames and max_maptrack_frames handling
On 27.11.19 13:07, George Dunlap wrote:
On 11/27/19 4:34 AM, Jürgen Groß wrote:
On 26.11.19 18:30, George Dunlap wrote:
On 11/26/19 5:17 PM, George Dunlap wrote:
- xl will use the libxl default for maptrack, but does its own default
calculation for grant frames: either 32 or 64, based on the max
possible mfn.
[snip]
@@ -199,13 +198,6 @@ static void parse_global_config(const char
*configfile,
if (!xlu_cfg_get_long (config, "max_grant_frames", , 0))
max_grant_frames = l;
- else {
- libxl_physinfo_init();
- max_grant_frames = (libxl_get_physinfo(ctx, ) != 0 ||
- !(physinfo.max_possible_mfn >> 32))
- ? 32 : 64;
- libxl_physinfo_dispose();
- }
Sorry, meant to add a patch to add this functionality back into the
hypervisor -- i.e., so that opt_max_grant_frames would be 32 on systems
with 32-bit mfns.
But this seems like a fairly strange calculation anyway; it's not clear
to me where it would have come from.
mfns above the 32-bit limit require to use grant v2. This in turn
doubles the grant frames needed for the same number of grants.
But is large mfns the *only* reason to use grant v2? Aren't modern
guests going to use grant v2 regardless of the max mfn size?
Large mfns leave the guest no choice. Linux kernel V2 support was
removed and I reintroduced it for being able to support large mfns in
guests.
Current Linux kernel will use V1 if the max mfn fits in 32 bits and V2
only if there can be memory above that boundary.
Juergen
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames and max_maptrack_frames handling
On 11/27/19 4:34 AM, Jürgen Groß wrote:
> On 26.11.19 18:30, George Dunlap wrote:
>> On 11/26/19 5:17 PM, George Dunlap wrote:
>>> - xl will use the libxl default for maptrack, but does its own default
>>> calculation for grant frames: either 32 or 64, based on the max
>>> possible mfn.
>>
>> [snip]
>>
>>> @@ -199,13 +198,6 @@ static void parse_global_config(const char
>>> *configfile,
>>> if (!xlu_cfg_get_long (config, "max_grant_frames", , 0))
>>> max_grant_frames = l;
>>> - else {
>>> - libxl_physinfo_init();
>>> - max_grant_frames = (libxl_get_physinfo(ctx, ) != 0 ||
>>> - !(physinfo.max_possible_mfn >> 32))
>>> - ? 32 : 64;
>>> - libxl_physinfo_dispose();
>>> - }
>>
>> Sorry, meant to add a patch to add this functionality back into the
>> hypervisor -- i.e., so that opt_max_grant_frames would be 32 on systems
>> with 32-bit mfns.
>>
>> But this seems like a fairly strange calculation anyway; it's not clear
>> to me where it would have come from.
> mfns above the 32-bit limit require to use grant v2. This in turn
> doubles the grant frames needed for the same number of grants.
But is large mfns the *only* reason to use grant v2? Aren't modern
guests going to use grant v2 regardless of the max mfn size?
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable test] 144313: tolerable FAIL - PUSHED
flight 144313 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/144313/ Failures :-/ but no regressions. Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 17 guest-saverestore.2 fail REGR. vs. 144301 test-armhf-armhf-xl-rtds16 guest-start/debian.repeat fail REGR. vs. 144301 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 144301 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 144301 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 144301 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 144301 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 144301 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 144301 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 144301 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 144301 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 144301 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: xen 5530782cfe70ed22fe44358f6a10c38916443b42 baseline version: xen 77beba7c921a286c31a2a76f26500047f353614a Last test of basis 144301 2019-11-26 02:30:10 Z1 days Testing same since 144313 2019-11-26 20:38:15 Z0 days1 attempts People who touched revisions under test: Anthony PERARD George Dunlap Jan Beulich jobs: build-amd64-xsm
[Xen-devel] [PATCH v2] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed
From: Julien Grall
A guest will setup a shared page with the hypervisor for each vCPU via
XENPMU_init. The page will then get mapped in the hypervisor and only
released when XENPMU_finish is called.
This means that if the guest fails to invoke XENPMU_finish, e.g if it is
destroyed rather than cleanly shut down, the page will stay mapped in the
hypervisor. One of the consequences is the domain can never be fully
destroyed as a page reference is still held.
As Xen should never rely on the guest to correctly clean-up any
allocation in the hypervisor, we should also unmap such pages during the
domain destruction if there are any left.
We can re-use the same logic as in pvpmu_finish(). To avoid
duplication, move the logic in a new function that can also be called
from vpmu_destroy().
NOTE: The call to vpmu_destroy() must also be moved from
arch_vcpu_destroy() into domain_relinquish_resources() such that the
reference on the mapped page does not prevent domain_destroy() (which
calls arch_vcpu_destroy()) from being called.
Also, whils it appears that vpmu_arch_destroy() is idempotent it is
by no means obvious. Hence move manipulation of the
VPMU_CONTEXT_ALLOCATED flag out of implementation specific code and
make sure it is cleared at the end of vpmu_arch_destroy().
Signed-off-by: Julien Grall
Signed-off-by: Paul Durrant
---
Cc: Jan Beulich
Cc: Andrew Cooper
Cc: Wei Liu
Cc: "Roger Pau Monné"
Cc: Jun Nakajima
Cc: Kevin Tian
v2:
- Re-word commit comment slightly
- Re-enforce idempotency of vmpu_arch_destroy()
- Move invocation of vpmu_destroy() earlier in
domain_relinquish_resources()
---
xen/arch/x86/cpu/vpmu.c | 49 +--
xen/arch/x86/cpu/vpmu_amd.c | 1 -
xen/arch/x86/cpu/vpmu_intel.c | 2 --
xen/arch/x86/domain.c | 10 ---
4 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index f397183ec3..08742a5e22 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -479,6 +479,8 @@ static int vpmu_arch_initialise(struct vcpu *v)
if ( ret )
printk(XENLOG_G_WARNING "VPMU: Initialization failed for %pv\n", v);
+else
+vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED);
return ret;
}
@@ -576,11 +578,36 @@ static void vpmu_arch_destroy(struct vcpu *v)
vpmu->arch_vpmu_ops->arch_vpmu_destroy(v);
}
+
+vpmu_reset(vpmu, VPMU_CONTEXT_ALLOCATED);
}
-void vpmu_destroy(struct vcpu *v)
+static void vpmu_cleanup(struct vcpu *v)
{
+struct vpmu_struct *vpmu = vcpu_vpmu(v);
+mfn_t mfn;
+void *xenpmu_data;
+
+spin_lock(>vpmu_lock);
+
vpmu_arch_destroy(v);
+xenpmu_data = vpmu->xenpmu_data;
+vpmu->xenpmu_data = NULL;
+
+spin_unlock(>vpmu_lock);
+
+if ( xenpmu_data )
+{
+mfn = domain_page_map_to_mfn(xenpmu_data);
+ASSERT(mfn_valid(mfn));
+unmap_domain_page_global(xenpmu_data);
+put_page_and_type(mfn_to_page(mfn));
+}
+}
+
+void vpmu_destroy(struct vcpu *v)
+{
+vpmu_cleanup(v);
put_vpmu(v);
}
@@ -639,9 +666,6 @@ static int pvpmu_init(struct domain *d, xen_pmu_params_t
*params)
static void pvpmu_finish(struct domain *d, xen_pmu_params_t *params)
{
struct vcpu *v;
-struct vpmu_struct *vpmu;
-mfn_t mfn;
-void *xenpmu_data;
if ( (params->vcpu >= d->max_vcpus) || (d->vcpu[params->vcpu] == NULL) )
return;
@@ -650,22 +674,7 @@ static void pvpmu_finish(struct domain *d,
xen_pmu_params_t *params)
if ( v != current )
vcpu_pause(v);
-vpmu = vcpu_vpmu(v);
-spin_lock(>vpmu_lock);
-
-vpmu_arch_destroy(v);
-xenpmu_data = vpmu->xenpmu_data;
-vpmu->xenpmu_data = NULL;
-
-spin_unlock(>vpmu_lock);
-
-if ( xenpmu_data )
-{
-mfn = domain_page_map_to_mfn(xenpmu_data);
-ASSERT(mfn_valid(mfn));
-unmap_domain_page_global(xenpmu_data);
-put_page_and_type(mfn_to_page(mfn));
-}
+vpmu_cleanup(v);
if ( v != current )
vcpu_unpause(v);
diff --git a/xen/arch/x86/cpu/vpmu_amd.c b/xen/arch/x86/cpu/vpmu_amd.c
index 3c6799b42c..8ca26f1e3a 100644
--- a/xen/arch/x86/cpu/vpmu_amd.c
+++ b/xen/arch/x86/cpu/vpmu_amd.c
@@ -534,7 +534,6 @@ int svm_vpmu_initialise(struct vcpu *v)
vpmu->arch_vpmu_ops = _vpmu_ops;
-vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED);
return 0;
}
diff --git a/xen/arch/x86/cpu/vpmu_intel.c b/xen/arch/x86/cpu/vpmu_intel.c
index 6e27f6ec8e..a92d882597 100644
--- a/xen/arch/x86/cpu/vpmu_intel.c
+++ b/xen/arch/x86/cpu/vpmu_intel.c
@@ -483,8 +483,6 @@ static int core2_vpmu_alloc_resource(struct vcpu *v)
memcpy(>xenpmu_data->pmu.c.intel, core2_vpmu_cxt, regs_off);
}
-vpmu_set(vpmu, VPMU_CONTEXT_ALLOCATED);
-
return 1;
out_err:
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index f1dd86e12e..f5c0c378ef 100644
--- a/xen/arch/x86/domain.c
+++
Re: [Xen-devel] UEFI support on Dell boxes (was: Re: Status of 4.13)
On Nov 27, 2019, at 04:16, Jan Beulich wrote: > > On 26.11.2019 22:20, Rich Persaud wrote: >> As an intermediate step, could we have an umbrella opt-in >> Kconfig option (CONFIG_EFI_NONSPEC_COMPATIBILITY?) that >> enables multiple EFI options for maximum hardware compatibility? >> For this thread and Xen 4.13, that would be >> EFI_SET_VIRTUAL_ADDRESS_MAP and efi=attr=uc. If more >> options/quirks are added in the future, downstreams using >> EFI_NONSPEC_COMPATIBILITY would get them by default. > > While I don't particularly like it, I'd be okay with having such > an option, provided it doesn't hamper code readability too much. > However - why would you stop at those two things? Why not also > exclude reboot through UEFI (as indicated by Andrew), or use of > runtime services as a whole? What about /mapbs? The fundamental > problem I see here really is - where would we draw the line? If we take this thread as an example, a middle ground was found among developers motivated to maintain the workarounds for downstream projects with affected hardware. Qubes, EVE & OpenXT are used on edge/client devices that often have (relative to servers) a shorter lifetime, with more device churn and support costs. These two initial options would address current pain points and enable the use of upstream Xen + EFI RS on more devices, e.g. for OTA updates with forward-sealed integrity measurements. The line could change if more downstreams adopt the option and/or new devices appear that have both customer adoption and problematic firmware behavior. Rich ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed
Hi, On 27/11/2019 09:44, Jan Beulich wrote: On 26.11.2019 18:17, Paul Durrant wrote: From: Julien Grall A guest will setup a shared page with the hypervisor for each vCPU via XENPMU_init. The page will then get mapped in the hypervisor and only released when XEMPMU_finish is called. This means that if the guest is not shutdown gracefully (such as via xl destroy), the page will stay mapped in the hypervisor. Isn't this still too weak a description? It's not the tool stack invoking XENPMU_finish, but the guest itself afaics. I.e. a misbehaving guest could prevent proper cleanup even with graceful shutdown. @@ -2224,6 +2221,9 @@ int domain_relinquish_resources(struct domain *d) if ( is_hvm_domain(d) ) hvm_domain_relinquish_resources(d); +for_each_vcpu ( d, v ) +vpmu_destroy(v); + return 0; } I think simple things which may allow shrinking the page lists should be done early in the function. As vpmu_destroy() looks to be idempotent, how about leveraging the very first for_each_vcpu() loop in the function (there are too many of them there anyway, at least for my taste)? This is not entirely obvious that vpmu_destroy() is idempotent. For instance, I can't find out who is clearing VCPU_CONTEXT_ALLOCATED. so I think vcpu_arch_destroy() would be executed over and over. I don't know whether this is an issue, but I can't figure out that is it not one. Did I miss anything? Cheers, ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On Wed, Nov 27, 2019 at 12:30:06PM +0100, Jan Beulich wrote:
> On 27.11.2019 12:22, Roger Pau Monné wrote:
> > On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
> >> On 26.11.2019 14:26, Roger Pau Monne wrote:
> >>> --- a/xen/arch/x86/hvm/irq.c
> >>> +++ b/xen/arch/x86/hvm/irq.c
> >>> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
> >>> via)
> >>> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> >>> {
> >>> struct hvm_domain *plat = >domain->arch.hvm;
> >>> -int vector;
> >>> +/*
> >>> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
> >>> when
> >>> + * using posted interrupts.
> >>> + */
> >>> +int vector = vlapic_has_pending_irq(v);
> >>
> >> Did you consider doing this conditionally either here ...
> >>
> >>> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct
> >>> vcpu *v)
> >>> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> >>> return hvm_intack_pic(0);
> >>>
> >>> -vector = vlapic_has_pending_irq(v);
> >>> if ( vector != -1 )
> >>> return hvm_intack_lapic(vector);
> >>
> >> ... or here?
> >
> > I'm afraid I don't follow. The whole point of this change is to ensure
> > vlapic_has_pending_irq is unconditionally called in
> > hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
> > conditionally...".
>
> Do it early when using interrupt posting, and keep it in its
> current place otherwise.
>
> >> I ask not only because the function isn't exactly
> >> cheap to call (as iirc you did also mention during the v2
> >> discussion), but also because of its interaction with Viridian
> >> and nested mode. In case of problems there, avoiding the use
> >> of interrupt posting would be a workaround in such cases then.
> >
> > Would you like me to export something like vlapic_sync_pir_to_irr and
> > call it unconditionally instead of calling vlapic_has_pending_irq?
>
> This looks to be another option, yes. Albeit instead of making
> non-static (which I assume is what you mean by "export"), maybe
> simply make this a static inline in vlapic.h then.
Yes, that would work and IMO is better than moving the call to
vlapic_has_pending_irq around. Are you OK with this approach?
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] x86/vmx: always sync PIR to IRR before vmentry
On Wed, Nov 27, 2019 at 12:34:09PM +0100, Jan Beulich wrote:
> On 27.11.2019 12:29, Roger Pau Monné wrote:
> > On Wed, Nov 27, 2019 at 12:16:37PM +0100, Jan Beulich wrote:
> >> On 27.11.2019 12:03, Roger Pau Monné wrote:
> >>> On Wed, Nov 27, 2019 at 02:07:16AM +, Tian, Kevin wrote:
> Then what's the difference from original logic?
> >>>
> >>> The original logic is:
> >>>
> >>> if ( running && (in_irq() || (v != current)) )
> >>> {
> >>> unsigned int cpu = v->processor;
> >>>
> >>> if ( cpu != smp_processor_id() )
> >>> send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
> >>> else if ( !softirq_pending(cpu) )
> >>> raise_softirq(VCPU_KICK_SOFTIRQ);
> >>> }
> >>>
> >>> Which I find much harder to understand. For example I'm not sure of
> >>> what's the benefit of doing the cpu != smp_processor_id() check
> >>> instead of simply doing v != current (like in the outer if condition).
> >>
> >> There are two aspects to consider: One is that v->processor
> >> may equal smp_processor_id() also for v != current. The other
> >> is that without this check in the if() it would need adding
> >> to the else-if(). I'm not sure to what degree which of the
> >> two matters functionality wise.
> >
> > Since the vCPU is running v->processor can only equal smp_processor_id
> > if v == current,
>
> What tells you that it is running? It had been running at the
> time the flag was latched (before vcpu_unblock()), but may
> have got de-scheduled in the meantime.
Right, but if it's not running then it doesn't really matter that we
send an IPI or raise a softirq, the PIR to IRR sync will happen anyway
before the vCPU is resumed.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] x86/vmx: always sync PIR to IRR before vmentry
On 27.11.2019 12:29, Roger Pau Monné wrote:
> On Wed, Nov 27, 2019 at 12:16:37PM +0100, Jan Beulich wrote:
>> On 27.11.2019 12:03, Roger Pau Monné wrote:
>>> On Wed, Nov 27, 2019 at 02:07:16AM +, Tian, Kevin wrote:
Then what's the difference from original logic?
>>>
>>> The original logic is:
>>>
>>> if ( running && (in_irq() || (v != current)) )
>>> {
>>> unsigned int cpu = v->processor;
>>>
>>> if ( cpu != smp_processor_id() )
>>> send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
>>> else if ( !softirq_pending(cpu) )
>>> raise_softirq(VCPU_KICK_SOFTIRQ);
>>> }
>>>
>>> Which I find much harder to understand. For example I'm not sure of
>>> what's the benefit of doing the cpu != smp_processor_id() check
>>> instead of simply doing v != current (like in the outer if condition).
>>
>> There are two aspects to consider: One is that v->processor
>> may equal smp_processor_id() also for v != current. The other
>> is that without this check in the if() it would need adding
>> to the else-if(). I'm not sure to what degree which of the
>> two matters functionality wise.
>
> Since the vCPU is running v->processor can only equal smp_processor_id
> if v == current,
What tells you that it is running? It had been running at the
time the flag was latched (before vcpu_unblock()), but may
have got de-scheduled in the meantime.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] UEFI support on Dell boxes (was: Re: Status of 4.13)
On Wed, Nov 27, 2019 at 10:14:56AM +0100, Jan Beulich wrote: > On 26.11.2019 22:20, Rich Persaud wrote: > > As an intermediate step, could we have an umbrella opt-in > > Kconfig option (CONFIG_EFI_NONSPEC_COMPATIBILITY?) that > > enables multiple EFI options for maximum hardware compatibility? > > For this thread and Xen 4.13, that would be > > EFI_SET_VIRTUAL_ADDRESS_MAP and efi=attr=uc. If more > > options/quirks are added in the future, downstreams using > > EFI_NONSPEC_COMPATIBILITY would get them by default. > > While I don't particularly like it, I'd be okay with having such > an option, provided it doesn't hamper code readability too much. > However - why would you stop at those two things? Why not also > exclude reboot through UEFI (as indicated by Andrew), or use of > runtime services as a whole? What about /mapbs? The fundamental > problem I see here really is - where would we draw the line? Yes, it isn't easy to draw that line for all the downstream projects at once. For example it looks like efi=no-rs is an acceptable compromise for Project EVE, while it isn't for Qubes or OpenXT. But moving from "apply this set of patches" to "enable those options" would be an improvement. Ideally Xen should work out of the box on as many boxes as possible. If that means enabling some workarounds by default, I'm fine with it (unless it _severely_ impact other configurations). In Qubes we struggle with hardware compatibility because of large variety of client hardware, firmware and configuration. Whatever we say here, in the end it boils down to "does project X work on my hardware?". Not sure about other Xen use cases, but we prefer to have the answer "yes", whenever it's reasonably possible. I think enabling efi=attr=uc and EFI_SET_VIRTUAL_ADDRESS_MAP by default is a reasonable approach. Defaulting to a different reboot method may be too, but I haven't seen too many machines impacted by this particular issue. Maybe because Xen+UEFI breaks much earlier there. FWIW we do enable efi=attr=uc, /mapbs and /noexitboot by default (until EFI_SET_VIRTUAL_ADDRESS_MAP was added). -- Best Regards, Marek Marczykowski-Górecki Invisible Things Lab A: Because it messes up the order in which people normally read text. Q: Why is top-posting such a bad thing? signature.asc Description: PGP signature ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On 27.11.2019 12:22, Roger Pau Monné wrote:
> On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
>> On 26.11.2019 14:26, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/irq.c
>>> +++ b/xen/arch/x86/hvm/irq.c
>>> @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
>>> via)
>>> struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
>>> {
>>> struct hvm_domain *plat = >domain->arch.hvm;
>>> -int vector;
>>> +/*
>>> + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
>>> when
>>> + * using posted interrupts.
>>> + */
>>> +int vector = vlapic_has_pending_irq(v);
>>
>> Did you consider doing this conditionally either here ...
>>
>>> @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu
>>> *v)
>>> if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
>>> return hvm_intack_pic(0);
>>>
>>> -vector = vlapic_has_pending_irq(v);
>>> if ( vector != -1 )
>>> return hvm_intack_lapic(vector);
>>
>> ... or here?
>
> I'm afraid I don't follow. The whole point of this change is to ensure
> vlapic_has_pending_irq is unconditionally called in
> hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
> conditionally...".
Do it early when using interrupt posting, and keep it in its
current place otherwise.
>> I ask not only because the function isn't exactly
>> cheap to call (as iirc you did also mention during the v2
>> discussion), but also because of its interaction with Viridian
>> and nested mode. In case of problems there, avoiding the use
>> of interrupt posting would be a workaround in such cases then.
>
> Would you like me to export something like vlapic_sync_pir_to_irr and
> call it unconditionally instead of calling vlapic_has_pending_irq?
This looks to be another option, yes. Albeit instead of making
non-static (which I assume is what you mean by "export"), maybe
simply make this a static inline in vlapic.h then.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] x86/vmx: always sync PIR to IRR before vmentry
On Wed, Nov 27, 2019 at 12:16:37PM +0100, Jan Beulich wrote:
> On 27.11.2019 12:03, Roger Pau Monné wrote:
> > On Wed, Nov 27, 2019 at 02:07:16AM +, Tian, Kevin wrote:
> >> Then what's the difference from original logic?
> >
> > The original logic is:
> >
> > if ( running && (in_irq() || (v != current)) )
> > {
> > unsigned int cpu = v->processor;
> >
> > if ( cpu != smp_processor_id() )
> > send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
> > else if ( !softirq_pending(cpu) )
> > raise_softirq(VCPU_KICK_SOFTIRQ);
> > }
> >
> > Which I find much harder to understand. For example I'm not sure of
> > what's the benefit of doing the cpu != smp_processor_id() check
> > instead of simply doing v != current (like in the outer if condition).
>
> There are two aspects to consider: One is that v->processor
> may equal smp_processor_id() also for v != current. The other
> is that without this check in the if() it would need adding
> to the else-if(). I'm not sure to what degree which of the
> two matters functionality wise.
Since the vCPU is running v->processor can only equal smp_processor_id
if v == current, and hence I think both checks achieve exactly the
same end result, it's just that IMO doing the outer one with v !=
current and the inner one with cpu != smp_processor_id() is confusing.
Maybe I'm missing something else that actually requires doing the
inner check with v->processor and smp_processor_id().
Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed
On 27.11.2019 12:14, Julien Grall wrote:
> Hi,
>
> On 27/11/2019 09:44, Jan Beulich wrote:
>> On 26.11.2019 18:17, Paul Durrant wrote:
>>> From: Julien Grall
>>>
>>> A guest will setup a shared page with the hypervisor for each vCPU via
>>> XENPMU_init. The page will then get mapped in the hypervisor and only
>>> released when XEMPMU_finish is called.
>>>
>>> This means that if the guest is not shutdown gracefully (such as via xl
>>> destroy), the page will stay mapped in the hypervisor.
>>
>> Isn't this still too weak a description? It's not the tool stack
>> invoking XENPMU_finish, but the guest itself afaics. I.e. a
>> misbehaving guest could prevent proper cleanup even with graceful
>> shutdown.
>>
>>> @@ -2224,6 +2221,9 @@ int domain_relinquish_resources(struct domain *d)
>>> if ( is_hvm_domain(d) )
>>> hvm_domain_relinquish_resources(d);
>>>
>>> +for_each_vcpu ( d, v )
>>> +vpmu_destroy(v);
>>> +
>>> return 0;
>>> }
>>
>> I think simple things which may allow shrinking the page lists
>> should be done early in the function. As vpmu_destroy() looks
>> to be idempotent, how about leveraging the very first
>> for_each_vcpu() loop in the function (there are too many of them
>> there anyway, at least for my taste)?
>
> This is not entirely obvious that vpmu_destroy() is idempotent.
>
> For instance, I can't find out who is clearing VCPU_CONTEXT_ALLOCATED.
> so I think vcpu_arch_destroy() would be executed over and over.
>
> I don't know whether this is an issue, but I can't figure out that is it
> not one. Did I miss anything?
If the function wasn't idempotent, then calling it unconditionally
from domain_relinquish_resources() would be wrong too. After all
the guest may have invoked XENPMU_finish.
As to VCPU_CONTEXT_ALLOCATED - I don't think this ever gets cleared
anywhere. But this by itself doesn't make the function non-
idempotent. The vpmu_clear_last invocation, for example, will
happen just once. And {amd,core2}_vpmu_destroy() look okay at the
first glance, too.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] xen/x86: vpmu: Unmap per-vCPU PMU page when the domain is destroyed
> -Original Message- > From: Julien Grall > Sent: 27 November 2019 11:15 > To: Jan Beulich ; Durrant, Paul > Cc: xen-devel@lists.xenproject.org; Andrew Cooper > ; Roger Pau Monné ; Wei > Liu > Subject: Re: [PATCH] xen/x86: vpmu: Unmap per-vCPU PMU page when the > domain is destroyed > > Hi, > > On 27/11/2019 09:44, Jan Beulich wrote: > > On 26.11.2019 18:17, Paul Durrant wrote: > >> From: Julien Grall > >> > >> A guest will setup a shared page with the hypervisor for each vCPU via > >> XENPMU_init. The page will then get mapped in the hypervisor and only > >> released when XEMPMU_finish is called. > >> > >> This means that if the guest is not shutdown gracefully (such as via xl > >> destroy), the page will stay mapped in the hypervisor. > > > > Isn't this still too weak a description? It's not the tool stack > > invoking XENPMU_finish, but the guest itself afaics. I.e. a > > misbehaving guest could prevent proper cleanup even with graceful > > shutdown. > > > >> @@ -2224,6 +2221,9 @@ int domain_relinquish_resources(struct domain *d) > >> if ( is_hvm_domain(d) ) > >> hvm_domain_relinquish_resources(d); > >> > >> +for_each_vcpu ( d, v ) > >> +vpmu_destroy(v); > >> + > >> return 0; > >> } > > > > I think simple things which may allow shrinking the page lists > > should be done early in the function. As vpmu_destroy() looks > > to be idempotent, how about leveraging the very first > > for_each_vcpu() loop in the function (there are too many of them > > there anyway, at least for my taste)? > > This is not entirely obvious that vpmu_destroy() is idempotent. > > For instance, I can't find out who is clearing VCPU_CONTEXT_ALLOCATED. > so I think vcpu_arch_destroy() would be executed over and over. > > I don't know whether this is an issue, but I can't figure out that is it > not one. Did I miss anything? It's sufficiently unobvious that it is a concern whether a guest invoking XENPMU_finish multiple times can cause harm. I'll see if I can clean that up. Paul > > Cheers, ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 2/2] x86/vmx: always sync PIR to IRR before vmentry
On Tue, Nov 26, 2019 at 05:50:32PM +0100, Jan Beulich wrote:
> On 26.11.2019 14:26, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/irq.c
> > +++ b/xen/arch/x86/hvm/irq.c
> > @@ -515,7 +515,11 @@ void hvm_set_callback_via(struct domain *d, uint64_t
> > via)
> > struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu *v)
> > {
> > struct hvm_domain *plat = >domain->arch.hvm;
> > -int vector;
> > +/*
> > + * Always call vlapic_has_pending_irq so that PIR is synced into IRR
> > when
> > + * using posted interrupts.
> > + */
> > +int vector = vlapic_has_pending_irq(v);
>
> Did you consider doing this conditionally either here ...
>
> > @@ -530,7 +534,6 @@ struct hvm_intack hvm_vcpu_has_pending_irq(struct vcpu
> > *v)
> > if ( vlapic_accept_pic_intr(v) && plat->vpic[0].int_output )
> > return hvm_intack_pic(0);
> >
> > -vector = vlapic_has_pending_irq(v);
> > if ( vector != -1 )
> > return hvm_intack_lapic(vector);
>
> ... or here?
I'm afraid I don't follow. The whole point of this change is to ensure
vlapic_has_pending_irq is unconditionally called in
hvm_vcpu_has_pending_irq, so I'm not sure what you mean by "doing this
conditionally...".
> I ask not only because the function isn't exactly
> cheap to call (as iirc you did also mention during the v2
> discussion), but also because of its interaction with Viridian
> and nested mode. In case of problems there, avoiding the use
> of interrupt posting would be a workaround in such cases then.
Would you like me to export something like vlapic_sync_pir_to_irr and
call it unconditionally instead of calling vlapic_has_pending_irq?
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 v3 1/2] x86/vmx: add ASSERT to prevent syncing PIR to IRR...
On Wed, Nov 27, 2019 at 03:09:51AM +, Tian, Kevin wrote:
> > From: Jan Beulich
> > Sent: Wednesday, November 27, 2019 12:59 AM
> >
> > On 26.11.2019 17:47, Roger Pau Monné wrote:
> > > On Tue, Nov 26, 2019 at 05:32:04PM +0100, Jan Beulich wrote:
> > >> On 26.11.2019 14:26, Roger Pau Monne wrote:
> > >>> --- a/xen/arch/x86/hvm/vmx/vmx.c
> > >>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > >>> @@ -2054,6 +2054,19 @@ static void vmx_sync_pir_to_irr(struct vcpu
> > *v)
> > >>> unsigned int group, i;
> > >>> DECLARE_BITMAP(pending_intr, NR_VECTORS);
> > >>>
> > >>> +if ( v != current && !atomic_read(>pause_count) )
> > >>> +{
> > >>> +/*
> > >>> + * Syncing PIR to IRR must not be done behind the back of the
> > >>> CPU,
> > >>> + * since the IRR is controlled by the hardware when the vCPU is
> > >>> + * executing. Only allow Xen to do such sync if the vCPU is the
> > current
> > >>> + * one or if it's paused: that's required in order to sync the
> > >>> lapic
> > >>> + * state before saving it.
> > >>> + */
> > >>
> > >> Is this stated this way by the SDM anywhere?
> > >
> > > No, I think the SDM is not very clear on this, there's a paragraph
> > > about PIR:
> > >
> > > "The logical processor performs a logical-OR of PIR into VIRR and
> > > clears PIR. No other agent can read or write a PIR bit (or group of
> > > bits) between the time it is read (to determine what to OR into VIRR)
> > > and when it is cleared."
> >
> > Well, this is about PIR, but my question was rather towards the
> > effects on vIRR.
> >
> > >> I ask because the
> > >> comment then really doesn't apply to just this function, but to
> > >> vlapic_{,test_and_}{set,clear}_vector() more generally. It's
> > >> not clear to me at all whether the CPU caches (in an incoherent
> > >> fashion) IRR (and maybe other APIC page elements), rather than
> > >> honoring the atomic updates these macros do.
> > >
> > > IMO syncing PIR to IRR when the vCPU is running on a different pCPU is
> > > likely to at least defeat the purpose of posted interrupts:
> >
> > I agree here.
> >
> > > when the
> > > CPU receives the posted interrupt vector it won't see the
> > > outstanding-notification bit in the posted-interrupt descriptor
> > > because the sync done from a different pCPU would have cleared it, at
> > > which point it's not clear to me that the processor will check vIRR
> > > for pending interrupts. The description in section 29.6
> > > POSTED-INTERRUPT PROCESSING doesn't explicitly mention whether the
> > > value of the outstanding-notification bit affects the logic of posted
> > > interrupt processing.
>
> I think the outstanding-notification is one-off checked for triggering
> interrupt posting process. Once the process starts, there is no need to
> look at it again. The step 3 of posting process in 29.6 clearly says:
>
> "The processor clears the outstanding-notification bit in the posted-
> interrupt descriptor. This is done atomically so as to leave the remainder
> of the descriptor unmodified (e.g., with a locked AND operation)."
Yes, my question would be what happens if the outstanding-notification
bit is 0, does the processor jump to step 6 then?
Does it just ignore the value of the outstanding-notification bit and
continue to step 4?
> But regardless of the hardware behavior, I think it's safe to restrict
> sync_pir_to_irr as this patch does.
>
> >
> > But overall this again is all posted interrupt centric when my
> > question was about vIRR, in particular whether the asserting you
> > add may need to be even more rigid.
> >
> > Anyway, let's see what the VMX maintainers have to say.
> >
>
> There is one paragraph in 29.6:
>
> "Use of the posted-interrupt descriptor differs from that of other data
> structures that are referenced by pointers in a VMCS. There is a general
> requirement that software ensure that each such data structure is
> modified only when no logical processor with a current VMCS that
> references it is in VMX non-root operation. That requirement does
> not apply to the posted-interrupt descriptor. There is a requirement,
> however, that such modifications be done using locked read-modify-write
> instructions."
>
> virtual-APIC page is pointer-referenced by VMCS, thus it falls into above
> general requirement. But I suppose there should be some exception with
> this page too, otherwise the point of posted interrupt is killed (if we have
> to kick the dest vcpu into root to update the vIRR). Let me confirm
> internally.
Ack, thanks. I think we can can hold off this improvement/restriction
until we get confirmation of the intended software behavior here.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] livepatch-build-tools regression
On 26/11/2019 18:37, Wieczorkiewicz, Pawel wrote: > It looks like gcc plays the usual dirty tricks with local variables renaming: > > - xen-syms > 7529: 82d0805fed50 8 OBJECT LOCAL DEFAULT 4230 lastpage.22857 > - livepatch >289: 8 OBJECT GLOBAL DEFAULT UND > hvm.c#lastpage.22856 > > Then, symbols resolution by name fails.. > > Can you please try to build the livepatch module with additional option > '—prelink' and give it a try ? My LP loading error is: (XEN) livepatch: lp: Unknown symbol: .LC7 When I pass --prelink to livepatch-build, it complains in a similar way: livepatch-build-tools/prelink: ERROR: output.o: livepatch_resolve_symbols: 80: lookup_local_symbol .LC7 (p2m.c) -- Thanks, Sergey ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13] x86/vmx: always sync PIR to IRR before vmentry
On 27.11.2019 12:03, Roger Pau Monné wrote:
> On Wed, Nov 27, 2019 at 02:07:16AM +, Tian, Kevin wrote:
>> Then what's the difference from original logic?
>
> The original logic is:
>
> if ( running && (in_irq() || (v != current)) )
> {
> unsigned int cpu = v->processor;
>
> if ( cpu != smp_processor_id() )
> send_IPI_mask(cpumask_of(cpu), posted_intr_vector);
> else if ( !softirq_pending(cpu) )
> raise_softirq(VCPU_KICK_SOFTIRQ);
> }
>
> Which I find much harder to understand. For example I'm not sure of
> what's the benefit of doing the cpu != smp_processor_id() check
> instead of simply doing v != current (like in the outer if condition).
There are two aspects to consider: One is that v->processor
may equal smp_processor_id() also for v != current. The other
is that without this check in the if() it would need adding
to the else-if(). I'm not sure to what degree which of the
two matters functionality wise.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames and max_maptrack_frames handling
> -Original Message-
> From: Ian Jackson
> Sent: 27 November 2019 11:10
> To: Durrant, Paul
> Cc: Ian Jackson ; George Dunlap
> ; Juergen Gross ; Stefano
> Stabellini ; Julien Grall ; Wei
> Liu ; Paul Durrant ; Andrew Cooper
> ; Konrad Rzeszutek Wilk
> ; Marek Marczykowski-Górecki
> ; Hans van Kranenburg ;
> Jan Beulich ; xen-devel@lists.xenproject.org
> Subject: RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames
> and max_maptrack_frames handling
>
> Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
> max_grant_frames and max_maptrack_frames handling"):
> > > -Original Message-
> > > From: Xen-devel On Behalf Of
> Ian
> > > Jackson
> > > I have seen reports of users who ran out of grant/maptrack frames
> > > because of updates to use multiring protocols etc. The error messages
> > > are not very good and the recommended workaround has been to increase
> > > the default limit on the hypervisor command line.
> > >
> > > It is important that we don't break that workaround!
> >
> > Alas it has apparently been broken for several releases now :-(
>
> I guess at least in Debian (where I have seen this) we haven't
> released with any affected versions yet...
I believe the problem was introduce in 4.10, so I think it would be prudent to
also back-port the final fix to stable trees from then on.
Paul
>
> Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH for-4.13 2/2] Rationalize max_grant_frames and max_maptrack_frames handling
Durrant, Paul writes ("RE: [Xen-devel] [PATCH for-4.13 2/2] Rationalize
max_grant_frames and max_maptrack_frames handling"):
> > -Original Message-
> > From: Xen-devel On Behalf Of Ian
> > Jackson
> > I have seen reports of users who ran out of grant/maptrack frames
> > because of updates to use multiring protocols etc. The error messages
> > are not very good and the recommended workaround has been to increase
> > the default limit on the hypervisor command line.
> >
> > It is important that we don't break that workaround!
>
> Alas it has apparently been broken for several releases now :-(
I guess at least in Debian (where I have seen this) we haven't
released with any affected versions yet...
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
