[seabios test] 149712: tolerable FAIL - PUSHED
flight 149712 seabios real [real] http://logs.test-lab.xenproject.org/osstest/logs/149712/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 149211 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 149211 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 149211 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 149211 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass version targeted for testing: seabios eaaf726038cb9b2d01312d6430b4e93842aa96eb baseline version: seabios 6a3b59ab9c7dc00331c21346052dfa6a0df45aa3 Last test of basis 149211 2020-03-30 13:24:09 Z 22 days Testing same since 149712 2020-04-21 10:44:40 Z0 days1 attempts People who touched revisions under test: Stefan Berger Stefan Berger 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-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 6a3b59a..eaaf726 eaaf726038cb9b2d01312d6430b4e93842aa96eb -> xen-tested-master
[linux-linus test] 149711: tolerable FAIL - PUSHED
flight 149711 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/149711/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail blocked in 149703 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 149703 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 149703 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 149703 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 149703 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 149703 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 149703 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 149703 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 149703 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: linuxae83d0b416db002fe95601e7f97f64b59514d936 baseline version: linux7a56db0299f9d43b4fe076838150c5cc293df131 Last test of basis 149703 2020-04-17 16:39:47 Z4 days Testing same since 149711 2020-04-21 10:41:40 Z0 days1 attempts People who touched revisions under test: "Eric W. Biederman" Adam Barber afzal mohammed Alex Deucher Alex Deucher Alexandru Tachici Andrei Vagin Ann T Ropea Arnaldo Carvalho de Melo Arnd Bergmann Ashutosh Dixit Atish
[xen-unstable-smoke test] 149727: tolerable all pass - PUSHED
flight 149727 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/149727/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 5730ac3c8346f56fe8ee90249cdcbdab2a4d5791 baseline version: xen 0796cb907f2c31046427510a6da6f4941f678b76 Last test of basis 149724 2020-04-21 20:01:12 Z0 days Testing same since 149727 2020-04-22 00:01:31 Z0 days1 attempts People who touched revisions under test: Julien Grall Julien Grall jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 0796cb907f..5730ac3c83 5730ac3c8346f56fe8ee90249cdcbdab2a4d5791 -> smoke
[linux-5.4 test] 149709: regressions - FAIL
flight 149709 linux-5.4 real [real] http://logs.test-lab.xenproject.org/osstest/logs/149709/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 149637 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 15 guest-saverestorefail REGR. vs. 149637 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass version targeted for testing: linux6ccc74c083c0d472ac64f3733f5b7bf3f99f261e baseline version: linuxbc844d58f697dff3ded4b410094ee89f5cedc04c Last test of basis 149637 2020-04-13 09:10:52 Z8 days Failing since149700 2020-04-17 09:09:37 Z4 days2 attempts Testing same since 149709 2020-04-21 10:41:23 Z0 days1 attempts 317 people touched revisions under test, not listing
Re: [PATCH] xen/arm: Avoid to open-code the relinquish state machine
On Sun, 19 Apr 2020, Julien Grall wrote:
> In commit 0dfffe01d5 "x86: Improve the efficiency of
> domain_relinquish_resources()", the x86 version of the function has been
> reworked to avoid open-coding the state machine and also add more
> documentation.
>
> Bring the Arm version on part with x86 by introducing a documented
> PROGRESS() macro to avoid latent bugs and make the new PROG_* states
> private to domain_relinquish_resources().
>
> Cc: Andrew Cooper
> Signed-off-by: Julien Grall
Reviewed-by: Stefano Stabellini
> ---
> xen/arch/arm/domain.c| 60 ++--
> xen/include/asm-arm/domain.h | 9 +-
> 2 files changed, 38 insertions(+), 31 deletions(-)
>
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index 6627be2922..31169326b2 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -674,7 +674,6 @@ int arch_domain_create(struct domain *d,
> int rc, count = 0;
>
> BUILD_BUG_ON(GUEST_MAX_VCPUS < MAX_VIRT_CPUS);
> -d->arch.relmem = RELMEM_not_started;
>
> /* Idle domains do not need this setup */
> if ( is_idle_domain(d) )
> @@ -950,13 +949,41 @@ static int relinquish_memory(struct domain *d, struct
> page_list_head *list)
> return ret;
> }
>
> +/*
> + * Record the current progress. Subsequent hypercall continuations will
> + * logically restart work from this point.
> + *
> + * PROGRESS() markers must not be in the middle of loops. The loop
> + * variable isn't preserved accross a continuation.
> + *
> + * To avoid redundant work, there should be a marker before each
> + * function which may return -ERESTART.
> + */
> +enum {
> +PROG_tee = 1,
> +PROG_xen,
> +PROG_page,
> +PROG_mapping,
> +PROG_done,
> +};
> +
> +#define PROGRESS(x) \
> +d->arch.rel_priv = PROG_ ## x; \
> +/* Fallthrough */ \
> +case PROG_ ## x
> +
> int domain_relinquish_resources(struct domain *d)
> {
> int ret = 0;
>
> -switch ( d->arch.relmem )
> +/*
> + * This hypercall can take minutes of wallclock time to complete. This
> + * logic implements a co-routine, stashing state in struct domain across
> + * hypercall continuation boundaries.
> + */
> +switch ( d->arch.rel_priv )
> {
> -case RELMEM_not_started:
> +case 0:
> ret = iommu_release_dt_devices(d);
> if ( ret )
> return ret;
> @@ -967,42 +994,27 @@ int domain_relinquish_resources(struct domain *d)
> */
> domain_vpl011_deinit(d);
>
> -d->arch.relmem = RELMEM_tee;
> -/* Fallthrough */
> -
> -case RELMEM_tee:
> +PROGRESS(tee):
> ret = tee_relinquish_resources(d);
> if (ret )
> return ret;
>
> -d->arch.relmem = RELMEM_xen;
> -/* Fallthrough */
> -
> -case RELMEM_xen:
> +PROGRESS(xen):
> ret = relinquish_memory(d, >xenpage_list);
> if ( ret )
> return ret;
>
> -d->arch.relmem = RELMEM_page;
> -/* Fallthrough */
> -
> -case RELMEM_page:
> +PROGRESS(page):
> ret = relinquish_memory(d, >page_list);
> if ( ret )
> return ret;
>
> -d->arch.relmem = RELMEM_mapping;
> -/* Fallthrough */
> -
> -case RELMEM_mapping:
> +PROGRESS(mapping):
> ret = relinquish_p2m_mapping(d);
> if ( ret )
> return ret;
>
> -d->arch.relmem = RELMEM_done;
> -/* Fallthrough */
> -
> -case RELMEM_done:
> +PROGRESS(done):
> break;
>
> default:
> @@ -1012,6 +1024,8 @@ int domain_relinquish_resources(struct domain *d)
> return 0;
> }
>
> +#undef PROGRESS
> +
> void arch_dump_domain_info(struct domain *d)
> {
> p2m_dump_info(d);
> diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
> index d39477a939..d2142c6707 100644
> --- a/xen/include/asm-arm/domain.h
> +++ b/xen/include/asm-arm/domain.h
> @@ -56,14 +56,7 @@ struct arch_domain
> struct vmmio vmmio;
>
> /* Continuable domain_relinquish_resources(). */
> -enum {
> -RELMEM_not_started,
> -RELMEM_tee,
> -RELMEM_xen,
> -RELMEM_page,
> -RELMEM_mapping,
> -RELMEM_done,
> -} relmem;
> +unsigned int rel_priv;
>
> struct {
> uint64_t offset;
> --
> 2.17.1
>
[xen-unstable-smoke test] 149724: tolerable all pass - PUSHED
flight 149724 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/149724/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 0796cb907f2c31046427510a6da6f4941f678b76 baseline version: xen 4803a67114279a656a54a23cebed646da32efeb6 Last test of basis 149720 2020-04-21 16:02:16 Z0 days Testing same since 149724 2020-04-21 20:01:12 Z0 days1 attempts People who touched revisions under test: Julien Grall Peng Fan Stefano Stabellini jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 4803a67114..0796cb907f 0796cb907f2c31046427510a6da6f4941f678b76 -> smoke
Re: [PATCH] pvcalls: Document explicitly the padding for all arches
On Mon, 20 Apr 2020, Jan Beulich wrote:
> On 20.04.2020 15:34, Julien Grall wrote:
> > Hi Jan,
> >
> > On 20/04/2020 09:04, Jan Beulich wrote:
> >> On 19.04.2020 12:49, Julien Grall wrote:
> >>> --- a/docs/misc/pvcalls.pandoc
> >>> +++ b/docs/misc/pvcalls.pandoc
> >>> @@ -246,9 +246,7 @@ The format is defined as follows:
> >>> uint32_t domain;
> >>> uint32_t type;
> >>> uint32_t protocol;
> >>> - #ifdef CONFIG_X86_32
> >>> uint8_t pad[4];
> >>> - #endif
> >>> } socket;
> >>> struct xen_pvcalls_connect {
> >>> uint64_t id;
> >>> @@ -257,16 +255,12 @@ The format is defined as follows:
> >>> uint32_t flags;
> >>> grant_ref_t ref;
> >>> uint32_t evtchn;
> >>> - #ifdef CONFIG_X86_32
> >>> uint8_t pad[4];
> >>> - #endif
> >>> } connect;
> >>> struct xen_pvcalls_release {
> >>> uint64_t id;
> >>> uint8_t reuse;
> >>> - #ifdef CONFIG_X86_32
> >>> uint8_t pad[7];
> >>> - #endif
> >>> } release;
> >>> struct xen_pvcalls_bind {
> >>> uint64_t id;
> >>> @@ -276,9 +270,7 @@ The format is defined as follows:
> >>> struct xen_pvcalls_listen {
> >>> uint64_t id;
> >>> uint32_t backlog;
> >>> - #ifdef CONFIG_X86_32
> >>> uint8_t pad[4];
> >>> - #endif
> >>> } listen;
> >>> struct xen_pvcalls_accept {
> >>> uint64_t id;
> >>
> >> I wonder on what grounds these #ifdef-s had been there - they're
> >> plain wrong with the types used in the public header.
> >>
> >>> --- a/xen/include/public/io/pvcalls.h
> >>> +++ b/xen/include/public/io/pvcalls.h
> >>> @@ -65,6 +65,7 @@ struct xen_pvcalls_request {
> >>> uint32_t domain;
> >>> uint32_t type;
> >>> uint32_t protocol;
> >>> + uint8_t pad[4];
> >>> } socket;
> >>> struct xen_pvcalls_connect {
> >>> uint64_t id;
> >>> @@ -73,10 +74,12 @@ struct xen_pvcalls_request {
> >>> uint32_t flags;
> >>> grant_ref_t ref;
> >>> uint32_t evtchn;
> >>> + uint8_t pad[4];
> >>> } connect;
> >>> struct xen_pvcalls_release {
> >>> uint64_t id;
> >>> uint8_t reuse;
> >>> + uint8_t pad[7];
> >>> } release;
> >>> struct xen_pvcalls_bind {
> >>> uint64_t id;
> >>> @@ -86,6 +89,7 @@ struct xen_pvcalls_request {
> >>> struct xen_pvcalls_listen {
> >>> uint64_t id;
> >>> uint32_t backlog;
> >>> + uint8_t pad[4];
> >>> } listen;
> >>
> >> I'm afraid we can't change these in such a way - your additions
> >> change sizeof() for the respective sub-structures on 32-bit x86,
> >> and hence this is not a backwards compatible adjustment.
> >
> > This is a bit confusing, each structure contain a 64-bit field so
> > I would have thought it the structure would be 8-byte aligned (as
> > on 32-bit Arm). But looking at the spec, a uint64_t will only
> > aligned to 4-byte.
> >
> > However, I am not sure why sizeof() matters here. I understand
> > the value would be different, but AFAICT, this is not used as part
> > of the protocol.
>
> Two independent components of a consumer of our interface could
> have a function taking (pointer to) struct xen_pvcalls_connect.
> If one component gets re-built with the new definition and the
> other doesn't, they'll disagree on what range of memory needs
> to be accessible. The instantiating side (using the old header)
> may have ended up placing the struct immediately ahead of a
> page boundary. The consuming side (using the changed header)
> would then encounter a fault if it wanted to access the struct
> as a whole (assignment, memcpy()).
Even if it was possible to use the sub-structs defined in the header
that way, keep in mind that we also wrote:
/* dummy member to force sizeof(struct xen_pvcalls_request)
* to match across archs */
struct xen_pvcalls_dummy {
uint8_t dummy[56];
} dummy;
And the spec also clarifies that the size of each specific request is
always 56 bytes. So I think that if we wanted, we could make the changes
Julien is suggesting without worrying about breaking anything.
Speaking of the patch, I think it would be good to have to make things
clearer.
[xen-unstable test] 149705: tolerable FAIL - PUSHED
flight 149705 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/149705/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-rtds 18 guest-localmigrate/x10 fail like 149695 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 149695 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 149695 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 149695 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 149695 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 149695 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 149695 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 149695 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 149695 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 149695 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass version targeted for testing: xen 82dd1a956d9b68f52e830d1dddfdfb4ab4d5a638 baseline version: xen a48e1323f9aa29f1ffb95594671b73de6bd7c1d4 Last test of basis 149695 2020-04-17 04:16:59 Z4 days Testing same since 149702 2020-04-17 13:36:57 Z4 days2 attempts People who touched revisions under test: Dario Faggioli Jeff Kubascik Sergey Dyasli jobs: build-amd64-xsm pass build-arm64-xsm pass
[ovmf test] 149708: all pass - PUSHED
flight 149708 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/149708/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 6e3c834ae47d1201c4ddcc6a6adc5e44718c7617 baseline version: ovmf c884b23ac40a1b1f56e21ebbb1f602fa2e0f05c9 Last test of basis 149698 2020-04-17 07:50:32 Z4 days Testing same since 149708 2020-04-21 10:39:57 Z0 days1 attempts People who touched revisions under test: Keysound Chang Maciej Rabeda Michael Kubacki 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 c884b23ac4..6e3c834ae4 6e3c834ae47d1201c4ddcc6a6adc5e44718c7617 -> xen-tested-master
Re: [bug report] drm/xen-front: Add support for Xen PV display frontend
On Tue, Apr 21, 2020 at 08:59:09PM +0200, Julia Lawall wrote:
>
>
> On Tue, 21 Apr 2020, Dan Carpenter wrote:
>
> > On Tue, Apr 21, 2020 at 05:29:02PM +0200, Julia Lawall wrote:
> > >
> > >
> > > On Tue, 21 Apr 2020, Dan Carpenter wrote:
> > >
> > > > Hi Kernel Janitors,
> > > >
> > > > Here is another idea that someone could work on, fixing the
> > > > IS_ERR_OR_NULL() checks in the xen driver.
> > > >
> > > > The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
> > > > display frontend" from Apr 3, 2018, leads to the following static
> > > > checker warning:
> > > >
> > > > drivers/gpu/drm/xen/xen_drm_front_gem.c:140
> > > > xen_drm_front_gem_create()
> > > > warn: passing zero to 'ERR_CAST'
> > > >
> > > > drivers/gpu/drm/xen/xen_drm_front_gem.c
> > > >133 struct drm_gem_object *xen_drm_front_gem_create(struct
> > > > drm_device *dev,
> > > >134 size_t size)
> > > >135 {
> > > >136 struct xen_gem_object *xen_obj;
> > > >137
> > > >138 xen_obj = gem_create(dev, size);
> > > >139 if (IS_ERR_OR_NULL(xen_obj))
> > > >140 return ERR_CAST(xen_obj);
> > >
> > > Are the other occurrences of this also a possible problem? There are a
> > > few others outside of xen.
> >
> > We sometimes check a parameter for IS_ERR_OR_NULL().
> >
> > void free_function(struct something *p)
> > {
> > if (IS_ERR_OR_NULL(p))
> > return;
> > }
> >
> > That's fine, absolutely harmless and not a bug. But if we are checking
> > a return value like this then probably most of the time it's invalid
> > code. Normally it's again like this code where we're dealing with an
> > impossible thing because the return is never NULL. The common bugs are
> > that it returns NULL to a caller which only expects error pointers or it
> > returns success instead of failure. But sometimes returning success can
> > be valid:
> >
> > obj = get_feature(dev);
> > if (IS_ERR_OR_NULL(obj))
> > return PTR_ERR(obj);
> >
> > It deliberately returns success because the rest of the function is
> > useless when we don't have the feature.
>
> The other cases are also with ERR_CAST:
I bet these are less likely to be correct because probably the optional
feature doesn't translate to a different optional feature.
>
> drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c in create_udp_flow
This can never be NULL, but look at this code:
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c
427 if (trans_spec) {
428 qp_flow = create_and_add_flow(qp_grp,
429
trans_spec);
430 if (IS_ERR_OR_NULL(qp_flow)) {
431 status = qp_flow ?
PTR_ERR(qp_flow) : -EFAULT;
432 break;
433 }
434 } else {
If the create_and_add_flow() returns NULL, that's a bug because it's not
an optional feature which can be disabled in the config etc. But this
code has been future proofed in case future users decide to write buggy
code the NULL gets changed to an error code.
Is -EFAULT the correct way to handle bugs that future programmers are
going to introduce? You have to very psychic to know the answer to
that.
> fs/overlayfs/namei.c in ovl_index_upper
This code is correct. There are actually a bunch of functions in VFS
which only return NULL and error pointers, never valid pointers. VFS
is weird like that.
> sound/soc/qcom/qdsp6/q6adm.c in q6adm_open
Never NULL. It should be IS_ERR().
> drivers/clk/clk.c in clk_hw_create_clk
This is checking a parameter so it's fine.
regards,
dan carpenter
[qemu-mainline test] 149706: regressions - FAIL
flight 149706 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/149706/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-libvirt-raw 7 xen-boot fail REGR. vs. 149681 Regressions which are regarded as allowable (not blocking): test-amd64-amd64-xl-rtds 16 guest-localmigrate fail REGR. vs. 149681 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 149681 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 149681 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 149681 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 149681 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 13 migrate-support-checkfail never pass test-arm64-arm64-xl-seattle 14 saverestore-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-thunderx 13 migrate-support-checkfail never pass test-arm64-arm64-xl-thunderx 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass version targeted for testing: qemuu3119154db04890fdf57022a43cf2ee594fd4da5a baseline version: qemuu20038cd7a8412feeb49c01f6ede89e36c8995472 Last test of basis 149681 2020-04-16 02:09:07 Z5 days Testing same since 149706 2020-04-21 10:39:06 Z0 days1 attempts People who touched revisions under test: Chen Qun Cédric Le Goater David Gibson Ganesh Goudar Laurent Vivier Nathan Chancellor Nicholas Piggin Peter Maydell Philippe Mathieu-Daudé Richard Henderson Sergei Trofimovich jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64
Re: [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range
On 21/04/2020 19:49, Stefano Stabellini wrote: + George On Sat, 18 Apr 2020, Julien Grall wrote: Hi, On 18/04/2020 00:12, Stefano Stabellini wrote: On Fri, 17 Apr 2020, Julien Grall wrote: Hi, The title claim this is a resend, but this is actually not the latest version of this patch. Can you explain why you decided to use v1 rather than v2? Because v2 added a printk for every read, and I thought you only wanted the range fix. Also, the printk is not a "warn once" so after the discussion where it became apparent that the register can be read many times, it sounded undesirable. You previously mentionned that you will use Peng's patch. From my perspective, this meant you are using the latest version of a patch not a previous one. So this was a bit of a surprised to me. Nonetheless I don't have a strong preference between the two. If you prefer v2 it is here: https://marc.info/?l=xen-devel=157440872522065 I understand the "warn" issue but we did agree with it back then. I am ok to drop it. However, may I request to be more forthcoming in your patch in the future? For instance in this case, this a link to the original patch and an explanation why you selected v1 would have been really useful. That's a good point, I can add it. So, for clarity, I'll keep v1 but expand on the commit message to say that we kept v1 to avoid spamming the console. I am fine with that: Acked-by: Julien Grall Do you need me to resent it? If it is OK for you as-is, you can give your ack here, and I'll go ahead and commit it. On Fri, 17 Apr 2020 at 23:16, Stefano Stabellini wrote: From: Peng Fan The end should be GICD_ISACTIVERN not GICD_ISACTIVER. (See https://marc.info/?l=xen-devel=158527653730795 for a discussion on what it would take to implement GICD_ISACTIVER/GICD_ICACTIVER properly.) Signed-off-by: Peng Fan Signed-off-by: Stefano Stabellini Reviewed-by: Stefano Stabellini I don't think you can be at the same time an author of the patch and a reviewer. Otherwise, I could review my own patch ;). Yeah ... that was not the intention. I changed the commit message so I had to add my signed-off-by for copyright reasons. At the same time I reviewed it even before changing the commit message so I added the reviewed-by. I agree it looks kind of weird. I don't feel this should go as-is. It is not clear in the commit message the changes you did and could lead confusion once merged. One may think you reviewed your own patch. In general when I tweak a commit message, I will not add my signed-off-by but just add [julieng: Tweak commit message] above my reviewed-by/acked-by tag. Alternatively, you can remove your reviewed-by and let another maintainer reviewing it. I will let you decide your preference and resend the patch appropriately. The Linux policy on Signed-off-by is really strict: basically any time a person touches a patch, even just to commit the patch (no changes to it), they add a Signed-off-by. So it is common there and other projects to have both Signed-off-by and Reviewed-by from the same individual. For instance on Linux: I don't think we used this policy so far for Xen Project. Before pointing out the Signed-off-by vs Reviewed-by, I actually looked online but I wasn't able to find why Signed-off-by and Reviewed-by was added together. commit b2a84de2a2deb76a6a51609845341f508c518c03 Reported-by: Linus Torvalds Acked-by: Linus Torvalds Reviewed-by: Catalin Marinas Signed-off-by: Will Deacon Signed-off-by: Catalin Marinas commit 33e45234987ea3ed4b05fc512f4441696478f12d Reviewed-by: Kees Cook Reviewed-by: Catalin Marinas Reviewed-by: Vincenzo Frascino Signed-off-by: Kristina Martsenko [Amit: Modified secondary cores key structure, comments] Signed-off-by: Amit Daniel Kachhap Signed-off-by: Catalin Marinas On QEMU: commit 22cd0945b8429f818a2d90f06f02bb526bfb319d Signed-off-by: Francisco Iglesias Signed-off-by: Edgar E. Iglesias Reviewed-by: Edgar E. Iglesias Message-id: 20180503214201.29082-2-frasse.igles...@gmail.com Signed-off-by: Peter Maydell commit 133d23b3ad1be53105b9950fb18858cf059f2da6 Signed-off-by: Alistair Francis Reviewed-by: Edgar E. Iglesias Signed-off-by: Edgar E. Iglesias Your suggestion of adding something between brackets like: [stefano: update commit message] is good because it clarifies things and I'd like to do that. But still, I think it would require the addition of my Signed-off-by. At the same time it doesn't look like we want to avoiding adding a Reviewed-by because a reviewer made a change to the commit message too? Agree, I also think we need to clarify in this case as it is more difficult to understand if the signed-off-by was by a contributor or someone who merged it. Of course, for this patch, I am happy to drop my Reviewed-by and resend and I'll do that. But I think it would be worth
Re: [bug report] drm/xen-front: Add support for Xen PV display frontend
On Tue, 21 Apr 2020, Dan Carpenter wrote:
> On Tue, Apr 21, 2020 at 05:29:02PM +0200, Julia Lawall wrote:
> >
> >
> > On Tue, 21 Apr 2020, Dan Carpenter wrote:
> >
> > > Hi Kernel Janitors,
> > >
> > > Here is another idea that someone could work on, fixing the
> > > IS_ERR_OR_NULL() checks in the xen driver.
> > >
> > > The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
> > > display frontend" from Apr 3, 2018, leads to the following static
> > > checker warning:
> > >
> > > drivers/gpu/drm/xen/xen_drm_front_gem.c:140 xen_drm_front_gem_create()
> > > warn: passing zero to 'ERR_CAST'
> > >
> > > drivers/gpu/drm/xen/xen_drm_front_gem.c
> > >133 struct drm_gem_object *xen_drm_front_gem_create(struct drm_device
> > > *dev,
> > >134 size_t size)
> > >135 {
> > >136 struct xen_gem_object *xen_obj;
> > >137
> > >138 xen_obj = gem_create(dev, size);
> > >139 if (IS_ERR_OR_NULL(xen_obj))
> > >140 return ERR_CAST(xen_obj);
> >
> > Are the other occurrences of this also a possible problem? There are a
> > few others outside of xen.
>
> We sometimes check a parameter for IS_ERR_OR_NULL().
>
> void free_function(struct something *p)
> {
> if (IS_ERR_OR_NULL(p))
> return;
> }
>
> That's fine, absolutely harmless and not a bug. But if we are checking
> a return value like this then probably most of the time it's invalid
> code. Normally it's again like this code where we're dealing with an
> impossible thing because the return is never NULL. The common bugs are
> that it returns NULL to a caller which only expects error pointers or it
> returns success instead of failure. But sometimes returning success can
> be valid:
>
> obj = get_feature(dev);
> if (IS_ERR_OR_NULL(obj))
> return PTR_ERR(obj);
>
> It deliberately returns success because the rest of the function is
> useless when we don't have the feature.
The other cases are also with ERR_CAST:
drivers/infiniband/hw/usnic/usnic_ib_qp_grp.c in create_udp_flow
fs/overlayfs/namei.c in ovl_index_upper
sound/soc/qcom/qdsp6/q6adm.c in q6adm_open
drivers/clk/clk.c in clk_hw_create_clk
julia
Re: [PATCH][RESEND] xen/arm: vgic-v3: fix GICD_ISACTIVER range
+ George On Sat, 18 Apr 2020, Julien Grall wrote: > Hi, > > On 18/04/2020 00:12, Stefano Stabellini wrote: > > On Fri, 17 Apr 2020, Julien Grall wrote: > > > Hi, > > > > > > The title claim this is a resend, but this is actually not the latest > > > version of this patch. Can you explain why you decided to use v1 > > > rather than v2? > > > > Because v2 added a printk for every read, and I thought you only wanted > > the range fix. Also, the printk is not a "warn once" so after the > > discussion where it became apparent that the register can be read many > > times, it sounded undesirable. > > You previously mentionned that you will use Peng's patch. From my perspective, > this meant you are using the latest version of a patch not a previous one. > > So this was a bit of a surprised to me. > > > > > Nonetheless I don't have a strong preference between the two. If you > > prefer v2 it is here: > > > > https://marc.info/?l=xen-devel=157440872522065 > I understand the "warn" issue but we did agree with it back then. I am ok to > drop it. However, may I request to be more forthcoming in your patch in the > future? > > For instance in this case, this a link to the original patch and an > explanation why you selected v1 would have been really useful. That's a good point, I can add it. So, for clarity, I'll keep v1 but expand on the commit message to say that we kept v1 to avoid spamming the console. > > Do you need me to resent it? If it is OK for you as-is, you can give > > your ack here, and I'll go ahead and commit it. > > > > > > > On Fri, 17 Apr 2020 at 23:16, Stefano Stabellini > > > wrote: > > > > > > > > From: Peng Fan > > > > > > > > The end should be GICD_ISACTIVERN not GICD_ISACTIVER. > > > > > > > > (See https://marc.info/?l=xen-devel=158527653730795 for a discussion > > > > on > > > > what it would take to implement GICD_ISACTIVER/GICD_ICACTIVER properly.) > > > > > > > > Signed-off-by: Peng Fan > > > > Signed-off-by: Stefano Stabellini > > > > Reviewed-by: Stefano Stabellini > > > > > > I don't think you can be at the same time an author of the patch and a > > > reviewer. Otherwise, I could review my own patch ;). > > > > Yeah ... that was not the intention. I changed the commit message so I > > had to add my signed-off-by for copyright reasons. > > At the same time I > > reviewed it even before changing the commit message so I added the > > reviewed-by. I agree it looks kind of weird. > > I don't feel this should go as-is. It is not clear in the commit message the > changes you did and could lead confusion once merged. One may think you > reviewed your own patch. > > In general when I tweak a commit message, I will not add my signed-off-by but > just add [julieng: Tweak commit message] above my reviewed-by/acked-by tag. > > Alternatively, you can remove your reviewed-by and let another maintainer > reviewing it. > > I will let you decide your preference and resend the patch appropriately. The Linux policy on Signed-off-by is really strict: basically any time a person touches a patch, even just to commit the patch (no changes to it), they add a Signed-off-by. So it is common there and other projects to have both Signed-off-by and Reviewed-by from the same individual. For instance on Linux: commit b2a84de2a2deb76a6a51609845341f508c518c03 Reported-by: Linus Torvalds Acked-by: Linus Torvalds Reviewed-by: Catalin Marinas Signed-off-by: Will Deacon Signed-off-by: Catalin Marinas commit 33e45234987ea3ed4b05fc512f4441696478f12d Reviewed-by: Kees Cook Reviewed-by: Catalin Marinas Reviewed-by: Vincenzo Frascino Signed-off-by: Kristina Martsenko [Amit: Modified secondary cores key structure, comments] Signed-off-by: Amit Daniel Kachhap Signed-off-by: Catalin Marinas On QEMU: commit 22cd0945b8429f818a2d90f06f02bb526bfb319d Signed-off-by: Francisco Iglesias Signed-off-by: Edgar E. Iglesias Reviewed-by: Edgar E. Iglesias Message-id: 20180503214201.29082-2-frasse.igles...@gmail.com Signed-off-by: Peter Maydell commit 133d23b3ad1be53105b9950fb18858cf059f2da6 Signed-off-by: Alistair Francis Reviewed-by: Edgar E. Iglesias Signed-off-by: Edgar E. Iglesias Your suggestion of adding something between brackets like: [stefano: update commit message] is good because it clarifies things and I'd like to do that. But still, I think it would require the addition of my Signed-off-by. At the same time it doesn't look like we want to avoiding adding a Reviewed-by because a reviewer made a change to the commit message too? Of course, for this patch, I am happy to drop my Reviewed-by and resend and I'll do that. But I think it would be worth clarifying this point at the project level. George, do we or the LinuxFoundation have a policy on whether we can or cannot have reviewed-by and signed-off-by from the same individual?
Re: [PATCH] Introduce a description of the Backport and Fixes tags
Stefano Stabellini writes ("[PATCH] Introduce a description of the Backport and
Fixes tags"):
> Create a new document under docs/process to describe our special tags.
> Add a description of the Fixes tag and the new Backport tag. Also
> clarify that lines with tags should not be split.
>
> Signed-off-by: Stefano Stabellini
> Acked-by: Wei Liu
Acked-by: Ian Jackson
> +When possible, please use the Fixes tag instead (or in addition).
Do we have any code to turn Fixes: into a list of commits to
backport to a particular stable branch ?
If not it might be easier to ask people to add both Backport: and
Fixes:.
Ian.
Re: [PATCH v2 4/4] x86: adjustments to guest handle treatment
Hi,
On 21/04/2020 18:30, Roger Pau Monné wrote:
On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
First of all avoid excessive conversions. copy_{from,to}_guest(), for
example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
I'm not sure I understand the difference between those two, as they
are both placeholders for linear guest addresses?
AFAICT XEN_GUEST_HANDLE should be used for guest pointers inside of an
hypercall struct, while XEN_GUEST_HANDLE_PARAM is for guest pointers
as hypercall arguments. But those are both just guest pointers,
whether they are a parameter to the hypercall or a field in a
struct, and hence could use the same type?
I assume there's some reason for not doing so, and I see the comment
about other arches, but again a linear guest address is just that in
all arches, regardless of it's placement.
On Arm:
* XEN_GUEST_HANDLE() will always be 64-bit on both 32-bit and 64-bit
hypervisor.
* XEN_GUEST_HANDLE_PARAM() will be 32-bit for 32-bit hypervisor. For
64-bit hypervisor, it will be 64-bit.
Per the ABI, each argument only fit a register. So you could not use
XEN_GUEST_HANDLE() as now an argument will be held in 2 registers on 32-bit.
We also want the structure layout to be the same for 32-bit and 64-bit.
So using XEN_GUEST_HANDLE_PARAM() everywhere is not the solution as the
virtual address is not the same.
We could possibly convert internally XEN_GUEST_HANDLE_PARAM() to
XEN_GUEST_HANDLE(), but I am not sure how this would be beneficial. We
would have to use 2 registers for arm 32-bit everytime.
Cheers,
--
Julien Grall
[PATCH] Introduce a description of the Backport and Fixes tags
Create a new document under docs/process to describe our special tags.
Add a description of the Fixes tag and the new Backport tag. Also
clarify that lines with tags should not be split.
Signed-off-by: Stefano Stabellini
Acked-by: Wei Liu
CC: Ian Jackson
CC: jbeul...@suse.com
CC: george.dun...@citrix.com
CC: jul...@xen.org
CC: lars.ku...@citrix.com
CC: andrew.coop...@citrix.com
CC: konrad.w...@oracle.com
---
Changes in v4:
- fix example
- use released trees instead of stable trees
- add (or in addition)
---
docs/process/tags.pandoc | 55
1 file changed, 55 insertions(+)
create mode 100644 docs/process/tags.pandoc
diff --git a/docs/process/tags.pandoc b/docs/process/tags.pandoc
new file mode 100644
index 00..1841cb87a8
--- /dev/null
+++ b/docs/process/tags.pandoc
@@ -0,0 +1,55 @@
+Tags: No line splitting
+---
+Do not split a tag across multiple lines, tags are exempt from the
+"wrap at 75 columns" rule in order to simplify parsing scripts. For
+example:
+
+Fixes: 67d01cdb5518 ("x86: infrastructure to allow converting certain
indirect calls to direct ones")
+
+
+Fixes Tag
+-
+
+If your patch fixes a bug in a specific commit, e.g. you found an issue using
+``git bisect``, please use the 'Fixes:' tag with the first 12 characters of
+the SHA-1 ID, and the one line summary.
+
+The following ``git config`` settings can be used to add a pretty format for
+outputting the above style in the ``git log`` or ``git show`` commands:
+
+[core]
+abbrev = 12
+[pretty]
+fixes = Fixes: %h (\"%s\")
+
+
+Backport Tag
+
+
+A backport tag is an optional tag in the commit message to request a
+given commit to be backported to the released trees:
+
+Backport: 4.9+
+
+It marks a commit for being a candidate for backports to all released
+trees from 4.9 onward.
+
+The backport requester is expected to specify which currently supported
+releases need the backport; but encouraged to specify a release as far
+back as possible which applies. If the requester doesn't know the oldest
+affected tree, they are encouraged to append a comment like the
+following:
+
+Backport: 4.9+ # maybe older
+
+Maintainers request the Backport tag to be added on commit. Contributors
+are welcome to mark their patches with the Backport tag when they deem
+appropriate. Maintainers will request for it to be removed when that is
+not the case.
+
+Please note that the Backport tag is a **request** for backport, which
+will still need to be evaluated by the maintainers. Maintainers might
+ask the requester to help with the backporting work if it is not
+trivial.
+
+When possible, please use the Fixes tag instead (or in addition).
--
2.17.1
Re: [PATCH v3] Introduce a description of the Backport and Fixes tags
On Mon, 20 Apr 2020, Jan Beulich wrote: > On 18.04.2020 00:24, Stefano Stabellini wrote: > > +Backport Tag > > + > > + > > +A backport tag is an optional tag in the commit message to request a > > +given commit to be backported to the stable trees: > > + > > +Backport: 4.9+ > > + > > +It marks a commit for being a candidate for backports to all stable > > +trees from 4.9 onward. > > Using the wording "stable trees" may, to some, imply ones still > under maintenance. How about omitting "stable", or replacing it > by "released"? OK > > +The backport requester is expected to specify which currently supported > > +releases need the backport; but encouraged to specify a release as far > > +back as possible which applies. If the requester doesn't know the oldest > > +affected tree, they are encouraged to append a comment like the > > +following: > > + > > +Backport: 4.9+ # maybe older > > + > > +Maintainers request the Backport tag to be added on commit. Contributors > > +are welcome to mark their patches with the Backport tag when they deem > > +appropriate. Maintainers will request for it to be removed when that is > > +not the case. > > + > > +Please note that the Backport tag is a **request** for backport, which > > +will still need to be evaluated by the stable tree maintainers. > > +Maintainers might ask the requester to help with the backporting work if > > +it is not trivial. > > + > > +When possible, please use the Fixes tag instead. > > Maybe amend with "(or in addition)"? I'm thinking in particular > about a case where a buggy change was already backported, but > didn't show up yet in a release from the respective branch(es). Sure > Previously I did suggest to add an indication that people requesting > backports should also be prepare to actually help with backporting. > I don't recall a verbal reply, and I also don't see any respective > update here. (I'm not fully trusting our mail system, i.e. it may > very well be that I did miss a reply.) I didn't reply, but I added two lines in that regards, see also above: > > +Maintainers might ask the requester to help with the backporting work if > > +it is not trivial.
Re: [PATCH v3] Introduce a description of the Backport and Fixes tags
On Mon, 20 Apr 2020, Jan Beulich wrote:
> On 20.04.2020 12:27, Wei Liu wrote:
> > On Mon, Apr 20, 2020 at 10:31:28AM +0100, Julien Grall wrote:
> >> On 17/04/2020 23:24, Stefano Stabellini wrote:
> >>> Create a new document under docs/process to describe our special tags.
> >>> Add a description of the Fixes tag and the new Backport tag. Also
> >>> clarify that lines with tags should not be split.
> >>>
> >>> Signed-off-by: Stefano Stabellini
> >>> CC: Ian Jackson
> >>> CC: Wei Liu
> >>> CC: jbeul...@suse.com
> >>> CC: george.dun...@citrix.com
> >>> CC: jul...@xen.org
> >>> CC: lars.ku...@citrix.com
> >>> CC: andrew.coop...@citrix.com
> >>> CC: konrad.w...@oracle.com
> >>> ---
> >>> Removing Acks as I added the description of "Fixes"
> >>> ---
> >>> docs/process/tags.pandoc | 55
> >>> 1 file changed, 55 insertions(+)
> >>> create mode 100644 docs/process/tags.pandoc
> >>>
> >>> diff --git a/docs/process/tags.pandoc b/docs/process/tags.pandoc
> >>> new file mode 100644
> >>> index 00..06b06dda01
> >>> --- /dev/null
> >>> +++ b/docs/process/tags.pandoc
> >>> @@ -0,0 +1,55 @@
> >>> +Tags: No line splitting
> >>> +---
> >>> +Do not split a tag across multiple lines, tags are exempt from the
> >>> +"wrap at 75 columns" rule in order to simplify parsing scripts. For
> >>> +example:
> >>> +
> >>> +Fixes: 67d01cdb5 ("x86: infrastructure to allow converting
> >>> certain indirect calls to direct ones")
> >>
> >> The SHA-1 ID is 9 characters but...
> >>
> >>> +
> >>> +
> >>> +Fixes Tag
> >>> +-
> >>> +
> >>> +If your patch fixes a bug in a specific commit, e.g. you found an issue
> >>> using
> >>> +``git bisect``, please use the 'Fixes:' tag with the first 12 characters
> >>> of
> >>> +the SHA-1 ID, and the one line summary.
> >>
> >> ... you request 12 characters here. Can you make sure the two match please?
> >>
> >> However, I am not entirely sure why we should mandate 12 characters. With
> >> the title, you should always be able to find back the commit if there is a
> >> clash.
> >
> > This is copied from Linux's document.
> >
> > I normally use 8-9 characters, but I don't mind using 12 either.
>
> Are they still saying 9? I've been asked to switch to 12 several
> weeks back ...
Yes, I just took it from Linux. I don't care 9 or 12. Given the
preference for 12, I'll keep 12 in the text and update the example to
match.
[xen-unstable-smoke test] 149720: tolerable all pass - PUSHED
flight 149720 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/149720/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 4803a67114279a656a54a23cebed646da32efeb6 baseline version: xen 7aacf6ac49829d8dd6242f67460f4d52d0d36503 Last test of basis 149713 2020-04-21 11:00:34 Z0 days Testing same since 149720 2020-04-21 16:02:16 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Wei Liu jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 7aacf6ac49..4803a67114 4803a67114279a656a54a23cebed646da32efeb6 -> smoke
Re: [bug report] drm/xen-front: Add support for Xen PV display frontend
On Tue, Apr 21, 2020 at 05:29:02PM +0200, Julia Lawall wrote:
>
>
> On Tue, 21 Apr 2020, Dan Carpenter wrote:
>
> > Hi Kernel Janitors,
> >
> > Here is another idea that someone could work on, fixing the
> > IS_ERR_OR_NULL() checks in the xen driver.
> >
> > The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
> > display frontend" from Apr 3, 2018, leads to the following static
> > checker warning:
> >
> > drivers/gpu/drm/xen/xen_drm_front_gem.c:140 xen_drm_front_gem_create()
> > warn: passing zero to 'ERR_CAST'
> >
> > drivers/gpu/drm/xen/xen_drm_front_gem.c
> >133 struct drm_gem_object *xen_drm_front_gem_create(struct drm_device
> > *dev,
> >134 size_t size)
> >135 {
> >136 struct xen_gem_object *xen_obj;
> >137
> >138 xen_obj = gem_create(dev, size);
> >139 if (IS_ERR_OR_NULL(xen_obj))
> >140 return ERR_CAST(xen_obj);
>
> Are the other occurrences of this also a possible problem? There are a
> few others outside of xen.
We sometimes check a parameter for IS_ERR_OR_NULL().
void free_function(struct something *p)
{
if (IS_ERR_OR_NULL(p))
return;
}
That's fine, absolutely harmless and not a bug. But if we are checking
a return value like this then probably most of the time it's invalid
code. Normally it's again like this code where we're dealing with an
impossible thing because the return is never NULL. The common bugs are
that it returns NULL to a caller which only expects error pointers or it
returns success instead of failure. But sometimes returning success can
be valid:
obj = get_feature(dev);
if (IS_ERR_OR_NULL(obj))
return PTR_ERR(obj);
It deliberately returns success because the rest of the function is
useless when we don't have the feature.
regards,
dan carpenter
[PATCH v16 1/3] mem_sharing: fix sharability check during fork reset
When resetting a VM fork we ought to only remove pages that were allocated for
the fork during it's execution and the contents copied over from the parent.
This can be determined if the page is sharable as special pages used by the
fork for other purposes will not pass this test. Unfortunately during the fork
reset loop we only partially check whether that's the case. A page's type may
indicate it is sharable (pass p2m_is_sharable) but that's not a sufficient
check by itself. All checks that are normally performed before a page is
converted to the sharable type need to be performed to avoid removing pages
from the p2m that may be used for other purposes. For example, currently the
reset loop also removes the vcpu info pages from the p2m, potentially putting
the guest into infinite page-fault loops.
For this we extend the existing nominate_page and page_make_sharable functions
to perform a validation-only run without actually converting the page.
Signed-off-by: Tamas K Lengyel
---
xen/arch/x86/mm/mem_sharing.c | 79 ++-
1 file changed, 50 insertions(+), 29 deletions(-)
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index e572e9e39d..d8ed660abb 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -633,31 +633,35 @@ unsigned int mem_sharing_get_nr_shared_mfns(void)
/* Functions that change a page's type and ownership */
static int page_make_sharable(struct domain *d,
struct page_info *page,
- int expected_refcnt)
+ int expected_refcnt,
+ bool validate_only)
{
-bool_t drop_dom_ref;
+int rc;
+bool drop_dom_ref = false;
-spin_lock(>page_alloc_lock);
+/* caller already has the lock when validating only */
+if ( !validate_only )
+spin_lock(>page_alloc_lock);
if ( d->is_dying )
{
-spin_unlock(>page_alloc_lock);
-return -EBUSY;
+rc = -EBUSY;
+goto out;
}
/* Change page type and count atomically */
if ( !get_page_and_type(page, d, PGT_shared_page) )
{
-spin_unlock(>page_alloc_lock);
-return -EINVAL;
+rc = -EINVAL;
+goto out;
}
/* Check it wasn't already sharable and undo if it was */
if ( (page->u.inuse.type_info & PGT_count_mask) != 1 )
{
-spin_unlock(>page_alloc_lock);
put_page_and_type(page);
-return -EEXIST;
+rc = -EEXIST;
+goto out;
}
/*
@@ -666,20 +670,31 @@ static int page_make_sharable(struct domain *d,
*/
if ( page->count_info != (PGC_allocated | (2 + expected_refcnt)) )
{
-spin_unlock(>page_alloc_lock);
/* Return type count back to zero */
put_page_and_type(page);
-return -E2BIG;
+rc = -E2BIG;
+goto out;
+}
+
+rc = 0;
+
+if ( validate_only )
+{
+put_page_and_type(page);
+goto out;
}
page_set_owner(page, dom_cow);
drop_dom_ref = !domain_adjust_tot_pages(d, -1);
page_list_del(page, >page_list);
-spin_unlock(>page_alloc_lock);
+out:
+if ( !validate_only )
+spin_unlock(>page_alloc_lock);
if ( drop_dom_ref )
put_domain(d);
-return 0;
+
+return rc;
}
static int page_make_private(struct domain *d, struct page_info *page)
@@ -809,8 +824,8 @@ static int debug_gref(struct domain *d, grant_ref_t ref)
return debug_gfn(d, gfn);
}
-static int nominate_page(struct domain *d, gfn_t gfn,
- int expected_refcnt, shr_handle_t *phandle)
+static int nominate_page(struct domain *d, gfn_t gfn, int expected_refcnt,
+ bool validate_only, shr_handle_t *phandle)
{
struct p2m_domain *hp2m = p2m_get_hostp2m(d);
p2m_type_t p2mt;
@@ -879,8 +894,8 @@ static int nominate_page(struct domain *d, gfn_t gfn,
}
/* Try to convert the mfn to the sharable type */
-ret = page_make_sharable(d, page, expected_refcnt);
-if ( ret )
+ret = page_make_sharable(d, page, expected_refcnt, validate_only);
+if ( ret || validate_only )
goto out;
/*
@@ -1392,13 +1407,13 @@ static int range_share(struct domain *d, struct domain
*cd,
* We only break out if we run out of memory as individual pages may
* legitimately be unsharable and we just want to skip over those.
*/
-rc = nominate_page(d, _gfn(start), 0, );
+rc = nominate_page(d, _gfn(start), 0, false, );
if ( rc == -ENOMEM )
break;
if ( !rc )
{
-rc = nominate_page(cd, _gfn(start), 0, );
+rc = nominate_page(cd, _gfn(start), 0, false, );
if ( rc == -ENOMEM )
break;
@@ -1476,7 +1491,7 @@ int mem_sharing_fork_page(struct domain *d, gfn_t gfn,
bool unsharing)
/* For read-only
[PATCH v16 3/3] xen/tools: VM forking toolstack side
Add necessary bits to implement "xl fork-vm" commands. The command allows the
user to specify how to launch the device model allowing for a late-launch model
in which the user can execute the fork without the device model and decide to
only later launch it.
Signed-off-by: Tamas K Lengyel
---
docs/man/xl.1.pod.in | 44 +
tools/libxc/include/xenctrl.h | 14 ++
tools/libxc/xc_memshr.c | 26 +++
tools/libxl/libxl.h | 12 ++
tools/libxl/libxl_create.c| 361 +++---
tools/libxl/libxl_dm.c| 2 +-
tools/libxl/libxl_dom.c | 43 +++-
tools/libxl/libxl_internal.h | 7 +
tools/libxl/libxl_types.idl | 1 +
tools/libxl/libxl_x86.c | 42
tools/xl/Makefile | 2 +-
tools/xl/xl.h | 5 +
tools/xl/xl_cmdtable.c| 15 ++
tools/xl/xl_forkvm.c | 149 ++
tools/xl/xl_vmcontrol.c | 14 ++
15 files changed, 571 insertions(+), 166 deletions(-)
create mode 100644 tools/xl/xl_forkvm.c
diff --git a/docs/man/xl.1.pod.in b/docs/man/xl.1.pod.in
index 09339282e6..59c03c6427 100644
--- a/docs/man/xl.1.pod.in
+++ b/docs/man/xl.1.pod.in
@@ -708,6 +708,50 @@ above).
=back
+=item B [I] I
+
+Create a fork of a running VM. The domain will be paused after the operation
+and remains paused while forks of it exist. Experimental and x86 only.
+Forks can only be made of domains with HAP enabled and on Intel hardware. The
+parent domain must be created with the xl toolstack and its configuration must
+not manually define max_grant_frames, max_maptrack_frames or
max_event_channels.
+
+B
+
+=over 4
+
+=item B<-p>
+
+Leave the fork paused after creating it.
+
+=item B<--launch-dm>
+
+Specify whether the device model (QEMU) should be launched for the fork. Late
+launch allows to start the device model for an already running fork.
+
+=item B<-C>
+
+The config file to use when launching the device model. Currently required
when
+launching the device model. Most config settings MUST match the parent domain
+exactly, only change VM name, disk path and network configurations.
+
+=item B<-Q>
+
+The path to the qemu save file to use when launching the device model.
Currently
+required when launching the device model.
+
+=item B<--fork-reset>
+
+Perform a reset operation of an already running fork. Note that resetting may
+be less performant then creating a new fork depending on how much memory the
+fork has deduplicated during its runtime.
+
+=item B<--max-vcpus>
+
+Specify the max-vcpus matching the parent domain when not launching the dm.
+
+=back
+
=item B [I]
Display the number of shared pages for a specified domain. If no domain is
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 5f25c5a6d4..0a6ff93229 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -2232,6 +2232,20 @@ int xc_memshr_range_share(xc_interface *xch,
uint64_t first_gfn,
uint64_t last_gfn);
+int xc_memshr_fork(xc_interface *xch,
+ uint32_t source_domain,
+ uint32_t client_domain,
+ bool allow_with_iommu);
+
+/*
+ * Note: this function is only intended to be used on short-lived forks that
+ * haven't yet aquired a lot of memory. In case the fork has a lot of memory
+ * it is likely more performant to create a new fork with xc_memshr_fork.
+ *
+ * With VMs that have a lot of memory this call may block for a long time.
+ */
+int xc_memshr_fork_reset(xc_interface *xch, uint32_t forked_domain);
+
/* Debug calls: return the number of pages referencing the shared frame backing
* the input argument. Should be one or greater.
*
diff --git a/tools/libxc/xc_memshr.c b/tools/libxc/xc_memshr.c
index 97e2e6a8d9..2300cc7075 100644
--- a/tools/libxc/xc_memshr.c
+++ b/tools/libxc/xc_memshr.c
@@ -239,6 +239,32 @@ int xc_memshr_debug_gref(xc_interface *xch,
return xc_memshr_memop(xch, domid, );
}
+int xc_memshr_fork(xc_interface *xch, uint32_t pdomid, uint32_t domid,
+ bool allow_with_iommu)
+{
+xen_mem_sharing_op_t mso;
+
+memset(, 0, sizeof(mso));
+
+mso.op = XENMEM_sharing_op_fork;
+mso.u.fork.parent_domain = pdomid;
+
+if ( allow_with_iommu )
+mso.u.fork.flags |= XENMEM_FORK_WITH_IOMMU_ALLOWED;
+
+return xc_memshr_memop(xch, domid, );
+}
+
+int xc_memshr_fork_reset(xc_interface *xch, uint32_t domid)
+{
+xen_mem_sharing_op_t mso;
+
+memset(, 0, sizeof(mso));
+mso.op = XENMEM_sharing_op_fork_reset;
+
+return xc_memshr_memop(xch, domid, );
+}
+
int xc_memshr_audit(xc_interface *xch)
{
xen_mem_sharing_op_t mso;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 71709dc585..d8da347d4e 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -2666,6 +2666,18 @@ int libxl_psr_get_hw_info(libxl_ctx *ctx,
libxl_psr_feat_type type,
[PATCH v16 0/3] VM forking
The following patches are part of the series that implement VM forking for
Intel HVM guests to allow for the fast creation of identical VMs without the
assosciated high startup costs of booting or restoring the VM from a savefile.
JIRA issue: https://xenproject.atlassian.net/browse/XEN-89
The fork operation is implemented as part of the "xl fork-vm" command:
xl fork-vm -C -Q -m
By default a fully functional fork is created. The user is in charge however to
create the appropriate config file for the fork and to generate the QEMU save
file before the fork-vm call is made. The config file needs to give the
fork a new name at minimum but other settings may also require changes. Certain
settings in the config file of both the parent and the fork have to be set to
default. Details are documented.
The interface also allows to split the forking into two steps:
xl fork-vm --launch-dm no \
-m \
-p
xl fork-vm --launch-dm late \
-C \
-Q \
The split creation model is useful when the VM needs to be created as fast as
possible. The forked VM can be unpaused without the device model being launched
to be monitored and accessed via VMI. Note however that without its device
model running (depending on what is executing in the VM) it is bound to
misbehave or even crash when its trying to access devices that would be
emulated by QEMU. We anticipate that for certain use-cases this would be an
acceptable situation, in case for example when fuzzing is performed of code
segments that don't access such devices.
Launching the device model requires the QEMU Xen savefile to be generated
manually from the parent VM. This can be accomplished simply by connecting to
its QMP socket and issuing the "xen-save-devices-state" command. For example
using the standard tool socat these commands can be used to generate the file:
socat - UNIX-CONNECT:/var/run/xen/qmp-libxl-
{ "execute": "qmp_capabilities" }
{ "execute": "xen-save-devices-state", \
"arguments": { "filename": "/path/to/save/qemu_state", \
"live": false} }
At runtime the forked VM starts running with an empty p2m which gets lazily
populated when the VM generates EPT faults, similar to how altp2m views are
populated. If the memory access is a read-only access, the p2m entry is
populated with a memory shared entry with its parent. For write memory accesses
or in case memory sharing wasn't possible (for example in case a reference is
held by a third party), a new page is allocated and the page contents are
copied over from the parent VM. Forks can be further forked if needed, thus
allowing for further memory savings.
A VM fork reset hypercall is also added that allows the fork to be reset to the
state it was just after a fork, also accessible via xl:
xl fork-vm --fork-reset -p
This is an optimization for cases where the forks are very short-lived and run
without a device model, so resetting saves some time compared to creating a
brand new fork provided the fork has not aquired a lot of memory. If the fork
has a lot of memory deduplicated it is likely going to be faster to create a
new fork from scratch and asynchronously destroying the old one.
The series has been tested with Windows VMs and functions as expected. Linux
VMs when forked from a running VM will have a frozen VNC screen. Linux VMs at
this time can only be forked with a working device model when the parent VM was
restored from a snapshot using "xl restore -p". This is a known limitation.
Also note that PVHVM/PVH Linux guests have not been tested. Forking most likely
works but PV devices and drivers would require additional wiring to set things
up properly since the guests are unaware of the forking taking place, unlike
the save/restore routine where the guest is made aware of the procedure.
Forking time has been measured to be 0.0007s, device model launch to be around
1s depending largely on the number of devices being emulated. Fork resets have
been measured to be 0.0001s under the optimal circumstances.
New in v16:
A better bugfix for fork reset issue
Minor fixes for the IOMMU allow patch based on feedback
Patch 1 fix for VM fork reset removing pages from the p2m that it shouldn't
Patch 2 adds option to fork a domain with IOMMU active
Patch 3 adds the toolstack-side code implementing VM forking and reset
Tamas K Lengyel (3):
mem_sharing: fix sharability check during fork reset
mem_sharing: allow forking domain with IOMMU enabled
xen/tools: VM forking toolstack side
docs/man/xl.1.pod.in | 44 +
tools/libxc/include/xenctrl.h | 14 ++
tools/libxc/xc_memshr.c | 26 +++
tools/libxl/libxl.h | 12 ++
tools/libxl/libxl_create.c| 361 +++---
tools/libxl/libxl_dm.c| 2 +-
tools/libxl/libxl_dom.c | 43 +++-
tools/libxl/libxl_internal.h | 7 +
tools/libxl/libxl_types.idl |
[PATCH v16 2/3] mem_sharing: allow forking domain with IOMMU enabled
The memory sharing subsystem by default doesn't allow a domain to share memory
if it has an IOMMU active for obvious security reasons. However, when fuzzing a
VM fork, the same security restrictions don't necessarily apply. While it makes
no sense to try to create a full fork of a VM that has an IOMMU attached as only
one domain can own the pass-through device at a time, creating a shallow fork
without a device model is still very useful for fuzzing kernel-mode drivers.
By allowing the parent VM to initialize the kernel-mode driver with a real
device that's pass-through, the driver can enter into a state more suitable for
fuzzing. Some of these initialization steps are quite complex and are easier to
perform when a real device is present. After the initialization, shallow forks
can be utilized for fuzzing code-segments in the device driver that don't
directly interact with the device.
Signed-off-by: Tamas K Lengyel
---
v16: Minor fixes based on feedback
---
xen/arch/x86/mm/mem_sharing.c | 20 +---
xen/include/public/memory.h | 4 +++-
2 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
index d8ed660abb..e690d2fa13 100644
--- a/xen/arch/x86/mm/mem_sharing.c
+++ b/xen/arch/x86/mm/mem_sharing.c
@@ -1445,7 +1445,8 @@ static int range_share(struct domain *d, struct domain
*cd,
return rc;
}
-static inline int mem_sharing_control(struct domain *d, bool enable)
+static inline int mem_sharing_control(struct domain *d, bool enable,
+ uint16_t flags)
{
if ( enable )
{
@@ -1455,7 +1456,8 @@ static inline int mem_sharing_control(struct domain *d,
bool enable)
if ( unlikely(!hap_enabled(d)) )
return -ENODEV;
-if ( unlikely(is_iommu_enabled(d)) )
+if ( unlikely(is_iommu_enabled(d) &&
+ !(flags & XENMEM_FORK_WITH_IOMMU_ALLOWED)) )
return -EXDEV;
}
@@ -1848,7 +1850,8 @@ int
mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
if ( rc )
goto out;
-if ( !mem_sharing_enabled(d) && (rc = mem_sharing_control(d, true)) )
+if ( !mem_sharing_enabled(d) &&
+ (rc = mem_sharing_control(d, true, 0)) )
return rc;
switch ( mso.op )
@@ -2086,7 +2089,9 @@ int
mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
struct domain *pd;
rc = -EINVAL;
-if ( mso.u.fork.pad[0] || mso.u.fork.pad[1] || mso.u.fork.pad[2] )
+if ( mso.u.fork.pad )
+goto out;
+if ( mso.u.fork.flags & ~XENMEM_FORK_WITH_IOMMU_ALLOWED )
goto out;
rc = rcu_lock_live_remote_domain_by_id(mso.u.fork.parent_domain,
@@ -2101,7 +2106,8 @@ int
mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
goto out;
}
-if ( !mem_sharing_enabled(pd) && (rc = mem_sharing_control(pd, true)) )
+if ( !mem_sharing_enabled(pd) &&
+ (rc = mem_sharing_control(pd, true, mso.u.fork.flags)) )
{
rcu_unlock_domain(pd);
goto out;
@@ -2122,7 +2128,7 @@ int
mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
struct domain *pd;
rc = -EINVAL;
-if ( mso.u.fork.pad[0] || mso.u.fork.pad[1] || mso.u.fork.pad[2] )
+if ( mso.u.fork.pad || mso.u.fork.flags )
goto out;
rc = -ENOSYS;
@@ -2159,7 +2165,7 @@ int mem_sharing_domctl(struct domain *d, struct
xen_domctl_mem_sharing_op *mec)
switch ( mec->op )
{
case XEN_DOMCTL_MEM_SHARING_CONTROL:
-rc = mem_sharing_control(d, mec->u.enable);
+rc = mem_sharing_control(d, mec->u.enable, 0);
break;
default:
diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
index d36d64b8dc..e56800357d 100644
--- a/xen/include/public/memory.h
+++ b/xen/include/public/memory.h
@@ -536,7 +536,9 @@ struct xen_mem_sharing_op {
} debug;
struct mem_sharing_op_fork { /* OP_FORK */
domid_t parent_domain;/* IN: parent's domain id */
-uint16_t pad[3]; /* Must be set to 0 */
+#define XENMEM_FORK_WITH_IOMMU_ALLOWED (1u << 0)
+uint16_t flags; /* IN: optional settings */
+uint32_t pad; /* Must be set to 0 */
} fork;
} u;
};
--
2.20.1
Re: [PATCH v2 4/4] x86: adjustments to guest handle treatment
On Tue, Apr 21, 2020 at 11:13:23AM +0200, Jan Beulich wrote:
> First of all avoid excessive conversions. copy_{from,to}_guest(), for
> example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
I'm not sure I understand the difference between those two, as they
are both placeholders for linear guest addresses?
AFAICT XEN_GUEST_HANDLE should be used for guest pointers inside of an
hypercall struct, while XEN_GUEST_HANDLE_PARAM is for guest pointers
as hypercall arguments. But those are both just guest pointers,
whether they are a parameter to the hypercall or a field in a
struct, and hence could use the same type?
I assume there's some reason for not doing so, and I see the comment
about other arches, but again a linear guest address is just that in
all arches, regardless of it's placement.
Sorry, this is likely tangential to your patch.
Thanks, Roger.
Re: [PATCH OSSTEST v3 2/2] make-flight: add a core scheduling job
On Tue, Apr 21, 2020 at 05:39:54PM +0100, Ian Jackson wrote:
> Roger Pau Monne writes ("[PATCH OSSTEST v3 2/2] make-flight: add a core
> scheduling job"):
> > Run a simple core scheduling tests on a host that has SMT support.
> > This is only enabled for Xen >= 4.13.
>
> Thanks, pushed to pretest.
>
> Once it makes it through staging the test will appear, but it will
> start failing (nonblockingly) until the SMT hostflag has been scanned.
> It would be sensible for me to run a special examine job for that.
Yes, I think so. Will try to keep an eye and ping you when it passes
osstest self push gate.
Thanks, Roger.
Re: [PATCH v2 1/4] x86/mm: no-one passes a NULL domain to init_xen_l4_slots()
On Tue, Apr 21, 2020 at 11:11:03AM +0200, Jan Beulich wrote:
> Drop the NULL checks - they've been introduced by commit 8d7b633ada
> ("x86/mm: Consolidate all Xen L4 slot writing into
> init_xen_l4_slots()") for no apparent reason.
>
> Signed-off-by: Jan Beulich
Reviewed-by: Roger Pau Monné
Thanks.
Re: [PATCH OSSTEST v3 2/2] make-flight: add a core scheduling job
Roger Pau Monne writes ("[PATCH OSSTEST v3 2/2] make-flight: add a core
scheduling job"):
> Run a simple core scheduling tests on a host that has SMT support.
> This is only enabled for Xen >= 4.13.
Thanks, pushed to pretest.
Once it makes it through staging the test will appear, but it will
start failing (nonblockingly) until the SMT hostflag has been scanned.
It would be sensible for me to run a special examine job for that.
Ian.
RE: [PATCH 0/3] Cleanup IOREQ server on exit
Ping v2?
> -Original Message-
> From: Maximilian Heyne
> Sent: 07 April 2020 10:16
> To: xen-devel@lists.xenproject.org
> Cc: Ian Jackson ; Paul Durrant
> Subject: Re: [PATCH 0/3] Cleanup IOREQ server on exit
>
> Could someone please have a look at this patch? It solves an actual issue:
> Try soft-reset with qemu-xen-traditional and it will fail.
>
> On 3/13/20 1:33 PM, Maximilian Heyne wrote:
> > Following up on commit 9c0eed61 ("qemu-trad: stop using the default IOREQ
> > server"), clean up the IOREQ server on exit. This fixes a bug with
> > soft-reset
> > that shows up as "bind interdomain ioctl error 22" because the event
> > channels
> > were not closed at the soft-reset and can't be bound again.
> >
> > For this I used the exit notifiers from QEMU that I backported together
> > with the
> > required generic notifier lists.
> >
> > Anthony Liguori (1):
> >Add support for generic notifier lists
> >
> > Gerd Hoffmann (1):
> >Add exit notifiers.
> >
> > Maximilian Heyne (1):
> >xen: cleanup IOREQ server on exit
> >
> > Makefile| 1 +
> > hw/xen_machine_fv.c | 11 +++
> > notify.c| 39 +++
> > notify.h| 43 +++
> > sys-queue.h | 5 +
> > sysemu.h| 5 +
> > vl.c| 20
> > 7 files changed, 124 insertions(+)
> > create mode 100644 notify.c
> > create mode 100644 notify.h
> >
>
>
>
> Amazon Development Center Germany GmbH
> Krausenstr. 38
> 10117 Berlin
> Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
> Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
> Sitz: Berlin
> Ust-ID: DE 289 237 879
>
[XEN PATCH v5 12/16] xen/build: Use if_changed for prelink*.o
We change the dependencies of prelink-efi.o so that we can use the same command line. The dependency on efi/built_in.o isn't needed because, we already have: efi/*.o: efi/built_in.o to build efi/*.o files that prelink-efi.o needs. Signed-off-by: Anthony PERARD Reviewed-by: Roger Pau Monné Acked-by: Jan Beulich --- Notes: v4: - fix rebuild, prelink.o and prelink-efi.o needs to be in targets xen/arch/x86/Makefile | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 92430192a74e..33e9ebf25074 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -128,11 +128,13 @@ prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o prelink-efi.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink-efi_lto.o efi/boot.init.o $(LD) $(XEN_LDFLAGS) -r -o $@ $^ else -prelink.o: $(ALL_OBJS) - $(LD) $(XEN_LDFLAGS) -r -o $@ $^ +prelink.o: $(ALL_OBJS) FORCE + $(call if_changed,ld) + +prelink-efi.o: $(filter-out %/efi/built_in.o,$(ALL_OBJS)) efi/boot.init.o efi/runtime.o efi/compat.o FORCE + $(call if_changed,ld) -prelink-efi.o: $(ALL_OBJS) efi/boot.init.o efi/runtime.o efi/compat.o - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out %/efi/built_in.o,$^) +targets += prelink.o prelink-efi.o endif $(TARGET)-syms: prelink.o xen.lds -- Anthony PERARD
[XEN PATCH v5 08/16] build: Introduce $(cpp_flags)
Signed-off-by: Anthony PERARD --- Notes: v5: - new patch xen/Rules.mk | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index 2e28c572305a..25bcf45612bd 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -123,6 +123,7 @@ $(obj-bin-y): XEN_CFLAGS := $(filter-out -flto,$(XEN_CFLAGS)) c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_CFLAGS) '-D__OBJECT_FILE__="$@"' a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(XEN_AFLAGS) +cpp_flags = $(filter-out -Wa$(comma)%,$(a_flags)) include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk @@ -207,7 +208,7 @@ quiet_cmd_cc_s_c = CC $@ cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@ quiet_cmd_s_S = CPP $@ -cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@ +cmd_s_S = $(CPP) $(cpp_flags) $< -o $@ %.i: %.c FORCE $(call if_changed,cpp_i_c) -- Anthony PERARD
[XEN PATCH v5 11/16] xen/build: factorise generation of the linker scripts
In Arm and X86 makefile, generating the linker script is the same, so we can simply have both call the same macro. We need to add *.lds files into extra-y so that Rules.mk can find the .*.cmd dependency file and load it. Change made to the command line: - Use of $(CPP) instead of $(CC) -E - Remove -Ui386. We don't compile Xen for i386 anymore, so that macro is never defined. Also it doesn't make sense on Arm. - Use $(cpp_flags) which simply filter -Wa,% options from $(a_flags). Signed-off-by: Anthony PERARD --- Notes: v5: - rename cc_lds_S to cpp_lds_S as the binary runned is now CPP - Use new cpp_flags instead of the open-coded filter of a_flags. v4: - fix rebuild by adding FORCE as dependency - Use $(CPP) - remove -Ui386 - avoid using "define" for cmd_cc_lds_S, as adding '; \' on each line is still mandatory for if_changed (or cmd) macro to work. xen/Rules.mk | 6 ++ xen/arch/arm/Makefile | 8 xen/arch/x86/Makefile | 8 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index 9150911296de..877f52d871fa 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -236,6 +236,12 @@ cmd_s_S = $(CPP) $(cpp_flags) $< -o $@ %.s: %.S FORCE $(call if_changed,cpp_s_S) +# Linker scripts, .lds.S -> .lds +quiet_cmd_cpp_lds_S = LDS $@ +cmd_cpp_lds_S = $(CPP) -P $(cpp_flags) -o $@ $<; \ +sed -e 's/.*\.lds\.o:/$(@F):/g' <$(dot-target).d >$(dot-target).d.new; \ +mv -f $(dot-target).d.new $(dot-target).d + # Add intermediate targets: # When building objects with specific suffix patterns, add intermediate # targets that the final targets are derived from. diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index b79ad55646bd..68cb258870eb 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -64,6 +64,8 @@ obj-y += vpsci.o obj-y += vuart.o extra-y += $(TARGET_SUBARCH)/head.o +extra-y += xen.lds + #obj-bin-y += o ifdef CONFIG_DTB_FILE @@ -122,10 +124,8 @@ $(TARGET)-syms: prelink.o xen.lds asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c $(CC) $(filter-out -flto,$(c_flags)) -S -o $@ $< -xen.lds: xen.lds.S - $(CC) -P -E -Ui386 $(a_flags) -o $@ $< - sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new - mv -f .xen.lds.d.new .xen.lds.d +xen.lds: xen.lds.S FORCE + $(call if_changed,cpp_lds_S) dtb.o: $(CONFIG_DTB_FILE) diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile index 44137d919b66..92430192a74e 100644 --- a/xen/arch/x86/Makefile +++ b/xen/arch/x86/Makefile @@ -72,6 +72,7 @@ obj-y += hpet.o obj-y += vm_event.o obj-y += xstate.o extra-y += asm-macros.i +extra-y += xen.lds x86_emulate.o: x86_emulate/x86_emulate.c x86_emulate/x86_emulate.h @@ -173,6 +174,7 @@ export XEN_BUILD_EFI := $(shell $(CC) $(XEN_CFLAGS) -c efi/check.c -o efi/check. # Check if the linker supports PE. XEN_BUILD_PE := $(if $(XEN_BUILD_EFI),$(shell $(LD) -mi386pep --subsystem=10 -o efi/check.efi efi/check.o 2>/dev/null && echo y)) CFLAGS-$(XEN_BUILD_EFI) += -DXEN_BUILD_EFI +extra-$(XEN_BUILD_PE) += efi.lds $(TARGET).efi: VIRT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A VIRT_START$$,,p') $(TARGET).efi: ALT_BASE = 0x$(shell $(NM) efi/relocs-dummy.o | sed -n 's, A ALT_START$$,,p') @@ -240,10 +242,8 @@ $(BASEDIR)/include/asm-x86/asm-macros.h: asm-macros.i Makefile $(call move-if-changed,$@.new,$@) efi.lds: AFLAGS-y += -DEFI -xen.lds efi.lds: xen.lds.S - $(CC) -P -E -Ui386 $(filter-out -Wa$(comma)%,$(a_flags)) -o $@ $< - sed -e 's/.*\.lds\.o:/$(@F):/g' <.$(@F).d >.$(@F).d.new - mv -f .$(@F).d.new .$(@F).d +xen.lds efi.lds: xen.lds.S FORCE + $(call if_changed,cpp_lds_S) boot/mkelf32: boot/mkelf32.c $(HOSTCC) $(HOSTCFLAGS) -o $@ $< -- Anthony PERARD
[XEN PATCH v5 02/16] xen/build: include include/config/auto.conf in main Makefile
We are going to generate the CFLAGS early from "xen/Makefile" instead of in "Rules.mk", but we need to include "config/auto.conf", so include it in "Makefile". Before including "config/auto.conf" we check which make target a user is calling, as some targets don't need "auto.conf". For targets that needs auto.conf, make will generate it (and a default .config if missing). root-make-done is to avoid doing the calculation again once Rules.mk takes over and is been executed with the root Makefile. When Rules.mk is including xen/Makefile, `config-build' and `need-config' are undefined so auto.conf will not be included again (it is already included by Rules.mk) and kconfig target are out of reach of Rules.mk. We are introducing a target %config to catch all targets for kconfig. So we need an extra target %/.config to prevent make from trying to regenerate $(XEN_ROOT)/.config that is included in Config.mk. The way targets are filtered is inspired by Kbuild, with some code imported from Linux. That's why there is PHONY variable that isn't used yet, for example. Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich --- Notes: v5: - move kconfig shorthand to the only file that uses it. v4: - check that root-make-done hasn't been set to an expected value instead of checking if it has been set at all. - Add a shorthand $(kconfig) to run kconfig targets. v3: - filter only for %config instead of both config %config - keep the multi-target pattern rule trick for include/config/auto.conf instead of using Linux's newer pattern (we dont have tristate.conf so don't need to change it) - use y/n for root-make-done, config-build, need-config instead of relying on ifdef and ifndef and on assigning an empty value meaning undef - use space for indentation - explain why %/.config is suddenly needed. xen/Makefile | 101 +++ 1 file changed, 78 insertions(+), 23 deletions(-) diff --git a/xen/Makefile b/xen/Makefile index e5f7b1ae13bc..643c689658f3 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -49,7 +49,76 @@ default: build .PHONY: dist dist: install -build install:: include/config/auto.conf + +ifneq ($(root-make-done),y) +# section to run before calling Rules.mk, but only once. +# +# To make sure we do not include .config for any of the *config targets +# catch them early, and hand them over to tools/kconfig/Makefile + +clean-targets := %clean +no-dot-config-targets := $(clean-targets) \ + uninstall debug cloc \ + cscope TAGS tags MAP gtags \ + xenversion + +config-build:= n +need-config := y + +ifneq ($(filter $(no-dot-config-targets), $(MAKECMDGOALS)),) +ifeq ($(filter-out $(no-dot-config-targets), $(MAKECMDGOALS)),) +need-config := n +endif +endif + +ifneq ($(filter %config,$(MAKECMDGOALS)),) +config-build := y +endif + +export root-make-done := y +endif # root-make-done + +include scripts/Kbuild.include + +# Shorthand for kconfig +kconfig = -f $(BASEDIR)/tools/kconfig/Makefile.kconfig ARCH=$(ARCH) SRCARCH=$(SRCARCH) HOSTCC="$(HOSTCC)" HOSTCXX="$(HOSTCXX)" + +ifeq ($(config-build),y) +# === +# *config targets only - make sure prerequisites are updated, and descend +# in tools/kconfig to make the *config target + +config: FORCE + $(MAKE) $(kconfig) $@ + +# Config.mk tries to include .config file, don't try to remake it +%/.config: ; + +%config: FORCE + $(MAKE) $(kconfig) $@ + +else # !config-build + +ifeq ($(need-config),y) +include include/config/auto.conf +# Read in dependencies to all Kconfig* files, make sure to run syncconfig if +# changes are detected. +include include/config/auto.conf.cmd + +# Allow people to just run `make` as before and not force them to configure +$(KCONFIG_CONFIG): + $(MAKE) $(kconfig) defconfig + +# The actual configuration files used during the build are stored in +# include/generated/ and include/config/. Update them if .config is newer than +# include/config/auto.conf (which mirrors .config). +# +# This exploits the 'multi-target pattern rule' trick. +# The syncconfig should be executed only once to make all the targets. +include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG) + $(MAKE) $(kconfig) syncconfig + +endif # need-config .PHONY: build install uninstall clean distclean MAP build install uninstall debug clean distclean MAP:: @@ -254,9 +323,6 @@ cscope: _MAP: $(NM) -n $(TARGET)-syms | grep -v '\(compiled\)\|\(\.o$$\)\|\( [aUw] \)\|\(\.\.ng$$\)\|\(LASH[RL]DI\)' > System.map -.PHONY: FORCE -FORCE: - %.o %.i %.s: %.c FORCE $(MAKE) -f $(BASEDIR)/Rules.mk -C $(*D) $(@F) @@ -277,25 +343,6 @@ $(foreach base,arch/x86/mm/guest_walk_% \ arch/x86/mm/shadow/guest_%, \ $(foreach ext,o i s,$(call
[XEN PATCH v5 05/16] build: Introduce documentation for xen Makefiles
This start explainning the variables that can be used in the many
Makefiles in xen/.
Most of the document copies and modifies text from Linux v5.4 document
linux.git/Documentation/kbuild/makefiles.rst. Modification are mostly
to avoid mentioning kbuild. Thus I've added the SPDX tag which was
only in index.rst in linux.git.
Signed-off-by: Anthony PERARD
Acked-by: Jan Beulich
---
Notes:
v4:
- new patch
docs/misc/xen-makefiles/makefiles.rst | 87 +++
xen/Rules.mk | 4 ++
2 files changed, 91 insertions(+)
create mode 100644 docs/misc/xen-makefiles/makefiles.rst
diff --git a/docs/misc/xen-makefiles/makefiles.rst
b/docs/misc/xen-makefiles/makefiles.rst
new file mode 100644
index ..a86e3a612d61
--- /dev/null
+++ b/docs/misc/xen-makefiles/makefiles.rst
@@ -0,0 +1,87 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=
+Xen Makefiles
+=
+
+Documentation for the build system of Xen, found in xen.git/xen/.
+
+Makefile files
+==
+
+Description of the syntax that can be used in most Makefiles named
+'Makefile'. ('xen/Makefile' isn't part of the description.)
+
+'Makefile's are consumed by 'Rules.mk' when building.
+
+Goal definitions
+
+
+ Goal definitions are the main part (heart) of the Makefile.
+ These lines define the files to be built, any special compilation
+ options, and any subdirectories to be entered recursively.
+
+ The most simple makefile contains one line:
+
+ Example::
+
+ obj-y += foo.o
+
+ This tells the build system that there is one object in that
+ directory, named foo.o. foo.o will be built from foo.c or foo.S.
+
+ The following pattern is often used to have object selected
+ depending on the configuration:
+
+ Example::
+
+ obj-$(CONFIG_FOO) += foo.o
+
+ $(CONFIG_FOO) can evaluates to y.
+ If CONFIG_FOO is not y, then the file will not be compiled nor linked.
+
+Descending down in directories
+--
+
+ A Makefile is only responsible for building objects in its own
+ directory. Files in subdirectories should be taken care of by
+ Makefiles in these subdirs. The build system will automatically
+ invoke make recursively in subdirectories, provided you let it know of
+ them.
+
+ To do so, obj-y is used.
+ acpi lives in a separate directory, and the Makefile present in
+ drivers/ tells the build system to descend down using the following
+ assignment.
+
+ Example::
+
+ #drivers/Makefile
+ obj-$(CONFIG_ACPI) += acpi/
+
+ If CONFIG_ACPI is set to 'y'
+ the corresponding obj- variable will be set, and the build system
+ will descend down in the apci directory.
+ The build system only uses this information to decide that it needs
+ to visit the directory, it is the Makefile in the subdirectory that
+ specifies what is modular and what is built-in.
+
+ It is good practice to use a `CONFIG_` variable when assigning directory
+ names. This allows the build system to totally skip the directory if the
+ corresponding `CONFIG_` option is 'y'.
+
+Compilation flags
+-
+
+CFLAGS-y and AFLAGS-y
+ These two flags apply only to the makefile in which they
+ are assigned. They are used for all the normal cc, as and ld
+ invocations happening during a recursive build.
+
+ $(CFLAGS-y) is necessary because the top Makefile owns the
+ variable $(XEN_CFLAGS) and uses it for compilation flags for the
+ entire tree. And the variable $(CFLAGS) is modified by Config.mk
+ which evaluated in every subdirs.
+
+ CFLAGS-y specifies options for compiling with $(CC).
+ AFLAGS-y specifies assembler options.
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 8e4b9e3a4a82..9d1e1ebf5e44 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -1,3 +1,7 @@
+#
+# See docs/misc/xen-makefiles/makefiles.rst on variables that can be used in
+# Makefile and are consumed by Rules.mk
+#
-include $(BASEDIR)/include/config/auto.conf
--
Anthony PERARD
[XEN PATCH v5 10/16] xen/build: Use if_changed_rules with %.o:%.c targets
Use $(dot-target) to have the target name prefix with a dot. Now, when the CC command has run, it is recorded in .*.cmd file, then if_changed_rules will compare it on subsequent runs. Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich --- xen/Rules.mk | 26 +- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index 5e08e14455d7..9150911296de 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -173,19 +173,27 @@ FORCE: SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR)) -%.o: %.c Makefile +quiet_cmd_cc_o_c = CC $@ ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y) - $(CC) $(c_flags) -c $< -o $(@D)/.$(@F).tmp -MQ $@ -ifeq ($(CONFIG_CC_IS_CLANG),y) - $(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@ -else - $(OBJCOPY) --redefine-sym $(
[XEN PATCH v5 13/16] xen,symbols: rework file symbols selection
We want to use the same rune to build mm/*/guest_*.o as the one use to
build every other *.o object. The consequence it that file symbols that
the program ./symbols prefer changes with CONFIG_ENFORCE_UNIQUE_SYMBOLS=y.
For example, when building arch/x86/mm/guest_walk_2.o from guest_walk.c,
this would be the difference of file symbol present in the object when
building with CONFIG_ENFORCE_UNIQUE_SYMBOLS=y:
(1) Currently we have those two file symbols:
guest_walk.c
guest_walk_2.o
(2) When building with the same rune, we will have:
arch/x86/mm/guest_walk.c
guest_walk_2.o
The order in which those symbols are present may be different. Building
without CONFIG_ENFORCE_UNIQUE_SYMBOLS will result in (1).
Currently, in case (1) ./symbols chooses the *.o symbol (object file
name). But in case (2), may choose the *.c symbol (source file name with
path component) if it is first
We want to have ./symbols choose the object file name symbol in both
cases. So this patch changes that ./symbols prefer the "object file
name" symbol over the "source file name with path component" symbols.
The new intended order of preference is:
- first object file name symbol (present only in object files
produced from multiply-compiled sources)
- first source file name with path components symbol
- last source file name without any path component symbol
Signed-off-by: Anthony PERARD
---
Notes:
v5:
- reword part of the commit message
v4:
- rescope enum symbol_type
- remove setting values to enums, as it's not needed.
- rename the enumeration symbols
xen/tools/symbols.c | 20
1 file changed, 16 insertions(+), 4 deletions(-)
diff --git a/xen/tools/symbols.c b/xen/tools/symbols.c
index 9f9e2c990061..b3a9465b32d3 100644
--- a/xen/tools/symbols.c
+++ b/xen/tools/symbols.c
@@ -84,7 +84,12 @@ static int read_symbol(FILE *in, struct sym_entry *s)
{
char str[500], type[20] = "";
char *sym, stype;
- static enum { symbol, single_source, multi_source } last;
+ static enum symbol_type {
+ symbol,
+ file_source,
+ path_source,
+ obj_file,
+ } last;
static char *filename;
int rc = -1;
@@ -125,13 +130,20 @@ static int read_symbol(FILE *in, struct sym_entry *s)
* prefer the first one if that names an object file or has a
* directory component (to cover multiply compiled files).
*/
- bool multi = strchr(str, '/') || (sym && sym[1] == 'o');
+ enum symbol_type current;
- if (multi || last != multi_source) {
+ if (sym && sym[1] == 'o')
+ current = obj_file;
+ else if (strchr(str, '/'))
+ current = path_source;
+ else
+ current = file_source;
+
+ if (current > last || last == file_source) {
free(filename);
filename = *str ? strdup(str) : NULL;
+ last = current;
}
- last = multi ? multi_source : single_source;
goto skip_tail;
}
--
Anthony PERARD
[XEN PATCH v5 01/16] build,xsm: Fix multiple call
Both script mkflask.sh and mkaccess_vector.sh generates multiple files. Exploits the 'multi-target pattern rule' trick to call each scripts only once. Signed-off-by: Anthony PERARD Reviewed-by: Jan Beulich --- Notes: v5: - Use simpler $(subst ) instead of $(patsubst ) - moved the patch ahead in the series v4: - new patch xen/xsm/flask/Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/xen/xsm/flask/Makefile b/xen/xsm/flask/Makefile index b1fd45421993..f001bb18d4ed 100644 --- a/xen/xsm/flask/Makefile +++ b/xen/xsm/flask/Makefile @@ -21,10 +21,10 @@ ALL_H_FILES = $(FLASK_H_FILES) $(AV_H_FILES) $(obj-y) ss/built_in.o: $(ALL_H_FILES) -$(FLASK_H_FILES): $(FLASK_H_DEPEND) +$(subst include/,%/,$(FLASK_H_FILES)): $(FLASK_H_DEPEND) $(CONFIG_SHELL) policy/mkflask.sh $(AWK) include $(FLASK_H_DEPEND) -$(AV_H_FILES): $(AV_H_DEPEND) +$(subst include/,%/,$(AV_H_FILES)): $(AV_H_DEPEND) $(CONFIG_SHELL) policy/mkaccess_vector.sh $(AWK) $(AV_H_DEPEND) obj-bin-$(CONFIG_XSM_FLASK_POLICY) += flask-policy.o -- Anthony PERARD
[XEN PATCH v5 04/16] xen/build: have the root Makefile generates the CFLAGS
Instead of generating the CFLAGS in Rules.mk everytime we enter a new
subdirectory, we are going to generate most of them a single time, and
export the result in the environment so that Rules.mk can use it. The
only flags left to be generated are the ones that depend on the
targets, but the variable $(c_flags) takes care of that.
Arch specific CFLAGS are generated by a new file "arch/*/arch.mk"
which is included by the root Makefile.
We export the *FLAGS via the environment variables XEN_*FLAGS because
Rules.mk still includes Config.mk and would add duplicated flags to
CFLAGS.
When running Rules.mk in the root directory (xen/), the variable
`root-make-done' is set, so `need-config' will remain undef and so the
root Makefile will not generate the cflags again.
We can't use CFLAGS in subdirectories to add flags to particular
targets, instead start to use CFLAGS-y. Idem for AFLAGS.
So there are two different CFLAGS-y, the one in xen/Makefile (and
arch.mk), and the one in subdirs that Rules.mk is going to use.
We can't add to XEN_CFLAGS because it is exported, so making change to
it might be propagated to subdirectory which isn't intended.
Some style change are introduced in this patch:
when LDFLAGS_DIRECT is included in LDFLAGS
use of CFLAGS-$(CONFIG_INDIRECT_THUNK) instead of ifeq().
The LTO change hasn't been tested properly, as LTO is marked as
broken.
Signed-off-by: Anthony PERARD
---
Notes:
v5:
- typos
- remove -flto from XEN_CFLAGS instead of calling lto broken and
commenting out lto stuff.
v4:
- typos
- Adding $(AFLAGS-y) to $(AFLAGS)
v3:
- squash "xen/build: introduce ccflags-y and CFLAGS_$@" here, with
those changes:
- rename ccflags-y to simply CFLAGS-y and start using AFLAGS-y in
subdirs.
- remove CFLAGS_$@, we don't need it yet.
- fix build of xen.lds and efi.lds which needed -D to be a_flags
- remove arch_ccflags, and modify c_flags directly
with that change, reorder c_flags, so that target specific flags are last.
- remove HAVE_AS_QUOTED_SYM from envvar and check XEN_CFLAGS to find if
it's there when adding -D__OBJECT_LABEL__.
- fix missing some flags in AFLAGS
(like -fshort-wchar in xen/arch/x86/efi/Makefile,
and -D__OBJECT_LABEL__ and CFLAGS-stack-boundary)
- keep COV_FLAGS generation in Rules.mk since it doesn't invovle to
call CC
- fix clang test for "asm()-s support .include." (in a new patch done
ahead)
- include Kconfig.include in xen/Makefile because as-option-add is
defined there now.
xen/Makefile | 58 +++
xen/Rules.mk | 73 ++--
xen/arch/arm/Makefile | 10 ++--
xen/arch/arm/Rules.mk | 23
xen/arch/arm/{Rules.mk => arch.mk} | 5 --
xen/arch/arm/efi/Makefile | 2 +-
xen/arch/x86/Makefile | 24
xen/arch/x86/Rules.mk | 91 ++
xen/arch/x86/{Rules.mk => arch.mk} | 17 ++
xen/arch/x86/efi/Makefile | 2 +-
xen/common/libelf/Makefile | 4 +-
xen/common/libfdt/Makefile | 4 +-
xen/include/Makefile | 2 +-
xen/xsm/flask/Makefile | 2 +-
xen/xsm/flask/ss/Makefile | 2 +-
15 files changed, 114 insertions(+), 205 deletions(-)
copy xen/arch/arm/{Rules.mk => arch.mk} (85%)
copy xen/arch/x86/{Rules.mk => arch.mk} (87%)
diff --git a/xen/Makefile b/xen/Makefile
index 643c689658f3..2a689b26a2ba 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -118,6 +118,64 @@ $(KCONFIG_CONFIG):
include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG)
$(MAKE) $(kconfig) syncconfig
+ifeq ($(CONFIG_DEBUG),y)
+CFLAGS += -O1
+else
+CFLAGS += -O2
+endif
+
+ifeq ($(CONFIG_FRAME_POINTER),y)
+CFLAGS += -fno-omit-frame-pointer
+else
+CFLAGS += -fomit-frame-pointer
+endif
+
+CFLAGS += -nostdinc -fno-builtin -fno-common
+CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
+$(call cc-option-add,CFLAGS,CC,-Wvla)
+CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h
+CFLAGS-$(CONFIG_DEBUG_INFO) += -g
+
+ifneq ($(CONFIG_CC_IS_CLANG),y)
+# Clang doesn't understand this command line argument, and doesn't appear to
+# have a suitable alternative. The resulting compiled binary does function,
+# but has an excessively large symbol table.
+CFLAGS += -Wa,--strip-local-absolute
+endif
+
+AFLAGS += -D__ASSEMBLY__
+
+CFLAGS += $(CFLAGS-y)
+# allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE
+CFLAGS += $(EXTRA_CFLAGS_XEN_CORE)
+
+# Most CFLAGS are safe for assembly files:
+# -std=gnu{89,99} gets confused by #-prefixed end-of-line comments
+# -flto makes no sense and annoys clang
+AFLAGS += $(filter-out -std=gnu% -flto,$(CFLAGS)) $(AFLAGS-y)
+
+# LDFLAGS are only passed directly to $(LD)
+LDFLAGS += $(LDFLAGS_DIRECT) $(LDFLAGS-y)
+
+ifeq ($(CONFIG_UBSAN),y)
+CFLAGS_UBSAN
[XEN PATCH v5 07/16] xen/build: Start using if_changed
This patch start to use if_changed introduced in a previous commit.
Whenever if_changed is called, the target must have FORCE as
dependency so that if_changed can check if the command line to be
run has changed, so the macro $(real-prereqs) must be used to
discover the dependencies without "FORCE".
Whenever a target isn't in obj-y, it should be added to extra-y so the
.*.cmd dependency file associated with the target can be loaded. This
is done for xsm/flask/ and both common/lib{elf,fdt}/ and
arch/x86/Makefile.
For the targets that generate .*.d dependency files, there's going to
be two dependency files (.*.d and .*.cmd) until we can merge them
together in a later patch via fixdep from Linux.
One cleanup, libelf-relocate.o doesn't exist anymore.
We import cmd_ld and cmd_objcopy from Linux v5.4.
Signed-off-by: Anthony PERARD
Reviewed-by: Roger Pau Monné
Acked-by: Jan Beulich
---
Notes:
v4:
- typos
- fix missing FORCE in libfdt/Makefile
- typo flask vs flash in xsm
- in xsm, use *_H_DEPEND in command lines of mkaccess and mkflask
instead of $(real-prereq) to avoid including the script as argument of
itself.
xen/Rules.mk | 68 +++---
xen/arch/arm/Makefile | 4 +--
xen/arch/x86/Makefile | 1 +
xen/arch/x86/efi/Makefile | 7 ++--
xen/common/libelf/Makefile | 12 ---
xen/common/libfdt/Makefile | 11 +++---
xen/xsm/flask/Makefile | 17 +++---
7 files changed, 85 insertions(+), 35 deletions(-)
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 21cac7f5f51a..2e28c572305a 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -56,6 +56,18 @@ SPECIAL_DATA_SECTIONS := rodata $(foreach a,1 2 4 8 16, \
include Makefile
+# Linking
+# ---
+
+quiet_cmd_ld = LD $@
+cmd_ld = $(LD) $(XEN_LDFLAGS) -r -o $@ $(real-prereqs)
+
+# Objcopy
+# ---
+
+quiet_cmd_objcopy = OBJCOPY $@
+cmd_objcopy = $(OBJCOPY) $(OBJCOPYFLAGS) $< $@
+
define gendep
ifneq ($(1),$(subst /,:,$(1)))
DEPS += $(dir $(1)).$(notdir $(1)).d
@@ -164,29 +176,47 @@ else
$(CC) $(c_flags) -c $< -o $@
endif
-%.o: %.S Makefile
- $(CC) $(a_flags) -c $< -o $@
+quiet_cmd_cc_o_S = CC $@
+cmd_cc_o_S = $(CC) $(a_flags) -c $< -o $@
+
+%.o: %.S FORCE
+ $(call if_changed,cc_o_S)
+
+
+quiet_cmd_obj_init_o = INIT_O $@
+define cmd_obj_init_o
+$(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name sz
rest; do \
+case "$$name" in \
+.*.local) ;; \
+.text|.text.*|.data|.data.*|.bss) \
+test $$sz != 0 || continue; \
+echo "Error: size of $<:$$name is 0x$$sz" >&2; \
+exit $$(expr $$idx + 1);; \
+esac; \
+done; \
+$(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section
.$(s)=.init.$(s)) $< $@
+endef
+
+$(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o FORCE
+ $(call if_changed,obj_init_o)
+
+quiet_cmd_cpp_i_c = CPP $@
+cmd_cpp_i_c = $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) $< -o $@
+
+quiet_cmd_cc_s_c = CC $@
+cmd_cc_s_c = $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@
-$(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile
- $(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name
sz rest; do \
- case "$$name" in \
- .*.local) ;; \
- .text|.text.*|.data|.data.*|.bss) \
- test $$sz != 0 || continue; \
- echo "Error: size of $<:$$name is 0x$$sz" >&2; \
- exit $$(expr $$idx + 1);; \
- esac; \
- done
- $(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section
.$(s)=.init.$(s)) $< $@
+quiet_cmd_s_S = CPP $@
+cmd_s_S = $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@
-%.i: %.c Makefile
- $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) $< -o $@
+%.i: %.c FORCE
+ $(call if_changed,cpp_i_c)
-%.s: %.c Makefile
- $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@
+%.s: %.c FORCE
+ $(call if_changed,cc_s_c)
-%.s: %.S Makefile
- $(CPP) $(filter-out -Wa$(comma)%,$(a_flags)) $< -o $@
+%.s: %.S FORCE
+ $(call if_changed,cpp_s_S)
# Add intermediate targets:
# When building objects with specific suffix patterns, add intermediate
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 9f1ab2335756..b79ad55646bd 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -97,8 +97,8 @@ prelink_lto.o: $(ALL_OBJS)
prelink.o: $(patsubst %/built_in.o,%/built_in_bin.o,$(ALL_OBJS)) prelink_lto.o
$(LD) $(XEN_LDFLAGS) -r -o $@ $^
else
-prelink.o: $(ALL_OBJS)
- $(LD) $(XEN_LDFLAGS) -r -o $@ $^
+prelink.o: $(ALL_OBJS) FORCE
+ $(call if_changed,ld)
endif
$(TARGET)-syms: prelink.o xen.lds
[XEN PATCH v5 14/16] build: use if_changed to build mm/*/guest_%.o
Use if_changed for building all guest_%.o objects, and make use of command that already exist. The current command only runs `CC`, but the runes to build every other object in Xen also runs `objcopy` (when CONFIG_ENFORCE_UNIQUE_SYMBOLS=y) which modify the file symbol. But with patch "xen,symbols: rework file symbols selection", ./symbols should still select the file symbols directive intended to be used for guest_%.o objects. The goal here is to reduce the number of commands written in makefiles. Signed-off-by: Anthony PERARD --- Notes: v5: - reword commit message v4: - remove the introduction of Kbuild's CFLAGS_$@ and simply use make's per-target variable customization. Mostly to avoid using $(eval ) which might not work as expected on make 3.80. xen/arch/x86/mm/Makefile| 14 -- xen/arch/x86/mm/hap/Makefile| 15 +-- xen/arch/x86/mm/shadow/Makefile | 14 -- 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/xen/arch/x86/mm/Makefile b/xen/arch/x86/mm/Makefile index a2431fde6bb4..a66a57314489 100644 --- a/xen/arch/x86/mm/Makefile +++ b/xen/arch/x86/mm/Makefile @@ -11,11 +11,13 @@ obj-y += p2m.o p2m-pt.o obj-$(CONFIG_HVM) += p2m-ept.o p2m-pod.o obj-y += paging.o -guest_walk_%.o: guest_walk.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_walk_%.o guest_walk_%.i guest_walk_%.s: CFLAGS-y += -DGUEST_PAGING_LEVELS=$* -guest_walk_%.i: guest_walk.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_walk_%.o: guest_walk.c FORCE + $(call if_changed_rule,cc_o_c) -guest_walk_%.s: guest_walk.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ +guest_walk_%.i: guest_walk.c FORCE + $(call if_changed,cpp_i_c) + +guest_walk_%.s: guest_walk.c FORCE + $(call if_changed,cc_s_c) diff --git a/xen/arch/x86/mm/hap/Makefile b/xen/arch/x86/mm/hap/Makefile index 22e7ad54bd33..34720b2fbe2e 100644 --- a/xen/arch/x86/mm/hap/Makefile +++ b/xen/arch/x86/mm/hap/Makefile @@ -5,11 +5,14 @@ obj-y += guest_walk_4level.o obj-y += nested_hap.o obj-y += nested_ept.o -guest_walk_%level.o: guest_walk.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_walk_%level.o guest_walk_%level.i guest_walk_%level.s: \ +CFLAGS-y += -DGUEST_PAGING_LEVELS=$* -guest_walk_%level.i: guest_walk.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_walk_%level.o: guest_walk.c FORCE + $(call if_changed_rule,cc_o_c) -guest_walk_%level.s: guest_walk.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ +guest_walk_%level.i: guest_walk.c FORCE + $(call if_changed,cpp_i_c) + +guest_walk_%level.s: guest_walk.c FORCE + $(call if_changed,cc_s_c) diff --git a/xen/arch/x86/mm/shadow/Makefile b/xen/arch/x86/mm/shadow/Makefile index 23d3ff10802c..e00f9cb1aaba 100644 --- a/xen/arch/x86/mm/shadow/Makefile +++ b/xen/arch/x86/mm/shadow/Makefile @@ -6,11 +6,13 @@ else obj-y += none.o endif -guest_%.o: multi.c Makefile - $(CC) $(c_flags) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_%.o guest_%.i guest_%.s: CFLAGS-y += -DGUEST_PAGING_LEVELS=$* -guest_%.i: multi.c Makefile - $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -c $< -o $@ +guest_%.o: multi.c FORCE + $(call if_changed_rule,cc_o_c) -guest_%.s: multi.c Makefile - $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -DGUEST_PAGING_LEVELS=$* -S $< -o $@ +guest_%.i: multi.c FORCE + $(call if_changed,cpp_i_c) + +guest_%.s: multi.c FORCE + $(call if_changed,cc_s_c) -- Anthony PERARD
[XEN PATCH v5 16/16] build,include: rework compat-build-header.py
Replace a mix of shell script and python script by all python script.
No change to the final generated headers.
Signed-off-by: Anthony PERARD
---
Notes:
v5:
- Removed -P from CPP when generating compat/%.i
-> keep removing linemarkers and keep de-duplicating empty lines.
So that all the blank line that currently exist in the generated
headers stays in place.
v4:
- new patch
xen/include/Makefile | 11 +--
xen/tools/compat-build-header.py | 52 ++--
2 files changed, 51 insertions(+), 12 deletions(-)
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 537085b82793..12660625119e 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -51,16 +51,7 @@ public-$(CONFIG_ARM) := $(wildcard public/arch-arm/*.h
public/arch-arm/*/*.h)
all: $(headers-y)
compat/%.h: compat/%.i Makefile $(BASEDIR)/tools/compat-build-header.py
- set -e; id=_$$(echo $@ | tr '[:lower:]-/.' '[:upper:]___'); \
- echo "#ifndef $$id" >$@.new; \
- echo "#define $$id" >>$@.new; \
- echo "#include " >>$@.new; \
- $(if $(filter-out compat/arch-%.h,$@),echo "#include <$(patsubst
compat/%,public/%,$@)>" >>$@.new;) \
- $(if $(prefix-y),echo "$(prefix-y)" >>$@.new;) \
- grep -v '^# [0-9]' $< | \
- $(PYTHON) $(BASEDIR)/tools/compat-build-header.py | uniq >>$@.new; \
- $(if $(suffix-y),echo "$(suffix-y)" >>$@.new;) \
- echo "#endif /* $$id */" >>$@.new
+ $(PYTHON) $(BASEDIR)/tools/compat-build-header.py <$< $@ "$(prefix-y)"
"$(suffix-y)" >>$@.new; \
mv -f $@.new $@
compat/%.i: compat/%.c Makefile
diff --git a/xen/tools/compat-build-header.py b/xen/tools/compat-build-header.py
index b85c43f13faf..34d5343c3dd9 100755
--- a/xen/tools/compat-build-header.py
+++ b/xen/tools/compat-build-header.py
@@ -2,6 +2,12 @@
import re,sys
+try:
+maketrans = str.maketrans
+except AttributeError:
+# For python2
+from string import maketrans
+
pats = [
[ r"__InClUdE__(.*)", r"#include\1\n#pragma pack(4)" ],
[ r"__IfDeF__ (XEN_HAVE.*)", r"#ifdef \1" ],
@@ -20,7 +26,49 @@ pats = [
[ r"(^|[^\w])long([^\w]|$$)", r"\1int\2" ]
];
+output_filename = sys.argv[1]
+
+# tr '[:lower:]-/.' '[:upper:]___'
+header_id = '_' + \
+output_filename.upper().translate(maketrans('-/.','___'))
+
+header = """#ifndef {0}
+#define {0}
+#include """.format(header_id)
+
+print(header)
+
+if not re.match("compat/arch-.*.h$", output_filename):
+x = output_filename.replace("compat/","public/")
+print('#include <%s>' % x)
+
+def print_if_nonempty(s):
+if len(s):
+print(s)
+
+print_if_nonempty(sys.argv[2])
+
+last_line_empty = False
for line in sys.stdin.readlines():
+line = line.rstrip()
+
+# Remove linemarkers generated by the preprocessor.
+if re.match(r"^# \d", line):
+continue
+
+# De-duplicate empty lines.
+if len(line) == 0:
+if not last_line_empty:
+print(line)
+last_line_empty = True
+continue
+else:
+last_line_empty = False
+
for pat in pats:
-line = re.subn(pat[0], pat[1], line)[0]
-print(line.rstrip())
+line = re.sub(pat[0], pat[1], line)
+print(line)
+
+print_if_nonempty(sys.argv[3])
+
+print("#endif /* %s */" % header_id)
--
Anthony PERARD
[XEN PATCH v5 06/16] xen/build: introduce if_changed and if_changed_rule
The if_changed macro from Linux, in addition to check if any files needs an update, check if the command line has changed since the last invocation. The latter will force a rebuild if any options to the executable have changed. if_changed_rule checks dependencies like if_changed, but execute rule_$(1) instead of cmd_$(1) when a target needs to be rebuilt. A rule_ macro can call more than one cmd_ macro. One of the cmd_ macro in a rule need to be call using a macro that record the command line, so cmd_and_record is introduced. It is similar to cmd_and_fixup from Linux but without a call to fixdep which we don't have yet. (We will later replace cmd_and_record by cmd_and_fixup.) Example of a rule_ macro: define rule_cc_o_c $(call cmd_and_record,cc_o_o) $(call cmd,objcopy) endef This needs one of the call to use cmd_and_record, otherwise no .*.cmd file will be created, and the target will keep been rebuilt. In order for if_changed to works correctly, we need to load the .%.cmd files that the macro generates, this is done by adding targets in to the $(targets) variable. We use intermediate_targets to add %.init.o dependency %.o to target since there aren't in obj-y. We also add $(MAKECMDGOALS) to targets so that when running for example `make common/memory.i`, make will load the associated .%.cmd dependency file. Beside the if_changed*, we import the machinery used for a "beautify output". The important one is when running make with V=2 which help to debug the makefiles by printing why a target is been rebuilt, via the $(echo-why) macro. if_changed and if_changed_rule aren't used yet. Most of this code is copied from Linux v5.4, including the documentation. Signed-off-by: Anthony PERARD Acked-by: Jan Beulich --- Notes: v5: - In documentation, fix some mix tab/space used for indentation, convert to all tab. - and fix a "note" in "if_changed" where the paragraph was cut in two. v4: - Use := in make whenever possible (instead of =) - insert new string in .gitignore somewhere more plausible. - import documentation from Linux .gitignore| 1 + docs/misc/xen-makefiles/makefiles.rst | 98 +++ xen/Makefile | 53 - xen/Rules.mk | 33 +++- xen/scripts/Kbuild.include| 107 ++ 5 files changed, 290 insertions(+), 2 deletions(-) diff --git a/.gitignore b/.gitignore index 4ca679ddbc9a..bfa53723b38b 100644 --- a/.gitignore +++ b/.gitignore @@ -1,4 +1,5 @@ .hg +.*.cmd .*.tmp *.orig *~ diff --git a/docs/misc/xen-makefiles/makefiles.rst b/docs/misc/xen-makefiles/makefiles.rst index a86e3a612d61..04bc72601c31 100644 --- a/docs/misc/xen-makefiles/makefiles.rst +++ b/docs/misc/xen-makefiles/makefiles.rst @@ -85,3 +85,101 @@ Compilation flags CFLAGS-y specifies options for compiling with $(CC). AFLAGS-y specifies assembler options. + + +Build system infrastructure +=== + +This chapter describe some of the macro used when building Xen. + +Macros +-- + + +if_changed + if_changed is the infrastructure used for the following commands. + + Usage:: + + target: source(s) FORCE + $(call if_changed,ld/objcopy/...) + + When the rule is evaluated, it is checked to see if any files + need an update, or the command line has changed since the last + invocation. The latter will force a rebuild if any options + to the executable have changed. + Any target that utilises if_changed must be listed in $(targets), + otherwise the command line check will fail, and the target will + always be built. + if_changed may be used in conjunction with custom commands as + defined in "Custom commands". + + Note: It is a typical mistake to forget the FORCE prerequisite. + Another common pitfall is that whitespace is sometimes + significant; for instance, the below will fail (note the extra space + after the comma):: + + target: source(s) FORCE + + **WRONG!** $(call if_changed, ld/objcopy/...) + + Note: + if_changed should not be used more than once per target. + It stores the executed command in a corresponding .cmd file + and multiple calls would result in overwrites and unwanted + results when the target is up to date and only the tests on + changed commands trigger execution of commands. + +ld + Link target. + + Example:: + + targets += setup setup.o bootsect bootsect.o + $(obj)/setup $(obj)/bootsect: %: %.o FORCE + $(call if_changed,ld) + + $(targets) are assigned all potential targets, by which the build + system knows the targets and will: + + 1) check for commandline changes + +
[XEN PATCH v5 00/16] xen: Build system improvements
Patch series available in this git branch:
https://xenbits.xen.org/git-http/people/aperard/xen-unstable.git
br.build-system-xen-v5
v5:
- few changes detailed in patch notes.
- 1 new patch
v4:
- some patch already applied.
- Have added patches from "xen/arm: Configure early printk via Kconfig" series.
- Some new patch to add documentation or fix issues, and patch to improve
compat header generation.
Other changes are detailed in patches.
v3:
- new patches that do some cleanup or fix issues
- have rework most patches, to have better commit message or change the coding
style, or fix issues that I've seen. There were some cases where CFLAGS were
missing.
See patch notes for details
- introduce if_changed*. That plenty of new patches on top of what we had in v2.
(those changes ignore CONFIG_LTO=y, I'll see about fixing that later)
(There is more to come in order to use fixdep from Linux, but that's not ready)
Hi,
I have work toward building Xen (the hypervisor) with Linux's build system,
Kbuild.
The main reason for that is to be able to have out-of-tree build. It's annoying
when a build fail because of the pvshim. Other benefit is a much faster
rebuild, and `make clean` doesn't take ages, and better dependencies to figure
out what needs to be rebuild.
So, we are not there yet, but the series already contain quite a few
improvement and cleanup. More patches are going to be added to the series.
Cheers,
Anthony PERARD (16):
build,xsm: Fix multiple call
xen/build: include include/config/auto.conf in main Makefile
xen/build: use new $(c_flags) and $(a_flags) instead of $(CFLAGS)
xen/build: have the root Makefile generates the CFLAGS
build: Introduce documentation for xen Makefiles
xen/build: introduce if_changed and if_changed_rule
xen/build: Start using if_changed
build: Introduce $(cpp_flags)
xen/build: use if_changed on built_in.o
xen/build: Use if_changed_rules with %.o:%.c targets
xen/build: factorise generation of the linker scripts
xen/build: Use if_changed for prelink*.o
xen,symbols: rework file symbols selection
build: use if_changed to build mm/*/guest_%.o
build,include: rework compat-build-source.py
build,include: rework compat-build-header.py
.gitignore| 1 +
docs/misc/xen-makefiles/makefiles.rst | 185
xen/Makefile | 212 ---
xen/Rules.mk | 235 --
xen/arch/arm/Makefile | 22 +--
xen/arch/arm/Rules.mk | 23 ---
xen/arch/arm/{Rules.mk => arch.mk}| 5 -
xen/arch/arm/efi/Makefile | 2 +-
xen/arch/x86/Makefile | 41 ++---
xen/arch/x86/Rules.mk | 91 +-
xen/arch/x86/{Rules.mk => arch.mk}| 17 +-
xen/arch/x86/efi/Makefile | 9 +-
xen/arch/x86/mm/Makefile | 14 +-
xen/arch/x86/mm/hap/Makefile | 15 +-
xen/arch/x86/mm/shadow/Makefile | 14 +-
xen/common/libelf/Makefile| 14 +-
xen/common/libfdt/Makefile| 13 +-
xen/include/Makefile | 16 +-
xen/scripts/Kbuild.include| 107
xen/tools/compat-build-header.py | 52 +-
xen/tools/compat-build-source.py | 8 +-
xen/tools/symbols.c | 20 ++-
xen/xsm/flask/Makefile| 19 ++-
xen/xsm/flask/ss/Makefile | 2 +-
24 files changed, 809 insertions(+), 328 deletions(-)
create mode 100644 docs/misc/xen-makefiles/makefiles.rst
copy xen/arch/arm/{Rules.mk => arch.mk} (85%)
copy xen/arch/x86/{Rules.mk => arch.mk} (87%)
--
Anthony PERARD
[XEN PATCH v5 03/16] xen/build: use new $(c_flags) and $(a_flags) instead of $(CFLAGS)
In a later patch ("xen/build: have the root Makefile generates the
CFLAGS), we want to generate the CFLAGS in xen/Makefile, then export
it and have Rules.mk use a CFLAGS from the environment variables. That
changes the flavor of the CFLAGS and flags intended for one target
(like -D__OBJECT_FILE__ and -M%) gets propagated and duplicated. So we
start by moving such flags out of $(CFLAGS) and into $(c_flags) which
is to be modified by only Rules.mk.
__OBJECT_FILE__ is only used by arch/x86/mm/*.c files, so having it in
$(c_flags) is enough, we don't need it in $(a_flags).
For include/Makefile and as-insn we can keep using CFLAGS, but since
it doesn't have -M* flags anymore there is no need to filter them out.
The XEN_BUILD_EFI tests in arch/x86/Makefile was filtering out
CFLAGS-y, but according to dd40177c1bc8 ("x86-64/EFI: add CFLAGS to
check compile"), it was done to filter out -MF. CFLAGS doesn't
have those flags anymore, so no filtering is needed.
This is inspired by the way Kbuild generates CFLAGS for each targets.
Signed-off-by: Anthony PERARD
Reviewed-by: Roger Pau Monné
Reviewed-by: Jan Beulich
---
Notes:
v4:
- drop change in as-insn macro, and keep filtering-out -M% %.d
v3:
- include/Makefile: Keep using CFLAGS, but since it doesn't have -M*
flags anymore, no need to filter it.
- Write c_flags and a_flags on a single line.
- arch/x86/Makefile: remove the filter-out of dependency flags
they are remove from CFLAGS anyway.
(was intended to be done in xen/build: have the root Makefile
generates the CFLAGS originally, move the change to this patch).
- also modify as-insn as it is now xen/ only.
xen/Rules.mk| 23 +++
xen/arch/arm/Makefile | 4 ++--
xen/arch/x86/Makefile | 6 +++---
xen/arch/x86/mm/Makefile| 6 +++---
xen/arch/x86/mm/hap/Makefile| 6 +++---
xen/arch/x86/mm/shadow/Makefile | 6 +++---
xen/include/Makefile| 2 +-
7 files changed, 26 insertions(+), 27 deletions(-)
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 9079df7978a7..3408a35dbf53 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -57,7 +57,6 @@ CFLAGS += -Werror -Wredundant-decls -Wno-pointer-arith
$(call cc-option-add,CFLAGS,CC,-Wvla)
CFLAGS += -pipe -D__XEN__ -include $(BASEDIR)/include/xen/config.h
CFLAGS-$(CONFIG_DEBUG_INFO) += -g
-CFLAGS += '-D__OBJECT_FILE__="$@"'
ifneq ($(CONFIG_CC_IS_CLANG),y)
# Clang doesn't understand this command line argument, and doesn't appear to
@@ -70,9 +69,6 @@ AFLAGS += -D__ASSEMBLY__
ALL_OBJS := $(ALL_OBJS-y)
-# Get gcc to generate the dependencies for us.
-CFLAGS-y += -MMD -MP -MF $(@D)/.$(@F).d
-
CFLAGS += $(CFLAGS-y)
# allow extra CFLAGS externally via EXTRA_CFLAGS_XEN_CORE
CFLAGS += $(EXTRA_CFLAGS_XEN_CORE)
@@ -146,9 +142,12 @@ endif
# Always build obj-bin files as binary even if they come from C source.
$(obj-bin-y): CFLAGS := $(filter-out -flto,$(CFLAGS))
+c_flags = -MMD -MP -MF $(@D)/.$(@F).d $(CFLAGS) '-D__OBJECT_FILE__="$@"'
+a_flags = -MMD -MP -MF $(@D)/.$(@F).d $(AFLAGS)
+
built_in.o: $(obj-y) $(extra-y)
ifeq ($(obj-y),)
- $(CC) $(CFLAGS) -c -x c /dev/null -o $@
+ $(CC) $(c_flags) -c -x c /dev/null -o $@
else
ifeq ($(CONFIG_LTO),y)
$(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^)
@@ -159,7 +158,7 @@ endif
built_in_bin.o: $(obj-bin-y) $(extra-y)
ifeq ($(obj-bin-y),)
- $(CC) $(AFLAGS) -c -x assembler /dev/null -o $@
+ $(CC) $(a_flags) -c -x assembler /dev/null -o $@
else
$(LD) $(LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^)
endif
@@ -178,7 +177,7 @@ SRCPATH := $(patsubst $(BASEDIR)/%,%,$(CURDIR))
%.o: %.c Makefile
ifeq ($(CONFIG_ENFORCE_UNIQUE_SYMBOLS),y)
- $(CC) $(CFLAGS) -c $< -o $(@D)/.$(@F).tmp -MQ $@
+ $(CC) $(c_flags) -c $< -o $(@D)/.$(@F).tmp -MQ $@
ifeq ($(CONFIG_CC_IS_CLANG),y)
$(OBJCOPY) --redefine-sym $<=$(SRCPATH)/$< $(@D)/.$(@F).tmp $@
else
@@ -186,11 +185,11 @@ else
endif
rm -f $(@D)/.$(@F).tmp
else
- $(CC) $(CFLAGS) -c $< -o $@
+ $(CC) $(c_flags) -c $< -o $@
endif
%.o: %.S Makefile
- $(CC) $(AFLAGS) -c $< -o $@
+ $(CC) $(a_flags) -c $< -o $@
$(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)): %.init.o: %.o Makefile
$(OBJDUMP) -h $< | sed -n '/[0-9]/{s,00*,0,g;p;}' | while read idx name
sz rest; do \
@@ -205,12 +204,12 @@ $(filter %.init.o,$(obj-y) $(obj-bin-y) $(extra-y)):
%.init.o: %.o Makefile
$(OBJCOPY) $(foreach s,$(SPECIAL_DATA_SECTIONS),--rename-section
.$(s)=.init.$(s)) $< $@
%.i: %.c Makefile
- $(CPP) $(filter-out -Wa$(comma)%,$(CFLAGS)) $< -o $@
+ $(CPP) $(filter-out -Wa$(comma)%,$(c_flags)) $< -o $@
%.s: %.c Makefile
- $(CC) $(filter-out -Wa$(comma)%,$(CFLAGS)) -S $< -o $@
+ $(CC) $(filter-out -Wa$(comma)%,$(c_flags)) -S $< -o $@
%.s: %.S Makefile
- $(CPP) $(filter-out -Wa$(comma)%,$(AFLAGS)) $< -o $@
+ $(CPP)
[XEN PATCH v5 09/16] xen/build: use if_changed on built_in.o
In the case where $(obj-y) is empty, we also replace $(c_flags) by $(XEN_CFLAGS) to avoid generating an .%.d dependency file. This avoid make trying to include %.h file in the ld command if $(obj-y) isn't empty anymore on a second run. Signed-off-by: Anthony PERARD --- Notes: v4: - Have cmd_ld_builtin depends on CONFIG_LTO, which simplify built_in.o rule. xen/Rules.mk | 21 +++-- 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/xen/Rules.mk b/xen/Rules.mk index 25bcf45612bd..5e08e14455d7 100644 --- a/xen/Rules.mk +++ b/xen/Rules.mk @@ -130,15 +130,24 @@ include $(BASEDIR)/arch/$(TARGET_ARCH)/Rules.mk c_flags += $(CFLAGS-y) a_flags += $(CFLAGS-y) $(AFLAGS-y) -built_in.o: $(obj-y) $(extra-y) -ifeq ($(obj-y),) - $(CC) $(c_flags) -c -x c /dev/null -o $@ -else +quiet_cmd_ld_builtin = LD $@ ifeq ($(CONFIG_LTO),y) - $(LD_LTO) -r -o $@ $(filter-out $(extra-y),$^) +cmd_ld_builtin = \ +$(LD_LTO) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) else - $(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$^) +cmd_ld_builtin = \ +$(LD) $(XEN_LDFLAGS) -r -o $@ $(filter-out $(extra-y),$(real-prereqs)) endif + +quiet_cmd_cc_builtin = LD $@ +cmd_cc_builtin = \ +$(CC) $(XEN_CFLAGS) -c -x c /dev/null -o $@ + +built_in.o: $(obj-y) $(extra-y) FORCE +ifeq ($(obj-y),) + $(call if_changed,cc_builtin) +else + $(call if_changed,ld_builtin) endif targets += built_in.o -- Anthony PERARD
[XEN PATCH v5 15/16] build,include: rework compat-build-source.py
Improvement are:
- give the path to xlat.lst as argument
- include `grep -v` in compat-build-source.py script, we don't need to
write this in several scripted language.
No changes in final compat/%.h headers.
Signed-off-by: Anthony PERARD
---
Notes:
v5:
- removed "have 'xlat.lst' path as a variable" from the patch.
v4:
- new patch
xen/include/Makefile | 3 +--
xen/tools/compat-build-source.py | 8 +++-
2 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 2a10725d689b..537085b82793 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -68,8 +68,7 @@ compat/%.i: compat/%.c Makefile
compat/%.c: public/%.h xlat.lst Makefile
$(BASEDIR)/tools/compat-build-source.py
mkdir -p $(@D)
- grep -v 'DEFINE_XEN_GUEST_HANDLE(long)' $< | \
- $(PYTHON) $(BASEDIR)/tools/compat-build-source.py >$@.new
+ $(PYTHON) $(BASEDIR)/tools/compat-build-source.py xlat.lst <$< >$@.new
mv -f $@.new $@
compat/.xlat/%.h: compat/%.h compat/.xlat/%.lst $(BASEDIR)/tools/get-fields.sh
Makefile
diff --git a/xen/tools/compat-build-source.py b/xen/tools/compat-build-source.py
index c664eb85e633..c7fc2c53db61 100755
--- a/xen/tools/compat-build-source.py
+++ b/xen/tools/compat-build-source.py
@@ -12,7 +12,11 @@ pats = [
[ r"XEN_GUEST_HANDLE(_[0-9A-Fa-f]+)?", r"COMPAT_HANDLE" ],
];
-xlatf = open('xlat.lst', 'r')
+try:
+xlatf = open(sys.argv[1], 'r')
+except IndexError:
+print('missing path to xlat.lst argument')
+sys.exit(1)
for line in xlatf.readlines():
match = re.subn(r"^\s*\?\s+(\w*)\s.*", r"\1", line.rstrip())
if match[1]:
@@ -24,6 +28,8 @@ for pat in pats:
pat[0] = re.compile(pat[0])
for line in sys.stdin.readlines():
+if 'DEFINE_XEN_GUEST_HANDLE(long)' in line:
+continue
for pat in pats:
line = re.sub(pat[0], pat[1], line)
print(line.rstrip())
--
Anthony PERARD
[PATCH v3 2/2] x86: validate VM assist value in arch_set_info_guest()
While I can't spot anything that would go wrong, just like the
respective hypercall only permits applicable bits to be set, we should
also do so when loading guest context.
Reported-by: Andrew Cooper
Signed-off-by: Jan Beulich
Acked-by: Andrew Cooper
---
I'd like to note that Arm lacks a field to save/restore vm_assist.
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -932,6 +932,9 @@ int arch_set_info_guest(
}
}
+if ( v->vcpu_id == 0 && (c(vm_assist) & ~arch_vm_assist_valid_mask(d)) )
+return -EINVAL;
+
if ( is_hvm_domain(d) )
{
for ( i = 0; i < ARRAY_SIZE(v->arch.dr); ++i )
[PATCH v3 1/2] x86/HVM: expose VM assist hypercall
In preparation for the addition of VMASST_TYPE_runstate_update_flag
commit 72c538cca957 ("arm: add support for vm_assist hypercall") enabled
the hypercall for Arm. I consider it not logical that it then isn't also
exposed to x86 HVM guests (with the same single feature permitted to be
enabled as Arm has); Linux actually tries to use it afaict.
Rather than introducing yet another thin wrapper around vm_assist(),
make that function the main handler, requiring a per-arch
arch_vm_assist_valid_mask() definition instead.
Signed-off-by: Jan Beulich
Acked-by: Andrew Cooper
---
v3: Rename to arch_vm_assist_valid_mask(). Have separate 32- and 64-bit
PV #define-s.
v2: Re-work vm_assist() handling/layering at the same time. Also adjust
arch_set_info_guest().
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -939,6 +939,9 @@ int arch_set_info_guest(
v->arch.dr6 = c(debugreg[6]);
v->arch.dr7 = c(debugreg[7]);
+if ( v->vcpu_id == 0 )
+d->vm_assist = c.nat->vm_assist;
+
hvm_set_info_guest(v);
goto out;
}
--- a/xen/arch/x86/hvm/hypercall.c
+++ b/xen/arch/x86/hvm/hypercall.c
@@ -128,6 +128,7 @@ static const hypercall_table_t hvm_hyper
#ifdef CONFIG_GRANT_TABLE
HVM_CALL(grant_table_op),
#endif
+HYPERCALL(vm_assist),
COMPAT_CALL(vcpu_op),
HVM_CALL(physdev_op),
COMPAT_CALL(xen_version),
--- a/xen/arch/x86/pv/hypercall.c
+++ b/xen/arch/x86/pv/hypercall.c
@@ -57,7 +57,7 @@ const hypercall_table_t pv_hypercall_tab
#ifdef CONFIG_GRANT_TABLE
COMPAT_CALL(grant_table_op),
#endif
-COMPAT_CALL(vm_assist),
+HYPERCALL(vm_assist),
COMPAT_CALL(update_va_mapping_otherdomain),
COMPAT_CALL(iret),
COMPAT_CALL(vcpu_op),
--- a/xen/common/compat/kernel.c
+++ b/xen/common/compat/kernel.c
@@ -37,11 +37,6 @@ CHECK_TYPE(capabilities_info);
CHECK_TYPE(domain_handle);
-#ifdef COMPAT_VM_ASSIST_VALID
-#undef VM_ASSIST_VALID
-#define VM_ASSIST_VALID COMPAT_VM_ASSIST_VALID
-#endif
-
#define DO(fn) int compat_##fn
#define COMPAT
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1517,20 +1517,23 @@ long do_vcpu_op(int cmd, unsigned int vc
return rc;
}
-#ifdef VM_ASSIST_VALID
-long vm_assist(struct domain *p, unsigned int cmd, unsigned int type,
- unsigned long valid)
+#ifdef arch_vm_assist_valid_mask
+long do_vm_assist(unsigned int cmd, unsigned int type)
{
+struct domain *currd = current->domain;
+const unsigned long valid = arch_vm_assist_valid_mask(currd);
+
if ( type >= BITS_PER_LONG || !test_bit(type, ) )
return -EINVAL;
switch ( cmd )
{
case VMASST_CMD_enable:
-set_bit(type, >vm_assist);
+set_bit(type, >vm_assist);
return 0;
+
case VMASST_CMD_disable:
-clear_bit(type, >vm_assist);
+clear_bit(type, >vm_assist);
return 0;
}
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -566,13 +566,6 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDL
return -ENOSYS;
}
-#ifdef VM_ASSIST_VALID
-DO(vm_assist)(unsigned int cmd, unsigned int type)
-{
-return vm_assist(current->domain, cmd, type, VM_ASSIST_VALID);
-}
-#endif
-
/*
* Local variables:
* mode: C
--- a/xen/include/asm-arm/config.h
+++ b/xen/include/asm-arm/config.h
@@ -195,8 +195,6 @@ extern unsigned long frametable_virt_end
#define watchdog_disable() ((void)0)
#define watchdog_enable() ((void)0)
-#define VM_ASSIST_VALID (1UL << VMASST_TYPE_runstate_update_flag)
-
#endif /* __ARM_CONFIG_H__ */
/*
* Local variables:
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -269,6 +269,8 @@ static inline void free_vcpu_guest_conte
static inline void arch_vcpu_block(struct vcpu *v) {}
+#define arch_vm_assist_valid_mask(d) (1UL << VMASST_TYPE_runstate_update_flag)
+
#endif /* __ASM_DOMAIN_H__ */
/*
--- a/xen/include/asm-x86/config.h
+++ b/xen/include/asm-x86/config.h
@@ -309,17 +309,6 @@ extern unsigned long xen_phys_start;
#define ARG_XLAT_START(v)\
(ARG_XLAT_VIRT_START + ((v)->vcpu_id << ARG_XLAT_VA_SHIFT))
-#define NATIVE_VM_ASSIST_VALID ((1UL << VMASST_TYPE_4gb_segments)| \
- (1UL << VMASST_TYPE_4gb_segments_notify) | \
- (1UL << VMASST_TYPE_writable_pagetables) | \
- (1UL << VMASST_TYPE_pae_extended_cr3)| \
- (1UL << VMASST_TYPE_architectural_iopl) | \
- (1UL << VMASST_TYPE_runstate_update_flag)| \
- (1UL << VMASST_TYPE_m2p_strict))
-#define VM_ASSIST_VALID NATIVE_VM_ASSIST_VALID
-#define COMPAT_VM_ASSIST_VALID (NATIVE_VM_ASSIST_VALID & \
- ((1UL << COMPAT_BITS_PER_LONG) - 1))
-
#define ELFSIZE 64
#define ARCH_CRASH_SAVE_VMCOREINFO
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@
[xen-unstable-smoke test] 149713: tolerable all pass - PUSHED
flight 149713 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/149713/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 7aacf6ac49829d8dd6242f67460f4d52d0d36503 baseline version: xen 82dd1a956d9b68f52e830d1dddfdfb4ab4d5a638 Last test of basis 149699 2020-04-17 08:00:46 Z4 days Testing same since 149713 2020-04-21 11:00:34 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Christian Lindig Jan Beulich Julien Grall Roger Pau Monné Tim Deegan jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-amd64pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 82dd1a956d..7aacf6ac49 7aacf6ac49829d8dd6242f67460f4d52d0d36503 -> smoke
[PATCH v3 0/2] x86: VM assist hypercall adjustments
1: HVM: expose VM assist hypercall 2: validate VM assist value in arch_set_info_guest() Jan
Re: [PATCH v10 1/3] x86/tlb: introduce a flush HVM ASIDs flag
On 21.04.2020 15:51, Roger Pau Monné wrote: > On Tue, Apr 21, 2020 at 02:59:10PM +0200, Jan Beulich wrote: >> On 21.04.2020 12:43, Roger Pau Monné wrote: >>> On Tue, Apr 21, 2020 at 12:21:12PM +0200, Jan Beulich wrote: On 16.04.2020 15:59, Roger Pau Monne wrote: > Introduce a specific flag to request a HVM guest linear TLB flush, > which is an ASID/VPID tickle that forces a guest linear to guest > physical TLB flush for all HVM guests. > > This was previously unconditionally done in each pre_flush call, but > that's not required: HVM guests not using shadow don't require linear > TLB flushes as Xen doesn't modify the guest page tables in that case > (ie: when using HAP). I'm afraid I'm being confused by this: Even in shadow mode Xen doesn't modify guest page tables, does it? >>> >>> I'm also confused now. It's my understand that when running in shadow >>> mode guest page tables are not actually used, and the guest uses Xen's >>> crafted shadow tables instead, which are based on the original guest >>> page tables suitably adjusted by Xen in order to do the p2m >>> translation in the HVM case, or the needed PTE adjustments in the PV >>> case. >>> >>> So guest page tables are not modified, but are also not used as they >>> are never loaded into cr3. >> >> This matches my understanding. > > Please bear with me, as I'm not sure if your question was because you > think the paragraph is not clear and/or should be expanded. > > The point of the paragraph you mention was to have a difference > between guests running in shadow mode vs guests running in HAP mode. > Maybe I should use guest loaded page pages, to differentiate between > guest created page tables and the page tables actually loaded in cr3 > in guest mode? How about using "the pages tables the guest runs on"? Jan
Re: [PATCH v10 1/3] x86/tlb: introduce a flush HVM ASIDs flag
On Tue, Apr 21, 2020 at 02:59:10PM +0200, Jan Beulich wrote:
> On 21.04.2020 12:43, Roger Pau Monné wrote:
> > On Tue, Apr 21, 2020 at 12:21:12PM +0200, Jan Beulich wrote:
> >> On 16.04.2020 15:59, Roger Pau Monne wrote:
> >>> Introduce a specific flag to request a HVM guest linear TLB flush,
> >>> which is an ASID/VPID tickle that forces a guest linear to guest
> >>> physical TLB flush for all HVM guests.
> >>>
> >>> This was previously unconditionally done in each pre_flush call, but
> >>> that's not required: HVM guests not using shadow don't require linear
> >>> TLB flushes as Xen doesn't modify the guest page tables in that case
> >>> (ie: when using HAP).
> >>
> >> I'm afraid I'm being confused by this: Even in shadow mode Xen
> >> doesn't modify guest page tables, does it?
> >
> > I'm also confused now. It's my understand that when running in shadow
> > mode guest page tables are not actually used, and the guest uses Xen's
> > crafted shadow tables instead, which are based on the original guest
> > page tables suitably adjusted by Xen in order to do the p2m
> > translation in the HVM case, or the needed PTE adjustments in the PV
> > case.
> >
> > So guest page tables are not modified, but are also not used as they
> > are never loaded into cr3.
>
> This matches my understanding.
Please bear with me, as I'm not sure if your question was because you
think the paragraph is not clear and/or should be expanded.
The point of the paragraph you mention was to have a difference
between guests running in shadow mode vs guests running in HAP mode.
Maybe I should use guest loaded page pages, to differentiate between
guest created page tables and the page tables actually loaded in cr3
in guest mode?
> >>> @@ -254,3 +257,14 @@ unsigned int flush_area_local(const void *va,
> >>> unsigned int flags)
> >>>
> >>> return flags;
> >>> }
> >>> +
> >>> +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
> >>> +{
> >>> +unsigned int flags = (is_pv_domain(d) || paging_mode_shadow(d) ?
> >>> FLUSH_TLB
> >>> + : 0) |
> >>> + (is_hvm_domain(d) && cpu_has_svm ?
> >>> FLUSH_HVM_ASID_CORE
> >>> + : 0);
> >>
> >> Why the is_pv_domain() part of the condition? Afaict for PV
> >> domains you can get here only if they have shadow mode enabled.
> >
> > Right now yes, the only way to get here for PV domains is when using
> > shadow, but if this helper gets used in other non-shadow PV paths then
> > Xen's needs to do a TLB flush.
>
> Why would a non-shdow PV path find a need to call this function?
The memory management code in PV guests also needs to perform TLB
flushes, so I wasn't sure whether the aim was to also switch it to use
guest_flush_tlb_mask.
I guess this doesn't make a lot of sense since PV guests just need a
plain TLB flush, and there's not a lot of benefit from having a helper
around that. Maybe for PV guests running in XPTI mode, where the flush
can be avoided as the return to guest path already flushes the page
tables? Anyway, will remove the is_pv_domain check.
Thanks, Roger.
Re: [PATCH v10 1/3] x86/tlb: introduce a flush HVM ASIDs flag
On 21.04.2020 12:43, Roger Pau Monné wrote:
> On Tue, Apr 21, 2020 at 12:21:12PM +0200, Jan Beulich wrote:
>> On 16.04.2020 15:59, Roger Pau Monne wrote:
>>> Introduce a specific flag to request a HVM guest linear TLB flush,
>>> which is an ASID/VPID tickle that forces a guest linear to guest
>>> physical TLB flush for all HVM guests.
>>>
>>> This was previously unconditionally done in each pre_flush call, but
>>> that's not required: HVM guests not using shadow don't require linear
>>> TLB flushes as Xen doesn't modify the guest page tables in that case
>>> (ie: when using HAP).
>>
>> I'm afraid I'm being confused by this: Even in shadow mode Xen
>> doesn't modify guest page tables, does it?
>
> I'm also confused now. It's my understand that when running in shadow
> mode guest page tables are not actually used, and the guest uses Xen's
> crafted shadow tables instead, which are based on the original guest
> page tables suitably adjusted by Xen in order to do the p2m
> translation in the HVM case, or the needed PTE adjustments in the PV
> case.
>
> So guest page tables are not modified, but are also not used as they
> are never loaded into cr3.
This matches my understanding.
>>> @@ -254,3 +257,14 @@ unsigned int flush_area_local(const void *va, unsigned
>>> int flags)
>>>
>>> return flags;
>>> }
>>> +
>>> +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
>>> +{
>>> +unsigned int flags = (is_pv_domain(d) || paging_mode_shadow(d) ?
>>> FLUSH_TLB
>>> + : 0) |
>>> + (is_hvm_domain(d) && cpu_has_svm ?
>>> FLUSH_HVM_ASID_CORE
>>> + : 0);
>>
>> Why the is_pv_domain() part of the condition? Afaict for PV
>> domains you can get here only if they have shadow mode enabled.
>
> Right now yes, the only way to get here for PV domains is when using
> shadow, but if this helper gets used in other non-shadow PV paths then
> Xen's needs to do a TLB flush.
Why would a non-shdow PV path find a need to call this function?
Jan
Re: [bug report] drm/xen-front: Add support for Xen PV display frontend
On 4/21/20 14:51, Dan Carpenter wrote:
> It turns out there aren't that many of these in xen.
>
> $ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
> drivers/gpu/drm/xen/xen_drm_front_kms.c:63: if (IS_ERR_OR_NULL(fb))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:86: if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:120:if
> (IS_ERR_OR_NULL(xen_obj->pages)) {
> drivers/gpu/drm/xen/xen_drm_front_gem.c:139:if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front_gem.c:197:if (IS_ERR_OR_NULL(xen_obj))
> drivers/gpu/drm/xen/xen_drm_front.c:403:if (IS_ERR_OR_NULL(obj)) {
>
> They're all wrong, because if the pointer was really NULL then it's
> not handled correctly and would eventually lead to a runtime failure.
>
> Normally Smatch is smart enough to know that the pointer isn't NULL so
> it doesn't generate a warning but yesterday I introduced a bug in Smatch
> by mistake. It's fixed now.
>
> regards,
> dan carpenter
>
Thank you,
I'll have a look at those
Re: [PATCH v2 1/2] x86/HVM: expose VM assist hypercall
On 21/04/2020 13:35, Julien Grall wrote: --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -700,6 +700,20 @@ static inline void pv_inject_sw_interrup pv_inject_event(); } +#define PV_VM_ASSIST_VALID ((1UL << VMASST_TYPE_4gb_segments) | \ + (1UL << VMASST_TYPE_4gb_segments_notify) | \ + (1UL << VMASST_TYPE_writable_pagetables) | \ + (1UL << VMASST_TYPE_pae_extended_cr3) | \ + (1UL << VMASST_TYPE_architectural_iopl) | \ + (1UL << VMASST_TYPE_runstate_update_flag)| \ + (1UL << VMASST_TYPE_m2p_strict)) +#define HVM_VM_ASSIST_VALID (1UL << VMASST_TYPE_runstate_update_flag) + +#define arch_vm_assist_valid(d) \ + (is_hvm_domain(d) ? HVM_VM_ASSIST_VALID \ + : is_pv_32bit_domain(d) ? (uint32_t)PV_VM_ASSIST_VALID \ >>> >>> I understand this is matching the current code, however without >>> looking at the rest of patch this is not clear why the cast. May >>> I suggest to add a comment explaining the rationale? >> >> Hmm, I can state that the rationale is history. Many of the assists in >> the low 32 bits are for 32-bit guests only. But we can't start refusing >> a 64-bit kernel requesting them. The ones in the high 32 bits are, for >> now, applicable to 64-bit guests only, and have always been refused for >> 32-bit ones. > > >> Imo if anything an explanation on where new bits should be put should >> go next to the VMASST_TYPE_* definitions in the public header, yet then >> again the public headers aren't (imo) a good place to put >> implementation detail comments. > > How about splitting PV_VM_ASSIST_VALID in two? One would contain > 64-bit PV specific flags and the other common PV flags? > > This should make the code more obvious and easier to read for someone > less familiar with the area. > > It also means we could have a BUILD_BUG_ON() to check at build time > that no flags are added above 32-bit. I like this suggestion, but would suggest a 64/32 split, rather than 64/common. These are a total mixed bag and every new one should be considered for all ABIs rather than falling automatically into some. ~Andrew
Re: [PATCH v2 1/2] x86/HVM: expose VM assist hypercall
On 21/04/2020 06:54, Jan Beulich wrote: > On 20.04.2020 19:53, Julien Grall wrote: >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -1517,20 +1517,23 @@ long do_vcpu_op(int cmd, unsigned int vc >>> return rc; >>> } >>> -#ifdef VM_ASSIST_VALID >>> -long vm_assist(struct domain *p, unsigned int cmd, unsigned int type, >>> - unsigned long valid) >>> +#ifdef arch_vm_assist_valid >> How about naming the function arch_vm_assist_valid_mask? > Certainly a possibility, albeit to me the gain would be marginal > and possibly not outweigh the growth in length. Andrew, any > preference? I prefer Julien's suggestion overall. It is obviously not a predicate, whereas the shorter name could easily be one. ~Andrew
Re: [PATCH v2 1/2] x86/HVM: expose VM assist hypercall
On 21/04/2020 06:54, Jan Beulich wrote: On 20.04.2020 19:53, Julien Grall wrote: --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1517,20 +1517,23 @@ long do_vcpu_op(int cmd, unsigned int vc return rc; } -#ifdef VM_ASSIST_VALID -long vm_assist(struct domain *p, unsigned int cmd, unsigned int type, - unsigned long valid) +#ifdef arch_vm_assist_valid How about naming the function arch_vm_assist_valid_mask? Certainly a possibility, albeit to me the gain would be marginal and possibly not outweigh the growth in length. Andrew, any preference? You have a point regarding the length of the function. --- a/xen/include/asm-x86/domain.h +++ b/xen/include/asm-x86/domain.h @@ -700,6 +700,20 @@ static inline void pv_inject_sw_interrup pv_inject_event(); } +#define PV_VM_ASSIST_VALID ((1UL << VMASST_TYPE_4gb_segments) | \ + (1UL << VMASST_TYPE_4gb_segments_notify) | \ + (1UL << VMASST_TYPE_writable_pagetables) | \ + (1UL << VMASST_TYPE_pae_extended_cr3) | \ + (1UL << VMASST_TYPE_architectural_iopl) | \ + (1UL << VMASST_TYPE_runstate_update_flag)| \ + (1UL << VMASST_TYPE_m2p_strict)) +#define HVM_VM_ASSIST_VALID (1UL << VMASST_TYPE_runstate_update_flag) + +#define arch_vm_assist_valid(d) \ + (is_hvm_domain(d) ? HVM_VM_ASSIST_VALID \ + : is_pv_32bit_domain(d) ? (uint32_t)PV_VM_ASSIST_VALID \ I understand this is matching the current code, however without looking at the rest of patch this is not clear why the cast. May I suggest to add a comment explaining the rationale? Hmm, I can state that the rationale is history. Many of the assists in the low 32 bits are for 32-bit guests only. But we can't start refusing a 64-bit kernel requesting them. The ones in the high 32 bits are, for now, applicable to 64-bit guests only, and have always been refused for 32-bit ones. > Imo if anything an explanation on where new bits should be put should go next to the VMASST_TYPE_* definitions in the public header, yet then again the public headers aren't (imo) a good place to put implementation detail comments. How about splitting PV_VM_ASSIST_VALID in two? One would contain 64-bit PV specific flags and the other common PV flags? This should make the code more obvious and easier to read for someone less familiar with the area. It also means we could have a BUILD_BUG_ON() to check at build time that no flags are added above 32-bit. Cheers, -- Julien Grall
Re: [PATCH] x86/shadow: make sh_remove_write_access() helper HVM only
On 21/04/2020 13:05, Jan Beulich wrote:
> Despite the inline attribute at least some clang versions warn about
> trace_shadow_wrmap_bf() being unused in !HVM builds. Include the helper
> in the #ifdef region.
>
> Fixes: 8b8d011ad868 ("x86/shadow: the guess_wrmap() hook is needed for HVM
> only")
> Reported-by: Andrew Cooper
> Signed-off-by: Jan Beulich
Reviewed-by: Andrew Cooper
[PATCH] x86/shadow: make sh_remove_write_access() helper HVM only
Despite the inline attribute at least some clang versions warn about
trace_shadow_wrmap_bf() being unused in !HVM builds. Include the helper
in the #ifdef region.
Fixes: 8b8d011ad868 ("x86/shadow: the guess_wrmap() hook is needed for HVM
only")
Reported-by: Andrew Cooper
Signed-off-by: Jan Beulich
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1756,6 +1756,7 @@ void sh_destroy_shadow(struct domain *d,
}
}
+#ifdef CONFIG_HVM
static inline void trace_shadow_wrmap_bf(mfn_t gmfn)
{
if ( tb_init_done )
@@ -1767,7 +1768,6 @@ static inline void trace_shadow_wrmap_bf
}
}
-#ifdef CONFIG_HVM
/**/
/* Remove all writeable mappings of a guest frame from the shadow tables
* Returns non-zero if we need to flush TLBs.
Re: [bug report] drm/xen-front: Add support for Xen PV display frontend
It turns out there aren't that many of these in xen.
$ grep IS_ERR_OR_NULL drivers/gpu/drm/xen/ -Rn
drivers/gpu/drm/xen/xen_drm_front_kms.c:63: if (IS_ERR_OR_NULL(fb))
drivers/gpu/drm/xen/xen_drm_front_gem.c:86: if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front_gem.c:120:if
(IS_ERR_OR_NULL(xen_obj->pages)) {
drivers/gpu/drm/xen/xen_drm_front_gem.c:139:if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front_gem.c:197:if (IS_ERR_OR_NULL(xen_obj))
drivers/gpu/drm/xen/xen_drm_front.c:403:if (IS_ERR_OR_NULL(obj)) {
They're all wrong, because if the pointer was really NULL then it's
not handled correctly and would eventually lead to a runtime failure.
Normally Smatch is smart enough to know that the pointer isn't NULL so
it doesn't generate a warning but yesterday I introduced a bug in Smatch
by mistake. It's fixed now.
regards,
dan carpenter
Re: [bug report] drm/xen-front: Add support for Xen PV display frontend
On 4/21/20 13:45, Dan Carpenter wrote:
> Hi Kernel Janitors,
Hi
>
> Here is another idea that someone could work on, fixing the
> IS_ERR_OR_NULL() checks in the xen driver.
>
> The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
> display frontend" from Apr 3, 2018, leads to the following static
> checker warning:
>
> drivers/gpu/drm/xen/xen_drm_front_gem.c:140 xen_drm_front_gem_create()
> warn: passing zero to 'ERR_CAST'
>
> drivers/gpu/drm/xen/xen_drm_front_gem.c
> 133 struct drm_gem_object *xen_drm_front_gem_create(struct drm_device
> *dev,
> 134 size_t size)
> 135 {
> 136 struct xen_gem_object *xen_obj;
> 137
> 138 xen_obj = gem_create(dev, size);
> 139 if (IS_ERR_OR_NULL(xen_obj))
> 140 return ERR_CAST(xen_obj);
>
> This warning is old and it's actually the result of a bug I had in my
> devel version of Smatch yesterday. xen_obj can't really be NULL, but
> if it were then the caller would return success which would probably
> create some issues.
>
> When a function returns both error pointers and NULL then NULL is a
> special case of success. Like say you have: "p = start_feature();".
> If there is an allocation failure, then the code should return -ENOMEM
> and the whole thing should fail. But if the feature is optional and
> the user has disabled it in the config then we return NULL and the
> caller has to be able to accept that. There are a lot of these
> IS_ERR_OR_NULL() checks in the xen driver...
>
> The one here is clearly buggy because returning NULL would lead to a
> run time failure. All these IS_ERR_OR_NULL() should be checked and
> probably changed to just IS_ERR().
>
> This sort of change is probably change is probably easiest if you build
> the Smatch DB and you can check if Smatch thinks the functions return
> NULL.
>
> ~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 | (-4095)-(-1) |
>INTERNAL | -1 | | struct xen_gem_object*(*)(struct
> drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 |
> 4065035897849303040 | INTERNAL | -1 | | struct
> xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 |
> 4065035897849303040 | INTERNAL | -1 | | struct
> xen_gem_object*(*)(struct drm_device*, ulong) |
> drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |
> INTERNAL | -1 | | struct xen_gem_object*(*)(struct
> drm_device*, ulong) |
>
> Smatch thinks that gem_create() sometimes returns NULL or error pointers
> but that's because of a bug in the unreleased version of Smatch and
> because gem_create() uses IS_ERR_OR_NULL().
>
> 141
> 142 return _obj->base;
> 143 }
>
> regards,
> dan carpenter
Thank you for the report, I will try to find some time to look into this
and come up with fixes.
Could you please (probably off-list) instruct me or give any pointers to
how to reproduce
the results of the analyzer shown above?
Thank you,
Oleksandr
[bug report] drm/xen-front: Add support for Xen PV display frontend
Hi Kernel Janitors,
Here is another idea that someone could work on, fixing the
IS_ERR_OR_NULL() checks in the xen driver.
The patch c575b7eeb89f: "drm/xen-front: Add support for Xen PV
display frontend" from Apr 3, 2018, leads to the following static
checker warning:
drivers/gpu/drm/xen/xen_drm_front_gem.c:140 xen_drm_front_gem_create()
warn: passing zero to 'ERR_CAST'
drivers/gpu/drm/xen/xen_drm_front_gem.c
133 struct drm_gem_object *xen_drm_front_gem_create(struct drm_device *dev,
134 size_t size)
135 {
136 struct xen_gem_object *xen_obj;
137
138 xen_obj = gem_create(dev, size);
139 if (IS_ERR_OR_NULL(xen_obj))
140 return ERR_CAST(xen_obj);
This warning is old and it's actually the result of a bug I had in my
devel version of Smatch yesterday. xen_obj can't really be NULL, but
if it were then the caller would return success which would probably
create some issues.
When a function returns both error pointers and NULL then NULL is a
special case of success. Like say you have: "p = start_feature();".
If there is an allocation failure, then the code should return -ENOMEM
and the whole thing should fail. But if the feature is optional and
the user has disabled it in the config then we return NULL and the
caller has to be able to accept that. There are a lot of these
IS_ERR_OR_NULL() checks in the xen driver...
The one here is clearly buggy because returning NULL would lead to a
run time failure. All these IS_ERR_OR_NULL() should be checked and
probably changed to just IS_ERR().
This sort of change is probably change is probably easiest if you build
the Smatch DB and you can check if Smatch thinks the functions return
NULL.
~/smatch/smatch_data/db/smdb.py return_states gem_create | grep INTERNAL
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 58 | (-4095)-(-1) |
INTERNAL | -1 | | struct xen_gem_object*(*)(struct
drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 62 | 4065035897849303040
| INTERNAL | -1 | | struct xen_gem_object*(*)(struct
drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 66 | 4065035897849303040
| INTERNAL | -1 | | struct xen_gem_object*(*)(struct
drm_device*, ulong) |
drivers/gpu/drm/xen/xen_drm_front_gem.c | gem_create | 68 | 0,(-4095)-(-1) |
INTERNAL | -1 | | struct xen_gem_object*(*)(struct
drm_device*, ulong) |
Smatch thinks that gem_create() sometimes returns NULL or error pointers
but that's because of a bug in the unreleased version of Smatch and
because gem_create() uses IS_ERR_OR_NULL().
141
142 return _obj->base;
143 }
regards,
dan carpenter
Re: [PATCH v10 1/3] x86/tlb: introduce a flush HVM ASIDs flag
On Tue, Apr 21, 2020 at 12:21:12PM +0200, Jan Beulich wrote:
> On 16.04.2020 15:59, Roger Pau Monne wrote:
> > Introduce a specific flag to request a HVM guest linear TLB flush,
> > which is an ASID/VPID tickle that forces a guest linear to guest
> > physical TLB flush for all HVM guests.
> >
> > This was previously unconditionally done in each pre_flush call, but
> > that's not required: HVM guests not using shadow don't require linear
> > TLB flushes as Xen doesn't modify the guest page tables in that case
> > (ie: when using HAP).
>
> I'm afraid I'm being confused by this: Even in shadow mode Xen
> doesn't modify guest page tables, does it?
I'm also confused now. It's my understand that when running in shadow
mode guest page tables are not actually used, and the guest uses Xen's
crafted shadow tables instead, which are based on the original guest
page tables suitably adjusted by Xen in order to do the p2m
translation in the HVM case, or the needed PTE adjustments in the PV
case.
So guest page tables are not modified, but are also not used as they
are never loaded into cr3.
> > @@ -254,3 +257,14 @@ unsigned int flush_area_local(const void *va, unsigned
> > int flags)
> >
> > return flags;
> > }
> > +
> > +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
> > +{
> > +unsigned int flags = (is_pv_domain(d) || paging_mode_shadow(d) ?
> > FLUSH_TLB
> > + : 0) |
> > + (is_hvm_domain(d) && cpu_has_svm ?
> > FLUSH_HVM_ASID_CORE
> > + : 0);
>
> Why the is_pv_domain() part of the condition? Afaict for PV
> domains you can get here only if they have shadow mode enabled.
Right now yes, the only way to get here for PV domains is when using
shadow, but if this helper gets used in other non-shadow PV paths then
Xen's needs to do a TLB flush.
> > --- a/xen/arch/x86/mm/shadow/private.h
> > +++ b/xen/arch/x86/mm/shadow/private.h
> > @@ -818,6 +818,12 @@ static inline int sh_check_page_has_no_refs(struct
> > page_info *page)
> > bool shadow_flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
> >void *ctxt);
> >
> > +static inline void sh_flush_local(const struct domain *d)
> > +{
> > +flush_local(FLUSH_TLB |
> > +(is_hvm_domain(d) && cpu_has_svm ? FLUSH_HVM_ASID_CORE :
> > 0));
> > +}
>
> I think the right side of | wants folding with its counterpart in
> guest_flush_tlb_mask(). Doing so would avoid guest_flush_tlb_mask()
> getting updated but this one forgotten. Perhaps split out
> guest_flush_tlb_flags() from guest_flush_tlb_mask()?
Can do.
> I also think this function should move into multi.c as long as it's
> needed only there.
Ack.
Thanks, Roger.
Re: [PATCH] x86: Enumeration for Control-flow Enforcement Technology
On 21/04/2020 08:11, Jan Beulich wrote: > On 20.04.2020 21:08, Andrew Cooper wrote: >> --- a/xen/include/public/arch-x86/cpufeatureset.h >> +++ b/xen/include/public/arch-x86/cpufeatureset.h >> @@ -229,6 +229,7 @@ XEN_CPUFEATURE(UMIP, 6*32+ 2) /*S User Mode >> Instruction Prevention */ >> XEN_CPUFEATURE(PKU, 6*32+ 3) /*H Protection Keys for Userspace */ >> XEN_CPUFEATURE(OSPKE, 6*32+ 4) /*! OS Protection Keys Enable */ >> XEN_CPUFEATURE(AVX512_VBMI2, 6*32+ 6) /*A Additional AVX-512 Vector Byte >> Manipulation Instrs */ >> +XEN_CPUFEATURE(CET_SS,6*32+ 7) /* CET - Shadow Stacks */ >> XEN_CPUFEATURE(GFNI, 6*32+ 8) /*A Galois Field Instrs */ >> XEN_CPUFEATURE(VAES, 6*32+ 9) /*A Vector AES Instrs */ >> XEN_CPUFEATURE(VPCLMULQDQ,6*32+10) /*A Vector Carry-less >> Multiplication Instrs */ >> @@ -255,6 +256,7 @@ XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A AVX512 >> Multiply Accumulation Single >> XEN_CPUFEATURE(MD_CLEAR, 9*32+10) /*A VERW clears microarchitectural >> buffers */ >> XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* MSR_TSX_FORCE_ABORT.RTM_ABORT */ >> XEN_CPUFEATURE(IBRSB, 9*32+26) /*A IBRS and IBPB support (used by >> Intel) */ >> +XEN_CPUFEATURE(CET_IBT, 6*32+20) /* CET - Indirect Branch Tracking >> */ > s/6/9/, moved up a line, and then Oops. I only spotted during final review that CET-SS and CET-IBT are in different feature leaves, then failed at adjusting the CET-IBT adequately. > Reviewed-by: Jan Beulich Thanks, > > I take it you intentionally don't mean to add #CP related bits yet, > first and foremost TRAP_control_flow or some such, as well as its > error code bits? Nor definitions for the bits within the MSRs you > add, nor XSAVE pieces? Those pieces aren't necessary to hide the MSRs, whereas this patch wants backporting in due course. Every "make the MSRs have correct architectural properties" will until MSR handling is fixed properly (and by this, I mean no default cases which leak state/availability, or discard writes). ~Andrew
Re: [PATCH v10 1/3] x86/tlb: introduce a flush HVM ASIDs flag
On 16.04.2020 15:59, Roger Pau Monne wrote:
> Introduce a specific flag to request a HVM guest linear TLB flush,
> which is an ASID/VPID tickle that forces a guest linear to guest
> physical TLB flush for all HVM guests.
>
> This was previously unconditionally done in each pre_flush call, but
> that's not required: HVM guests not using shadow don't require linear
> TLB flushes as Xen doesn't modify the guest page tables in that case
> (ie: when using HAP).
I'm afraid I'm being confused by this: Even in shadow mode Xen
doesn't modify guest page tables, does it?
> @@ -254,3 +257,14 @@ unsigned int flush_area_local(const void *va, unsigned
> int flags)
>
> return flags;
> }
> +
> +void guest_flush_tlb_mask(const struct domain *d, const cpumask_t *mask)
> +{
> +unsigned int flags = (is_pv_domain(d) || paging_mode_shadow(d) ?
> FLUSH_TLB
> + : 0) |
> + (is_hvm_domain(d) && cpu_has_svm ?
> FLUSH_HVM_ASID_CORE
> + : 0);
Why the is_pv_domain() part of the condition? Afaict for PV
domains you can get here only if they have shadow mode enabled.
> --- a/xen/arch/x86/mm/shadow/private.h
> +++ b/xen/arch/x86/mm/shadow/private.h
> @@ -818,6 +818,12 @@ static inline int sh_check_page_has_no_refs(struct
> page_info *page)
> bool shadow_flush_tlb(bool (*flush_vcpu)(void *ctxt, struct vcpu *v),
>void *ctxt);
>
> +static inline void sh_flush_local(const struct domain *d)
> +{
> +flush_local(FLUSH_TLB |
> +(is_hvm_domain(d) && cpu_has_svm ? FLUSH_HVM_ASID_CORE : 0));
> +}
I think the right side of | wants folding with its counterpart in
guest_flush_tlb_mask(). Doing so would avoid guest_flush_tlb_mask()
getting updated but this one forgotten. Perhaps split out
guest_flush_tlb_flags() from guest_flush_tlb_mask()?
I also think this function should move into multi.c as long as it's
needed only there.
Jan
Re: [PATCH] x86: Enumeration for Control-flow Enforcement Technology
On Mon, Apr 20, 2020 at 08:08:29PM +0100, Andrew Cooper wrote: > The CET spec has been published and guest kernels are starting to get support. > Introduce the CPUID and MSRs, and fully block the MSRs from guest use. > > Signed-off-by: Andrew Cooper Acked-by: Wei Liu
Re: [PATCH 1/1] x86/vtd: Mask DMAR faults for IGD devices
On 17.04.2020 15:36, Brendan Kerrigan wrote:
> The Intel graphics device records DMAR faults regardless
> of the Fault Process Disable bit being set.
I can't seem to be able to find a place where we would set FPD.
Why do we need the workaround then, or what am I missing?
> When this fault
> occurs, enable the Interrupt Mask (IM) bit in the Fault
> Event Control (FECTL) register to prevent the continued
> recording of the fault.
This should mention the underlying erratum in one way or another.
> --- a/xen/drivers/passthrough/vtd/iommu.c
> +++ b/xen/drivers/passthrough/vtd/iommu.c
> @@ -41,6 +41,8 @@
> #include "vtd.h"
> #include "../ats.h"
>
> +#define IS_IGD(seg, id) (0 == seg && 0 == PCI_BUS(id) && 2 == PCI_SLOT(id)
> && 0 == PCI_FUNC(id))
> +
> struct mapped_rmrr {
> struct list_head list;
> u64 base, end;
> @@ -872,6 +874,13 @@ static int iommu_page_fault_do_one(struct vtd_iommu
> *iommu, int type,
> printk(XENLOG_G_WARNING VTDPREFIX "%s: reason %02x - %s\n",
> kind, fault_reason, reason);
>
> +if ( DMA_REMAP == fault_type && type && IS_IGD(seg, source_id) ) {
Using existing infrastructure would be at least some improvement, in
particular to avoid this workaround triggering on unaffected
hardware (including such where there's something else at :00:02.0).
See e.g. is_igd_drhd(), and note that most uses of it are currently
further qualified to limit what hardware quirk workarounds get applied
to. In any event you'll want to clarify in the description that this
won't ever be done on hardware not having this issue, at least as long
as this isn't obvious from the code.
Also - why the "type" part of the conditional? The erratum text
doesn't talk about only DMA reads being affected.
Finally, style-wise the brace belongs on its own line.
> +u32 fectl = dmar_readl(iommu->reg, DMAR_FECTL_REG);
> +fectl |= DMA_FECTL_IM;
While this is the recommended workaround, I don't see you undoing
the masking anywhere later, and I'm also unconvinced this should be
done upon seeing the first fault. One option might be to do so on the
the first fault when FPD is set for the offending device. Or else,
following the title, it might want/need doing unconditionally at
initialization time.
> +dmar_writel(iommu->reg, DMAR_FECTL_REG, fectl);
> +printk(XENLOG_G_WARNING VTDPREFIX "Disabling DMAR faults for IGD\n");
If, as suggested above, IM will get cleared again at some point,
this printk() should imo not be issued more than once.
Jan
Re: [PATCH v15 2/3] mem_sharing: allow forking domain with IOMMU enabled
On 17.04.2020 19:06, Tamas K Lengyel wrote:
> @@ -2063,9 +2065,10 @@ int
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
> case XENMEM_sharing_op_fork:
> {
> struct domain *pd;
> +bool allow_iommu;
>
> rc = -EINVAL;
> -if ( mso.u.fork.pad[0] || mso.u.fork.pad[1] || mso.u.fork.pad[2] )
> +if ( mso.u.fork.pad[0] || mso.u.fork.pad[1] )
> goto out;
Rather than outright dropping this, you now want to bail on
any bits set in flags except the one that's currently defined.
Jan
[PATCH v2 4/4] x86: adjustments to guest handle treatment
First of all avoid excessive conversions. copy_{from,to}_guest(), for
example, work fine with all of XEN_GUEST_HANDLE{,_64,_PARAM}().
Further
- do_physdev_op_compat() didn't use the param form for its parameter,
- {hap,shadow}_track_dirty_vram() wrongly used the param form,
- compat processor Px logic failed to check compatibility of native and
compat structures not further converted.
As this eliminates all users of guest_handle_from_param() and as there's
no real need to allow for conversions in both directions, drop the
macros as well.
Signed-off-by: Jan Beulich
---
v2: New.
--- a/xen/arch/x86/compat.c
+++ b/xen/arch/x86/compat.c
@@ -15,7 +15,7 @@ typedef long ret_t;
#endif
/* Legacy hypercall (as of 0x00030202). */
-ret_t do_physdev_op_compat(XEN_GUEST_HANDLE(physdev_op_t) uop)
+ret_t do_physdev_op_compat(XEN_GUEST_HANDLE_PARAM(physdev_op_t) uop)
{
typeof(do_physdev_op) *fn =
(void *)pv_hypercall_table[__HYPERVISOR_physdev_op].native;
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -678,7 +678,7 @@ static long microcode_update_helper(void
return ret;
}
-int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
+int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
{
int ret;
struct ucode_buf *buffer;
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4441,20 +4441,16 @@ static int _handle_iomem_range(unsigned
{
if ( s > ctxt->s && !(s >> (paddr_bits - PAGE_SHIFT)) )
{
-e820entry_t ent;
-XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
-XEN_GUEST_HANDLE(e820entry_t) buffer;
-
if ( !guest_handle_is_null(ctxt->map.buffer) )
{
+e820entry_t ent;
+
if ( ctxt->n + 1 >= ctxt->map.nr_entries )
return -EINVAL;
ent.addr = (uint64_t)ctxt->s << PAGE_SHIFT;
ent.size = (uint64_t)(s - ctxt->s) << PAGE_SHIFT;
ent.type = E820_RESERVED;
-buffer_param = guest_handle_cast(ctxt->map.buffer, e820entry_t);
-buffer = guest_handle_from_param(buffer_param, e820entry_t);
-if ( __copy_to_guest_offset(buffer, ctxt->n, , 1) )
+if ( __copy_to_guest_offset(ctxt->map.buffer, ctxt->n, , 1) )
return -EFAULT;
}
ctxt->n++;
@@ -4715,8 +4711,7 @@ long arch_memory_op(unsigned long cmd, X
case XENMEM_machine_memory_map:
{
struct memory_map_context ctxt;
-XEN_GUEST_HANDLE(e820entry_t) buffer;
-XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer_param;
+XEN_GUEST_HANDLE_PARAM(e820entry_t) buffer;
unsigned int i;
bool store;
@@ -4732,8 +4727,7 @@ long arch_memory_op(unsigned long cmd, X
if ( store && ctxt.map.nr_entries < e820.nr_map + 1 )
return -EINVAL;
-buffer_param = guest_handle_cast(ctxt.map.buffer, e820entry_t);
-buffer = guest_handle_from_param(buffer_param, e820entry_t);
+buffer = guest_handle_cast(ctxt.map.buffer, e820entry_t);
if ( store && !guest_handle_okay(buffer, ctxt.map.nr_entries) )
return -EFAULT;
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -59,7 +59,7 @@
int hap_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
unsigned long nr,
- XEN_GUEST_HANDLE_PARAM(void) guest_dirty_bitmap)
+ XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
{
long rc = 0;
struct sh_dirty_vram *dirty_vram;
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -3171,7 +3171,7 @@ static void sh_clean_dirty_bitmap(struct
int shadow_track_dirty_vram(struct domain *d,
unsigned long begin_pfn,
unsigned long nr,
-XEN_GUEST_HANDLE_PARAM(void) guest_dirty_bitmap)
+XEN_GUEST_HANDLE(void) guest_dirty_bitmap)
{
int rc = 0;
unsigned long end_pfn = begin_pfn + nr;
--- a/xen/arch/x86/oprofile/backtrace.c
+++ b/xen/arch/x86/oprofile/backtrace.c
@@ -74,11 +74,8 @@ dump_guest_backtrace(struct vcpu *vcpu,
}
else
{
-XEN_GUEST_HANDLE(const_frame_head_t) guest_head;
-XEN_GUEST_HANDLE_PARAM(const_frame_head_t) guest_head_param =
+XEN_GUEST_HANDLE_PARAM(const_frame_head_t) guest_head =
const_guest_handle_from_ptr(head, frame_head_t);
-guest_head = guest_handle_from_param(guest_head_param,
-const_frame_head_t);
/* Also check accessibility of one struct frame_head beyond */
if (!guest_handle_okay(guest_head, 2))
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -285,9 +285,7 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
guest_from_compat_handle(data, op->u.microcode.data);
-
[PATCH v2 3/4] x86/mm: monitor table is HVM-only
Move the per-vCPU field to the HVM sub-structure.
Signed-off-by: Jan Beulich
Acked-by: Andrew Cooper
Acked-by: Tim Deegan
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -545,7 +545,7 @@ void write_ptbase(struct vcpu *v)
* Should be called after CR3 is updated.
*
* Uses values found in vcpu->arch.(guest_table and guest_table_user), and
- * for HVM guests, arch.monitor_table and hvm's guest CR3.
+ * for HVM guests, arch.hvm.monitor_table and hvm's guest CR3.
*
* Update ref counts to shadow tables appropriately.
*/
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -393,7 +393,7 @@ static mfn_t hap_make_monitor_table(stru
l4_pgentry_t *l4e;
mfn_t m4mfn;
-ASSERT(pagetable_get_pfn(v->arch.monitor_table) == 0);
+ASSERT(pagetable_get_pfn(v->arch.hvm.monitor_table) == 0);
if ( (pg = hap_alloc(d)) == NULL )
goto oom;
@@ -579,10 +579,10 @@ void hap_teardown(struct domain *d, bool
{
if ( paging_get_hostmode(v) && paging_mode_external(d) )
{
-mfn = pagetable_get_mfn(v->arch.monitor_table);
+mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
if ( mfn_valid(mfn) && (mfn_x(mfn) != 0) )
hap_destroy_monitor_table(v, mfn);
-v->arch.monitor_table = pagetable_null();
+v->arch.hvm.monitor_table = pagetable_null();
}
}
}
@@ -758,10 +758,10 @@ static void hap_update_paging_modes(stru
v->arch.paging.mode = hap_paging_get_mode(v);
-if ( pagetable_is_null(v->arch.monitor_table) )
+if ( pagetable_is_null(v->arch.hvm.monitor_table) )
{
mfn_t mmfn = hap_make_monitor_table(v);
-v->arch.monitor_table = pagetable_from_mfn(mmfn);
+v->arch.hvm.monitor_table = pagetable_from_mfn(mmfn);
make_cr3(v, mmfn);
hvm_update_host_cr3(v);
}
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -2465,10 +2465,10 @@ static void sh_update_paging_modes(struc
_INTERNAL_NAME(sh_paging_mode, 2);
}
-if ( pagetable_is_null(v->arch.monitor_table) )
+if ( pagetable_is_null(v->arch.hvm.monitor_table) )
{
mfn_t mmfn = v->arch.paging.mode->shadow.make_monitor_table(v);
-v->arch.monitor_table = pagetable_from_mfn(mmfn);
+v->arch.hvm.monitor_table = pagetable_from_mfn(mmfn);
make_cr3(v, mmfn);
hvm_update_host_cr3(v);
}
@@ -2502,10 +2502,10 @@ static void sh_update_paging_modes(struc
return;
}
-old_mfn = pagetable_get_mfn(v->arch.monitor_table);
-v->arch.monitor_table = pagetable_null();
+old_mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
+v->arch.hvm.monitor_table = pagetable_null();
new_mfn = v->arch.paging.mode->shadow.make_monitor_table(v);
-v->arch.monitor_table = pagetable_from_mfn(new_mfn);
+v->arch.hvm.monitor_table = pagetable_from_mfn(new_mfn);
SHADOW_PRINTK("new monitor table %"PRI_mfn "\n",
mfn_x(new_mfn));
@@ -2724,11 +2724,11 @@ void shadow_teardown(struct domain *d, b
#ifdef CONFIG_HVM
if ( shadow_mode_external(d) )
{
-mfn_t mfn = pagetable_get_mfn(v->arch.monitor_table);
+mfn_t mfn = pagetable_get_mfn(v->arch.hvm.monitor_table);
if ( mfn_valid(mfn) && (mfn_x(mfn) != 0) )
v->arch.paging.mode->shadow.destroy_monitor_table(v,
mfn);
-v->arch.monitor_table = pagetable_null();
+v->arch.hvm.monitor_table = pagetable_null();
}
#endif /* CONFIG_HVM */
}
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -1521,7 +1521,7 @@ sh_make_monitor_table(struct vcpu *v)
{
struct domain *d = v->domain;
-ASSERT(pagetable_get_pfn(v->arch.monitor_table) == 0);
+ASSERT(pagetable_get_pfn(v->arch.hvm.monitor_table) == 0);
/* Guarantee we can get the memory we need */
shadow_prealloc(d, SH_type_monitor_table, CONFIG_PAGING_LEVELS);
@@ -3699,7 +3699,7 @@ sh_update_linear_entries(struct vcpu *v)
/* Don't try to update the monitor table if it doesn't exist */
if ( !shadow_mode_external(d) ||
- pagetable_get_pfn(v->arch.monitor_table) == 0 )
+ pagetable_get_pfn(v->arch.hvm.monitor_table) == 0 )
return;
#if SHADOW_PAGING_LEVELS == 4
@@ -3717,7 +3717,7 @@ sh_update_linear_entries(struct vcpu *v)
{
l4_pgentry_t *ml4e;
-ml4e = map_domain_page(pagetable_get_mfn(v->arch.monitor_table));
+ml4e = map_domain_page(pagetable_get_mfn(v->arch.hvm.monitor_table));
ml4e[l4_table_offset(SH_LINEAR_PT_VIRT_START)] =
[PATCH v2 2/4] x86/shadow: sh_update_linear_entries() is a no-op for PV
Consolidate the shadow_mode_external() in here: Check this once at the
start of the function.
Signed-off-by: Jan Beulich
Acked-by: Andrew Cooper
Acked-by: Tim Deegan
---
v2: Delete stale part of comment.
---
Tim - I'm re-posting as I wasn't entirely sure whether you meant to drop
the entire PV part of the comment, or only the last two sentences.
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -3680,20 +3680,7 @@ sh_update_linear_entries(struct vcpu *v)
{
struct domain *d = v->domain;
-/* Linear pagetables in PV guests
- * --
- *
- * Guest linear pagetables, which map the guest pages, are at
- * LINEAR_PT_VIRT_START. Shadow linear pagetables, which map the
- * shadows, are at SH_LINEAR_PT_VIRT_START. Most of the time these
- * are set up at shadow creation time, but (of course!) the PAE case
- * is subtler. Normal linear mappings are made by having an entry
- * in the top-level table that points to itself (shadow linear) or
- * to the guest top-level table (guest linear). For PAE, to set up
- * a linear map requires us to copy the four top-level entries into
- * level-2 entries. That means that every time we change a PAE l3e,
- * we need to reflect the change into the copy.
- *
+/*
* Linear pagetables in HVM guests
* ---
*
@@ -3711,34 +3698,30 @@ sh_update_linear_entries(struct vcpu *v)
*/
/* Don't try to update the monitor table if it doesn't exist */
-if ( shadow_mode_external(d)
- && pagetable_get_pfn(v->arch.monitor_table) == 0 )
+if ( !shadow_mode_external(d) ||
+ pagetable_get_pfn(v->arch.monitor_table) == 0 )
return;
#if SHADOW_PAGING_LEVELS == 4
-/* For PV, one l4e points at the guest l4, one points at the shadow
- * l4. No maintenance required.
- * For HVM, just need to update the l4e that points to the shadow l4. */
+/* For HVM, just need to update the l4e that points to the shadow l4. */
-if ( shadow_mode_external(d) )
+/* Use the linear map if we can; otherwise make a new mapping */
+if ( v == current )
{
-/* Use the linear map if we can; otherwise make a new mapping */
-if ( v == current )
-{
-__linear_l4_table[l4_linear_offset(SH_LINEAR_PT_VIRT_START)] =
-l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
- __PAGE_HYPERVISOR_RW);
-}
-else
-{
-l4_pgentry_t *ml4e;
-ml4e = map_domain_page(pagetable_get_mfn(v->arch.monitor_table));
-ml4e[l4_table_offset(SH_LINEAR_PT_VIRT_START)] =
-l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
- __PAGE_HYPERVISOR_RW);
-unmap_domain_page(ml4e);
-}
+__linear_l4_table[l4_linear_offset(SH_LINEAR_PT_VIRT_START)] =
+l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
+ __PAGE_HYPERVISOR_RW);
+}
+else
+{
+l4_pgentry_t *ml4e;
+
+ml4e = map_domain_page(pagetable_get_mfn(v->arch.monitor_table));
+ml4e[l4_table_offset(SH_LINEAR_PT_VIRT_START)] =
+l4e_from_pfn(pagetable_get_pfn(v->arch.shadow_table[0]),
+ __PAGE_HYPERVISOR_RW);
+unmap_domain_page(ml4e);
}
#elif SHADOW_PAGING_LEVELS == 3
@@ -3752,7 +3735,6 @@ sh_update_linear_entries(struct vcpu *v)
* the shadows.
*/
-ASSERT(shadow_mode_external(d));
{
/* Install copies of the shadow l3es into the monitor l2 table
* that maps SH_LINEAR_PT_VIRT_START. */
@@ -3803,20 +3785,16 @@ sh_update_linear_entries(struct vcpu *v)
#error this should not happen
#endif
-if ( shadow_mode_external(d) )
-{
-/*
- * Having modified the linear pagetable mapping, flush local host TLBs.
- * This was not needed when vmenter/vmexit always had the side effect
- * of flushing host TLBs but, with ASIDs, it is possible to finish
- * this CR3 update, vmenter the guest, vmexit due to a page fault,
- * without an intervening host TLB flush. Then the page fault code
- * could use the linear pagetable to read a top-level shadow page
- * table entry. But, without this change, it would fetch the wrong
- * value due to a stale TLB.
- */
-flush_tlb_local();
-}
+/*
+ * Having modified the linear pagetable mapping, flush local host TLBs.
+ * This was not needed when vmenter/vmexit always had the side effect of
+ * flushing host TLBs but, with ASIDs, it is possible to finish this CR3
+ * update, vmenter the guest, vmexit due to a page fault, without an
+ * intervening host TLB flush. Then the page fault code could use the
+ * linear pagetable to read a top-level shadow page table
[PATCH v2 1/4] x86/mm: no-one passes a NULL domain to init_xen_l4_slots()
Drop the NULL checks - they've been introduced by commit 8d7b633ada
("x86/mm: Consolidate all Xen L4 slot writing into
init_xen_l4_slots()") for no apparent reason.
Signed-off-by: Jan Beulich
---
v2: Adjust comment ahead of the function.
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1653,7 +1653,7 @@ static int promote_l3_table(struct page_
* This function must write all ROOT_PAGETABLE_PV_XEN_SLOTS, to clobber any
* values a guest may have left there from promote_l4_table().
*
- * l4t and l4mfn are mandatory, but l4mfn doesn't need to be the mfn under
+ * l4t, l4mfn, and d are mandatory, but l4mfn doesn't need to be the mfn under
* *l4t. All other parameters are optional and will either fill or zero the
* appropriate slots. Pagetables not shared with guests will gain the
* extended directmap.
@@ -1665,7 +1665,7 @@ void init_xen_l4_slots(l4_pgentry_t *l4t
* PV vcpus need a shortened directmap. HVM and Idle vcpus get the full
* directmap.
*/
-bool short_directmap = d && !paging_mode_external(d);
+bool short_directmap = !paging_mode_external(d);
/* Slot 256: RO M2P (if applicable). */
l4t[l4_table_offset(RO_MPT_VIRT_START)] =
@@ -1686,10 +1686,9 @@ void init_xen_l4_slots(l4_pgentry_t *l4t
mfn_eq(sl4mfn, INVALID_MFN) ? l4e_empty() :
l4e_from_mfn(sl4mfn, __PAGE_HYPERVISOR_RW);
-/* Slot 260: Per-domain mappings (if applicable). */
+/* Slot 260: Per-domain mappings. */
l4t[l4_table_offset(PERDOMAIN_VIRT_START)] =
-d ? l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW)
- : l4e_empty();
+l4e_from_page(d->arch.perdomain_l3_pg, __PAGE_HYPERVISOR_RW);
/* Slot 261-: text/data/bss, RW M2P, vmap, frametable, directmap. */
#ifndef NDEBUG
[PATCH v2 0/4] x86: mm (mainly shadow) adjustments
(Not so) large (anymore) parts of this series are to further isolate pieces which are needed for HVM only, and hence would better not be built with HVM=n. But there are also a few other items which I've noticed along the road, including the new in v2 4th patch. 1: mm: no-one passes a NULL domain to init_xen_l4_slots() 2: shadow: sh_update_linear_entries() is a no-op for PV 3: mm: monitor table is HVM-only 4: adjustments to guest handle treatment Jan
[PATCH v4 03/13] numa: Teach ram block notifiers about resizeable ram blocks
Ram block notifiers are currently not aware of resizes. Especially to
handle resizes during migration, but also to implement actually resizeable
ram blocks (make everything between used_length and max_length
inaccessible), we want to teach ram block notifiers about resizeable
ram.
Introduce the basic infrastructure but keep using max_size in the
existing notifiers. Supply the max_size when adding and removing ram
blocks. Also, notify on resizes.
Acked-by: Paul Durrant
Reviewed-by: Peter Xu
Cc: Richard Henderson
Cc: Paolo Bonzini
Cc: "Dr. David Alan Gilbert"
Cc: Eduardo Habkost
Cc: Marcel Apfelbaum
Cc: Stefano Stabellini
Cc: Anthony Perard
Cc: Paul Durrant
Cc: "Michael S. Tsirkin"
Cc: xen-devel@lists.xenproject.org
Cc: Igor Mammedov
Signed-off-by: David Hildenbrand
---
exec.c | 11 +--
hw/core/numa.c | 22 +-
hw/i386/xen/xen-mapcache.c | 7 ---
include/exec/ramlist.h | 13 +
target/i386/hax-mem.c | 5 +++--
target/i386/sev.c | 18 ++
util/vfio-helpers.c| 16
7 files changed, 60 insertions(+), 32 deletions(-)
diff --git a/exec.c b/exec.c
index 4f804347a6..83304e51c6 100644
--- a/exec.c
+++ b/exec.c
@@ -2115,6 +2115,11 @@ int qemu_ram_resize(RAMBlock *block, ram_addr_t newsize,
Error **errp)
return -EINVAL;
}
+/* Notify before modifying the ram block and touching the bitmaps. */
+if (block->host) {
+ram_block_notify_resize(block->host, block->used_length, newsize);
+}
+
cpu_physical_memory_clear_dirty_range(block->offset, block->used_length);
block->used_length = newsize;
cpu_physical_memory_set_dirty_range(block->offset, block->used_length,
@@ -2281,7 +2286,8 @@ static void ram_block_add(RAMBlock *new_block, Error
**errp, bool shared)
qemu_madvise(new_block->host, new_block->max_length,
QEMU_MADV_DONTFORK);
}
-ram_block_notify_add(new_block->host, new_block->max_length);
+ram_block_notify_add(new_block->host, new_block->used_length,
+ new_block->max_length);
}
}
@@ -2461,7 +2467,8 @@ void qemu_ram_free(RAMBlock *block)
}
if (block->host) {
-ram_block_notify_remove(block->host, block->max_length);
+ram_block_notify_remove(block->host, block->used_length,
+block->max_length);
}
qemu_mutex_lock_ramlist();
diff --git a/hw/core/numa.c b/hw/core/numa.c
index dc5e5b4046..fe6ca5c50d 100644
--- a/hw/core/numa.c
+++ b/hw/core/numa.c
@@ -857,11 +857,12 @@ void query_numa_node_mem(NumaNodeMem node_mem[],
MachineState *ms)
static int ram_block_notify_add_single(RAMBlock *rb, void *opaque)
{
const ram_addr_t max_size = qemu_ram_get_max_length(rb);
+const ram_addr_t size = qemu_ram_get_used_length(rb);
void *host = qemu_ram_get_host_addr(rb);
RAMBlockNotifier *notifier = opaque;
if (host) {
-notifier->ram_block_added(notifier, host, max_size);
+notifier->ram_block_added(notifier, host, size, max_size);
}
return 0;
}
@@ -878,20 +879,31 @@ void ram_block_notifier_remove(RAMBlockNotifier *n)
QLIST_REMOVE(n, next);
}
-void ram_block_notify_add(void *host, size_t size)
+void ram_block_notify_add(void *host, size_t size, size_t max_size)
{
RAMBlockNotifier *notifier;
QLIST_FOREACH(notifier, _list.ramblock_notifiers, next) {
-notifier->ram_block_added(notifier, host, size);
+notifier->ram_block_added(notifier, host, size, max_size);
}
}
-void ram_block_notify_remove(void *host, size_t size)
+void ram_block_notify_remove(void *host, size_t size, size_t max_size)
{
RAMBlockNotifier *notifier;
QLIST_FOREACH(notifier, _list.ramblock_notifiers, next) {
-notifier->ram_block_removed(notifier, host, size);
+notifier->ram_block_removed(notifier, host, size, max_size);
+}
+}
+
+void ram_block_notify_resize(void *host, size_t old_size, size_t new_size)
+{
+RAMBlockNotifier *notifier;
+
+QLIST_FOREACH(notifier, _list.ramblock_notifiers, next) {
+if (notifier->ram_block_resized) {
+notifier->ram_block_resized(notifier, host, old_size, new_size);
+}
}
}
diff --git a/hw/i386/xen/xen-mapcache.c b/hw/i386/xen/xen-mapcache.c
index 5b120ed44b..d6dcea65d1 100644
--- a/hw/i386/xen/xen-mapcache.c
+++ b/hw/i386/xen/xen-mapcache.c
@@ -169,7 +169,8 @@ static void xen_remap_bucket(MapCacheEntry *entry,
if (entry->vaddr_base != NULL) {
if (!(entry->flags & XEN_MAPCACHE_ENTRY_DUMMY)) {
-ram_block_notify_remove(entry->vaddr_base, entry->size);
+ram_block_notify_remove(entry->vaddr_base, entry->size,
+entry->size);
}
if (munmap(entry->vaddr_base, entry->size) != 0) {
perror("unmap fails");
@@ -211,7 +212,7 @@
Re: [PATCH 3/3] x86/pv: Don't use IST for NMI/#MC/#DB in !CONFIG_PV builds
On 20.04.2020 16:59, Andrew Cooper wrote:
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -441,12 +441,18 @@ struct tss_page {
> };
> DECLARE_PER_CPU(struct tss_page, tss_page);
>
> +/*
> + * Interrupt Stack Tables. Used to force a stack switch on a CPL0=>0
> + * interrupt/exception. #DF uses IST all the time to detect stack overflows
> + * cleanly. NMI/#MC/#DB only need IST to cover the SYSCALL gap, and
> therefore
> + * only necessary with PV guests.
> + */
Is it really only the SYSCALL gap that we mean to cover? In particular
for #MC I'd suspect it is a good idea to switch stacks as well, to get
onto a distinct memory page in case the #MC was stack related. With
NMI it might as well be better to switch; I agree we don't need any
switching for #DB.
I also think that the comment at the top of current.h would want
updating with these adjustments (which I notice lacks the #DB part
anyway).
Jan
Re: [PATCH 2/3] x86/boot: Don't enable EFER.SCE for !CONFIG_PV builds
On 20.04.2020 16:59, Andrew Cooper wrote:
> --- a/xen/arch/x86/efi/efi-boot.h
> +++ b/xen/arch/x86/efi/efi-boot.h
> @@ -238,7 +238,8 @@ static void __init noreturn efi_arch_post_exit_boot(void)
> /* Set system registers and transfer control. */
> asm volatile("pushq $0\n\tpopfq");
> rdmsrl(MSR_EFER, efer);
> -efer |= EFER_SCE;
> +if ( IS_ENABLED(CONFIG_PV) )
> +efer |= EFER_SCE;
> if ( cpu_has_nx )
> efer |= EFER_NX;
> wrmsrl(MSR_EFER, efer);
Switch to simply ORing in trampoline_efer here?
With or without the adjustment
Reviewed-by: Jan Beulich
Jan
Re: [PATCH 1/3] x86/S3: Use percpu_traps_init() rather than opencoding SYSCALL/SYSENTER restoration
On 20.04.2020 16:59, Andrew Cooper wrote: > @@ -46,24 +36,13 @@ void restore_rest_processor_state(void) > /* Restore full CR4 (inc MCE) now that the IDT is in place. */ > write_cr4(mmu_cr4_features); > > -/* Recover syscall MSRs */ > -wrmsrl(MSR_LSTAR, saved_lstar); > -wrmsrl(MSR_CSTAR, saved_cstar); > -wrmsrl(MSR_STAR, XEN_MSR_STAR); > -wrmsrl(MSR_SYSCALL_MASK, XEN_SYSCALL_MASK); > +/* (re)initialise SYSCALL/SYSENTER state, amongst other things. */ > +percpu_traps_init(); Without tweaks to subarch_percpu_traps_init() this assumes that, now and going forward, map_domain_page() will work reliably at this early point of resume. I'm not opposed, i.e. Acked-by: Jan Beulich but it would feel less fragile if the redundant re-writing of the stubs would be skipped in the BSP resume case (I didn't check whether it's also redundant for APs, but I suspect it's not, as the stub pages may get allocated anew). Jan
Re: [PATCH] x86: Enumeration for Control-flow Enforcement Technology
On 20.04.2020 21:08, Andrew Cooper wrote: > --- a/xen/include/public/arch-x86/cpufeatureset.h > +++ b/xen/include/public/arch-x86/cpufeatureset.h > @@ -229,6 +229,7 @@ XEN_CPUFEATURE(UMIP, 6*32+ 2) /*S User Mode > Instruction Prevention */ > XEN_CPUFEATURE(PKU, 6*32+ 3) /*H Protection Keys for Userspace */ > XEN_CPUFEATURE(OSPKE, 6*32+ 4) /*! OS Protection Keys Enable */ > XEN_CPUFEATURE(AVX512_VBMI2, 6*32+ 6) /*A Additional AVX-512 Vector Byte > Manipulation Instrs */ > +XEN_CPUFEATURE(CET_SS,6*32+ 7) /* CET - Shadow Stacks */ > XEN_CPUFEATURE(GFNI, 6*32+ 8) /*A Galois Field Instrs */ > XEN_CPUFEATURE(VAES, 6*32+ 9) /*A Vector AES Instrs */ > XEN_CPUFEATURE(VPCLMULQDQ,6*32+10) /*A Vector Carry-less Multiplication > Instrs */ > @@ -255,6 +256,7 @@ XEN_CPUFEATURE(AVX512_4FMAPS, 9*32+ 3) /*A AVX512 > Multiply Accumulation Single > XEN_CPUFEATURE(MD_CLEAR, 9*32+10) /*A VERW clears microarchitectural > buffers */ > XEN_CPUFEATURE(TSX_FORCE_ABORT, 9*32+13) /* MSR_TSX_FORCE_ABORT.RTM_ABORT */ > XEN_CPUFEATURE(IBRSB, 9*32+26) /*A IBRS and IBPB support (used by > Intel) */ > +XEN_CPUFEATURE(CET_IBT, 6*32+20) /* CET - Indirect Branch Tracking */ s/6/9/, moved up a line, and then Reviewed-by: Jan Beulich I take it you intentionally don't mean to add #CP related bits yet, first and foremost TRAP_control_flow or some such, as well as its error code bits? Nor definitions for the bits within the MSRs you add, nor XSAVE pieces? Jan
Re: [PATCH v15 2/3] mem_sharing: allow forking domain with IOMMU enabled
On Mon, Apr 20, 2020 at 08:19:12AM -0600, Tamas K Lengyel wrote:
> On Mon, Apr 20, 2020 at 1:57 AM Roger Pau Monné wrote:
> >
> > On Fri, Apr 17, 2020 at 10:06:32AM -0700, Tamas K Lengyel wrote:
> > > diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> > > index d36d64b8dc..1d2149def3 100644
> > > --- a/xen/include/public/memory.h
> > > +++ b/xen/include/public/memory.h
> > > @@ -536,7 +536,9 @@ struct xen_mem_sharing_op {
> > > } debug;
> > > struct mem_sharing_op_fork { /* OP_FORK */
> > > domid_t parent_domain;/* IN: parent's domain id */
> > > -uint16_t pad[3]; /* Must be set to 0 */
> > > +#define XENMEM_FORK_WITH_IOMMU_ALLOWED 1 /* Allow forking domain with
> > > IOMMU */
> >
> > Since this is a flags field, can you express this is as: (1u << 0).
>
> I was thinking of doing that then it won't fit into a single line. For
> this particular flag it would also make no difference.
Right, but when new flags are added it looks weird IMO to have:
#define XENMEM_FORK_WITH_IOMMU_ALLOWED 1
#define XENMEM_FOO_BAR_WITH_FOO(1u << 1)
Thanks, Roger.
Re: [PATCH v2] sched: print information about scheduling granularity
On 20/04/2020 14:45, Jürgen Groß wrote:
> On 20.04.20 15:06, Sergey Dyasli wrote:
>> Currently it might be not obvious which scheduling mode (e.g. core-
>> scheduling) is being used by the scheduler. Alleviate this by printing
>> additional information about the selected granularity per-cpupool.
>>
>> Note: per-cpupool granularity selection is not implemented yet.
>> The single global value is being used for each cpupool.
>
> This is misleading. You are using the per-cpupool values, but they
> are all the same right now.
This is what I meant by my note, but I might need to improve the wording
since the current one looks ambiguous to you.
>
>>
>> Signed-off-by: Sergey Dyasli
>> ---
>> v2:
>> - print information on a separate line
>> - use per-cpupool granularity
>> - updated commit message
>>
>> CC: Juergen Gross
>> CC: Dario Faggioli
>> CC: George Dunlap
>> CC: Jan Beulich
>> ---
>> xen/common/sched/cpupool.c | 26 ++
>> 1 file changed, 26 insertions(+)
>>
>> diff --git a/xen/common/sched/cpupool.c b/xen/common/sched/cpupool.c
>> index d40345b585..68106f6c15 100644
>> --- a/xen/common/sched/cpupool.c
>> +++ b/xen/common/sched/cpupool.c
>> @@ -40,6 +40,30 @@ static DEFINE_SPINLOCK(cpupool_lock);
>> static enum sched_gran __read_mostly opt_sched_granularity =
>> SCHED_GRAN_cpu;
>> static unsigned int __read_mostly sched_granularity = 1;
>> +static void sched_gran_print(enum sched_gran mode, unsigned int gran)
>> +{
>> + char *str = "";
>> +
>> + switch ( mode )
>> + {
>> + case SCHED_GRAN_cpu:
>> + str = "cpu";
>> + break;
>> + case SCHED_GRAN_core:
>> + str = "core";
>> + break;
>> + case SCHED_GRAN_socket:
>> + str = "socket";
>> + break;
>> + default:
>> + ASSERT_UNREACHABLE();
>> + break;
>> + }
>
> With this addition it might make sense to have an array indexed by
> mode to get the string. This array could then be used in
> sched_select_granularity(), too.
I had thoughts about that, and with your suggestion looks like I need
to go and do it.
>
>> +
>> + printk("Scheduling granularity: %s, %u CPU%s per sched-resource\n",
>> + str, gran, gran == 1 ? "" : "s");
>> +}
>> +
>> #ifdef CONFIG_HAS_SCHED_GRANULARITY
>> static int __init sched_select_granularity(const char *str)
>> {
>> @@ -115,6 +139,7 @@ static void __init cpupool_gran_init(void)
>> warning_add(fallback);
>> sched_granularity = gran;
>> + sched_gran_print(opt_sched_granularity, sched_granularity);
>> }
>> unsigned int cpupool_get_granularity(const struct cpupool *c)
>> @@ -911,6 +936,7 @@ void dump_runq(unsigned char key)
>> {
>> printk("Cpupool %d:\n", (*c)->cpupool_id);
>> printk("Cpus: %*pbl\n", CPUMASK_PR((*c)->cpu_valid));
>> + sched_gran_print((*c)->gran, cpupool_get_granularity(*c));
>> schedule_dump(*c);
>> }
>
--
Thanks,
Sergey
Re: [XTF 2/4] lib: always append CR after LF in vsnprintf()
> On 20. Apr 2020, at 21:26, Andrew Cooper wrote: > > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you can confirm the sender and know the > content is safe. > > > > On 16/04/2020 12:36, Wieczorkiewicz, Pawel wrote: >>> Unfortunately, this comes with collateral damage. >>> >>> # ./xtf-runner hvm64 example >>> Executing 'xl create -p tests/example/test-hvm64-example.cfg' >>> Executing 'xl console test-hvm64-example' >>> Executing 'xl unpause test-hvm64-example' >>> --- Xen Test Framework --- >>> >>> Found Xen: 4.14 >>> >>> Environment: HVM 64bit (Long mode 4 levels) >>> >>> Hello World >>> >>> Test result: SUCCESS >>> >>> >>> Combined test results: >>> test-hvm64-example CRASH >>> >> I never use xtf-runner script to execute tests. I do it the old fashion way: >> >> # xl create -c test-hvm64-example.cfg >> Parsing config from test-hvm64-example.cfg > > I presume you mean hvm64-cpuid here, but... > >> Guest cpuid information >> Native cpuid: >> : -> >> 000d:756e6547:6c65746e:49656e69 >> >> 0001: -> 000306e4:00400800:f7ba2203:1fcbfbff >> >> >> 0002: -> 76036301:00f0b2ff::00ca >> 0003: -> ::: >> 0004: >> -> 7c000121:01c0003f:003f: >> >> 0004:0001 -> >> 7c000122:01c0003f:003f: >> >> >>0004:0002 -> 7c000143:01c0003f:01ff: >> >> >> >> 0004:0003 -> 7c000163:04c0003f:4fff:0006 >> 0004:0004 -> ::: >> 0005: >> -> 0040:0040:0003:1120 >> >>0006: -> >> 0077:0002:0009: >> >> >> 0007: -> :0281::9c000400 >> >> >> >> 0008: -> ::: >> 0009: -> ::: >>000a: >> -> 07300403:::0603 >> >> 000b: -> >> ::: >> >> >> 000c: -> ::: >> >> >> >> 000d: -> 0007:0240:0340: >> 000d:0001 -> 0001::: >> >> 000d:0002 -> 0100:0240:: >> >> 4000: -> >> 4005:566e6558:65584d4d:4d4d566e >> >> >> 4001: -> >> 0004000b::: >>
Re: [PATCH 3/3] x86/pv: Compile out compat_gdt in !CONFIG_PV builds
On 20.04.2020 19:08, Andrew Cooper wrote: > On 20/04/2020 16:47, Jan Beulich wrote: >> On 20.04.2020 16:39, Andrew Cooper wrote: >>> On 20/04/2020 15:12, Jan Beulich wrote: On 17.04.2020 17:50, Andrew Cooper wrote: > There is no need for the Compat GDT if there are no 32bit PV guests. This > saves 4k per online CPU > > Bloat-o-meter reports the following savings in Xen itself: > > add/remove: 0/3 grow/shrink: 1/4 up/down: 7/-4612 (-4605) > Function old new delta > cpu_smpboot_free12491256 +7 > per_cpu__compat_gdt_l1e8 - -8 > per_cpu__compat_gdt8 - -8 > init_idt_traps 442 420 -22 > load_system_tables 414 364 -50 > trap_init444 280-164 > cpu_smpboot_callback1255 991-264 > boot_compat_gdt 4096 - -4096 > Total: Before=3062726, After=3058121, chg -0.15% > > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Wei Liu > CC: Roger Pau Monné > > The increase in cpu_smpboot_free() appears to be a consequence of a > totally > different layout of basic blocks. > --- > xen/arch/x86/cpu/common.c | 5 +++-- > xen/arch/x86/desc.c | 2 ++ > xen/arch/x86/smpboot.c| 5 - > xen/arch/x86/traps.c | 10 +++--- > 4 files changed, 16 insertions(+), 6 deletions(-) > > diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c > index 1b33f1ed71..7b093cb421 100644 > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -752,8 +752,9 @@ void load_system_tables(void) > > _set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss, >sizeof(*tss) - 1, SYS_DESC_tss_avail); > - _set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss, > - sizeof(*tss) - 1, SYS_DESC_tss_busy); > + if ( IS_ENABLED(CONFIG_PV32) ) > + _set_tssldt_desc(compat_gdt + TSS_ENTRY, (unsigned long)tss, > + sizeof(*tss) - 1, SYS_DESC_tss_busy); Wouldn't this better be "if ( opt_pv32 )"? Also elsewhere then. >>> Doing it like this specifically ensures that there is never a case where >>> things are half configured. >> But this way you set up something in the GDT that's never going >> to be used when "pv=no-32". Why leave a TSS accessible that we >> don't need? > > Defence in depth. > > Having it only partially set up is more likely to fail in a security > relevant way, than having it fully set up. Well, I'm not convinced, but anyway Acked-by: Jan Beulich > This particular example is poor. There is no need to have the TSS in > either GDT after the `ltr` instruction at boot for 64bit, because we > don't task switch, but ISTR you requesting that this stayed as-was for > consistency. (For 32bit Xen, it was strictly necessary for the #DF task > switch to work.) Well, I'm simply of the opinion that what the TR holds should point to something valid in the currently active GDT. (As an alternative we could decide to zap TSS descriptors from _both_ GDTs once we're past the LTR. This wouldn't even conflict with resume from S3, as we re-write the TSS descriptors immediately ahead of the LTR.) > However, the other logic, particularly the cached l1e pointing at the > percpu compat_gdt is more liable to go wrong in interesting ways. Possibly, yes. Jan
Re: [PATCH 1/3] x86/pv: Options to disable and/or compile out 32bit PV support
On 20.04.2020 20:05, Andrew Cooper wrote:
> On 20/04/2020 15:05, Jan Beulich wrote:
>> On 17.04.2020 17:50, Andrew Cooper wrote:
>>> This is the start of some performance and security-hardening improvements,
>>> based on the fact that 32bit PV guests are few and far between these days.
>>>
>>> Ring1 is full or architectural corner cases, such as counting as supervisor
>> ... full of ...
>>
>>> --- a/docs/misc/xen-command-line.pandoc
>>> +++ b/docs/misc/xen-command-line.pandoc
>>> @@ -1694,7 +1694,17 @@ The following resources are available:
>>> CDP, one COS will corespond two CBMs other than one with CAT, due to
>>> the
>>> sum of CBMs is fixed, that means actual `cos_max` in use will
>>> automatically
>>> reduce to half when CDP is enabled.
>>> -
>>> +
>>> +### pv
>>> += List of [ 32= ]
>>> +
>>> +Applicability: x86
>>> +
>>> +Controls for aspects of PV guest support.
>>> +
>>> +* The `32` boolean controls whether 32bit PV guests can be created. It
>>> +defaults to `true`, and is ignored when `CONFIG_PV32` is compiled out.
>> Rather than ignoring in the way you do, how about ...
>>
>>> --- a/xen/arch/x86/pv/domain.c
>>> +++ b/xen/arch/x86/pv/domain.c
>>> @@ -16,6 +16,39 @@
>>> #include
>>> #include
>>>
>>> +#ifdef CONFIG_PV32
>>> +int8_t __read_mostly opt_pv32 = -1;
>>> +#endif
>>> +
>>> +static int parse_pv(const char *s)
>>> +{
>>> +const char *ss;
>>> +int val, rc = 0;
>>> +
>>> +do {
>>> +ss = strchr(s, ',');
>>> +if ( !ss )
>>> +ss = strchr(s, '\0');
>>> +
>>> +if ( (val = parse_boolean("32", s, ss)) >= 0 )
>>> +{
>>> +#ifdef CONFIG_PV32
>>> +opt_pv32 = val;
>>> +#else
>>> +printk(XENLOG_INFO
>>> + "CONFIG_PV32 disabled - ignoring 'pv=32' setting\n");
>>> +#endif
>>> +}
>> ... moving the #ifdef ahead of the if(), and the #endif here (may
>> want converting to "else if" with a placeholder if(0) for this
>> purpose), with the separate printk() dropped?
>
> In this specific case, it would be even more awkward as there is no use
> of val outside of the ifdef.
True, but can be dealt with in the body of the suggested placeholder
if(0).
>> I'm in particular
>> concerned that we may gain a large number of such printk()s over
>> time, if we added them in such cases.
>
> The printk() was a bit of an afterthought, but deliberately avoiding the
> -EINVAL path was specifically not.
>
> In the case that the user tries to use `pv=no-32` without CONFIG_PV32,
> they should see something other than
>
> (XEN) parameter "pv=no-32" unknown!
Why - to this specific build of Xen the parameter is unknown.
> I don't think overloading the return value is a clever move, because
> then every parse function has to take care of ensuring that -EOPNOTSUPP
> (or ENODEV?) never clobbers -EINVAL.
I didn't suggest overloading the return value. Instead I
specifically want this to go the -EINVAL path.
> We could have a generic helper which looks like:
>
> static inline void ignored_param(const char *cfg, const char *name,
> const char *s, const char *ss)
> {
> printk(XENLOG_INFO "%s disabled - ignoring '%s=%*.s' setting\n",
> cfg, name, s, (int)(ss - s));
> }
>
> which at least would keep all the users consistent.
Further bloating the binary with (almost) useless string literals.
I'd specifically like to avoid this.
>> See parse_iommu_param() for
>> how I'd prefer things to look like in the long run.
>
> I'm aware of that, just as you are aware of my specific dislike for the
> ifndefs, which make the logic opaque and hard to follow.
A matter of taste and/or perception, I guess, but yes, I'm aware.
I don't recall a suggestion (from you or anyone else) as to a good
alternative, though. What I'd like to achieve is that command line
options valid only in certain configurations get similar treatment
no matter by whom they were added. It doesn't need to be my way of
handling, but I'm pretty convinced that consistency especially in
such "user interface" aspects is very desirable goal.
Jan
