[xen-unstable-smoke test] 155763: regressions - FAIL
flight 155763 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/155763/
Regressions :-(
Tests which did not succeed and are blocking,
including tests which could not be run:
test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 155584
Tests which did not succeed, but are not blocking:
test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass
test-armhf-armhf-xl 15 migrate-support-checkfail never pass
test-armhf-armhf-xl 16 saverestore-support-checkfail never pass
version targeted for testing:
xen 534b3d09958fdc4df64872c2ab19feb4b1eebc5a
baseline version:
xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9
Last test of basis 155584 2020-10-09 02:01:25 Z4 days
Failing since155612 2020-10-09 18:01:22 Z3 days 26 attempts
Testing same since 155708 2020-10-11 23:00:25 Z1 days 10 attempts
People who touched revisions under test:
Andrew Cooper
Jan Beulich
Jason Andryuk
Juergen Gross
Roger Pau Monné
Trammell Hudson
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 fail
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 534b3d09958fdc4df64872c2ab19feb4b1eebc5a
Author: Juergen Gross
Date: Sun Oct 11 14:24:01 2020 +0200
tools/libs/store: add disclaimer to header file regarding ignored options
Add a disclaimer to the libxenstore header file that all of the open
flags (socket only connection, read only connection) are ignored.
Signed-off-by: Juergen Gross
Acked-by: Wei Liu
commit 1b810a9d5a39230e76073b1a753cd2c34ded65fc
Author: Jason Andryuk
Date: Thu Oct 1 19:53:37 2020 -0400
libxl: only query VNC when enabled
QEMU without VNC support (configure --disable-vnc) will return an error
when VNC is queried over QMP since it does not recognize the QMP
command. This will cause libxl to fail starting the domain even if VNC
is not enabled. Therefore only query QEMU for VNC support when using
VNC, so a VNC-less QEMU will function in this configuration.
'goto out' jumps to the call to device_model_postconfig_done(), the
final callback after the chain of vnc queries. This bypasses all the
QMP VNC queries.
Signed-off-by: Jason Andryuk
Acked-by: Wei Liu
commit 8a62dee9ceff3056c7e0bd9632bac39bee2a51b3
Author: Jan Beulich
Date: Fri Oct 2 12:30:34 2020 +0200
x86/vLAPIC: don't leak regs page from vlapic_init() upon error
Fixes: 8a981e0bf25e ("Make map_domain_page_global fail")
Signed-off-by: Jan Beulich
Reviewed-by: Andrew Cooper
commit 8a71d50ed40bfa78c37722dc11995ac2563662c3
Author: Trammell Hudson
Date: Fri Oct 2 07:18:21 2020 -0400
efi: Enable booting unified hypervisor/kernel/initrd images
This patch adds support for bundling the xen.efi hypervisor, the xen.cfg
configuration file, the Linux kernel and initrd, as well as the XSM,
and architectural specific files into a single "unified" EFI executable.
This allows an administrator to update the components independently
without requiring rebuilding xen, as well as to replace the components
in an existing image.
The resulting EFI executable can be invoked directly from the UEFI Boot
Manager, removing the need to use a separate loader like grub as well
as removing dependencies on local filesystem access. And since it is
a single file, it can be signed and validated by UEFI Secure Boot without
requring the shim protocol.
It is inspired by systemd-boot's unified kernel technique and borrows the
function to locate PE sections from systemd's LGPL'ed code. During EFI
boot, Xen looks at its own loaded image to locate the
[libvirt test] 155762: regressions - FAIL
flight 155762 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/155762/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-i386-libvirt6 libvirt-buildfail REGR. vs. 151777 build-arm64-libvirt 6 libvirt-buildfail REGR. vs. 151777 build-armhf-libvirt 6 libvirt-buildfail REGR. vs. 151777 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-pair 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-vhd 1 build-check(1) blocked n/a test-amd64-amd64-libvirt-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt 1 build-check(1) blocked n/a test-amd64-i386-libvirt-pair 1 build-check(1) blocked n/a test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 1 build-check(1) blocked n/a test-amd64-i386-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-libvirt 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-qcow2 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-armhf-armhf-libvirt 1 build-check(1) blocked n/a test-armhf-armhf-libvirt-raw 1 build-check(1) blocked n/a version targeted for testing: libvirt accdc0e7730739f398e392c23bc8380d3574a878 baseline version: libvirt 2c846fa6bcc11929c9fb857a22430fb9945654ad Last test of basis 151777 2020-07-10 04:19:19 Z 95 days Failing since151818 2020-07-11 04:18:52 Z 94 days 89 attempts Testing same since 155762 2020-10-13 04:19:10 Z0 days1 attempts People who touched revisions under test: Andika Triwidada Andrea Bolognani Balázs Meskó Bastien Orivel Bihong Yu Binfeng Wu Boris Fiuczynski Christian Ehrhardt Cole Robinson Collin Walling Cornelia Huck Côme Borsoi Daniel Henrique Barboza Daniel P. Berrange Daniel P. Berrangé Erik Skultety Fabian Freyer Fangge Jin Fedora Weblate Translation Han Han Hao Wang Ian Wienand Jamie Strandboge Jamie Strandboge Jean-Baptiste Holcroft Jianan Gao Jim Fehlig Jin Yan Jiri Denemark Jonathon Jongsma Ján Tomko Kashyap Chamarthy Kevin Locke Laine Stump Liao Pingfang Lin Ma Lin Ma Marc Hartmayer Marek Marczykowski-Górecki Markus Schade Martin Kletzander Masayoshi Mizuma Matt Coleman Matt Coleman Mauro Matteo Cascella Michal Privoznik Michał Smyk Milo Casagrande Neal Gompa Nikolay Shirokovskiy Olesya Gerasimenko Patrick Magauran Paulo de Rezende Pinatti Pavel Hrdina Peter Krempa Pino Toscano Pino Toscano Piotr Drąg Prathamesh Chavan Roman Bogorodskiy Roman Bolshakov Ryan Schmidt Sam Hartman Scott Shambarger Sebastian Mitterle Simon Gaiser Stefan Bader Stefan Berger Szymon Scholz Thomas Huth Tim Wiederhake Tomáš Golembiovský Wang Xin Weblate Yang Hang Yanqiu Zhang Yi Li Yi Wang Yuri Chornoivan Zheng Chuan Zhenyu Zheng 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 fail build-arm64-libvirt fail build-armhf-libvirt fail build-i386-libvirt fail build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm blocked test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmblocked test-amd64-amd64-libvirt-xsm blocked test-arm64-arm64-libvirt-xsm blocked test-amd64-i386-libvirt-xsm blocked test-amd64-amd64-libvirt
[qemu-mainline test] 155754: regressions - FAIL
flight 155754 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/155754/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-qemuu-freebsd11-amd64 13 guest-startfail REGR. vs. 152631 test-amd64-amd64-qemuu-freebsd12-amd64 13 guest-startfail REGR. vs. 152631 test-amd64-i386-qemuu-rhel6hvm-amd 12 redhat-install fail REGR. vs. 152631 test-amd64-amd64-qemuu-nested-intel 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-freebsd10-i386 13 guest-startfail REGR. vs. 152631 test-armhf-armhf-xl-vhd 12 debian-di-installfail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-qemuu-rhel6hvm-intel 12 redhat-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail REGR. vs. 152631 test-arm64-arm64-libvirt-xsm 14 guest-start fail REGR. vs. 152631 test-amd64-amd64-qemuu-nested-amd 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install fail REGR. vs. 152631 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-win7-amd64 12 windows-install fail REGR. vs. 152631 test-armhf-armhf-libvirt-raw 12 debian-di-installfail REGR. vs. 152631 test-amd64-i386-xl-qemuu-ws16-amd64 12 windows-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-ws16-amd64 12 windows-install fail REGR. vs. 152631 test-amd64-i386-freebsd10-amd64 13 guest-start fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 12 debian-hvm-install fail REGR. vs. 152631 test-armhf-armhf-libvirt 14 guest-start fail REGR. vs. 152631 Tests which are failing intermittently (not blocking): test-amd64-i386-xl-raw 12 debian-di-install fail in 155743 pass in 155754 test-amd64-amd64-i386-pvgrub 12 debian-di-install fail in 155743 pass in 155754 test-amd64-amd64-xl-qcow212 debian-di-install fail pass in 155743 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-rtds 18 guest-start/debian.repeatfail like 152631 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfai
Re: [PATCH 0/2] Add Xen CpusAccel
On 10/12/20 10:16 PM, Claudio Fontana wrote:
On 10/12/20 10:07 PM, Jason Andryuk wrote:
Xen was left behind when CpusAccel became mandatory and fails the assert
in qemu_init_vcpu(). It relied on the same dummy cpu threads as qtest.
Move the qtest cpu functions to a common location and reuse them for
Xen.
Jason Andryuk (2):
accel: move qtest CpusAccel functions to a common location
accel: Add xen CpusAccel using dummy-cpu
.../qtest-cpus.c => dummy/dummy-cpus.c} | 22 +--
.../qtest-cpus.h => dummy/dummy-cpus.h} | 10 -
accel/dummy/meson.build | 7 ++
accel/meson.build | 1 +
accel/qtest/meson.build | 1 -
accel/qtest/qtest.c | 7 +-
accel/xen/xen-all.c | 10 +
7 files changed, 34 insertions(+), 24 deletions(-)
rename accel/{qtest/qtest-cpus.c => dummy/dummy-cpus.c} (76%)
rename accel/{qtest/qtest-cpus.h => dummy/dummy-cpus.h} (59%)
create mode 100644 accel/dummy/meson.build
Yep, forgot completely, sorry.
Good opportunity to ask the Xen folks to add testing
to our Gitlab CI, so this doesn't happen again :)
Acked-by: Claudio Fontana
Re: [OSSTEST PATCH 16/82] abolish "kernkind"; desupport non-pvops kernels
On Wed, Oct 07, 2020 at 06:59:18PM +0100, Ian Jackson wrote: > From: Ian Jackson > > This was for distinguishing the old-style Xenolinux kernels from pvops > kernels. > > We have not actually tested any non-pvops kernels for a very very long > time. Delete this now because the runvar is slightly in the way of > test host reuse. > > (Sorry for the wide CC but it seems better to make sure anyone who > might object can do so.) No objection from me FWIW. Wei.
Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
On 06.10.2020 10:13, Paul Durrant wrote: > > >> -Original Message- >> From: Jan Beulich >> Sent: 01 October 2020 15:42 >> To: Don Slutz >> Cc: xen-de...@lists.xen.org; Boris Ostrovsky ; >> Ian Jackson >> ; Jun Nakajima ; Kevin Tian >> ; >> Stefano Stabellini ; Tim Deegan ; >> Andrew Cooper >> ; Konrad Rzeszutek Wilk ; >> George Dunlap >> ; Paul Durrant >> Subject: Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT >> >> On 19.08.2020 18:52, Don Slutz wrote: >>> This adds synchronization of the 6 vcpu registers (only 32bits of >>> them) that QEMU's vmport.c and vmmouse.c needs between Xen and QEMU. >>> This is how VMware defined the use of these registers. >>> >>> This is to avoid a 2nd and 3rd exchange between QEMU and Xen to >>> fetch and put these 6 vcpu registers used by the code in QEMU's >>> vmport.c and vmmouse.c >> >> I'm unconvinced this warrants a new ioreq type, and all the overhead >> associated with it. I'd be curious to know what Paul or the qemu >> folks think here. >> > > The current shared ioreq_t does appear have enough space to accommodate 6 > 32-bit registers (in the addr, data, count and size) fields co couldn't the > new IOREQ_TYPE_VMWARE_PORT type be dealt with by simply unioning the regs > with these fields? That avoids the need for a whole new shared page. Hmm, yes, good point. But this is assuming we're going to be fine with using 32-bit registers now and going forward. Personally I'd prefer a mechanism less constrained by the specific needs of the current VMware interface, i.e. potentially allowing to scale to 64-bit registers as well as any of the remaining 9 ones (leaving aside %rsp). Jan
RE: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT
> -Original Message- > From: Jan Beulich > Sent: 13 October 2020 10:38 > To: p...@xen.org > Cc: 'Don Slutz' ; xen-de...@lists.xen.org; 'Boris > Ostrovsky' > ; 'Ian Jackson' ; 'Jun > Nakajima' > ; 'Kevin Tian' ; 'Stefano > Stabellini' > ; 'Tim Deegan' ; 'Andrew Cooper' > ; > 'Konrad Rzeszutek Wilk' ; 'George Dunlap' > > Subject: Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT > > On 06.10.2020 10:13, Paul Durrant wrote: > > > > > >> -Original Message- > >> From: Jan Beulich > >> Sent: 01 October 2020 15:42 > >> To: Don Slutz > >> Cc: xen-de...@lists.xen.org; Boris Ostrovsky ; > >> Ian Jackson > >> ; Jun Nakajima ; Kevin Tian > >> ; > >> Stefano Stabellini ; Tim Deegan ; > >> Andrew Cooper > >> ; Konrad Rzeszutek Wilk > >> ; George Dunlap > >> ; Paul Durrant > >> Subject: Re: [XEN PATCH v14 7/8] Add IOREQ_TYPE_VMWARE_PORT > >> > >> On 19.08.2020 18:52, Don Slutz wrote: > >>> This adds synchronization of the 6 vcpu registers (only 32bits of > >>> them) that QEMU's vmport.c and vmmouse.c needs between Xen and QEMU. > >>> This is how VMware defined the use of these registers. > >>> > >>> This is to avoid a 2nd and 3rd exchange between QEMU and Xen to > >>> fetch and put these 6 vcpu registers used by the code in QEMU's > >>> vmport.c and vmmouse.c > >> > >> I'm unconvinced this warrants a new ioreq type, and all the overhead > >> associated with it. I'd be curious to know what Paul or the qemu > >> folks think here. > >> > > > > The current shared ioreq_t does appear have enough space to accommodate 6 > > 32-bit registers (in the > addr, data, count and size) fields co couldn't the new IOREQ_TYPE_VMWARE_PORT > type be dealt with by > simply unioning the regs with these fields? That avoids the need for a whole > new shared page. > > Hmm, yes, good point. But this is assuming we're going to be fine with > using 32-bit registers now and going forward. Personally I'd prefer a > mechanism less constrained by the specific needs of the current VMware > interface, i.e. potentially allowing to scale to 64-bit registers as > well as any of the remaining 9 ones (leaving aside %rsp). > I think that should probably be additional work, not needed for this series. We could look to expand and re-structure the ioreq_t structure with some headroom. An emulator aware of the new structure to resource map a different set of shared pages. Paul > Jan
Re: [PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}_l1e()
On 08.10.2020 17:15, Roger Pau Monné wrote:
> On Wed, Sep 16, 2020 at 03:08:40PM +0200, Jan Beulich wrote:
>> +void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
>> + mfn_t sl1mfn, const void *sl1e,
>> + const struct domain *d)
>> +{
>> +unsigned long gfn;
>> +struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
>> +
>> +ASSERT(is_hvm_domain(d));
>> +
>> +if ( !dirty_vram /* tracking disabled? */ ||
>> + !(l1f & _PAGE_RW) /* read-only mapping? */ ||
>> + !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
>> +return;
>> +
>> +gfn = gfn_x(mfn_to_gfn(d, mfn));
>> +/* Page sharing not supported on shadow PTs */
>> +BUG_ON(SHARED_M2P(gfn));
>> +
>> +if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
>> +{
>> +unsigned long i = gfn - dirty_vram->begin_pfn;
>> +const struct page_info *page = mfn_to_page(mfn);
>> +bool dirty = false;
>> +paddr_t sl1ma = mfn_to_maddr(sl1mfn) | PAGE_OFFSET(sl1e);
>> +
>> +if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
>> +{
>> +/* Last reference */
>> +if ( dirty_vram->sl1ma[i] == INVALID_PADDR )
>> +{
>> +/* We didn't know it was that one, let's say it is dirty */
>> +dirty = true;
>> +}
>> +else
>> +{
>> +ASSERT(dirty_vram->sl1ma[i] == sl1ma);
>> +dirty_vram->sl1ma[i] = INVALID_PADDR;
>> +if ( l1f & _PAGE_DIRTY )
>> +dirty = true;
>> +}
>> +}
>> +else
>> +{
>> +/* We had more than one reference, just consider the page
>> dirty. */
>> +dirty = true;
>> +/* Check that it's not the one we recorded. */
>> +if ( dirty_vram->sl1ma[i] == sl1ma )
>> +{
>> +/* Too bad, we remembered the wrong one... */
>> +dirty_vram->sl1ma[i] = INVALID_PADDR;
>> +}
>> +else
>> +{
>> +/*
>> + * Ok, our recorded sl1e is still pointing to this page,
>> let's
>> + * just hope it will remain.
>> + */
>> +}
>> +}
>> +
>> +if ( dirty )
>> +{
>> +dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
>
> Could you use _set_bit here?
In addition to what Andrew has said - this would be a non cosmetic
change, which I wouldn't want to do in a patch merely moving this
code.
>> @@ -1194,7 +1094,9 @@ static int shadow_set_l1e(struct domain
>> new_sl1e = shadow_l1e_flip_flags(new_sl1e, rc);
>> /* fall through */
>> case 0:
>> -shadow_vram_get_l1e(new_sl1e, sl1e, sl1mfn, d);
>> +shadow_vram_get_mfn(shadow_l1e_get_mfn(new_sl1e),
>> +shadow_l1e_get_flags(new_sl1e),
>> +sl1mfn, sl1e, d);
>
> As you have moved this function into a HVM build time file, don't you
> need to guard this call, or alternatively provide a dummy handler for
> !CONFIG_HVM in private.h?
>
> Same for shadow_vram_put_mfn.
All uses are inside conditionals using shadow_mode_refcounts(), i.e.
the compiler's DCE pass will eliminate the calls. All we need are
declarations to be in scope.
Jan
Re: [PATCH v2 3/4] x86/shim: don't permit HVM and PV_SHIM_EXCLUSIVE at the same time
On 08.10.2020 16:52, Roger Pau Monné wrote: > On Wed, Sep 16, 2020 at 03:08:00PM +0200, Jan Beulich wrote: >> This combination doesn't really make sense (and there likely are more); >> in particular even if the code built with both options set, HVM guests >> wouldn't work (and I think one wouldn't be able to create one in the >> first place). The alternative here would be some presumably intrusive >> #ifdef-ary to get this combination to actually build (but still not >> work) again. >> >> Signed-off-by: Jan Beulich > > I can see the desire for being able to remove code, and the point > Andrew made about one option not making another disappear in a > completely different menu section. > > Yet I don't see how to converge the two together, unless we completely > change our menu layouts, and even then I'm not sure I see how we could > structure this. Hence: > > Acked-by: Roger Pau Monné Thanks. Andrew - are you okay with this going in then? Or if not, do you have any thoughts towards an alternative approach? Jan
Re: [PATCH v2 5/6] x86: guard against straight-line speculation past RET
On 08.10.2020 18:28, Roger Pau Monné wrote:
> On Mon, Sep 28, 2020 at 02:31:49PM +0200, Jan Beulich wrote:
>> Under certain conditions CPUs can speculate into the instruction stream
>> past a RET instruction. Guard against this just like 3b7dab93f240
>> ("x86/spec-ctrl: Protect against CALL/JMP straight-line speculation")
>> did - by inserting an "INT $3" insn. It's merely the mechanics of how to
>> achieve this that differ: A set of macros gets introduced to post-
>> process RET insns issued by the compiler (or living in assembly files).
>>
>> Unfortunately for clang this requires further features their built-in
>> assembler doesn't support: We need to be able to override insn mnemonics
>> produced by the compiler (which may be impossible, if internally
>> assembly mnemonics never get generated), and we want to use \(text)
>> escaping / quoting in the auxiliary macro.
>>
>> Signed-off-by: Jan Beulich
>
> Code LGTM.
>
> Acked-by: Roger Pau Monné
Thanks.
>> ---
>> TBD: Should this depend on CONFIG_SPECULATIVE_HARDEN_BRANCH?
>
> I don't see the additions done in 3b7dab93f240 being guarded by
> CONFIG_SPECULATIVE_HARDEN_BRANCH, so in that regard I would say no.
> However those are already guarded by CONFIG_INDIRECT_THUNK so it's
> slightly weird that the addition of such protections cannot be turned
> off in any way.
>
> I would be fine with having the additions done in 3b7dab93f240
> protected by CONFIG_SPECULATIVE_HARDEN_BRANCH, and then the additions
> done here also.
Okay, perhaps I'll make a separate patch then to add the conditional
at all respective places.
Jan
[xen-unstable-smoke test] 155768: regressions - FAIL
flight 155768 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/155768/
Regressions :-(
Tests which did not succeed and are blocking,
including tests which could not be run:
test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 155584
Tests which did not succeed, but are not blocking:
test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass
test-armhf-armhf-xl 15 migrate-support-checkfail never pass
test-armhf-armhf-xl 16 saverestore-support-checkfail never pass
version targeted for testing:
xen 534b3d09958fdc4df64872c2ab19feb4b1eebc5a
baseline version:
xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9
Last test of basis 155584 2020-10-09 02:01:25 Z4 days
Failing since155612 2020-10-09 18:01:22 Z3 days 27 attempts
Testing same since 155708 2020-10-11 23:00:25 Z1 days 11 attempts
People who touched revisions under test:
Andrew Cooper
Jan Beulich
Jason Andryuk
Juergen Gross
Roger Pau Monné
Trammell Hudson
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 fail
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 534b3d09958fdc4df64872c2ab19feb4b1eebc5a
Author: Juergen Gross
Date: Sun Oct 11 14:24:01 2020 +0200
tools/libs/store: add disclaimer to header file regarding ignored options
Add a disclaimer to the libxenstore header file that all of the open
flags (socket only connection, read only connection) are ignored.
Signed-off-by: Juergen Gross
Acked-by: Wei Liu
commit 1b810a9d5a39230e76073b1a753cd2c34ded65fc
Author: Jason Andryuk
Date: Thu Oct 1 19:53:37 2020 -0400
libxl: only query VNC when enabled
QEMU without VNC support (configure --disable-vnc) will return an error
when VNC is queried over QMP since it does not recognize the QMP
command. This will cause libxl to fail starting the domain even if VNC
is not enabled. Therefore only query QEMU for VNC support when using
VNC, so a VNC-less QEMU will function in this configuration.
'goto out' jumps to the call to device_model_postconfig_done(), the
final callback after the chain of vnc queries. This bypasses all the
QMP VNC queries.
Signed-off-by: Jason Andryuk
Acked-by: Wei Liu
commit 8a62dee9ceff3056c7e0bd9632bac39bee2a51b3
Author: Jan Beulich
Date: Fri Oct 2 12:30:34 2020 +0200
x86/vLAPIC: don't leak regs page from vlapic_init() upon error
Fixes: 8a981e0bf25e ("Make map_domain_page_global fail")
Signed-off-by: Jan Beulich
Reviewed-by: Andrew Cooper
commit 8a71d50ed40bfa78c37722dc11995ac2563662c3
Author: Trammell Hudson
Date: Fri Oct 2 07:18:21 2020 -0400
efi: Enable booting unified hypervisor/kernel/initrd images
This patch adds support for bundling the xen.efi hypervisor, the xen.cfg
configuration file, the Linux kernel and initrd, as well as the XSM,
and architectural specific files into a single "unified" EFI executable.
This allows an administrator to update the components independently
without requiring rebuilding xen, as well as to replace the components
in an existing image.
The resulting EFI executable can be invoked directly from the UEFI Boot
Manager, removing the need to use a separate loader like grub as well
as removing dependencies on local filesystem access. And since it is
a single file, it can be signed and validated by UEFI Secure Boot without
requring the shim protocol.
It is inspired by systemd-boot's unified kernel technique and borrows the
function to locate PE sections from systemd's LGPL'ed code. During EFI
boot, Xen looks at its own loaded image to locate the
[PATCH] hvmloader: flip "ACPI data" to ACPI NVS type for ACPI table region
ACPI specification contains statements describing memory marked with regular
"ACPI data" type as reclaimable by the guest. Although the guest shouldn't
really do it if it wants kexec or similar functionality to work, there
could still be ambiguities in treating these regions as potentially regular
RAM.
One such an example is SeaBIOS which currently reports "ACPI data" regions as
RAM to the guest in its e801 call. The guest then tries to use this region
for initrd placement and gets stuck. While arguably SeaBIOS needs to be fixed
here, that is just one example of the potential problems from using
reclaimable memory type.
Flip the type to "ACPI NVS" which doesn't have this ambiguity in it and is
described by the spec as non-reclaimable (so cannot ever be treated like RAM).
Signed-off-by: Igor Druzhinin
---
tools/firmware/hvmloader/e820.c | 7 ---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/tools/firmware/hvmloader/e820.c b/tools/firmware/hvmloader/e820.c
index 38bcf18..8870099 100644
--- a/tools/firmware/hvmloader/e820.c
+++ b/tools/firmware/hvmloader/e820.c
@@ -202,16 +202,17 @@ int build_e820_table(struct e820entry *e820,
nr++;
/*
- * Mark populated reserved memory that contains ACPI tables as ACPI data.
+ * Mark populated reserved memory that contains ACPI tables as ACPI NVS.
* That should help the guest to treat it correctly later: e.g. pass to
- * the next kernel on kexec or reclaim if necessary.
+ * the next kernel on kexec and prevent space reclaim which is possible
+ * with regular ACPI data type accoring to ACPI spec v6.3.
*/
if ( acpi_enabled )
{
e820[nr].addr = RESERVED_MEMBASE;
e820[nr].size = acpi_mem_end - RESERVED_MEMBASE;
-e820[nr].type = E820_ACPI;
+e820[nr].type = E820_NVS;
nr++;
}
--
2.7.4
Re: [PATCH v2 1/6] x86: replace __ASM_{CL,ST}AC
On 28/09/2020 13:29, Jan Beulich wrote: > --- a/xen/arch/x86/arch.mk > +++ b/xen/arch/x86/arch.mk > @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand % > $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE) > $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT) > $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) > +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) Kconfig > --- /dev/null > +++ b/xen/include/asm-x86/asm-defns.h > @@ -0,0 +1,9 @@ > +#ifndef HAVE_AS_CLAC_STAC > +.macro clac > +.byte 0x0f, 0x01, 0xca > +.endm > + > +.macro stac > +.byte 0x0f, 0x01, 0xcb > +.endm > +#endif > --- a/xen/include/asm-x86/asm_defns.h > +++ b/xen/include/asm-x86/asm_defns.h We cannot have two files which differ by the vertical positioning of a dash. How about asm-insn.h for the former, seeing as that is what it contains. ~Andrew
Re: [PATCH v2 2/6] x86: reduce CET-SS related #ifdef-ary
On 28/09/2020 13:30, Jan Beulich wrote:
> Commit b586a81b7a90 ("x86/CET: Fix build following c/s 43b98e7190") had
> to introduce a number of #ifdef-s to make the build work with older tool
> chains. Introduce an assembler macro covering for tool chains not
> knowing of CET-SS, allowing some conditionals where just SETSSBSY is the
> problem to be dropped again.
>
> No change to generated code.
>
> Signed-off-by: Jan Beulich
> Reviewed-by: Roger Pau Monné
> ---
> Now that I've done this I'm no longer sure which direction is better to
> follow: On one hand this introduces dead code (even if just NOPs) into
> CET-SS-disabled builds. Otoh this is a step towards breaking the tool
> chain version dependency of the feature.
I've said before. You cannot break the toolchain dependency without
hardcoding memory operands. I'm not prepared to let that happen.
There is no problem requiring newer toolchains for newer features
(you're definitely not having CET-IBT, for example), and there is a
(unacceptably, IMO) large cost to this work.
~Andrew
Re: [PATCH v2 1/6] x86: replace __ASM_{CL,ST}AC
On 13.10.2020 13:04, Andrew Cooper wrote: > On 28/09/2020 13:29, Jan Beulich wrote: >> --- a/xen/arch/x86/arch.mk >> +++ b/xen/arch/x86/arch.mk >> @@ -20,6 +20,7 @@ $(call as-option-add,CFLAGS,CC,"rdrand % >> $(call as-option-add,CFLAGS,CC,"rdfsbase %rax",-DHAVE_AS_FSGSBASE) >> $(call as-option-add,CFLAGS,CC,"xsaveopt (%rax)",-DHAVE_AS_XSAVEOPT) >> $(call as-option-add,CFLAGS,CC,"rdseed %eax",-DHAVE_AS_RDSEED) >> +$(call as-option-add,CFLAGS,CC,"clac",-DHAVE_AS_CLAC_STAC) > > Kconfig I know that's your view, and you know I disagree. I don't see the thread I had started to have lead to any consensus. >> --- /dev/null >> +++ b/xen/include/asm-x86/asm-defns.h >> @@ -0,0 +1,9 @@ >> +#ifndef HAVE_AS_CLAC_STAC >> +.macro clac >> +.byte 0x0f, 0x01, 0xca >> +.endm >> + >> +.macro stac >> +.byte 0x0f, 0x01, 0xcb >> +.endm >> +#endif >> --- a/xen/include/asm-x86/asm_defns.h >> +++ b/xen/include/asm-x86/asm_defns.h > > We cannot have two files which differ by the vertical positioning of a dash. Why "cannot"? One is the helper of the other, so them being named almost identically is quite sensible imo (and no-one is supposed to include the new one directly). In any event I'd at most see this be "we don't want to". > How about asm-insn.h for the former, seeing as that is what it contains. Until "x86: fold indirect_thunk_asm.h into asm-defns.h", where it starts to be more than just plain insn replacements. And I suspect more non-insn macros will appear over time. I'd have suggested asm-macros.h in case the present name really can't be reached consensus on, but we have a (generated) file of this name already. Jan
Re: [PATCH v2 2/6] x86: reduce CET-SS related #ifdef-ary
On 13.10.2020 13:20, Andrew Cooper wrote:
> On 28/09/2020 13:30, Jan Beulich wrote:
>> Commit b586a81b7a90 ("x86/CET: Fix build following c/s 43b98e7190") had
>> to introduce a number of #ifdef-s to make the build work with older tool
>> chains. Introduce an assembler macro covering for tool chains not
>> knowing of CET-SS, allowing some conditionals where just SETSSBSY is the
>> problem to be dropped again.
>>
>> No change to generated code.
>>
>> Signed-off-by: Jan Beulich
>> Reviewed-by: Roger Pau Monné
>> ---
>> Now that I've done this I'm no longer sure which direction is better to
>> follow: On one hand this introduces dead code (even if just NOPs) into
>> CET-SS-disabled builds. Otoh this is a step towards breaking the tool
>> chain version dependency of the feature.
>
> I've said before. You cannot break the toolchain dependency without
> hardcoding memory operands. I'm not prepared to let that happen.
>
> There is no problem requiring newer toolchains for newer features
> (you're definitely not having CET-IBT, for example), and there is a
> (unacceptably, IMO) large cost to this work.
I'm aware of your view. What remains unclear to me is whether your
reply is merely a remark on this post-commit-message comment, or
whether it is an objection to the tidying (as I view it) the patch
does.
Jan
Re: [PATCH RFC PKS/PMEM 24/58] fs/freevxfs: Utilize new kmap_thread()
> - kaddr = kmap(pp); > + kaddr = kmap_thread(pp); > memcpy(kaddr, vip->vii_immed.vi_immed + offset, PAGE_SIZE); > - kunmap(pp); > + kunmap_thread(pp); You only Cced me on this particular patch, which means I have absolutely no idea what kmap_thread and kunmap_thread actually do, and thus can't provide an informed review. That being said I think your life would be a lot easier if you add helpers for the above code sequence and its counterpart that copies to a potential hughmem page first, as that hides the implementation details from most users.
Re: [PATCH v2 2/6] x86: reduce CET-SS related #ifdef-ary
On 28/09/2020 13:37, Jan Beulich wrote:
> On 28.09.2020 14:30, Jan Beulich wrote:
>> Commit b586a81b7a90 ("x86/CET: Fix build following c/s 43b98e7190") had
>> to introduce a number of #ifdef-s to make the build work with older tool
>> chains. Introduce an assembler macro covering for tool chains not
>> knowing of CET-SS, allowing some conditionals where just SETSSBSY is the
>> problem to be dropped again.
>>
>> No change to generated code.
>>
>> Signed-off-by: Jan Beulich
>> Reviewed-by: Roger Pau Monné
>> ---
>> Now that I've done this I'm no longer sure which direction is better to
>> follow: On one hand this introduces dead code (even if just NOPs) into
>> CET-SS-disabled builds.
> A possible compromise here might be to ...
>
>> --- a/xen/include/asm-x86/asm-defns.h
>> +++ b/xen/include/asm-x86/asm-defns.h
>> @@ -7,3 +7,9 @@
>> .byte 0x0f, 0x01, 0xcb
>> .endm
>> #endif
>> +
>> +#ifndef CONFIG_HAS_AS_CET_SS
>> +.macro setssbsy
>> +.byte 0xf3, 0x0f, 0x01, 0xe8
>> +.endm
>> +#endif
> ... comment out this macro's body. If we went this route, incssp
> and wrssp could be dealt with in similar ways, to allow dropping
> further #ifdef-s.
No, because how you've got something which reads as a real instruction
which sometimes disappears into nothing. (Interestingly, zero length
alternatives do appear to compile, and this is clearly a bug.)
The thing which matters is the clarity of code in its surrounding
context, and this isn't an improvement.
~Andrew
Re: [PATCH v9 1/8] xen/common: introduce a new framework for save/restore of 'domain' context
On 05.10.2020 10:03, Paul Durrant wrote:
>> From: Andrew Cooper
>> Sent: 02 October 2020 22:20
>>
>> On 24/09/2020 14:10, Paul Durrant wrote:
>>> +int domain_save_end(struct domain_context *c)
>>> +{
>>> +struct domain *d = c->domain;
>>> +size_t len = ROUNDUP(c->len, DOMAIN_SAVE_ALIGN) - c->len; /* padding */
>>
>> DOMAIN_SAVE_ALIGN - (c->len & (DOMAIN_SAVE_ALIGN - 1))
>>
>> isn't vulnerable to overflow.
>>
>
> ...and significantly uglier code. What's actually wrong with what I wrote?
I don't think there's anything "wrong" or "vulnerable" here, but
I still can see Andrew's point. The "vulnerable" aspect applies
only in the (highly hypothetical I think) cases of either
sizeof(size_t) < sizeof(int) or size_t being a signed type, afaict.
But since it's easy (and imo not "significantly uglier") to write
code that is free of any wrapping or overflowing behavior, I
think it is sensible to actually write it that way.
Jan
Re: [PATCH v9 6/8] common/domain: add a domain context record for shared_info...
On 05.10.2020 12:39, Andrew Cooper wrote:
> On 24/09/2020 14:10, Paul Durrant wrote:
>> +static int load_shared_info(struct domain *d, struct domain_context *c)
>> +{
>> +struct domain_shared_info_context ctxt;
>> +size_t hdr_size = offsetof(typeof(ctxt), buffer);
>> +unsigned int i;
>> +int rc;
>> +
>> +rc = DOMAIN_LOAD_BEGIN(SHARED_INFO, c, &i);
>> +if ( rc )
>> +return rc;
>> +
>> +if ( i ) /* expect only a single instance */
>> +return -ENXIO;
>> +
>> +rc = domain_load_data(c, &ctxt, hdr_size);
>> +if ( rc )
>> +return rc;
>> +
>> +if ( ctxt.buffer_size > sizeof(shared_info_t) ||
>> + (ctxt.flags & ~DOMAIN_SAVE_32BIT_SHINFO) )
>> +return -EINVAL;
>> +
>> +if ( ctxt.flags & DOMAIN_SAVE_32BIT_SHINFO )
>> +{
>> +#ifdef CONFIG_COMPAT
>> +has_32bit_shinfo(d) = true;
>
> d->arch.has_32bit_shinfo
But this is common code, i.e. using d->arch directly is a layering
violation. I know your dislike of lvalues disguised by function-
like macros, but what do you do?
Jan
Re: [PATCH v2 5/6] x86: guard against straight-line speculation past RET
On 28/09/2020 13:31, Jan Beulich wrote:
> Under certain conditions CPUs can speculate into the instruction stream
> past a RET instruction. Guard against this just like 3b7dab93f240
> ("x86/spec-ctrl: Protect against CALL/JMP straight-line speculation")
> did - by inserting an "INT $3" insn. It's merely the mechanics of how to
> achieve this that differ: A set of macros gets introduced to post-
> process RET insns issued by the compiler (or living in assembly files).
>
> Unfortunately for clang this requires further features their built-in
> assembler doesn't support: We need to be able to override insn mnemonics
> produced by the compiler (which may be impossible, if internally
> assembly mnemonics never get generated), and we want to use \(text)
> escaping / quoting in the auxiliary macro.
>
> Signed-off-by: Jan Beulich
> ---
> TBD: Should this depend on CONFIG_SPECULATIVE_HARDEN_BRANCH?
> TBD: Would be nice to avoid the additions in .init.text, but a query to
> the binutils folks regarding the ability to identify the section
> stuff is in (by Peter Zijlstra over a year ago:
> https://sourceware.org/pipermail/binutils/2019-July/107528.html)
> has been left without helpful replies.
> ---
> v2: Fix build with newer clang. Use int3 mnemonic. Also override retq.
>
> --- a/xen/Makefile
> +++ b/xen/Makefile
> @@ -145,7 +145,15 @@ t2 = $(call as-insn,$(CC) -I$(BASEDIR)/i
> # https://bugs.llvm.org/show_bug.cgi?id=36110
> t3 = $(call as-insn,$(CC),".macro FOO;.endm"$(close); asm volatile
> $(open)".macro FOO;.endm",-no-integrated-as)
>
> -CLANG_FLAGS += $(call or,$(t1),$(t2),$(t3))
> +# Check whether \(text) escaping in macro bodies is supported.
> +t4 = $(call as-insn,$(CC),".macro m ret:req; \\(ret) $$\\ret; .endm; m
> 8",,-no-integrated-as)
> +
> +# Check whether macros can override insn mnemonics in inline assembly.
> +t5 = $(call as-insn,$(CC),".macro ret; .error; .endm; .macro retq; .error;
> .endm",-no-integrated-as)
> +
> +acc1 := $(call or,$(t1),$(t2),$(t3),$(t4))
> +
> +CLANG_FLAGS += $(call or,$(acc1),$(t5))
I'm not happy taking this until there is toolchain support visible in
the future.
We *cannot* rule out the use of IAS forever more, because there are
features far more important than ret speculation which depend on it.
> endif
>
> CLANG_FLAGS += -Werror=unknown-warning-option
> --- a/xen/include/asm-x86/asm-defns.h
> +++ b/xen/include/asm-x86/asm-defns.h
> @@ -50,3 +50,22 @@
> .macro INDIRECT_JMP arg:req
> INDIRECT_BRANCH jmp \arg
> .endm
> +
> +/*
> + * To guard against speculation past RET, insert a breakpoint insn
> + * immediately after them.
> + */
> +.macro ret operand:vararg
> +ret$ \operand
> +.endm
> +.macro retq operand:vararg
> +ret$ \operand
> +.endm
> +.macro ret$ operand:vararg
> +.purgem ret
> +ret \operand
You're substituting retq for ret, which defeats the purpose of unwrapping.
I will repeat my previous feedback. Do away with this
wrapping/unwrapping and just use a raw .byte. Its simpler and faster.
~Andrew
Re: [PATCH v9 0/4] efi: Unified Xen hypervisor/kernel/initrd images
On 09.10.2020 16:43, Trammell Hudson wrote: > Any further thoughts on this patch series? Three out of four of > them have been reviewed or acked by at least one reviewer, with > only the last one currently unreviewed. "unreviewed" isn't correct. I did review it, but I'm opposed to parts of the resulting behavior. Jan
Re: Xen Coding style and clang-format
On 12.10.2020 20:09, George Dunlap wrote:
>> On Oct 7, 2020, at 11:19 AM, Anastasiia Lukianenko
>> wrote:
>> So I want to know if the community is ready to add new formatting
>> options and edit old ones. Below I will give examples of what
>> corrections the checker is currently making (the first variant in each
>> case is existing code and the second variant is formatted by checker).
>> If they fit the standards, then I can document them in the coding
>> style. If not, then I try to configure the checker. But the idea is
>> that we need to choose one option that will be considered correct.
>> 1) Function prototype when the string length is longer than the allowed
>> -static int __init
>> -acpi_parse_gic_cpu_interface(struct acpi_subtable_header *header,
>> - const unsigned long end)
>> +static int __init acpi_parse_gic_cpu_interface(
>> +struct acpi_subtable_header *header, const unsigned long end)
>
> Jan already commented on this one; is there any way to tell the checker to
> ignore this discrepancy?
>
> If not, I think we should just choose one; I’d go with the latter.
>
>> 2) Wrapping an operation to a new line when the string length is longer
>> than the allowed
>> -status = acpi_get_table(ACPI_SIG_SPCR, 0,
>> -(struct acpi_table_header **)&spcr);
>> +status =
>> +acpi_get_table(ACPI_SIG_SPCR, 0, (struct acpi_table_header
>> **)&spcr);
>
> Personally I prefer the first version.
Same here.
>> 3) Space after brackets
>> -return ((char *) base + offset);
>> +return ((char *)base + offset);
>
> This seems like a good change to me.
>
>> 4) Spaces in brackets in switch condition
>> -switch ( domctl->cmd )
>> +switch (domctl->cmd)
>
> This is explicitly against the current coding style.
>
>> 5) Spaces in brackets in operation
>> -imm = ( insn >> BRANCH_INSN_IMM_SHIFT ) & BRANCH_INSN_IMM_MASK;
>> +imm = (insn >> BRANCH_INSN_IMM_SHIFT) & BRANCH_INSN_IMM_MASK;
>
> I *think* this is already the official style.
>
>> 6) Spaces in brackets in return
>> -return ( !sym->name[2] || sym->name[2] == '.' );
>> +return (!sym->name[2] || sym->name[2] == '.');
>
> Similarly, I think this is already the official style.
>
>> 7) Space after sizeof
>> -clean_and_invalidate_dcache_va_range(new_ptr, sizeof (*new_ptr) *
>> len);
>> +clean_and_invalidate_dcache_va_range(new_ptr, sizeof(*new_ptr) *
>> len);
>
> I think this is correct.
I agree with George on all of the above.
>> 8) Spaces before comment if it’s on the same line
>> -case R_ARM_MOVT_ABS: /* S + A */
>> +case R_ARM_MOVT_ABS:/* S + A */
>>
>> -if ( tmp == 0UL ) /* Are any bits set? */
>> -return result + size; /* Nope. */
>> +if ( tmp == 0UL ) /* Are any bits set? */
>> +return result + size; /* Nope. */
>
> Seem OK to me.
I don't think we have any rules how far apart a comment needs
to be; I don't think there should be any complaints or
"corrections" here.
>> 9) Space after for_each_vcpu
>> -for_each_vcpu(d, v)
>> +for_each_vcpu (d, v)
>
> Er, not sure about this one. This is actually a macro; but obviously it
> looks like for ( ).
>
> I think Jan will probably have an opinion, and I think he’ll be back
> tomorrow; so maybe wait just a day or two before starting to prep your series.
This makes it look like Linux style. In Xen it ought to be one
of
for_each_vcpu(d, v)
for_each_vcpu ( d, v )
depending on whether the author of a change considers
for_each_vcpu an ordinary identifier or kind of a keyword.
>> 10) Spaces in declaration
>> -union hsr hsr = { .bits = regs->hsr };
>> +union hsr hsr = {.bits = regs->hsr};
>
> I’m fine with this too.
I think we commonly put the blanks there that are being suggested
to get dropped, so I'm not convinced this is a change we would
want the tool making or suggesting.
Jan
[linux-linus test] 155758: regressions - FAIL
flight 155758 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/155758/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemut-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-xsm7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-examine 6 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt 7 xen-install fail REGR. vs. 152332 test-amd64-i386-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-amd64-i386-qemut-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-coresched-i386-xl 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-pvshim 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-raw7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-i386 7 xen-installfail REGR. vs. 152332 test-amd64-i386-xl-qemuu-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-ovmf-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-arm64-arm64-examine 8 reboot fail REGR. vs. 152332 test-arm64-arm64-xl-credit2 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-xl-credit2 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-xl-vhd 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-xl-cubietruck 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-libvirt 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-examine 8 reboot fail REGR. vs. 152332 test-armhf-armhf-xl-multivcpu 8 xen-bootfail REGR. vs. 152332 test-armhf-armhf-xl-credit1 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-xl 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-libvirt-raw 8 xen-boot fail REGR. vs. 152332 Regressions which are regarded as allowable (not blocking): test-armhf-armhf-xl-rtds 8 xen-boot fail REGR. vs. 152332 Tests which did not succeed, but are not blocking: test-arm64-arm64-xl-seattle 11 leak-check/basis(11)fail blocked in 152332 test-arm64-arm64-xl-xsm 11 leak-check/basis(11)fail blocked in 152332 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152332 test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never
Re: [PATCH] hvmloader: flip "ACPI data" to ACPI NVS type for ACPI table region
On 13.10.2020 12:50, Igor Druzhinin wrote: > ACPI specification contains statements describing memory marked with regular > "ACPI data" type as reclaimable by the guest. Although the guest shouldn't > really do it if it wants kexec or similar functionality to work, there > could still be ambiguities in treating these regions as potentially regular > RAM. > > One such an example is SeaBIOS which currently reports "ACPI data" regions as > RAM to the guest in its e801 call. The guest then tries to use this region > for initrd placement and gets stuck. Any theory on why it would get stuck? Having read the thread rooting at Sander's report, it hasn't become clear to me where the collision there is. A consumer of E801 (rather than E820) intends to not use ACPI data, and hence I consider SeaBIOS right in this regard (the lack of considering holes is a problem, though). > --- a/tools/firmware/hvmloader/e820.c > +++ b/tools/firmware/hvmloader/e820.c > @@ -202,16 +202,17 @@ int build_e820_table(struct e820entry *e820, > nr++; > > /* > - * Mark populated reserved memory that contains ACPI tables as ACPI data. > + * Mark populated reserved memory that contains ACPI tables as ACPI NVS. > * That should help the guest to treat it correctly later: e.g. pass to > - * the next kernel on kexec or reclaim if necessary. > + * the next kernel on kexec and prevent space reclaim which is possible > + * with regular ACPI data type accoring to ACPI spec v6.3. Preventing space reclaim is not the business of hvmloader. As per above, an ACPI unaware OS ought to be permitted to use as ordinary RAM all the space the tables occupy. Therefore at the very least the comment needs to reflect that this preventing of space reclaim is a workaround, not correct behavior. Also as a nit: "according". As a consequence I think we will also want to adjust Xen itself to automatically disable ACPI when it ends up consuming E801 data. Or alternatively we should consider dropping all E801-related code (as being inapplicable to 64-bit systems). Jan
Re: [PATCH v2 6/6] x86: limit amount of INT3 in IND_THUNK_*
On 28/09/2020 13:32, Jan Beulich wrote: > There's no point having every replacement variant to also specify the > INT3 - just have it once in the base macro. When patching, NOPs will get > inserted, which are fine to speculate through (until reaching the INT3). > > Signed-off-by: Jan Beulich > --- > I also wonder whether the LFENCE in IND_THUNK_RETPOLINE couldn't be > replaced by INT3 as well. Of course the effect will be marginal, as the > size of the thunk will still be 16 bytes when including tail padding > resulting from alignment. There are surprising performance implications from the choice of speculation blocker. RSB filling in particular had a benefit (up to 6% iirc) from unrolling the loop. Any differences here are likely to be marginal, whereas for inline retpoline, the code volume reduction might easily be the winning factor. > --- > v2: New. > > --- a/xen/arch/x86/indirect-thunk.S > +++ b/xen/arch/x86/indirect-thunk.S > @@ -11,6 +11,8 @@ > > #include > > +.purgem ret This needs a comment. ~Andrew
Re: [PATCH] hvmloader: flip "ACPI data" to ACPI NVS type for ACPI table region
On 13/10/2020 13:51, Jan Beulich wrote: > On 13.10.2020 12:50, Igor Druzhinin wrote: >> ACPI specification contains statements describing memory marked with regular >> "ACPI data" type as reclaimable by the guest. Although the guest shouldn't >> really do it if it wants kexec or similar functionality to work, there >> could still be ambiguities in treating these regions as potentially regular >> RAM. >> >> One such an example is SeaBIOS which currently reports "ACPI data" regions as >> RAM to the guest in its e801 call. The guest then tries to use this region >> for initrd placement and gets stuck. > > Any theory on why it would get stuck? Having read the thread rooting > at Sander's report, it hasn't become clear to me where the collision > there is. A consumer of E801 (rather than E820) intends to not use > ACPI data, and hence I consider SeaBIOS right in this regard (the > lack of considering holes is a problem, though). QEMU's fw_cfg Linux boot loader (that is used by our direct kernel boot method) is usign E801 to find the top of RAM and places images below that address. Since now it's 0xfc0 it gets located right in a PCI hole below - which causes the loader to hang. >> --- a/tools/firmware/hvmloader/e820.c >> +++ b/tools/firmware/hvmloader/e820.c >> @@ -202,16 +202,17 @@ int build_e820_table(struct e820entry *e820, >> nr++; >> >> /* >> - * Mark populated reserved memory that contains ACPI tables as ACPI >> data. >> + * Mark populated reserved memory that contains ACPI tables as ACPI NVS. >> * That should help the guest to treat it correctly later: e.g. pass to >> - * the next kernel on kexec or reclaim if necessary. >> + * the next kernel on kexec and prevent space reclaim which is possible >> + * with regular ACPI data type accoring to ACPI spec v6.3. > > Preventing space reclaim is not the business of hvmloader. As per above, > an ACPI unaware OS ought to be permitted to use as ordinary RAM all the > space the tables occupy. Therefore at the very least the comment needs > to reflect that this preventing of space reclaim is a workaround, not > correct behavior. Agree to modify the comment. > Also as a nit: "according". > > As a consequence I think we will also want to adjust Xen itself to > automatically disable ACPI when it ends up consuming E801 data. Or > alternatively we should consider dropping all E801-related code (as > being inapplicable to 64-bit systems). I'm not following here. What Xen has to do with E801? It's a SeaBIOS implemented call that happened to be used by QEMU option ROM. We cannot drop it from there as it's part of BIOS spec. Igor
Re: [PATCH v2 4/4] x86/shadow: refactor shadow_vram_{get,put}_l1e()
On 10.10.2020 09:45, Roger Pau Monné wrote:
> On Thu, Oct 08, 2020 at 04:36:47PM +0100, Andrew Cooper wrote:
>> On 08/10/2020 16:15, Roger Pau Monné wrote:
>>> On Wed, Sep 16, 2020 at 03:08:40PM +0200, Jan Beulich wrote:
+void shadow_vram_put_mfn(mfn_t mfn, unsigned int l1f,
+ mfn_t sl1mfn, const void *sl1e,
+ const struct domain *d)
+{
+unsigned long gfn;
+struct sh_dirty_vram *dirty_vram = d->arch.hvm.dirty_vram;
+
+ASSERT(is_hvm_domain(d));
+
+if ( !dirty_vram /* tracking disabled? */ ||
+ !(l1f & _PAGE_RW) /* read-only mapping? */ ||
+ !mfn_valid(mfn) /* mfn can be invalid in mmio_direct */)
+return;
+
+gfn = gfn_x(mfn_to_gfn(d, mfn));
+/* Page sharing not supported on shadow PTs */
+BUG_ON(SHARED_M2P(gfn));
+
+if ( (gfn >= dirty_vram->begin_pfn) && (gfn < dirty_vram->end_pfn) )
+{
+unsigned long i = gfn - dirty_vram->begin_pfn;
+const struct page_info *page = mfn_to_page(mfn);
+bool dirty = false;
+paddr_t sl1ma = mfn_to_maddr(sl1mfn) | PAGE_OFFSET(sl1e);
+
+if ( (page->u.inuse.type_info & PGT_count_mask) == 1 )
+{
+/* Last reference */
+if ( dirty_vram->sl1ma[i] == INVALID_PADDR )
+{
+/* We didn't know it was that one, let's say it is dirty
*/
+dirty = true;
+}
+else
+{
+ASSERT(dirty_vram->sl1ma[i] == sl1ma);
+dirty_vram->sl1ma[i] = INVALID_PADDR;
+if ( l1f & _PAGE_DIRTY )
+dirty = true;
+}
+}
+else
+{
+/* We had more than one reference, just consider the page
dirty. */
+dirty = true;
+/* Check that it's not the one we recorded. */
+if ( dirty_vram->sl1ma[i] == sl1ma )
+{
+/* Too bad, we remembered the wrong one... */
+dirty_vram->sl1ma[i] = INVALID_PADDR;
+}
+else
+{
+/*
+ * Ok, our recorded sl1e is still pointing to this page,
let's
+ * just hope it will remain.
+ */
+}
+}
+
+if ( dirty )
+{
+dirty_vram->dirty_bitmap[i / 8] |= 1 << (i % 8);
>>> Could you use _set_bit here?
>>
>> __set_bit() uses 4-byte accesses. This uses 1-byte accesses.
>
> Right, this is allocated using alloc directly, not the bitmap helper,
> and the size if rounded to byte level, not unsigned int.
>
>> Last I checked, there is a boundary issue at the end of the dirty_bitmap.
>>
>> Both Julien and I have considered changing our bit infrastructure to use
>> byte accesses, which would make them more generally useful.
>
> Does indeed seem useful to me, as we could safely expand the usage of
> the bitmap ops without risking introducing bugs.
Aren't there architectures being handicapped when it comes to sub-word
accesses? At least common code may better not make assumptions towards
more fine grained accesses ...
As to x86, couldn't we make the macros evaluate alignof(*(addr)) and
choose byte-based accesses when it's less than 4?
Jan
[xen-unstable-smoke test] 155774: regressions - FAIL
flight 155774 xen-unstable-smoke real [real]
http://logs.test-lab.xenproject.org/osstest/logs/155774/
Regressions :-(
Tests which did not succeed and are blocking,
including tests which could not be run:
test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 155584
Tests which did not succeed, but are not blocking:
test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass
test-armhf-armhf-xl 15 migrate-support-checkfail never pass
test-armhf-armhf-xl 16 saverestore-support-checkfail never pass
version targeted for testing:
xen 534b3d09958fdc4df64872c2ab19feb4b1eebc5a
baseline version:
xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9
Last test of basis 155584 2020-10-09 02:01:25 Z4 days
Failing since155612 2020-10-09 18:01:22 Z3 days 28 attempts
Testing same since 155708 2020-10-11 23:00:25 Z1 days 12 attempts
People who touched revisions under test:
Andrew Cooper
Jan Beulich
Jason Andryuk
Juergen Gross
Roger Pau Monné
Trammell Hudson
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 fail
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 534b3d09958fdc4df64872c2ab19feb4b1eebc5a
Author: Juergen Gross
Date: Sun Oct 11 14:24:01 2020 +0200
tools/libs/store: add disclaimer to header file regarding ignored options
Add a disclaimer to the libxenstore header file that all of the open
flags (socket only connection, read only connection) are ignored.
Signed-off-by: Juergen Gross
Acked-by: Wei Liu
commit 1b810a9d5a39230e76073b1a753cd2c34ded65fc
Author: Jason Andryuk
Date: Thu Oct 1 19:53:37 2020 -0400
libxl: only query VNC when enabled
QEMU without VNC support (configure --disable-vnc) will return an error
when VNC is queried over QMP since it does not recognize the QMP
command. This will cause libxl to fail starting the domain even if VNC
is not enabled. Therefore only query QEMU for VNC support when using
VNC, so a VNC-less QEMU will function in this configuration.
'goto out' jumps to the call to device_model_postconfig_done(), the
final callback after the chain of vnc queries. This bypasses all the
QMP VNC queries.
Signed-off-by: Jason Andryuk
Acked-by: Wei Liu
commit 8a62dee9ceff3056c7e0bd9632bac39bee2a51b3
Author: Jan Beulich
Date: Fri Oct 2 12:30:34 2020 +0200
x86/vLAPIC: don't leak regs page from vlapic_init() upon error
Fixes: 8a981e0bf25e ("Make map_domain_page_global fail")
Signed-off-by: Jan Beulich
Reviewed-by: Andrew Cooper
commit 8a71d50ed40bfa78c37722dc11995ac2563662c3
Author: Trammell Hudson
Date: Fri Oct 2 07:18:21 2020 -0400
efi: Enable booting unified hypervisor/kernel/initrd images
This patch adds support for bundling the xen.efi hypervisor, the xen.cfg
configuration file, the Linux kernel and initrd, as well as the XSM,
and architectural specific files into a single "unified" EFI executable.
This allows an administrator to update the components independently
without requiring rebuilding xen, as well as to replace the components
in an existing image.
The resulting EFI executable can be invoked directly from the UEFI Boot
Manager, removing the need to use a separate loader like grub as well
as removing dependencies on local filesystem access. And since it is
a single file, it can be signed and validated by UEFI Secure Boot without
requring the shim protocol.
It is inspired by systemd-boot's unified kernel technique and borrows the
function to locate PE sections from systemd's LGPL'ed code. During EFI
boot, Xen looks at its own loaded image to locate the
Re: [PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()
On 05.10.2020 14:23, Andrew Cooper wrote:
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool
> remove)
> if ( IS_ENABLED(CONFIG_PV32) )
> FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
>
> +if ( stack_base[cpu] )
> +memguard_unguard_stack(stack_base[cpu]);
> +
> if ( remove )
> {
> FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
> FREE_XENHEAP_PAGE(idt_tables[cpu]);
>
> if ( stack_base[cpu] )
> -{
> -memguard_unguard_stack(stack_base[cpu]);
> FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
> -}
> }
> }
In my initial reply to Marek's report I did suggest putting the fix
in the alloc path in order to keep the pages "guarded" while the CPU
is parked, as the CPU during that period may still access at least
some of the stacks (e.g. when sending it an NMI IPI to enter a deeper
C state).
Otherwise, if the fix really was to remain here, the if() could now
also be dropped. And in this case I'd also suggest adding the new
piece of code a few lines earlier, so that all the
FREE_XENHEAP_PAGE() stay close together.
Jan
Re: [PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()
On 13/10/2020 14:16, Jan Beulich wrote:
> On 05.10.2020 14:23, Andrew Cooper wrote:
>> --- a/xen/arch/x86/smpboot.c
>> +++ b/xen/arch/x86/smpboot.c
>> @@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool
>> remove)
>> if ( IS_ENABLED(CONFIG_PV32) )
>> FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
>>
>> +if ( stack_base[cpu] )
>> +memguard_unguard_stack(stack_base[cpu]);
>> +
>> if ( remove )
>> {
>> FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
>> FREE_XENHEAP_PAGE(idt_tables[cpu]);
>>
>> if ( stack_base[cpu] )
>> -{
>> -memguard_unguard_stack(stack_base[cpu]);
>> FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
>> -}
>> }
>> }
> In my initial reply to Marek's report I did suggest putting the fix
> in the alloc path in order to keep the pages "guarded" while the CPU
> is parked
In which case you should have identified that bug explicitly.
Because I can't read your mind while fixing the other real bugs in your
suggestion.
~Andrew
Re: [PATCH v2 2/6] x86: reduce CET-SS related #ifdef-ary
On 13.10.2020 13:40, Andrew Cooper wrote: > (Interestingly, zero length > alternatives do appear to compile, and this is clearly a bug.) Why? The replacement code may be intended to be all NOPs. Jan
Re: [SECOND RESEND] [PATCH] tools/python: Pass linker to Python build process
On Sun, Oct 11, 2020 at 06:11:39PM -0700, Elliott Mitchell wrote: > Unexpectedly the environment variable which needs to be passed is > $LDSHARED and not $LD. Otherwise Python may find the build `ld` instead > of the host `ld`. > > Replace $(LDFLAGS) with $(SHLIB_LDFLAGS) as Python needs shared objects > it can load at runtime, not executables. > > This uses $(CC) instead of $(LD) since Python distutils appends $CFLAGS > to $LDFLAGS which breaks many linkers. > > Signed-off-by: Elliott Mitchell > --- > This is now the *third* time this has been sent to the list. Mark Pryor > has tested and confirms Python cross-building is working. There is one > wart left which I'm unsure of the best approach for. > > Having looked around a bit, I believe this is a Python 2/3 compatibility > issue. "distutils" for Python 2 likely lacked a separate $LDSHARED or > $LD variable, whereas Python 3 does have this. Alas this is pointless > due to the above (unless you can cause distutils.py to do the final link > step separately). I think this is well-reasoned but I don't have time to figure out and verify the details. Marek, do you have any comment on this? > --- > tools/pygrub/Makefile | 9 + > tools/python/Makefile | 9 + > 2 files changed, 10 insertions(+), 8 deletions(-) > > diff --git a/tools/pygrub/Makefile b/tools/pygrub/Makefile > index 3063c4998f..37b2146214 100644 > --- a/tools/pygrub/Makefile > +++ b/tools/pygrub/Makefile > @@ -3,20 +3,21 @@ XEN_ROOT = $(CURDIR)/../.. > include $(XEN_ROOT)/tools/Rules.mk > > PY_CFLAGS = $(CFLAGS) $(PY_NOOPT_CFLAGS) > -PY_LDFLAGS = $(LDFLAGS) $(APPEND_LDFLAGS) > +PY_LDFLAGS = $(SHLIB_LDFLAGS) $(APPEND_LDFLAGS) > INSTALL_LOG = build/installed_files.txt > > .PHONY: all > all: build > .PHONY: build > build: > - CC="$(CC)" CFLAGS="$(PY_CFLAGS)" LDFLAGS="$(PY_LDFLAGS)" $(PYTHON) > setup.py build > + CC="$(CC)" CFLAGS="$(PY_CFLAGS)" LDSHARED="$(CC)" > LDFLAGS="$(PY_LDFLAGS)" $(PYTHON) setup.py build > > .PHONY: install > install: all > $(INSTALL_DIR) $(DESTDIR)/$(bindir) > - CC="$(CC)" CFLAGS="$(PY_CFLAGS)" LDFLAGS="$(PY_LDFLAGS)" $(PYTHON) \ > - setup.py install --record $(INSTALL_LOG) $(PYTHON_PREFIX_ARG) \ > + CC="$(CC)" CFLAGS="$(PY_CFLAGS)" LDSHARED="$(CC)" \ > + LDFLAGS="$(PY_LDFLAGS)" $(PYTHON) setup.py install \ > + --record $(INSTALL_LOG) $(PYTHON_PREFIX_ARG) \ >--root="$(DESTDIR)" --install-scripts=$(LIBEXEC_BIN) --force > set -e; if [ $(bindir) != $(LIBEXEC_BIN) -a \ >"`readlink -f $(DESTDIR)/$(bindir)`" != \ > diff --git a/tools/python/Makefile b/tools/python/Makefile > index 8d22c03676..b675f5b4de 100644 > --- a/tools/python/Makefile > +++ b/tools/python/Makefile > @@ -5,19 +5,20 @@ include $(XEN_ROOT)/tools/Rules.mk > all: build > > PY_CFLAGS = $(CFLAGS) $(PY_NOOPT_CFLAGS) > -PY_LDFLAGS = $(LDFLAGS) $(APPEND_LDFLAGS) > +PY_LDFLAGS = $(SHLIB_LDFLAGS) $(APPEND_LDFLAGS) > INSTALL_LOG = build/installed_files.txt > > .PHONY: build > build: > - CC="$(CC)" CFLAGS="$(PY_CFLAGS)" $(PYTHON) setup.py build > + CC="$(CC)" CFLAGS="$(PY_CFLAGS)" LDSHARED="$(CC)" > LDFLAGS="$(PY_LDFLAGS)" $(PYTHON) setup.py build > > .PHONY: install > install: > $(INSTALL_DIR) $(DESTDIR)$(LIBEXEC_BIN) > > - CC="$(CC)" CFLAGS="$(PY_CFLAGS)" LDFLAGS="$(PY_LDFLAGS)" $(PYTHON) \ > - setup.py install --record $(INSTALL_LOG) $(PYTHON_PREFIX_ARG) \ > + CC="$(CC)" CFLAGS="$(PY_CFLAGS)" LDSHARED="$(CC)" \ > + LDFLAGS="$(PY_LDFLAGS)" $(PYTHON) setup.py install \ > + --record $(INSTALL_LOG) $(PYTHON_PREFIX_ARG) \ > --root="$(DESTDIR)" --force > > $(INSTALL_PYTHON_PROG) scripts/convert-legacy-stream > $(DESTDIR)$(LIBEXEC_BIN) > -- > 2.20.1 > > > -- > (\___(\___(\__ --=> 8-) EHM <=-- __/)___/)___/) > \BS (| ehem+sig...@m5p.com PGP 87145445 |) / > \_CS\ | _ -O #include O- _ | / _/ > 8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445 > >
Re: [PATCH v2 5/6] x86: guard against straight-line speculation past RET
On 13.10.2020 14:00, Andrew Cooper wrote: > On 28/09/2020 13:31, Jan Beulich wrote: >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -145,7 +145,15 @@ t2 = $(call as-insn,$(CC) -I$(BASEDIR)/i >> # https://bugs.llvm.org/show_bug.cgi?id=36110 >> t3 = $(call as-insn,$(CC),".macro FOO;.endm"$(close); asm volatile >> $(open)".macro FOO;.endm",-no-integrated-as) >> >> -CLANG_FLAGS += $(call or,$(t1),$(t2),$(t3)) >> +# Check whether \(text) escaping in macro bodies is supported. >> +t4 = $(call as-insn,$(CC),".macro m ret:req; \\(ret) $$\\ret; .endm; m >> 8",,-no-integrated-as) >> + >> +# Check whether macros can override insn mnemonics in inline assembly. >> +t5 = $(call as-insn,$(CC),".macro ret; .error; .endm; .macro retq; .error; >> .endm",-no-integrated-as) >> + >> +acc1 := $(call or,$(t1),$(t2),$(t3),$(t4)) >> + >> +CLANG_FLAGS += $(call or,$(acc1),$(t5)) > > I'm not happy taking this until there is toolchain support visible in > the future. > > We *cannot* rule out the use of IAS forever more, because there are > features far more important than ret speculation which depend on it. So what do you suggest? We can't have both, afaics, so we need to pick either being able to use the integrated assembler or being able to guard RET. >> --- a/xen/include/asm-x86/asm-defns.h >> +++ b/xen/include/asm-x86/asm-defns.h >> @@ -50,3 +50,22 @@ >> .macro INDIRECT_JMP arg:req >> INDIRECT_BRANCH jmp \arg >> .endm >> + >> +/* >> + * To guard against speculation past RET, insert a breakpoint insn >> + * immediately after them. >> + */ >> +.macro ret operand:vararg >> +ret$ \operand >> +.endm >> +.macro retq operand:vararg >> +ret$ \operand >> +.endm >> +.macro ret$ operand:vararg >> +.purgem ret >> +ret \operand > > You're substituting retq for ret, which defeats the purpose of unwrapping. I'm afraid I don't understand the "defeats" aspect. > I will repeat my previous feedback. Do away with this > wrapping/unwrapping and just use a raw .byte. Its simpler and faster. Well, I could now also repeat my prior response to your prior feedback, but since iirc you didn't reply back then I don't expect you would now. Instead I'll - once again - give in despite my believe that this is the cleaner approach, and that in cases like this one - when there are pros and cons to either approach - it should be the author's choice rather than the reviewer's. Jan
Re: [PATCH] x86/msr: fix handling of MSR_IA32_PERF_{STATUS/CTL}
On 07.10.2020 18:41, Roger Pau Monné wrote:
> On Wed, Oct 07, 2020 at 01:06:08PM +0100, Andrew Cooper wrote:
>> On 06/10/2020 17:23, Roger Pau Monne wrote:
>>> Currently a PV hardware domain can also be given control over the CPU
>>> frequency, and such guest is allowed to write to MSR_IA32_PERF_CTL.
>>
>> This might be how the current logic "works", but its straight up broken.
>>
>> PERF_CTL is thread scope, so unless dom0 is identity pinned and has one
>> vcpu for every pcpu, it cannot use the interface correctly.
>
> Selecting cpufreq=dom0-kernel will force vCPU pinning. I'm not able
> however to see anywhere that would force dom0 vCPUs == pCPUs.
Unless there are other overriding command line options, doesn't the
way sched_select_initial_cpu() works guarantee this?
>>> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
>>> index d8ed83f869..41baa3b7a1 100644
>>> --- a/xen/include/xen/sched.h
>>> +++ b/xen/include/xen/sched.h
>>> @@ -1069,6 +1069,12 @@ extern enum cpufreq_controller {
>>> FREQCTL_none, FREQCTL_dom0_kernel, FREQCTL_xen
>>> } cpufreq_controller;
>>>
>>> +static inline bool is_cpufreq_controller(const struct domain *d)
>>> +{
>>> +return ((cpufreq_controller == FREQCTL_dom0_kernel) &&
>>> +is_hardware_domain(d));
>>
>> This won't compile on !CONFIG_X86, due to CONFIG_HAS_CPUFREQ
>
> It does seem to build on Arm, because this is only used in x86 code:
>
> https://gitlab.com/xen-project/people/royger/xen/-/jobs/778207412
>
> The extern declaration of cpufreq_controller is just above, so if you
> tried to use is_cpufreq_controller on Arm you would get a link time
> error, otherwise it builds fine. The compiler removes the function on
> Arm as it has the inline attribute and it's not used.
>
> Alternatively I could look into moving cpufreq_controller (and
> is_cpufreq_controller) out of sched.h into somewhere else, I haven't
> looked at why it needs to live there.
>
>> Honestly - I don't see any point to this code. Its opt-in via the
>> command line only, and doesn't provide adequate checks for enablement.
>> (It's not as if we're lacking complexity or moving parts when it comes
>> to power/frequency management).
>
> Right, I could do a pre-patch to remove this, but I also don't think
> we should block this fix on removing FREQCTL_dom0_kernel, so I would
> rather fix the regression and then remove the feature if we agree it
> can be removed.
I agree.
Jan
Re: [PATCH] x86/msr: fix handling of MSR_IA32_PERF_{STATUS/CTL}
On 06.10.2020 18:23, Roger Pau Monne wrote:
> Currently a PV hardware domain can also be given control over the CPU
> frequency, and such guest is allowed to write to MSR_IA32_PERF_CTL.
> However since commit 322ec7c89f6 the default behavior has been changed
> to reject accesses to not explicitly handled MSRs, preventing PV
> guests that manage CPU frequency from reading
> MSR_IA32_PERF_{STATUS/CTL}.
>
> Additionally some HVM guests (Windows at least) will attempt to read
> MSR_IA32_PERF_CTL and will panic if given back a #GP fault:
>
> vmx.c:3035:d8v0 RDMSR 0x0199 unimplemented
> d8v0 VIRIDIAN CRASH: 3b c096 f806871c1651 da0253683720 0
>
> Move the handling of MSR_IA32_PERF_{STATUS/CTL} to the common MSR
> handling shared between HVM and PV guests, and add an explicit case
> for reads to MSR_IA32_PERF_{STATUS/CTL}.
>
> Restore previous behavior and allow PV guests with the required
> permissions to read the contents of the mentioned MSRs. Non privileged
> guests will get 0 when trying to read those registers, as writes to
> MSR_IA32_PERF_CTL by such guest will already be silently dropped.
>
> Fixes: 322ec7c89f6 ('x86/pv: disallow access to unknown MSRs')
> Fixes: 84e848fd7a1 ('x86/hvm: disallow access to unknown MSRs')
> Signed-off-by: Roger Pau Monné
I would have given this my R-b, but Andrew's "straight up broken"
comment needs resolving first, one way or another.
Jan
Re: [PATCH v2 1/2] xen/events: access last_priority and last_vcpu_id together
On 12.10.2020 11:27, Juergen Gross wrote: > The queue for a fifo event is depending on the vcpu_id and the > priority of the event. When sending an event it might happen the > event needs to change queues and the old queue needs to be kept for > keeping the links between queue elements intact. For this purpose > the event channel contains last_priority and last_vcpu_id values > elements for being able to identify the old queue. > > In order to avoid races always access last_priority and last_vcpu_id > with a single atomic operation avoiding any inconsistencies. > > Signed-off-by: Juergen Gross I seem to vaguely recall that at the time this seemingly racy access was done on purpose by David. Did you go look at the old commits to understand whether there really is a race which can't be tolerated within the spec? > --- a/xen/include/xen/sched.h > +++ b/xen/include/xen/sched.h > @@ -114,8 +114,7 @@ struct evtchn > u16 virq; /* state == ECS_VIRQ */ > } u; > u8 priority; > -u8 last_priority; > -u16 last_vcpu_id; > +u32 fifo_lastq;/* Data for fifo events identifying last queue. */ This grows struct evtchn's size on at least 32-bit Arm. I'd like to suggest including "priority" in the union, and call the new field simply "fifo" or some such. Jan
Re: [PATCH v2 2/2] xen/evtchn: rework per event channel lock
On 12.10.2020 11:27, Juergen Gross wrote: > Currently the lock for a single event channel needs to be taken with > interrupts off, which causes deadlocks in some cases. > > Rework the per event channel lock to be non-blocking for the case of > sending an event and removing the need for disabling interrupts for > taking the lock. > > The lock is needed for avoiding races between sending an event or > querying the channel's state against removal of the event channel. > > Use a locking scheme similar to a rwlock, but with some modifications: > > - sending an event or querying the event channel's state uses an > operation similar to read_trylock(), in case of not obtaining the > lock the sending is omitted or a default state is returned And how come omitting the send or returning default state is valid? Jan
Re: [PATCH v2] build: always use BASEDIR for xen sub-directory
On 07.10.2020 16:57, Bertrand Marquis wrote: > Modify Makefiles using $(XEN_ROOT)/xen to use $(BASEDIR) instead. > > This is removing the dependency to xen subdirectory preventing using a > wrong configuration file when xen subdirectory is duplicated for > compilation tests. > > BASEDIR is set in xen/lib/x86/Makefile as this Makefile is directly > called from the tools build and install process and BASEDIR is not set > there. > > Signed-off-by: Bertrand Marquis And once again Acked-by: Jan Beulich Jan
Re: [PATCH] x86/msr: handle IA32_THERM_STATUS
On 07.10.2020 12:20, Roger Pau Monne wrote:
> Windows 8 will attempt to read MSR_IA32_THERM_STATUS and panic if a
> #GP fault is injected as a result:
>
> vmx.c:3035:d8v0 RDMSR 0x019c unimplemented
> d8v0 VIRIDIAN CRASH: 3b c096 f8061de31651 f4088a613720 0
>
> So handle the MSR and return 0 instead.
>
> Note that this is done on the generic MSR handler, and PV guest will
> also get 0 back when trying to read the MSR. There doesn't seem to be
> much value in handling the MSR for HVM guests only.
>
> Fixes: 84e848fd7a1 ('x86/hvm: disallow access to unknown MSRs')
> Signed-off-by: Roger Pau Monné
Acked-by: Jan Beulich
[PATCH v2 0/3] Add Xen CpusAccel
Xen was left behind when CpusAccel became mandatory and fails the assert
in qemu_init_vcpu(). It relied on the same dummy cpu threads as qtest.
Move the qtest cpu functions to a common location and reuse them for
Xen.
v2:
New patch "accel: Remove _WIN32 ifdef from qtest-cpus.c"
Use accel/dummy-cpus.c for filename
Put prototype in include/sysemu/cpus.h
Jason Andryuk (3):
accel: Remove _WIN32 ifdef from qtest-cpus.c
accel: move qtest CpusAccel functions to a common location
accel: Add xen CpusAccel using dummy-cpus
accel/{qtest/qtest-cpus.c => dummy-cpus.c} | 27 --
accel/meson.build | 8 +++
accel/qtest/meson.build| 1 -
accel/qtest/qtest-cpus.h | 17 --
accel/qtest/qtest.c| 5 +++-
accel/xen/xen-all.c| 8 +++
include/sysemu/cpus.h | 3 +++
7 files changed, 27 insertions(+), 42 deletions(-)
rename accel/{qtest/qtest-cpus.c => dummy-cpus.c} (71%)
delete mode 100644 accel/qtest/qtest-cpus.h
--
2.25.1
[PATCH v2 3/3] accel: Add xen CpusAccel using dummy-cpus
Xen was broken by commit 1583a3898853 ("cpus: extract out qtest-specific
code to accel/qtest"). Xen relied on qemu_init_vcpu() calling
qemu_dummy_start_vcpu() in the default case, but that was replaced by
g_assert_not_reached().
Add a minimal "CpusAccel" for Xen using the dummy-cpus implementation
used by qtest.
Signed-off-by: Jason Andryuk
---
accel/meson.build | 1 +
accel/xen/xen-all.c | 8
2 files changed, 9 insertions(+)
diff --git a/accel/meson.build b/accel/meson.build
index 9a417396bd..b26cca227a 100644
--- a/accel/meson.build
+++ b/accel/meson.build
@@ -12,3 +12,4 @@ dummy_ss.add(files(
))
specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true:
dummy_ss)
+specific_ss.add_all(when: ['CONFIG_XEN'], if_true: dummy_ss)
diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
index 60b971d0a8..878a4089d9 100644
--- a/accel/xen/xen-all.c
+++ b/accel/xen/xen-all.c
@@ -16,6 +16,7 @@
#include "hw/xen/xen_pt.h"
#include "chardev/char.h"
#include "sysemu/accel.h"
+#include "sysemu/cpus.h"
#include "sysemu/xen.h"
#include "sysemu/runstate.h"
#include "migration/misc.h"
@@ -153,6 +154,10 @@ static void xen_setup_post(MachineState *ms, AccelState
*accel)
}
}
+const CpusAccel xen_cpus = {
+.create_vcpu_thread = dummy_start_vcpu_thread,
+};
+
static int xen_init(MachineState *ms)
{
MachineClass *mc = MACHINE_GET_CLASS(ms);
@@ -180,6 +185,9 @@ static int xen_init(MachineState *ms)
* opt out of system RAM being allocated by generic code
*/
mc->default_ram_id = NULL;
+
+cpus_register_accel(&xen_cpus);
+
return 0;
}
--
2.25.1
Re: [PATCH v2 01/11] x86/hvm: drop vcpu parameter from vlapic EOI callbacks
On Fri, Oct 02, 2020 at 10:48:07AM +0200, Jan Beulich wrote:
> On 30.09.2020 12:40, Roger Pau Monne wrote:
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -459,13 +459,10 @@ void vlapic_EOI_set(struct vlapic *vlapic)
> >
> > void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
> > {
> > -struct vcpu *v = vlapic_vcpu(vlapic);
> > -struct domain *d = v->domain;
> > -
> > if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
> > -vioapic_update_EOI(d, vector);
> > +vioapic_update_EOI(vector);
> >
> > -hvm_dpci_msi_eoi(d, vector);
> > +hvm_dpci_msi_eoi(vector);
> > }
>
> What about viridian_synic_wrmsr() -> vlapic_EOI_set() ->
> vlapic_handle_EOI()? You'd probably have noticed this if you
> had tried to (consistently) drop the respective parameters from
> the intermediate functions as well.
>
> Question of course is in how far viridian_synic_wrmsr() for
> HV_X64_MSR_EOI makes much sense when v != current. Paul, Wei?
There's already an assert at the top of viridian_synic_wrmsr of v ==
current, which I assume is why I thought this change was fine. I can
purge the passing of v (current) further, but it wasn't really needed
for the rest of the series.
> A secondary question of course is whether passing around the
> pointers isn't really cheaper than the obtaining of 'current'.
Well, while there's indeed a performance aspect here, I think
using current is much clearer than passing a vcpu around. I could
rename the parameter to curr or some such, but I think using
current and not passing a vcpu parameter is clearer.
Thanks, Roger.
Re: [PATCH] x86/ucode/intel: Improve description for gathering the microcode revision
On 12.10.2020 16:25, Andrew Cooper wrote: > Obtaining the microcode revision on Intel CPUs is complicated for backwards > compatibility reasons. Update apply_microcode() to use a slightly more > efficient CPUID invocation, now that the documentation has been updated to > confirm that any CPUID instruction is fine, not just CPUID.1 > > Signed-off-by: Andrew Cooper Reviewed-by: Jan Beulich
Re: [PATCH v2 3/3] accel: Add xen CpusAccel using dummy-cpus
On 10/13/20 4:05 PM, Jason Andryuk wrote:
> Xen was broken by commit 1583a3898853 ("cpus: extract out qtest-specific
> code to accel/qtest"). Xen relied on qemu_init_vcpu() calling
> qemu_dummy_start_vcpu() in the default case, but that was replaced by
> g_assert_not_reached().
>
> Add a minimal "CpusAccel" for Xen using the dummy-cpus implementation
> used by qtest.
>
> Signed-off-by: Jason Andryuk
> ---
> accel/meson.build | 1 +
> accel/xen/xen-all.c | 8
> 2 files changed, 9 insertions(+)
>
> diff --git a/accel/meson.build b/accel/meson.build
> index 9a417396bd..b26cca227a 100644
> --- a/accel/meson.build
> +++ b/accel/meson.build
> @@ -12,3 +12,4 @@ dummy_ss.add(files(
> ))
>
> specific_ss.add_all(when: ['CONFIG_SOFTMMU', 'CONFIG_POSIX'], if_true:
> dummy_ss)
> +specific_ss.add_all(when: ['CONFIG_XEN'], if_true: dummy_ss)
> diff --git a/accel/xen/xen-all.c b/accel/xen/xen-all.c
> index 60b971d0a8..878a4089d9 100644
> --- a/accel/xen/xen-all.c
> +++ b/accel/xen/xen-all.c
> @@ -16,6 +16,7 @@
> #include "hw/xen/xen_pt.h"
> #include "chardev/char.h"
> #include "sysemu/accel.h"
> +#include "sysemu/cpus.h"
> #include "sysemu/xen.h"
> #include "sysemu/runstate.h"
> #include "migration/misc.h"
> @@ -153,6 +154,10 @@ static void xen_setup_post(MachineState *ms, AccelState
> *accel)
> }
> }
>
> +const CpusAccel xen_cpus = {
> +.create_vcpu_thread = dummy_start_vcpu_thread,
> +};
> +
> static int xen_init(MachineState *ms)
> {
> MachineClass *mc = MACHINE_GET_CLASS(ms);
> @@ -180,6 +185,9 @@ static int xen_init(MachineState *ms)
> * opt out of system RAM being allocated by generic code
> */
> mc->default_ram_id = NULL;
> +
> +cpus_register_accel(&xen_cpus);
> +
> return 0;
> }
>
>
Reviewed-by: Claudio Fontana
Re: [PATCH v2 01/11] x86/hvm: drop vcpu parameter from vlapic EOI callbacks
On 13.10.2020 16:08, Roger Pau Monné wrote:
> On Fri, Oct 02, 2020 at 10:48:07AM +0200, Jan Beulich wrote:
>> On 30.09.2020 12:40, Roger Pau Monne wrote:
>>> --- a/xen/arch/x86/hvm/vlapic.c
>>> +++ b/xen/arch/x86/hvm/vlapic.c
>>> @@ -459,13 +459,10 @@ void vlapic_EOI_set(struct vlapic *vlapic)
>>>
>>> void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
>>> {
>>> -struct vcpu *v = vlapic_vcpu(vlapic);
>>> -struct domain *d = v->domain;
>>> -
>>> if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
>>> -vioapic_update_EOI(d, vector);
>>> +vioapic_update_EOI(vector);
>>>
>>> -hvm_dpci_msi_eoi(d, vector);
>>> +hvm_dpci_msi_eoi(vector);
>>> }
>>
>> What about viridian_synic_wrmsr() -> vlapic_EOI_set() ->
>> vlapic_handle_EOI()? You'd probably have noticed this if you
>> had tried to (consistently) drop the respective parameters from
>> the intermediate functions as well.
>>
>> Question of course is in how far viridian_synic_wrmsr() for
>> HV_X64_MSR_EOI makes much sense when v != current. Paul, Wei?
>
> There's already an assert at the top of viridian_synic_wrmsr of v ==
> current, which I assume is why I thought this change was fine. I can
> purge the passing of v (current) further, but it wasn't really needed
> for the rest of the series.
To a large degree that's up to you. It's just that, as said, if
you had done so, you'd likely have noticed the issue, and hence
doing so here and elsewhere may provide reassurance that there's
no further similar case lurking anywhere.
>> A secondary question of course is whether passing around the
>> pointers isn't really cheaper than the obtaining of 'current'.
>
> Well, while there's indeed a performance aspect here, I think
> using current is much clearer than passing a vcpu around. I could
> rename the parameter to curr or some such, but I think using
> current and not passing a vcpu parameter is clearer.
Personally I'd prefer "curr" named function parameters. But if
Andrew or Wei agree with your approach, I'm not going to object.
Jan
Re: [PATCH v2 2/2] xen/evtchn: rework per event channel lock
On 13.10.20 16:02, Jan Beulich wrote: On 12.10.2020 11:27, Juergen Gross wrote: Currently the lock for a single event channel needs to be taken with interrupts off, which causes deadlocks in some cases. Rework the per event channel lock to be non-blocking for the case of sending an event and removing the need for disabling interrupts for taking the lock. The lock is needed for avoiding races between sending an event or querying the channel's state against removal of the event channel. Use a locking scheme similar to a rwlock, but with some modifications: - sending an event or querying the event channel's state uses an operation similar to read_trylock(), in case of not obtaining the lock the sending is omitted or a default state is returned And how come omitting the send or returning default state is valid? This is explained in the part of the commit message you didn't cite: With this locking scheme it is mandatory that a writer will always either start with an unbound or free event channel or will end with an unbound or free event channel, as otherwise the reaction of a reader not getting the lock would be wrong. Juergen
Re: [PATCH v2 1/2] xen/events: access last_priority and last_vcpu_id together
On 13.10.20 15:58, Jan Beulich wrote: On 12.10.2020 11:27, Juergen Gross wrote: The queue for a fifo event is depending on the vcpu_id and the priority of the event. When sending an event it might happen the event needs to change queues and the old queue needs to be kept for keeping the links between queue elements intact. For this purpose the event channel contains last_priority and last_vcpu_id values elements for being able to identify the old queue. In order to avoid races always access last_priority and last_vcpu_id with a single atomic operation avoiding any inconsistencies. Signed-off-by: Juergen Gross I seem to vaguely recall that at the time this seemingly racy access was done on purpose by David. Did you go look at the old commits to understand whether there really is a race which can't be tolerated within the spec? At least the comments in the code tell us that the race regarding the writing of priority (not last_priority) is acceptable. Especially Julien was rather worried by the current situation. In case you can convince him the current handling is fine, we can easily drop this patch. --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -114,8 +114,7 @@ struct evtchn u16 virq; /* state == ECS_VIRQ */ } u; u8 priority; -u8 last_priority; -u16 last_vcpu_id; +u32 fifo_lastq;/* Data for fifo events identifying last queue. */ This grows struct evtchn's size on at least 32-bit Arm. I'd like to suggest including "priority" in the union, and call the new field simply "fifo" or some such. This will add quite some complexity as suddenly all writes to the union will need to be made through a cmpxchg() scheme. Regarding the growth: struct evtchn is aligned to 64 bytes. So there is no growth at all, as the size will not be larger than those 64 bytes. Juergen
Re: [PATCH v2 1/2] xen/events: access last_priority and last_vcpu_id together
On 13.10.2020 16:20, Jürgen Groß wrote: > On 13.10.20 15:58, Jan Beulich wrote: >> On 12.10.2020 11:27, Juergen Gross wrote: >>> The queue for a fifo event is depending on the vcpu_id and the >>> priority of the event. When sending an event it might happen the >>> event needs to change queues and the old queue needs to be kept for >>> keeping the links between queue elements intact. For this purpose >>> the event channel contains last_priority and last_vcpu_id values >>> elements for being able to identify the old queue. >>> >>> In order to avoid races always access last_priority and last_vcpu_id >>> with a single atomic operation avoiding any inconsistencies. >>> >>> Signed-off-by: Juergen Gross >> >> I seem to vaguely recall that at the time this seemingly racy >> access was done on purpose by David. Did you go look at the >> old commits to understand whether there really is a race which >> can't be tolerated within the spec? > > At least the comments in the code tell us that the race regarding > the writing of priority (not last_priority) is acceptable. Ah, then it was comments. I knew I read something to this effect somewhere, recently. > Especially Julien was rather worried by the current situation. In > case you can convince him the current handling is fine, we can > easily drop this patch. Julien, in the light of the above - can you clarify the specific concerns you (still) have? >>> --- a/xen/include/xen/sched.h >>> +++ b/xen/include/xen/sched.h >>> @@ -114,8 +114,7 @@ struct evtchn >>> u16 virq; /* state == ECS_VIRQ */ >>> } u; >>> u8 priority; >>> -u8 last_priority; >>> -u16 last_vcpu_id; >>> +u32 fifo_lastq;/* Data for fifo events identifying last queue. */ >> >> This grows struct evtchn's size on at least 32-bit Arm. I'd >> like to suggest including "priority" in the union, and call the >> new field simply "fifo" or some such. > > This will add quite some complexity as suddenly all writes to the > union will need to be made through a cmpxchg() scheme. > > Regarding the growth: struct evtchn is aligned to 64 bytes. So there > is no growth at all, as the size will not be larger than those 64 > bytes. Oh, I didn't spot this attribute, which I consider at least suspicious. Without XSM I'm getting the impression that on 32-bit Arm the structure's size would be 32 bytes or less without it, so it looks as if it shouldn't be there unconditionally. Jan
Re: [PATCH v2 03/11] x86/vlapic: introduce an EOI callback mechanism
On Fri, Oct 02, 2020 at 11:39:50AM +0200, Jan Beulich wrote:
> On 30.09.2020 12:41, Roger Pau Monne wrote:
> > Add a new vlapic_set_irq_callback helper in order to inject a vector
> > and set a callback to be executed when the guest performs the end of
> > interrupt acknowledgment.
>
> On v1 I did ask
>
> "One thing I don't understand at all for now is how these
> callbacks are going to be re-instated after migration for
> not-yet-EOIed interrupts."
>
> Afaics I didn't get an answer on this.
Oh sorry, I remember your comment and I've changed further patches to
address this.
The setter of the callback will be in charge for setting the callback
again on resume. That's why vlapic_set_callback is not a static
function, and is added to the header.
Patch 5/11 [0] contains an example of hw the vIO-APIC resets the callbacks
on load.
>
> > ---
> > RFC: should callbacks also be executed in vlapic_do_init (which is
> > called by vlapic_reset). We would need to make sure ISR and IRR
> > are cleared using some kind of test and clear atomic functionality to
> > make this race free.
>
> I guess this can't be decided at this point of the series, as it
> may depend on what exactly the callbacks mean to do. It may even
> be that whether a callback wants to do something depends on
> whether it gets called "normally" or from vlapic_do_init().
Well, lets try to get some progress on the other questions and we will
eventually get here I think.
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -144,7 +144,32 @@ bool vlapic_test_irq(const struct vlapic *vlapic,
> > uint8_t vec)
> > return vlapic_test_vector(vec, &vlapic->regs->data[APIC_IRR]);
> > }
> >
> > -void vlapic_set_irq(struct vlapic *vlapic, uint8_t vec, uint8_t trig)
> > +void vlapic_set_callback(struct vlapic *vlapic, unsigned int vec,
> > + vlapic_eoi_callback_t *callback, void *data)
> > +{
> > +unsigned long flags;
> > +unsigned int index = vec - 16;
> > +
> > +if ( !callback || vec < 16 || vec >= X86_NR_VECTORS )
> > +{
> > +ASSERT_UNREACHABLE();
> > +return;
> > +}
> > +
> > +spin_lock_irqsave(&vlapic->callback_lock, flags);
> > +if ( vlapic->callbacks[index].callback &&
> > + vlapic->callbacks[index].callback != callback )
> > +printk(XENLOG_G_WARNING
> > + "%pv overriding vector %#x callback %ps (%p) with %ps
> > (%p)\n",
> > + vlapic_vcpu(vlapic), vec, vlapic->callbacks[index].callback,
> > + vlapic->callbacks[index].callback, callback, callback);
> > +vlapic->callbacks[index].callback = callback;
> > +vlapic->callbacks[index].data = data;
>
> Should "data" perhaps also be compared in the override check above?
Could do, there might indeed be cases where the callback is the same
but data has changed and should be interesting to log.
Thanks, Roger.
[0]
https://lore.kernel.org/xen-devel/20200930104108.35969-6-roger@citrix.com/
Re: [PATCH] x86/smpboot: Unconditionally call memguard_unguard_stack() in cpu_smpboot_free()
On 13.10.2020 15:23, Andrew Cooper wrote:
> On 13/10/2020 14:16, Jan Beulich wrote:
>> On 05.10.2020 14:23, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/smpboot.c
>>> +++ b/xen/arch/x86/smpboot.c
>>> @@ -971,16 +971,16 @@ static void cpu_smpboot_free(unsigned int cpu, bool
>>> remove)
>>> if ( IS_ENABLED(CONFIG_PV32) )
>>> FREE_XENHEAP_PAGE(per_cpu(compat_gdt, cpu));
>>>
>>> +if ( stack_base[cpu] )
>>> +memguard_unguard_stack(stack_base[cpu]);
>>> +
>>> if ( remove )
>>> {
>>> FREE_XENHEAP_PAGE(per_cpu(gdt, cpu));
>>> FREE_XENHEAP_PAGE(idt_tables[cpu]);
>>>
>>> if ( stack_base[cpu] )
>>> -{
>>> -memguard_unguard_stack(stack_base[cpu]);
>>> FREE_XENHEAP_PAGES(stack_base[cpu], STACK_ORDER);
>>> -}
>>> }
>>> }
>> In my initial reply to Marek's report I did suggest putting the fix
>> in the alloc path in order to keep the pages "guarded" while the CPU
>> is parked
>
> In which case you should have identified that bug explicitly.
>
> Because I can't read your mind while fixing the other real bugs in your
> suggestion.
I'm sorry for the brevity at that point - it was a Sunday, and I merely
thought I'd write down my observation after reading the report. And of
course I'm curious as to the other real bugs in my suggestion (when I
anyway said "something like").
Jan
Re: [PATCH v2 04/11] x86/vmsi: use the newly introduced EOI callbacks
On Fri, Oct 02, 2020 at 05:25:34PM +0200, Jan Beulich wrote:
> On 30.09.2020 12:41, Roger Pau Monne wrote:
> > Remove the unconditional call to hvm_dpci_msi_eoi in vlapic_handle_EOI
> > and instead use the newly introduced EOI callback mechanism in order
> > to register a callback for MSI vectors injected from passed through
> > devices.
>
> What I'm kind of missing here is a word on why this is an improvement:
> After all ...
>
> > --- a/xen/arch/x86/hvm/vlapic.c
> > +++ b/xen/arch/x86/hvm/vlapic.c
> > @@ -496,8 +496,6 @@ void vlapic_handle_EOI(struct vlapic *vlapic, u8 vector)
> > if ( vlapic_test_vector(vector, &vlapic->regs->data[APIC_TMR]) )
> > vioapic_update_EOI(vector);
> >
> > -hvm_dpci_msi_eoi(vector);
>
> ... you're exchanging this direct call for a more complex model with
> an indirect one (to the same function).
Sure. But this direct call will be made for each vlapic EOI, while my
added callback will only be executed if the vector was injected by
thee vmsi code, and hence will remove pointless calls to
hvm_dpci_msi_eoi.
It's IMO not feasible to be adding hardcoded calls to
vlapic_handle_EOI for each possible subsystem or emulated device that
wants to be notified of EOIs, hence we need some kind of generic
framework to achieve this.
> > @@ -119,7 +126,8 @@ void vmsi_deliver_pirq(struct domain *d, const struct
> > hvm_pirq_dpci *pirq_dpci)
> >
> > ASSERT(pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI);
> >
> > -vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode);
> > +vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode,
> > trig_mode,
> > + hvm_dpci_msi_eoi, NULL);
> > }
>
> While I agree with your reply to Paul regarding Dom0, I still think
> the entire if() in hvm_dpci_msi_eoi() should be converted into a
> conditional here. There's no point registering the callback if it's
> not going to do anything.
>
> However, looking further, the "!hvm_domain_irq(d)->dpci &&
> !is_hardware_domain(d)" can be simply dropped altogether, right away.
> It's now fulfilled by the identical check at the top of
> hvm_dirq_assist(), thus guarding the sole call site of this function.
>
> The !is_iommu_enabled(d) is slightly more involved to prove, but it
> should also be possible to simply drop. What might help here is a
> separate change to suppress opening of HVM_DPCI_SOFTIRQ when there's
> no IOMMU in the system, as then it becomes obvious that this part of
> the condition is guaranteed by hvm_do_IRQ_dpci(), being the only
> site where the softirq can get raised (apart from the softirq
> handler itself).
>
> To sum up - the call above can probably stay as is, but the callback
> can be simplified as a result of the change.
Yes, I agree. Would you be fine with converting the check in the
callback into an assert, or would you rather have it removed
completely?
> > --- a/xen/drivers/passthrough/io.c
> > +++ b/xen/drivers/passthrough/io.c
> > @@ -874,7 +874,7 @@ static int _hvm_dpci_msi_eoi(struct domain *d,
> > return 0;
> > }
> >
> > -void hvm_dpci_msi_eoi(unsigned int vector)
> > +void hvm_dpci_msi_eoi(unsigned int vector, void *data)
> > {
> > struct domain *d = current->domain;
>
> Instead of passing NULL for data and latching d from current, how
> about you make the registration pass d to more easily use here?
Yes, I think that's fine - we already have the domain pointer in
vmsi_deliver_callback so it could be passed as the callback private
data.
Thanks, Roger.
Re: [PATCH v2] build: always use BASEDIR for xen sub-directory
> On 13 Oct 2020, at 15:04, Jan Beulich wrote: > > On 07.10.2020 16:57, Bertrand Marquis wrote: >> Modify Makefiles using $(XEN_ROOT)/xen to use $(BASEDIR) instead. >> >> This is removing the dependency to xen subdirectory preventing using a >> wrong configuration file when xen subdirectory is duplicated for >> compilation tests. >> >> BASEDIR is set in xen/lib/x86/Makefile as this Makefile is directly >> called from the tools build and install process and BASEDIR is not set >> there. >> >> Signed-off-by: Bertrand Marquis > > And once again > Acked-by: Jan Beulich > And thanks :-) Bertrand
Re: [PATCH v2 2/2] xen/evtchn: rework per event channel lock
On 12.10.2020 11:27, Juergen Gross wrote:
> @@ -798,9 +786,11 @@ void send_guest_vcpu_virq(struct vcpu *v, uint32_t virq)
>
> d = v->domain;
> chn = evtchn_from_port(d, port);
> -spin_lock(&chn->lock);
> -evtchn_port_set_pending(d, v->vcpu_id, chn);
> -spin_unlock(&chn->lock);
> +if ( evtchn_tryread_lock(chn) )
> +{
> +evtchn_port_set_pending(d, v->vcpu_id, chn);
> +evtchn_read_unlock(chn);
> +}
>
> out:
> spin_unlock_irqrestore(&v->virq_lock, flags);
> @@ -829,9 +819,11 @@ void send_guest_global_virq(struct domain *d, uint32_t
> virq)
> goto out;
>
> chn = evtchn_from_port(d, port);
> -spin_lock(&chn->lock);
> -evtchn_port_set_pending(d, chn->notify_vcpu_id, chn);
> -spin_unlock(&chn->lock);
> +if ( evtchn_tryread_lock(chn) )
> +{
> +evtchn_port_set_pending(d, v->vcpu_id, chn);
Is this simply a copy-and-paste mistake (re-using the code from
send_guest_vcpu_virq()), or is there a reason you switch from
where to obtain the vCPU to send to (in which case this ought
to be mentioned in the description, and in which case you could
use literal zero)?
> --- a/xen/include/xen/event.h
> +++ b/xen/include/xen/event.h
> @@ -105,6 +105,45 @@ void notify_via_xen_event_channel(struct domain *ld, int
> lport);
> #define bucket_from_port(d, p) \
> ((group_from_port(d, p))[((p) % EVTCHNS_PER_GROUP) / EVTCHNS_PER_BUCKET])
>
> +#define EVENT_WRITE_LOCK_INCMAX_VIRT_CPUS
Isn't the ceiling on simultaneous readers the number of pCPU-s,
and the value here then needs to be NR_CPUS + 1 to accommodate
the maximum number of readers? Furthermore, with you dropping
the disabling of interrupts, one pCPU can acquire a read lock
now more than once, when interrupting a locked region.
> +static inline void evtchn_write_lock(struct evtchn *evtchn)
> +{
> +int val;
> +
> +/* No barrier needed, atomic_add_return() is full barrier. */
> +for ( val = atomic_add_return(EVENT_WRITE_LOCK_INC, &evtchn->lock);
> + val != EVENT_WRITE_LOCK_INC;
> + val = atomic_read(&evtchn->lock) )
> +cpu_relax();
> +}
> +
> +static inline void evtchn_write_unlock(struct evtchn *evtchn)
> +{
> +arch_lock_release_barrier();
> +
> +atomic_sub(EVENT_WRITE_LOCK_INC, &evtchn->lock);
> +}
> +
> +static inline bool evtchn_tryread_lock(struct evtchn *evtchn)
The corresponding "generic" function is read_trylock() - I'd
suggest to use the same base name, with the evtchn_ prefix.
> @@ -274,12 +312,12 @@ static inline int evtchn_port_poll(struct domain *d,
> evtchn_port_t port)
> if ( port_is_valid(d, port) )
> {
> struct evtchn *evtchn = evtchn_from_port(d, port);
> -unsigned long flags;
>
> -spin_lock_irqsave(&evtchn->lock, flags);
> -if ( evtchn_usable(evtchn) )
> +if ( evtchn_tryread_lock(evtchn) && evtchn_usable(evtchn) )
> +{
> rc = evtchn_is_pending(d, evtchn);
> -spin_unlock_irqrestore(&evtchn->lock, flags);
> +evtchn_read_unlock(evtchn);
> +}
> }
This needs to be two nested if()-s, as you need to drop the lock
even when evtchn_usable() returns false.
Jan
Re: [PATCH v2 2/2] xen/evtchn: rework per event channel lock
On 13.10.2020 16:13, Jürgen Groß wrote: > On 13.10.20 16:02, Jan Beulich wrote: >> On 12.10.2020 11:27, Juergen Gross wrote: >>> Currently the lock for a single event channel needs to be taken with >>> interrupts off, which causes deadlocks in some cases. >>> >>> Rework the per event channel lock to be non-blocking for the case of >>> sending an event and removing the need for disabling interrupts for >>> taking the lock. >>> >>> The lock is needed for avoiding races between sending an event or >>> querying the channel's state against removal of the event channel. >>> >>> Use a locking scheme similar to a rwlock, but with some modifications: >>> >>> - sending an event or querying the event channel's state uses an >>>operation similar to read_trylock(), in case of not obtaining the >>>lock the sending is omitted or a default state is returned >> >> And how come omitting the send or returning default state is valid? > > This is explained in the part of the commit message you didn't cite: > > With this locking scheme it is mandatory that a writer will always > either start with an unbound or free event channel or will end with > an unbound or free event channel, as otherwise the reaction of a reader > not getting the lock would be wrong. Oh, I did read this latter part as something extra to be aware of, not as this being the correctness guarantee. Could you make the connection more clear? Jan
Re: [PATCH] hvmloader: flip "ACPI data" to ACPI NVS type for ACPI table region
On 13.10.2020 14:59, Igor Druzhinin wrote: > On 13/10/2020 13:51, Jan Beulich wrote: >> As a consequence I think we will also want to adjust Xen itself to >> automatically disable ACPI when it ends up consuming E801 data. Or >> alternatively we should consider dropping all E801-related code (as >> being inapplicable to 64-bit systems). > > I'm not following here. What Xen has to do with E801? It's a SeaBIOS > implemented > call that happened to be used by QEMU option ROM. We cannot drop it from there > as it's part of BIOS spec. Any ACPI aware OS has to use E820 (and nothing else). Hence our own use of E801 should either be dropped, or lead to the disabling of ACPI. Otherwise real firmware using logic similar to SeaBIOS'es (but hopefully properly accounting for holes) could make us use ACPI table space as normal RAM. Jan
Re: [PATCH v2 03/11] x86/vlapic: introduce an EOI callback mechanism
On 13.10.2020 16:30, Roger Pau Monné wrote: > On Fri, Oct 02, 2020 at 11:39:50AM +0200, Jan Beulich wrote: >> On 30.09.2020 12:41, Roger Pau Monne wrote: >>> Add a new vlapic_set_irq_callback helper in order to inject a vector >>> and set a callback to be executed when the guest performs the end of >>> interrupt acknowledgment. >> >> On v1 I did ask >> >> "One thing I don't understand at all for now is how these >> callbacks are going to be re-instated after migration for >> not-yet-EOIed interrupts." >> >> Afaics I didn't get an answer on this. > > Oh sorry, I remember your comment and I've changed further patches to > address this. > > The setter of the callback will be in charge for setting the callback > again on resume. That's why vlapic_set_callback is not a static > function, and is added to the header. > > Patch 5/11 [0] contains an example of hw the vIO-APIC resets the callbacks > on load. Ah, I see - I didn't get there yet. Could you mention this behavior in the description here, or maybe in a code comment next to the declaration (or definition) of the function? Jan
Re: [PATCH v2 04/11] x86/vmsi: use the newly introduced EOI callbacks
On 13.10.2020 16:47, Roger Pau Monné wrote: > On Fri, Oct 02, 2020 at 05:25:34PM +0200, Jan Beulich wrote: >> On 30.09.2020 12:41, Roger Pau Monne wrote: >>> @@ -119,7 +126,8 @@ void vmsi_deliver_pirq(struct domain *d, const struct >>> hvm_pirq_dpci *pirq_dpci) >>> >>> ASSERT(pirq_dpci->flags & HVM_IRQ_DPCI_GUEST_MSI); >>> >>> -vmsi_deliver(d, vector, dest, dest_mode, delivery_mode, trig_mode); >>> +vmsi_deliver_callback(d, vector, dest, dest_mode, delivery_mode, >>> trig_mode, >>> + hvm_dpci_msi_eoi, NULL); >>> } >> >> While I agree with your reply to Paul regarding Dom0, I still think >> the entire if() in hvm_dpci_msi_eoi() should be converted into a >> conditional here. There's no point registering the callback if it's >> not going to do anything. >> >> However, looking further, the "!hvm_domain_irq(d)->dpci && >> !is_hardware_domain(d)" can be simply dropped altogether, right away. >> It's now fulfilled by the identical check at the top of >> hvm_dirq_assist(), thus guarding the sole call site of this function. >> >> The !is_iommu_enabled(d) is slightly more involved to prove, but it >> should also be possible to simply drop. What might help here is a >> separate change to suppress opening of HVM_DPCI_SOFTIRQ when there's >> no IOMMU in the system, as then it becomes obvious that this part of >> the condition is guaranteed by hvm_do_IRQ_dpci(), being the only >> site where the softirq can get raised (apart from the softirq >> handler itself). >> >> To sum up - the call above can probably stay as is, but the callback >> can be simplified as a result of the change. > > Yes, I agree. Would you be fine with converting the check in the > callback into an assert, or would you rather have it removed > completely? Either way is fine with me, I think. Jan
Re: [PATCH] hvmloader: flip "ACPI data" to ACPI NVS type for ACPI table region
On 13/10/2020 16:35, Jan Beulich wrote: > On 13.10.2020 14:59, Igor Druzhinin wrote: >> On 13/10/2020 13:51, Jan Beulich wrote: >>> As a consequence I think we will also want to adjust Xen itself to >>> automatically disable ACPI when it ends up consuming E801 data. Or >>> alternatively we should consider dropping all E801-related code (as >>> being inapplicable to 64-bit systems). >> >> I'm not following here. What Xen has to do with E801? It's a SeaBIOS >> implemented >> call that happened to be used by QEMU option ROM. We cannot drop it from >> there >> as it's part of BIOS spec. > > Any ACPI aware OS has to use E820 (and nothing else). Hence our > own use of E801 should either be dropped, or lead to the > disabling of ACPI. Otherwise real firmware using logic similar > to SeaBIOS'es (but hopefully properly accounting for holes) > could make us use ACPI table space as normal RAM. It's not us using it - it's a boot loader from QEMU in a form of option ROM that works in 16bit pre-OS environment which is not OS and relies on e801 BIOS call. I'm sure any ACPI aware OS does indeed use E820 but the problem here is not an OS. The option ROM is loaded using fw_cfg from QEMU so it's not our code. Technically it's one foreign code (QEMU boot loader) talking to another foreign code (SeaBIOS) which provides information based on E820 that we gave them. So I'm afraid decision to dynamically disable ACPI (whatever you mean by this) cannot be made by sole usage of this call by a pre-OS boot loader. Igor
Re: [PATCH] x86/traps: 'Fix' safety of read_registers() in #DF path
On 12.10.2020 15:49, Andrew Cooper wrote:
> All interrupts and exceptions pass a struct cpu_user_regs up into C. This
> contains the legacy vm86 fields from 32bit days, which are beyond the
> hardware-pushed frame.
>
> Accessing these fields is generally illegal, as they are logically out of
> bounds for anything other than an interrupt/exception hitting ring1/3 code.
>
> Unfortunately, the #DF handler uses these fields as part of preparing the
> state dump, and being IST, accesses the adjacent stack frame.
>
> This has been broken forever, but c/s 6001660473 "x86/shstk: Rework the stack
> layout to support shadow stacks" repositioned the #DF stack to be adjacent to
> the guard page, which turns this OoB write into a fatal pagefault:
>
> (XEN) *** DOUBLE FAULT ***
> (XEN) [ Xen-4.15-unstable x86_64 debug=y Tainted: C ]
> (XEN) [ Xen-4.15-unstable x86_64 debug=y Tainted: C ]
> (XEN) CPU:4
> (XEN) RIP:e008:[] traps.c#read_registers+0x29/0xc1
> (XEN) RFLAGS: 00050086 CONTEXT: hypervisor (d1v0)
> ...
> (XEN) Xen call trace:
> (XEN)[] R traps.c#read_registers+0x29/0xc1
> (XEN)[] F do_double_fault+0x3d/0x7e
> (XEN)[] F double_fault+0x107/0x110
> (XEN)
> (XEN) Pagetable walk from 830236f6d008:
> (XEN) L4[0x106] = 8000bfa9b063
> (XEN) L3[0x008] = 000236ffd063
> (XEN) L2[0x1b7] = 000236ffc063
> (XEN) L1[0x16d] = 800236f6d161
> (XEN)
> (XEN)
> (XEN) Panic on CPU 4:
> (XEN) FATAL PAGE FAULT
> (XEN) [error_code=0003]
> (XEN) Faulting linear address: 830236f6d008
> (XEN)
> (XEN)
>
> and rendering the main #DF analysis broken.
>
> The proper fix is to delete cpu_user_regs.es and later, so no
> interrupt/exception path can access OoB, but this needs disentangling from the
> PV ABI first.
>
> Not-really-fixes: 6001660473 ("x86/shstk: Rework the stack layout to support
> shadow stacks")
> Signed-off-by: Andrew Cooper
Reviewed-by: Jan Beulich
Is it perhaps worth also saying explicitly that the other IST
stacks don't suffer the same problem because show_registers()
makes an local copy of the passed in struct? (Doing so also
for #DF would apparently be an alternative solution.)
Jan
Re: [PATCH] hvmloader: flip "ACPI data" to ACPI NVS type for ACPI table region
On 13.10.2020 17:47, Igor Druzhinin wrote: > On 13/10/2020 16:35, Jan Beulich wrote: >> On 13.10.2020 14:59, Igor Druzhinin wrote: >>> On 13/10/2020 13:51, Jan Beulich wrote: As a consequence I think we will also want to adjust Xen itself to automatically disable ACPI when it ends up consuming E801 data. Or alternatively we should consider dropping all E801-related code (as being inapplicable to 64-bit systems). >>> >>> I'm not following here. What Xen has to do with E801? It's a SeaBIOS >>> implemented >>> call that happened to be used by QEMU option ROM. We cannot drop it from >>> there >>> as it's part of BIOS spec. >> >> Any ACPI aware OS has to use E820 (and nothing else). Hence our >> own use of E801 should either be dropped, or lead to the >> disabling of ACPI. Otherwise real firmware using logic similar >> to SeaBIOS'es (but hopefully properly accounting for holes) >> could make us use ACPI table space as normal RAM. > > It's not us using it - it's a boot loader from QEMU in a form of option ROM > that works in 16bit pre-OS environment which is not OS and relies on e801 > BIOS call. > I'm sure any ACPI aware OS does indeed use E820 but the problem here is not > an OS. > > The option ROM is loaded using fw_cfg from QEMU so it's not our code. > Technically > it's one foreign code (QEMU boot loader) talking to another foreign code > (SeaBIOS) > which provides information based on E820 that we gave them. > > So I'm afraid decision to dynamically disable ACPI (whatever you mean by this) > cannot be made by sole usage of this call by a pre-OS boot loader. I guess this is simply a misunderstanding. I'm not talking about your change or hvmloader or the boot loader at all. I was merely noticing a consequence of your findings on the behavior of Xen itself: Use of ACPI and use of E801 are exclusive of one another. Jan
Re: [PATCH] x86/vmx: Revert "x86/VMX: sanitize rIP before re-entering guest"
On 09.10.2020 17:09, Andrew Cooper wrote:
> At the time of XSA-170, the x86 instruction emulator really was broken, and
> would allow arbitrary non-canonical values to be loaded into %rip. This was
> fixed after the embargo by c/s 81d3a0b26c1 "x86emul: limit-check branch
> targets".
>
> However, in a demonstration that off-by-one errors really are one of the
> hardest programming issues we face, everyone involved with XSA-170, myself
> included, mistook the statement in the SDM which says:
>
> If the processor supports N < 64 linear-address bits, bits 63:N must be
> identical
>
> to mean "must be canonical". A real canonical check is bits 63:N-1.
>
> VMEntries really do tolerate a not-quite-canonical %rip, specifically to cater
> to the boundary condition at 0x8000.
>
> Now that the emulator has been fixed, revert the XSA-170 change to fix
> architectural behaviour at the boundary case. The XTF test case for XSA-170
> exercises this corner case, and still passes.
>
> Fixes: ffbbfda377 ("x86/VMX: sanitize rIP before re-entering guest")
> Signed-off-by: Andrew Cooper
But why revert the change rather than fix ...
> @@ -4280,38 +4280,6 @@ void vmx_vmexit_handler(struct cpu_user_regs *regs)
> out:
> if ( nestedhvm_vcpu_in_guestmode(v) )
> nvmx_idtv_handling();
> -
> -/*
> - * VM entry will fail (causing the guest to get crashed) if rIP (and
> - * rFLAGS, but we don't have an issue there) doesn't meet certain
> - * criteria. As we must not allow less than fully privileged mode to have
> - * such an effect on the domain, we correct rIP in that case (accepting
> - * this not being architecturally correct behavior, as the injected #GP
> - * fault will then not see the correct [invalid] return address).
> - * And since we know the guest will crash, we crash it right away if it
> - * already is in most privileged mode.
> - */
> -mode = vmx_guest_x86_mode(v);
> -if ( mode == 8 ? !is_canonical_address(regs->rip)
... the wrong use of is_canonical_address() here? By reverting
you open up avenues for XSAs in case we get things wrong elsewhere,
including ...
> - : regs->rip != regs->eip )
... for 32-bit guests.
Jan
[xen-unstable-smoke test] 155778: regressions - FAIL
flight 155778 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/155778/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 155584 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen a95f31376ba4ae911536c647e1a583d144ccab73 baseline version: xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9 Last test of basis 155584 2020-10-09 02:01:25 Z4 days Failing since155612 2020-10-09 18:01:22 Z3 days 29 attempts Testing same since 155778 2020-10-13 14:01:27 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Jan Beulich Jason Andryuk Juergen Gross Nick Rosbrook Nick Rosbrook Roger Pau Monné Trammell Hudson 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 fail 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 a95f31376ba4ae911536c647e1a583d144ccab73 Author: Nick Rosbrook Date: Sun Oct 11 19:31:25 2020 -0400 golang/xenlight: standardize generated code comment There is a standard format for generated Go code header comments, as set by [1]. Modify gengotypes.py to follow this standard, and use the additional // source: convention used by protoc-gen-go. This change is motivated by the fact that since 41aea82de2, the comment would include the absolute path to libxl_types.idl, therefore creating unintended diffs when generating code across different machines. This approach fixes that problem. [1] https://github.com/golang/go/issues/13560 Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit c60f9e4360ec857bb0164387378e12ae8e66e189 Author: Nick Rosbrook Date: Sun Oct 11 19:31:24 2020 -0400 golang/xenlight: do not hard code libxl dir in gengotypes.py Currently, in order to 'import idl' in gengotypes.py, we derive the path of the libxl source directory from the XEN_ROOT environment variable, and append that to sys.path so python can see idl.py. Since the the recent move of libxl to tools/libs/light, this hard coding breaks the build. Instead, check for the environment variable LIBXL_SRC_DIR, but move this check to a try-except block (with empty except). This simply makes the real error more visible, and does not strictly require that LIBXL_SRC_DIR is used. Finally, update the Makefile to set LIBXL_SRC_DIR rather than XEN_ROOT when calling gengotypes.py. Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit 534b3d09958fdc4df64872c2ab19feb4b1eebc5a Author: Juergen Gross Date: Sun Oct 11 14:24:01 2020 +0200 tools/libs/store: add disclaimer to header file regarding ignored options Add a disclaimer to the libxenstore header file that all of the open flags (socket only connection, read only connection) are ignored. Signed-off-by: Juergen Gross Acked-by: Wei Liu commit 1b810a9d5a39230e76073b1a753cd2c34ded65fc Author: Jason Andryuk Date: Thu Oct 1 19:53:37 2020 -0400 libxl: only query VNC when enabled QEMU without VNC support (configure --disable-vnc) will return an error when VNC is queried over QMP since it does not recognize the QMP command. This will cause libxl to fail starting the domain even if VNC is not enabled. Therefore only query QEMU for VNC support when using VNC, s
Re: [PATCH v2 0/3] Add Xen CpusAccel
On 13/10/20 16:05, Jason Andryuk wrote:
> Xen was left behind when CpusAccel became mandatory and fails the assert
> in qemu_init_vcpu(). It relied on the same dummy cpu threads as qtest.
> Move the qtest cpu functions to a common location and reuse them for
> Xen.
>
> v2:
> New patch "accel: Remove _WIN32 ifdef from qtest-cpus.c"
> Use accel/dummy-cpus.c for filename
> Put prototype in include/sysemu/cpus.h
>
> Jason Andryuk (3):
> accel: Remove _WIN32 ifdef from qtest-cpus.c
> accel: move qtest CpusAccel functions to a common location
> accel: Add xen CpusAccel using dummy-cpus
>
> accel/{qtest/qtest-cpus.c => dummy-cpus.c} | 27 --
> accel/meson.build | 8 +++
> accel/qtest/meson.build| 1 -
> accel/qtest/qtest-cpus.h | 17 --
> accel/qtest/qtest.c| 5 +++-
> accel/xen/xen-all.c| 8 +++
> include/sysemu/cpus.h | 3 +++
> 7 files changed, 27 insertions(+), 42 deletions(-)
> rename accel/{qtest/qtest-cpus.c => dummy-cpus.c} (71%)
> delete mode 100644 accel/qtest/qtest-cpus.h
>
Acked-by: Paolo Bonzini
[xen-unstable test] 155759: tolerable FAIL
flight 155759 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/155759/ Failures :-/ but no regressions. Tests which are failing intermittently (not blocking): test-amd64-amd64-libvirt-vhd 16 guest-saverestore fail in 155712 pass in 155759 test-amd64-amd64-pair 26 guest-migrate/src_host/dst_host fail pass in 155712 test-amd64-i386-xl-qemut-win7-amd64 12 windows-install fail pass in 155712 test-amd64-amd64-examine 4 memdisk-try-append fail pass in 155712 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemut-win7-amd64 19 guest-stop fail in 155712 like 155673 test-armhf-armhf-libvirt 16 saverestore-support-checkfail like 155712 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 155712 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 155712 test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 155712 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 155712 test-armhf-armhf-libvirt-raw 15 saverestore-support-checkfail like 155712 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 155712 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 155712 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 15 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 16 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 15 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 16 saverestore-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 19 guest-stop fail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 15 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 14 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail never pass version targeted for testing: xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9 baseline version: xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9 Last test of basis 155759 2020-10-13 01:53:11 Z0 days Testing same since (not found) 0 attempts jobs: build-amd64-x
[ovmf test] 155765: all pass - PUSHED
flight 155765 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/155765/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 9380177354387f03c8ff9eadb7ae94aa453b9469 baseline version: ovmf 5d1af380d3e4bd840fa324db33ca4f739136e654 Last test of basis 155757 2020-10-13 01:44:44 Z0 days Testing same since 155765 2020-10-13 06:07:35 Z0 days1 attempts People who touched revisions under test: fengyunhua Jan Bobek Liming Gao Michael D Kinney Michael Kubacki Yunhua Feng jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 pass test-amd64-i386-xl-qemuu-ovmf-amd64 pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/ovmf.git 5d1af380d3..9380177354 9380177354387f03c8ff9eadb7ae94aa453b9469 -> xen-tested-master
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Fri, 9 Oct 2020, ira.we...@intel.com wrote:
> From: Ira Weiny
>
> The kmap() calls in this FS are localized to a single thread. To avoid
> the over head of global PKRS updates use the new kmap_thread() call.
>
> Cc: Nicolas Pitre
> Signed-off-by: Ira Weiny
Acked-by: Nicolas Pitre
> fs/cramfs/inode.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 912308600d39..003c014a42ed 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -247,8 +247,8 @@ static void *cramfs_blkdev_read(struct super_block *sb,
> unsigned int offset,
> struct page *page = pages[i];
>
> if (page) {
> - memcpy(data, kmap(page), PAGE_SIZE);
> - kunmap(page);
> + memcpy(data, kmap_thread(page), PAGE_SIZE);
> + kunmap_thread(page);
> put_page(page);
> } else
> memset(data, 0, PAGE_SIZE);
> @@ -826,7 +826,7 @@ static int cramfs_readpage(struct file *file, struct page
> *page)
>
> maxblock = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> bytes_filled = 0;
> - pgdata = kmap(page);
> + pgdata = kmap_thread(page);
>
> if (page->index < maxblock) {
> struct super_block *sb = inode->i_sb;
> @@ -914,13 +914,13 @@ static int cramfs_readpage(struct file *file, struct
> page *page)
>
> memset(pgdata + bytes_filled, 0, PAGE_SIZE - bytes_filled);
> flush_dcache_page(page);
> - kunmap(page);
> + kunmap_thread(page);
> SetPageUptodate(page);
> unlock_page(page);
> return 0;
>
> err:
> - kunmap(page);
> + kunmap_thread(page);
> ClearPageUptodate(page);
> SetPageError(page);
> unlock_page(page);
> --
> 2.28.0.rc0.12.gb6a658bd00c9
>
>
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Fri, Oct 9, 2020 at 12:52 PM wrote:
>
> From: Ira Weiny
>
> The kmap() calls in this FS are localized to a single thread. To avoid
> the over head of global PKRS updates use the new kmap_thread() call.
>
> Cc: Nicolas Pitre
> Signed-off-by: Ira Weiny
> ---
> fs/cramfs/inode.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 912308600d39..003c014a42ed 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -247,8 +247,8 @@ static void *cramfs_blkdev_read(struct super_block *sb,
> unsigned int offset,
> struct page *page = pages[i];
>
> if (page) {
> - memcpy(data, kmap(page), PAGE_SIZE);
> - kunmap(page);
> + memcpy(data, kmap_thread(page), PAGE_SIZE);
> + kunmap_thread(page);
Why does this need a sleepable kmap? This looks like a textbook
kmap_atomic() use case.
[PATCH] hw/xen: Set suppress-vmdesc for Xen machines
xen-save-devices-state doesn't currently generate a vmdesc, so restore always triggers "Expected vmdescription section, but got 0". This is not a problem when restore comes from a file. However, when QEMU runs in a linux stubdom and comes over a console, EOF is not received. This causes a delay restoring - though it does restore. Setting suppress-vmdesc skips looking for the vmdesc during restore and avoids the wait. The other approach would be generate a vmdesc in qemu_save_device_state. Since COLO shared that function, and the vmdesc is just discarded on restore, we choose to skip it. Reported-by: Marek Marczykowski-Górecki Signed-off-by: Jason Andryuk --- hw/i386/pc_piix.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c index 3c2ae0612b..0cf22a57ad 100644 --- a/hw/i386/pc_piix.c +++ b/hw/i386/pc_piix.c @@ -987,7 +987,7 @@ static void xenfv_4_2_machine_options(MachineClass *m) pc_i440fx_4_2_machine_options(m); m->desc = "Xen Fully-virtualized PC"; m->max_cpus = HVM_MAX_VCPUS; -m->default_machine_opts = "accel=xen"; +m->default_machine_opts = "accel=xen,suppress-vmdesc=on"; } DEFINE_PC_MACHINE(xenfv_4_2, "xenfv-4.2", pc_xen_hvm_init, @@ -999,7 +999,7 @@ static void xenfv_3_1_machine_options(MachineClass *m) m->desc = "Xen Fully-virtualized PC"; m->alias = "xenfv"; m->max_cpus = HVM_MAX_VCPUS; -m->default_machine_opts = "accel=xen"; +m->default_machine_opts = "accel=xen,suppress-vmdesc=on"; } DEFINE_PC_MACHINE(xenfv, "xenfv-3.1", pc_xen_hvm_init, -- 2.25.1
[xen-unstable-smoke test] 155779: regressions - FAIL
flight 155779 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/155779/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 155584 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 9e5a9d0e6886f521453a63a2854ff6d06fa0d028 baseline version: xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9 Last test of basis 155584 2020-10-09 02:01:25 Z4 days Failing since155612 2020-10-09 18:01:22 Z4 days 30 attempts Testing same since 155779 2020-10-13 17:01:26 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Bertrand Marquis Jan Beulich Jason Andryuk Juergen Gross Nick Rosbrook Nick Rosbrook Roger Pau Monné Trammell Hudson 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 fail 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 9e5a9d0e6886f521453a63a2854ff6d06fa0d028 Author: Bertrand Marquis Date: Wed Oct 7 15:57:51 2020 +0100 build: always use BASEDIR for xen sub-directory Modify Makefiles using $(XEN_ROOT)/xen to use $(BASEDIR) instead. This is removing the dependency to xen subdirectory preventing using a wrong configuration file when xen subdirectory is duplicated for compilation tests. BASEDIR is set in xen/lib/x86/Makefile as this Makefile is directly called from the tools build and install process and BASEDIR is not set there. Signed-off-by: Bertrand Marquis Acked-by: Jan Beulich Acked-by: Wei Liu commit a95f31376ba4ae911536c647e1a583d144ccab73 Author: Nick Rosbrook Date: Sun Oct 11 19:31:25 2020 -0400 golang/xenlight: standardize generated code comment There is a standard format for generated Go code header comments, as set by [1]. Modify gengotypes.py to follow this standard, and use the additional // source: convention used by protoc-gen-go. This change is motivated by the fact that since 41aea82de2, the comment would include the absolute path to libxl_types.idl, therefore creating unintended diffs when generating code across different machines. This approach fixes that problem. [1] https://github.com/golang/go/issues/13560 Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit c60f9e4360ec857bb0164387378e12ae8e66e189 Author: Nick Rosbrook Date: Sun Oct 11 19:31:24 2020 -0400 golang/xenlight: do not hard code libxl dir in gengotypes.py Currently, in order to 'import idl' in gengotypes.py, we derive the path of the libxl source directory from the XEN_ROOT environment variable, and append that to sys.path so python can see idl.py. Since the the recent move of libxl to tools/libs/light, this hard coding breaks the build. Instead, check for the environment variable LIBXL_SRC_DIR, but move this check to a try-except block (with empty except). This simply makes the real error more visible, and does not strictly require that LIBXL_SRC_DIR is used. Finally, update the Makefile to set LIBXL_SRC_DIR rather than XEN_ROOT when calling gengotypes.py. Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit 534b3d09958fdc4df64872c2ab19feb4b1eebc5a Author: Juergen Gross Date: Sun Oct 11 14:24:01 2020 +0200 tools/libs/store: add disclaimer to header file regardin
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Tue, Oct 13, 2020 at 11:44:29AM -0700, Dan Williams wrote:
> On Fri, Oct 9, 2020 at 12:52 PM wrote:
> >
> > From: Ira Weiny
> >
> > The kmap() calls in this FS are localized to a single thread. To avoid
> > the over head of global PKRS updates use the new kmap_thread() call.
> >
> > Cc: Nicolas Pitre
> > Signed-off-by: Ira Weiny
> > ---
> > fs/cramfs/inode.c | 10 +-
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> > index 912308600d39..003c014a42ed 100644
> > --- a/fs/cramfs/inode.c
> > +++ b/fs/cramfs/inode.c
> > @@ -247,8 +247,8 @@ static void *cramfs_blkdev_read(struct super_block *sb,
> > unsigned int offset,
> > struct page *page = pages[i];
> >
> > if (page) {
> > - memcpy(data, kmap(page), PAGE_SIZE);
> > - kunmap(page);
> > + memcpy(data, kmap_thread(page), PAGE_SIZE);
> > + kunmap_thread(page);
>
> Why does this need a sleepable kmap? This looks like a textbook
> kmap_atomic() use case.
There's a lot of code of this form. Could we perhaps have:
static inline void copy_to_highpage(struct page *to, void *vfrom, unsigned int
size)
{
char *vto = kmap_atomic(to);
memcpy(vto, vfrom, size);
kunmap_atomic(vto);
}
in linux/highmem.h ?
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Tue, Oct 13, 2020 at 12:37 PM Matthew Wilcox wrote:
>
> On Tue, Oct 13, 2020 at 11:44:29AM -0700, Dan Williams wrote:
> > On Fri, Oct 9, 2020 at 12:52 PM wrote:
> > >
> > > From: Ira Weiny
> > >
> > > The kmap() calls in this FS are localized to a single thread. To avoid
> > > the over head of global PKRS updates use the new kmap_thread() call.
> > >
> > > Cc: Nicolas Pitre
> > > Signed-off-by: Ira Weiny
> > > ---
> > > fs/cramfs/inode.c | 10 +-
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> > > index 912308600d39..003c014a42ed 100644
> > > --- a/fs/cramfs/inode.c
> > > +++ b/fs/cramfs/inode.c
> > > @@ -247,8 +247,8 @@ static void *cramfs_blkdev_read(struct super_block
> > > *sb, unsigned int offset,
> > > struct page *page = pages[i];
> > >
> > > if (page) {
> > > - memcpy(data, kmap(page), PAGE_SIZE);
> > > - kunmap(page);
> > > + memcpy(data, kmap_thread(page), PAGE_SIZE);
> > > + kunmap_thread(page);
> >
> > Why does this need a sleepable kmap? This looks like a textbook
> > kmap_atomic() use case.
>
> There's a lot of code of this form. Could we perhaps have:
>
> static inline void copy_to_highpage(struct page *to, void *vfrom, unsigned
> int size)
> {
> char *vto = kmap_atomic(to);
>
> memcpy(vto, vfrom, size);
> kunmap_atomic(vto);
> }
>
> in linux/highmem.h ?
Nice, yes, that could also replace the local ones in lib/iov_iter.c
(memcpy_{to,from}_page())
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Tue, Oct 13, 2020 at 08:36:43PM +0100, Matthew Wilcox wrote:
> static inline void copy_to_highpage(struct page *to, void *vfrom, unsigned
> int size)
> {
> char *vto = kmap_atomic(to);
>
> memcpy(vto, vfrom, size);
> kunmap_atomic(vto);
> }
>
> in linux/highmem.h ?
You mean, like
static void memcpy_from_page(char *to, struct page *page, size_t offset, size_t
len)
{
char *from = kmap_atomic(page);
memcpy(to, from + offset, len);
kunmap_atomic(from);
}
static void memcpy_to_page(struct page *page, size_t offset, const char *from,
size_t len)
{
char *to = kmap_atomic(page);
memcpy(to + offset, from, len);
kunmap_atomic(to);
}
static void memzero_page(struct page *page, size_t offset, size_t len)
{
char *addr = kmap_atomic(page);
memset(addr + offset, 0, len);
kunmap_atomic(addr);
}
in lib/iov_iter.c? FWIW, I don't like that "highpage" in the name and
highmem.h as location - these make perfect sense regardless of highmem;
they are normal memory operations with page + offset used instead of
a pointer...
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Tue, Oct 13, 2020 at 08:36:43PM +0100, Matthew Wilcox wrote:
> On Tue, Oct 13, 2020 at 11:44:29AM -0700, Dan Williams wrote:
> > On Fri, Oct 9, 2020 at 12:52 PM wrote:
> > >
> > > From: Ira Weiny
> > >
> > > The kmap() calls in this FS are localized to a single thread. To avoid
> > > the over head of global PKRS updates use the new kmap_thread() call.
> > >
> > > Cc: Nicolas Pitre
> > > Signed-off-by: Ira Weiny
> > > ---
> > > fs/cramfs/inode.c | 10 +-
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> > > index 912308600d39..003c014a42ed 100644
> > > --- a/fs/cramfs/inode.c
> > > +++ b/fs/cramfs/inode.c
> > > @@ -247,8 +247,8 @@ static void *cramfs_blkdev_read(struct super_block
> > > *sb, unsigned int offset,
> > > struct page *page = pages[i];
> > >
> > > if (page) {
> > > - memcpy(data, kmap(page), PAGE_SIZE);
> > > - kunmap(page);
> > > + memcpy(data, kmap_thread(page), PAGE_SIZE);
> > > + kunmap_thread(page);
> >
> > Why does this need a sleepable kmap? This looks like a textbook
> > kmap_atomic() use case.
>
> There's a lot of code of this form. Could we perhaps have:
>
> static inline void copy_to_highpage(struct page *to, void *vfrom, unsigned
> int size)
> {
> char *vto = kmap_atomic(to);
>
> memcpy(vto, vfrom, size);
> kunmap_atomic(vto);
> }
>
> in linux/highmem.h ?
Christoph had the same idea. I'll work on it.
Ira
Re: [PATCH RFC PKS/PMEM 33/58] fs/cramfs: Utilize new kmap_thread()
On Tue, Oct 13, 2020 at 09:01:49PM +0100, Al Viro wrote:
> On Tue, Oct 13, 2020 at 08:36:43PM +0100, Matthew Wilcox wrote:
>
> > static inline void copy_to_highpage(struct page *to, void *vfrom, unsigned
> > int size)
> > {
> > char *vto = kmap_atomic(to);
> >
> > memcpy(vto, vfrom, size);
> > kunmap_atomic(vto);
> > }
> >
> > in linux/highmem.h ?
>
> You mean, like
> static void memcpy_from_page(char *to, struct page *page, size_t offset,
> size_t len)
> {
> char *from = kmap_atomic(page);
> memcpy(to, from + offset, len);
> kunmap_atomic(from);
> }
>
> static void memcpy_to_page(struct page *page, size_t offset, const char
> *from, size_t len)
> {
> char *to = kmap_atomic(page);
> memcpy(to + offset, from, len);
> kunmap_atomic(to);
> }
>
> static void memzero_page(struct page *page, size_t offset, size_t len)
> {
> char *addr = kmap_atomic(page);
> memset(addr + offset, 0, len);
> kunmap_atomic(addr);
> }
>
> in lib/iov_iter.c? FWIW, I don't like that "highpage" in the name and
> highmem.h as location - these make perfect sense regardless of highmem;
> they are normal memory operations with page + offset used instead of
> a pointer...
I was thinking along those lines as well especially because of the direction
this patch set takes kmap().
Thanks for pointing these out to me. How about I lift them to a common header?
But if not highmem.h where?
Ira
Re: [PATCH RFC PKS/PMEM 24/58] fs/freevxfs: Utilize new kmap_thread()
On Tue, Oct 13, 2020 at 12:25:44PM +0100, Christoph Hellwig wrote: > > - kaddr = kmap(pp); > > + kaddr = kmap_thread(pp); > > memcpy(kaddr, vip->vii_immed.vi_immed + offset, PAGE_SIZE); > > - kunmap(pp); > > + kunmap_thread(pp); > > You only Cced me on this particular patch, which means I have absolutely > no idea what kmap_thread and kunmap_thread actually do, and thus can't > provide an informed review. Sorry the list was so big I struggled with who to CC and on which patches. > > That being said I think your life would be a lot easier if you add > helpers for the above code sequence and its counterpart that copies > to a potential hughmem page first, as that hides the implementation > details from most users. Matthew Wilcox and Al Viro have suggested similar ideas. https://lore.kernel.org/lkml/20201013205012.gi2046...@iweiny-desk2.sc.intel.com/ Ira
[qemu-mainline test] 155769: regressions - FAIL
flight 155769 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/155769/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-amd64-qemuu-freebsd11-amd64 13 guest-startfail REGR. vs. 152631 test-amd64-amd64-qemuu-freebsd12-amd64 13 guest-startfail REGR. vs. 152631 test-amd64-i386-qemuu-rhel6hvm-amd 12 redhat-install fail REGR. vs. 152631 test-amd64-amd64-qemuu-nested-intel 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-freebsd10-i386 13 guest-startfail REGR. vs. 152631 test-armhf-armhf-xl-vhd 12 debian-di-installfail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-qemuu-rhel6hvm-intel 12 redhat-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail REGR. vs. 152631 test-arm64-arm64-libvirt-xsm 14 guest-start fail REGR. vs. 152631 test-amd64-amd64-qemuu-nested-amd 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-win7-amd64 12 windows-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-debianhvm-amd64 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-ovmf-amd64 12 debian-hvm-install fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-win7-amd64 12 windows-install fail REGR. vs. 152631 test-armhf-armhf-libvirt-raw 12 debian-di-installfail REGR. vs. 152631 test-amd64-i386-xl-qemuu-ws16-amd64 12 windows-install fail REGR. vs. 152631 test-amd64-amd64-xl-qemuu-ws16-amd64 12 windows-install fail REGR. vs. 152631 test-amd64-i386-freebsd10-amd64 13 guest-start fail REGR. vs. 152631 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 12 debian-hvm-install fail REGR. vs. 152631 test-armhf-armhf-libvirt 14 guest-start fail REGR. vs. 152631 Tests which are failing intermittently (not blocking): test-amd64-amd64-xl-qcow2 12 debian-di-install fail in 155754 pass in 155769 test-amd64-amd64-libvirt-vhd 19 guest-start/debian.repeat fail pass in 155754 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-rtds 18 guest-start/debian.repeat fail in 155754 like 152631 test-amd64-i386-xl-pvshim14 guest-start fail never pass test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 15 migrate-support-checkfail never pass test-amd64-i386-libvirt 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 15 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 16 saverestore-support-checkfail never pass test-arm64-arm64-xl 15 migrate-support-checkfail never pass test-arm64-arm64-xl 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 15 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 15 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 16 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 15 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 15 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 16 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 14 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 15 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 15 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 16 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 15 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 16 saverestore-support-checkf
[xen-unstable-smoke test] 155782: regressions - FAIL
flight 155782 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/155782/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 155584 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 9e5a9d0e6886f521453a63a2854ff6d06fa0d028 baseline version: xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9 Last test of basis 155584 2020-10-09 02:01:25 Z4 days Failing since155612 2020-10-09 18:01:22 Z4 days 31 attempts Testing same since 155779 2020-10-13 17:01:26 Z0 days2 attempts People who touched revisions under test: Andrew Cooper Bertrand Marquis Jan Beulich Jason Andryuk Juergen Gross Nick Rosbrook Nick Rosbrook Roger Pau Monné Trammell Hudson 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 fail 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 9e5a9d0e6886f521453a63a2854ff6d06fa0d028 Author: Bertrand Marquis Date: Wed Oct 7 15:57:51 2020 +0100 build: always use BASEDIR for xen sub-directory Modify Makefiles using $(XEN_ROOT)/xen to use $(BASEDIR) instead. This is removing the dependency to xen subdirectory preventing using a wrong configuration file when xen subdirectory is duplicated for compilation tests. BASEDIR is set in xen/lib/x86/Makefile as this Makefile is directly called from the tools build and install process and BASEDIR is not set there. Signed-off-by: Bertrand Marquis Acked-by: Jan Beulich Acked-by: Wei Liu commit a95f31376ba4ae911536c647e1a583d144ccab73 Author: Nick Rosbrook Date: Sun Oct 11 19:31:25 2020 -0400 golang/xenlight: standardize generated code comment There is a standard format for generated Go code header comments, as set by [1]. Modify gengotypes.py to follow this standard, and use the additional // source: convention used by protoc-gen-go. This change is motivated by the fact that since 41aea82de2, the comment would include the absolute path to libxl_types.idl, therefore creating unintended diffs when generating code across different machines. This approach fixes that problem. [1] https://github.com/golang/go/issues/13560 Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit c60f9e4360ec857bb0164387378e12ae8e66e189 Author: Nick Rosbrook Date: Sun Oct 11 19:31:24 2020 -0400 golang/xenlight: do not hard code libxl dir in gengotypes.py Currently, in order to 'import idl' in gengotypes.py, we derive the path of the libxl source directory from the XEN_ROOT environment variable, and append that to sys.path so python can see idl.py. Since the the recent move of libxl to tools/libs/light, this hard coding breaks the build. Instead, check for the environment variable LIBXL_SRC_DIR, but move this check to a try-except block (with empty except). This simply makes the real error more visible, and does not strictly require that LIBXL_SRC_DIR is used. Finally, update the Makefile to set LIBXL_SRC_DIR rather than XEN_ROOT when calling gengotypes.py. Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit 534b3d09958fdc4df64872c2ab19feb4b1eebc5a Author: Juergen Gross Date: Sun Oct 11 14:24:01 2020 +0200 tools/libs/store: add disclaimer to header file regardin
[seabios test] 155770: tolerable FAIL - PUSHED
flight 155770 seabios real [real] http://logs.test-lab.xenproject.org/osstest/logs/155770/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 155136 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 155136 test-amd64-i386-xl-qemuu-win7-amd64 19 guest-stop fail like 155136 test-amd64-i386-xl-qemuu-ws16-amd64 19 guest-stop fail like 155136 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 13 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 20 debian-hvm-install/l1/l2 fail never pass version targeted for testing: seabios c685fe3ff2d402caefc1487d99bb486c4a510b8b baseline version: seabios 849c5e50b6f474df6cc113130575bcdccfafcd9e Last test of basis 155136 2020-09-30 11:09:37 Z 13 days Testing same since 155770 2020-10-13 09:10:37 Z0 days1 attempts People who touched revisions under test: Gerd Hoffmann jobs: build-amd64-xsm pass build-i386-xsm pass build-amd64 pass build-i386 pass build-amd64-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm pass test-amd64-i386-xl-qemuu-debianhvm-i386-xsm pass test-amd64-amd64-qemuu-nested-amdfail test-amd64-i386-qemuu-rhel6hvm-amd pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-i386-xl-qemuu-debianhvm-amd64 pass test-amd64-amd64-qemuu-freebsd11-amd64 pass test-amd64-amd64-qemuu-freebsd12-amd64 pass test-amd64-amd64-xl-qemuu-win7-amd64 fail test-amd64-i386-xl-qemuu-win7-amd64 fail test-amd64-amd64-xl-qemuu-ws16-amd64 fail test-amd64-i386-xl-qemuu-ws16-amd64 fail test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrictpass test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict pass test-amd64-amd64-qemuu-nested-intel pass test-amd64-i386-qemuu-rhel6hvm-intel pass test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow pass test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/osstest/seabios.git 849c5e5..c685fe3 c685fe3ff2d402caefc1487d99bb486c4a510b8b -> xen-tested-master
Re: [PATCH] hvmloader: flip "ACPI data" to ACPI NVS type for ACPI table region
On 13/10/2020 16:54, Jan Beulich wrote: > On 13.10.2020 17:47, Igor Druzhinin wrote: >> On 13/10/2020 16:35, Jan Beulich wrote: >>> On 13.10.2020 14:59, Igor Druzhinin wrote: On 13/10/2020 13:51, Jan Beulich wrote: > As a consequence I think we will also want to adjust Xen itself to > automatically disable ACPI when it ends up consuming E801 data. Or > alternatively we should consider dropping all E801-related code (as > being inapplicable to 64-bit systems). I'm not following here. What Xen has to do with E801? It's a SeaBIOS implemented call that happened to be used by QEMU option ROM. We cannot drop it from there as it's part of BIOS spec. >>> >>> Any ACPI aware OS has to use E820 (and nothing else). Hence our >>> own use of E801 should either be dropped, or lead to the >>> disabling of ACPI. Otherwise real firmware using logic similar >>> to SeaBIOS'es (but hopefully properly accounting for holes) >>> could make us use ACPI table space as normal RAM. >> >> It's not us using it - it's a boot loader from QEMU in a form of option ROM >> that works in 16bit pre-OS environment which is not OS and relies on e801 >> BIOS call. >> I'm sure any ACPI aware OS does indeed use E820 but the problem here is not >> an OS. >> >> The option ROM is loaded using fw_cfg from QEMU so it's not our code. >> Technically >> it's one foreign code (QEMU boot loader) talking to another foreign code >> (SeaBIOS) >> which provides information based on E820 that we gave them. >> >> So I'm afraid decision to dynamically disable ACPI (whatever you mean by >> this) >> cannot be made by sole usage of this call by a pre-OS boot loader. > > I guess this is simply a misunderstanding. I'm not talking about > your change or hvmloader or the boot loader at all. I was merely > noticing a consequence of your findings on the behavior of Xen > itself: Use of ACPI and use of E801 are exclusive of one another. Sorry, yes. I forgot e801 is also used by Xen as an alternative to e820. Igor
Re: [PATCH 0/4] xen/arm: Unbreak ACPI
On Mon, 12 Oct 2020, Elliott Mitchell wrote: > On Mon, Oct 12, 2020 at 12:02:14PM -0700, Stefano Stabellini wrote: > > On Sat, 10 Oct 2020, Julien Grall wrote: > > > Therefore, I think the code should not try to find the STAO. Instead, it > > > should check whether the SPCR table is present. > > > > Yes, that makes sense, but that brings me to the next question. > > > > SPCR seems to be required by SBBR, however, Masami wrote that he could > > boot on a system without SPCR, which gets me very confused for two > > reasons: > > > > 1) Why there is no SPCR? Isn't it supposed to be mandatory? Is it > > because there no UART on Masami's system? > > I'm on different hardware, but some folks have setup Tianocore for it. > According to Documentation/arm64/acpi_object_usage.rst, > "Required: DSDT, FADT, GTDT, MADT, MCFG, RSDP, SPCR, XSDT". Yet when > booting a Linux kernel directly on the hardware it lists APIC, BGRT, > CSRT, DSDT, DBG2, FACP, GTDT, PPTT, RSDP, and XSDT. > > I don't know whether Linux's ACPI code omits mention of some required > tables and merely panics if they're absent. Yet I'm speculating the list > of required tables has shrunk, SPCR is no longer required, and the > documentation is out of date. Perhaps SPCR was required in early Linux > ACPI implementations, but more recent ones removed that requirement? I have just checked and SPCR is still a mandatory table in the latest SBBR specification. It is probably one of those cases where the firmware claims to be SBBR compliant, but it is not, and it happens to work with Linux.
Re: [PATCH 0/4] xen/arm: Unbreak ACPI
On Tue, Oct 13, 2020 at 06:06:26PM -0700, Stefano Stabellini wrote: > On Mon, 12 Oct 2020, Elliott Mitchell wrote: > > I'm on different hardware, but some folks have setup Tianocore for it. > > According to Documentation/arm64/acpi_object_usage.rst, > > "Required: DSDT, FADT, GTDT, MADT, MCFG, RSDP, SPCR, XSDT". Yet when > > booting a Linux kernel directly on the hardware it lists APIC, BGRT, > > CSRT, DSDT, DBG2, FACP, GTDT, PPTT, RSDP, and XSDT. > > > > I don't know whether Linux's ACPI code omits mention of some required > > tables and merely panics if they're absent. Yet I'm speculating the list > > of required tables has shrunk, SPCR is no longer required, and the > > documentation is out of date. Perhaps SPCR was required in early Linux > > ACPI implementations, but more recent ones removed that requirement? > > I have just checked and SPCR is still a mandatory table in the latest > SBBR specification. It is probably one of those cases where the firmware > claims to be SBBR compliant, but it is not, and it happens to work with > Linux. Is meeting the SBBR specification supposed to be a requirement of running Xen-ARM? I don't seen any mention of such. `find docs xen/arch/arm -type f -print0 | xargs -0 grep -eSBBR` produces no output. Perhaps you've been adding this as a presumptive requirement since previously the only hardware capable of running Xen due to an appropriately unlocked bootloader was SBBR compliant? If so, it seems time to either add this as an explicit requirement and document it, or else remove this implicit requirement and start acting as such. The Raspberry PI 4B has a UEFI implementation available which is based on Tianocore. No statement has been made of it qualifying as SBBR. Yet it is clearly mostly able to boot Xen, just this is exposing issues. -- (\___(\___(\__ --=> 8-) EHM <=-- __/)___/)___/) \BS (| ehem+sig...@m5p.com PGP 87145445 |) / \_CS\ | _ -O #include O- _ | / _/ 8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445
[xen-unstable-smoke test] 155786: regressions - FAIL
flight 155786 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/155786/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 155584 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 9e5a9d0e6886f521453a63a2854ff6d06fa0d028 baseline version: xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9 Last test of basis 155584 2020-10-09 02:01:25 Z5 days Failing since155612 2020-10-09 18:01:22 Z4 days 32 attempts Testing same since 155779 2020-10-13 17:01:26 Z0 days3 attempts People who touched revisions under test: Andrew Cooper Bertrand Marquis Jan Beulich Jason Andryuk Juergen Gross Nick Rosbrook Nick Rosbrook Roger Pau Monné Trammell Hudson 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 fail 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 9e5a9d0e6886f521453a63a2854ff6d06fa0d028 Author: Bertrand Marquis Date: Wed Oct 7 15:57:51 2020 +0100 build: always use BASEDIR for xen sub-directory Modify Makefiles using $(XEN_ROOT)/xen to use $(BASEDIR) instead. This is removing the dependency to xen subdirectory preventing using a wrong configuration file when xen subdirectory is duplicated for compilation tests. BASEDIR is set in xen/lib/x86/Makefile as this Makefile is directly called from the tools build and install process and BASEDIR is not set there. Signed-off-by: Bertrand Marquis Acked-by: Jan Beulich Acked-by: Wei Liu commit a95f31376ba4ae911536c647e1a583d144ccab73 Author: Nick Rosbrook Date: Sun Oct 11 19:31:25 2020 -0400 golang/xenlight: standardize generated code comment There is a standard format for generated Go code header comments, as set by [1]. Modify gengotypes.py to follow this standard, and use the additional // source: convention used by protoc-gen-go. This change is motivated by the fact that since 41aea82de2, the comment would include the absolute path to libxl_types.idl, therefore creating unintended diffs when generating code across different machines. This approach fixes that problem. [1] https://github.com/golang/go/issues/13560 Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit c60f9e4360ec857bb0164387378e12ae8e66e189 Author: Nick Rosbrook Date: Sun Oct 11 19:31:24 2020 -0400 golang/xenlight: do not hard code libxl dir in gengotypes.py Currently, in order to 'import idl' in gengotypes.py, we derive the path of the libxl source directory from the XEN_ROOT environment variable, and append that to sys.path so python can see idl.py. Since the the recent move of libxl to tools/libs/light, this hard coding breaks the build. Instead, check for the environment variable LIBXL_SRC_DIR, but move this check to a try-except block (with empty except). This simply makes the real error more visible, and does not strictly require that LIBXL_SRC_DIR is used. Finally, update the Makefile to set LIBXL_SRC_DIR rather than XEN_ROOT when calling gengotypes.py. Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit 534b3d09958fdc4df64872c2ab19feb4b1eebc5a Author: Juergen Gross Date: Sun Oct 11 14:24:01 2020 +0200 tools/libs/store: add disclaimer to header file regardin
[linux-linus test] 155777: regressions - FAIL
flight 155777 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/155777/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemut-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-xsm7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-intel 7 xen-install fail REGR. vs. 152332 test-amd64-i386-examine 6 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt 7 xen-install fail REGR. vs. 152332 test-amd64-i386-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-amd64-i386-qemut-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-coresched-i386-xl 7 xen-install fail REGR. vs. 152332 test-amd64-i386-qemuu-rhel6hvm-amd 7 xen-installfail REGR. vs. 152332 test-amd64-i386-xl-qemuu-debianhvm-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-ws16-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-pvshim 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-raw7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-freebsd10-i386 7 xen-installfail REGR. vs. 152332 test-amd64-i386-xl-qemuu-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-shadow 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-win7-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemuu-ovmf-amd64 7 xen-install fail REGR. vs. 152332 test-amd64-i386-xl-qemut-stubdom-debianhvm-amd64-xsm 7 xen-install fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 10 xen-install/src_host fail REGR. vs. 152332 test-amd64-i386-libvirt-pair 11 xen-install/dst_host fail REGR. vs. 152332 test-amd64-amd64-xl-qcow212 debian-di-installfail REGR. vs. 152332 build-arm64 6 xen-buildfail REGR. vs. 152332 test-armhf-armhf-xl-credit2 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-xl-vhd 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-xl-cubietruck 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-libvirt 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-examine 8 reboot fail REGR. vs. 152332 test-armhf-armhf-xl-multivcpu 8 xen-bootfail REGR. vs. 152332 test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-xl-credit1 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-xl 8 xen-boot fail REGR. vs. 152332 test-armhf-armhf-libvirt-raw 8 xen-boot fail REGR. vs. 152332 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 20 guest-localmigrate/x10 fail REGR. vs. 152332 test-armhf-armhf-xl-rtds 8 xen-boot fail REGR. vs. 152332 Tests which did not succeed, but are not blocking: test-arm64-arm64-examine 1 build-check(1) blocked n/a test-arm64-arm64-libvirt-xsm 1 build-check(1) blocked n/a test-arm64-arm64-xl 1 build-check(1) blocked n/a test-arm64-arm64-xl-credit1 1 build-check(1) blocked n/a test-arm64-arm64-xl-credit2 1 build-check(1) blocked n/a test-arm64-arm64-xl-seattle 1 build-check(1) blocked n/a test-arm64-arm64-xl-thunderx 1 build-check(1) blocked n/a build-arm64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemut-win7-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemuu-win7-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemut-ws16-amd64 19 guest-stopfail like 152332 test-amd64-amd64-xl-qemuu-ws16-amd64 19 guest-stopfail like 152332 test-amd64-amd64-libvirt-xsm 15 migrate-support-
[xen-unstable-smoke test] 155790: regressions - FAIL
flight 155790 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/155790/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-xl-xsm 8 xen-boot fail REGR. vs. 155584 Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 15 migrate-support-checkfail never pass test-armhf-armhf-xl 15 migrate-support-checkfail never pass test-armhf-armhf-xl 16 saverestore-support-checkfail never pass version targeted for testing: xen 9e5a9d0e6886f521453a63a2854ff6d06fa0d028 baseline version: xen 25849c8b16f2a5b7fcd0a823e80a5f1b590291f9 Last test of basis 155584 2020-10-09 02:01:25 Z5 days Failing since155612 2020-10-09 18:01:22 Z4 days 33 attempts Testing same since 155779 2020-10-13 17:01:26 Z0 days4 attempts People who touched revisions under test: Andrew Cooper Bertrand Marquis Jan Beulich Jason Andryuk Juergen Gross Nick Rosbrook Nick Rosbrook Roger Pau Monné Trammell Hudson 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 fail 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 9e5a9d0e6886f521453a63a2854ff6d06fa0d028 Author: Bertrand Marquis Date: Wed Oct 7 15:57:51 2020 +0100 build: always use BASEDIR for xen sub-directory Modify Makefiles using $(XEN_ROOT)/xen to use $(BASEDIR) instead. This is removing the dependency to xen subdirectory preventing using a wrong configuration file when xen subdirectory is duplicated for compilation tests. BASEDIR is set in xen/lib/x86/Makefile as this Makefile is directly called from the tools build and install process and BASEDIR is not set there. Signed-off-by: Bertrand Marquis Acked-by: Jan Beulich Acked-by: Wei Liu commit a95f31376ba4ae911536c647e1a583d144ccab73 Author: Nick Rosbrook Date: Sun Oct 11 19:31:25 2020 -0400 golang/xenlight: standardize generated code comment There is a standard format for generated Go code header comments, as set by [1]. Modify gengotypes.py to follow this standard, and use the additional // source: convention used by protoc-gen-go. This change is motivated by the fact that since 41aea82de2, the comment would include the absolute path to libxl_types.idl, therefore creating unintended diffs when generating code across different machines. This approach fixes that problem. [1] https://github.com/golang/go/issues/13560 Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit c60f9e4360ec857bb0164387378e12ae8e66e189 Author: Nick Rosbrook Date: Sun Oct 11 19:31:24 2020 -0400 golang/xenlight: do not hard code libxl dir in gengotypes.py Currently, in order to 'import idl' in gengotypes.py, we derive the path of the libxl source directory from the XEN_ROOT environment variable, and append that to sys.path so python can see idl.py. Since the the recent move of libxl to tools/libs/light, this hard coding breaks the build. Instead, check for the environment variable LIBXL_SRC_DIR, but move this check to a try-except block (with empty except). This simply makes the real error more visible, and does not strictly require that LIBXL_SRC_DIR is used. Finally, update the Makefile to set LIBXL_SRC_DIR rather than XEN_ROOT when calling gengotypes.py. Signed-off-by: Nick Rosbrook Reviewed-by: George Dunlap commit 534b3d09958fdc4df64872c2ab19feb4b1eebc5a Author: Juergen Gross Date: Sun Oct 11 14:24:01 2020 +0200 tools/libs/store: add disclaimer to header file regardin
[GIT PULL] xen: branch for v5.10-rc1
Linus, Please git pull the following tag: git://git.kernel.org/pub/scm/linux/kernel/git/xen/tip.git for-linus-5.10b-rc1-tag xen: branch for v5.10-rc1 It contains: - 2 small cleanup patches - A fix for avoiding error messages when initializing MCA banks in a Xen dom0 - A small series for converting the Xen gntdev driver to use pin_user_pages*() instead of get_user_pages*() - An intermediate fix for running as a Xen guest on Arm with KPTI enabled (the final solution will need a new Xen functionality) Thanks. Juergen arch/arm/include/asm/xen/page.h | 5 + arch/arm/xen/enlighten.c | 6 -- arch/arm64/include/asm/xen/page.h | 6 ++ arch/x86/xen/enlighten_pv.c | 9 + arch/x86/xen/mmu_pv.c | 2 +- drivers/xen/gntdev.c | 17 + drivers/xen/pvcalls-front.c | 2 +- 7 files changed, 35 insertions(+), 12 deletions(-) Hui Su (1): x86/xen: Fix typo in xen_pagetable_p2m_free() Jing Xiangfeng (1): xen: remove redundant initialization of variable ret Juergen Gross (1): x86/xen: disable Firmware First mode for correctable memory errors Souptick Joarder (2): xen/gntdev.c: Mark pages as dirty xen/gntdev.c: Convert get_user_pages*() to pin_user_pages*() Stefano Stabellini (1): xen/arm: do not setup the runstate info page if kpti is enabled
Re: [PATCH v2 2/2] xen/evtchn: rework per event channel lock
On 13.10.20 17:28, Jan Beulich wrote:
On 12.10.2020 11:27, Juergen Gross wrote:
@@ -798,9 +786,11 @@ void send_guest_vcpu_virq(struct vcpu *v, uint32_t virq)
d = v->domain;
chn = evtchn_from_port(d, port);
-spin_lock(&chn->lock);
-evtchn_port_set_pending(d, v->vcpu_id, chn);
-spin_unlock(&chn->lock);
+if ( evtchn_tryread_lock(chn) )
+{
+evtchn_port_set_pending(d, v->vcpu_id, chn);
+evtchn_read_unlock(chn);
+}
out:
spin_unlock_irqrestore(&v->virq_lock, flags);
@@ -829,9 +819,11 @@ void send_guest_global_virq(struct domain *d, uint32_t
virq)
goto out;
chn = evtchn_from_port(d, port);
-spin_lock(&chn->lock);
-evtchn_port_set_pending(d, chn->notify_vcpu_id, chn);
-spin_unlock(&chn->lock);
+if ( evtchn_tryread_lock(chn) )
+{
+evtchn_port_set_pending(d, v->vcpu_id, chn);
Is this simply a copy-and-paste mistake (re-using the code from
send_guest_vcpu_virq()), or is there a reason you switch from
where to obtain the vCPU to send to (in which case this ought
to be mentioned in the description, and in which case you could
use literal zero)?
Thanks for spotting! Its a copy-and-paste mistake.
--- a/xen/include/xen/event.h
+++ b/xen/include/xen/event.h
@@ -105,6 +105,45 @@ void notify_via_xen_event_channel(struct domain *ld, int
lport);
#define bucket_from_port(d, p) \
((group_from_port(d, p))[((p) % EVTCHNS_PER_GROUP) / EVTCHNS_PER_BUCKET])
+#define EVENT_WRITE_LOCK_INCMAX_VIRT_CPUS
Isn't the ceiling on simultaneous readers the number of pCPU-s,
and the value here then needs to be NR_CPUS + 1 to accommodate
the maximum number of readers? Furthermore, with you dropping
the disabling of interrupts, one pCPU can acquire a read lock
now more than once, when interrupting a locked region.
Yes, I think you are right.
So at least 2 * (NR-CPUS + 1), or even 3 * (NR_CPUS + 1) for covering
NMIs, too?
+static inline void evtchn_write_lock(struct evtchn *evtchn)
+{
+int val;
+
+/* No barrier needed, atomic_add_return() is full barrier. */
+for ( val = atomic_add_return(EVENT_WRITE_LOCK_INC, &evtchn->lock);
+ val != EVENT_WRITE_LOCK_INC;
+ val = atomic_read(&evtchn->lock) )
+cpu_relax();
+}
+
+static inline void evtchn_write_unlock(struct evtchn *evtchn)
+{
+arch_lock_release_barrier();
+
+atomic_sub(EVENT_WRITE_LOCK_INC, &evtchn->lock);
+}
+
+static inline bool evtchn_tryread_lock(struct evtchn *evtchn)
The corresponding "generic" function is read_trylock() - I'd
suggest to use the same base name, with the evtchn_ prefix.
Okay.
@@ -274,12 +312,12 @@ static inline int evtchn_port_poll(struct domain *d,
evtchn_port_t port)
if ( port_is_valid(d, port) )
{
struct evtchn *evtchn = evtchn_from_port(d, port);
-unsigned long flags;
-spin_lock_irqsave(&evtchn->lock, flags);
-if ( evtchn_usable(evtchn) )
+if ( evtchn_tryread_lock(evtchn) && evtchn_usable(evtchn) )
+{
rc = evtchn_is_pending(d, evtchn);
-spin_unlock_irqrestore(&evtchn->lock, flags);
+evtchn_read_unlock(evtchn);
+}
}
This needs to be two nested if()-s, as you need to drop the lock
even when evtchn_usable() returns false.
Oh, yes.
Juergen
Re: [PATCH v2 2/2] xen/evtchn: rework per event channel lock
On 14.10.2020 08:00, Jürgen Groß wrote: > On 13.10.20 17:28, Jan Beulich wrote: >> On 12.10.2020 11:27, Juergen Gross wrote: >>> --- a/xen/include/xen/event.h >>> +++ b/xen/include/xen/event.h >>> @@ -105,6 +105,45 @@ void notify_via_xen_event_channel(struct domain *ld, >>> int lport); >>> #define bucket_from_port(d, p) \ >>> ((group_from_port(d, p))[((p) % EVTCHNS_PER_GROUP) / >>> EVTCHNS_PER_BUCKET]) >>> >>> +#define EVENT_WRITE_LOCK_INCMAX_VIRT_CPUS >> >> Isn't the ceiling on simultaneous readers the number of pCPU-s, >> and the value here then needs to be NR_CPUS + 1 to accommodate >> the maximum number of readers? Furthermore, with you dropping >> the disabling of interrupts, one pCPU can acquire a read lock >> now more than once, when interrupting a locked region. > > Yes, I think you are right. > > So at least 2 * (NR-CPUS + 1), or even 3 * (NR_CPUS + 1) for covering > NMIs, too? Hard to say: Even interrupts can in principle nest. I'd go further and use e.g. INT_MAX / 4, albeit no matter what value we choose there'll remain a theoretical risk. I'm therefore not fully convinced of the concept, irrespective of it providing an elegant solution to the problem at hand. I'd be curious what others think. Jan
