Re: [PATCH 2/2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()
On 2020-06-24 20:02, Souptick Joarder wrote:
In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].
[1] Documentation/core-api/pin_user_pages.rst
[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/
Signed-off-by: Souptick Joarder
Cc: John Hubbard
Cc: Boris Ostrovsky
Cc: Paul Durrant
---
Hi,
I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.
drivers/xen/privcmd.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 0da417c..eb05254 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -595,7 +595,7 @@ static int lock_pages(
if (requested > nr_pages)
return -ENOSPC;
- page_count = get_user_pages_fast(
+ page_count = pin_user_pages_fast(
(unsigned long) kbufs[i].uptr,
requested, FOLL_WRITE, pages);
if (page_count < 0) {
@@ -612,13 +612,7 @@ static int lock_pages(
static void unlock_pages(struct page *pages[], unsigned int nr_pages)
{
- unsigned int i;
-
- for (i = 0; i < nr_pages; i++) {
- if (!PageDirty(page))
- set_page_dirty_lock(page);
- put_page(pages[i]);
- }
+ unpin_user_pages_dirty_lock(pages, nr_pages, 1);
"true", not "1", is the correct way to call that function.
Also, this approach changes the behavior slightly, but I think it's
reasonable to just set_page_dirty_lock() on the whole range--hard to
see much benefit in checking PageDirty first.
}
static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
thanks,
--
John Hubbard
NVIDIA
[qemu-mainline test] 151328: regressions - FAIL
flight 151328 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/151328/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-amd64-i386-qemuu-rhel6hvm-intel 10 redhat-install fail REGR. vs. 151065 test-amd64-i386-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-amd64-i386-freebsd10-amd64 11 guest-start fail REGR. vs. 151065 test-amd64-i386-libvirt-pair 21 guest-start/debian fail REGR. vs. 151065 test-amd64-i386-qemuu-rhel6hvm-amd 10 redhat-install fail REGR. vs. 151065 test-amd64-amd64-libvirt 12 guest-start fail REGR. vs. 151065 test-amd64-amd64-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-amd64-i386-libvirt 12 guest-start fail REGR. vs. 151065 test-amd64-i386-freebsd10-i386 11 guest-startfail REGR. vs. 151065 test-amd64-amd64-libvirt-pair 21 guest-start/debian fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-qemuu-nested-amd 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-qemuu-nested-intel 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qcow210 debian-di-installfail REGR. vs. 151065 test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail REGR. vs. 151065 test-arm64-arm64-libvirt-xsm 12 guest-start fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-amd64-shadow 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-ovmf-amd64 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-libvirt-vhd 10 debian-di-installfail REGR. vs. 151065 test-amd64-i386-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 151065 test-armhf-armhf-libvirt-raw 10 debian-di-installfail REGR. vs. 151065 test-armhf-armhf-xl-vhd 10 debian-di-installfail REGR. vs. 151065 test-armhf-armhf-libvirt 12 guest-start fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-win7-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-win7-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-amd64-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 151065 test-amd64-i386-xl-qemuu-ws16-amd64 10 windows-install fail REGR. vs. 151065 Tests which did not succeed, but are not blocking: test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 151065 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-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 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-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit2 14 saverestore-support-checkfail never pass test-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-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14
[libvirt test] 151330: tolerable all pass - PUSHED
flight 151330 libvirt real [real] http://logs.test-lab.xenproject.org/osstest/logs/151330/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 151308 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 151308 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 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-arm64-arm64-libvirt-qcow2 12 migrate-support-checkfail never pass test-arm64-arm64-libvirt-qcow2 13 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass version targeted for testing: libvirt c5815b31976f3982d18c7f6c1367ab6e403eb7eb baseline version: libvirt 597fdabbc0e7c256b61a6b5ffda64bebfddc3807 Last test of basis 151308 2020-06-23 04:19:04 Z1 days Testing same since 151330 2020-06-24 05:57:45 Z0 days1 attempts People who touched revisions under test: Andrea Bolognani Daniel Henrique Barboza Daniel P. Berrangé Dmitry Nesterenko Jonathon Jongsma Ján Tomko Liao Pingfang Menno Lageman Michal Privoznik Peter Krempa Satheesh Rajendran jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64 pass build-arm64 pass build-armhf pass build-i386 pass build-amd64-libvirt pass build-arm64-libvirt pass build-armhf-libvirt pass build-i386-libvirt pass build-amd64-pvopspass build-arm64-pvopspass build-armhf-pvopspass build-i386-pvops pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsmpass test-amd64-amd64-libvirt-xsm pass test-arm64-arm64-libvirt-xsm pass test-amd64-i386-libvirt-xsm pass test-amd64-amd64-libvirt pass test-arm64-arm64-libvirt pass test-armhf-armhf-libvirt pass test-amd64-i386-libvirt pass test-amd64-amd64-libvirt-pairpass test-amd64-i386-libvirt-pair pass test-arm64-arm64-libvirt-qcow2 pass test-armhf-armhf-libvirt-raw pass test-amd64-amd64-libvirt-vhd pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/libvirt.git 597fdabbc0..c5815b3197 c5815b31976f3982d18c7f6c1367ab6e403eb7eb -> xen-tested-master
[PATCH 2/2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()
In 2019, we introduced pin_user_pages*() and now we are converting
get_user_pages*() to the new API as appropriate. [1] & [2] could
be referred for more information. This is case 5 as per document [1].
[1] Documentation/core-api/pin_user_pages.rst
[2] "Explicit pinning of user-space pages":
https://lwn.net/Articles/807108/
Signed-off-by: Souptick Joarder
Cc: John Hubbard
Cc: Boris Ostrovsky
Cc: Paul Durrant
---
Hi,
I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.
drivers/xen/privcmd.c | 10 ++
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index 0da417c..eb05254 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -595,7 +595,7 @@ static int lock_pages(
if (requested > nr_pages)
return -ENOSPC;
- page_count = get_user_pages_fast(
+ page_count = pin_user_pages_fast(
(unsigned long) kbufs[i].uptr,
requested, FOLL_WRITE, pages);
if (page_count < 0) {
@@ -612,13 +612,7 @@ static int lock_pages(
static void unlock_pages(struct page *pages[], unsigned int nr_pages)
{
- unsigned int i;
-
- for (i = 0; i < nr_pages; i++) {
- if (!PageDirty(page))
- set_page_dirty_lock(page);
- put_page(pages[i]);
- }
+ unpin_user_pages_dirty_lock(pages, nr_pages, 1);
}
static long privcmd_ioctl_dm_op(struct file *file, void __user *udata)
--
1.9.1
[PATCH 1/2] xen/privcmd: Corrected error handling path and mark pages dirty
Previously, if lock_pages() end up partially mapping pages, it used
to return -ERRNO due to which unlock_pages() have to go through
each pages[i] till *nr_pages* to validate them. This can be avoided
by passing correct number of partially mapped pages & -ERRNO separately,
while returning from lock_pages() due to error.
With this fix unlock_pages() doesn't need to validate pages[i] till
*nr_pages* for error scenario and few condition checks can be ignored.
As discussed, pages need to be marked as dirty before unpinned it in
unlock_pages() which was oversight.
Signed-off-by: Souptick Joarder
Cc: John Hubbard
Cc: Boris Ostrovsky
Cc: Paul Durrant
---
Hi,
I'm compile tested this, but unable to run-time test, so any testing
help is much appriciated.
drivers/xen/privcmd.c | 34 +++---
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c
index a250d11..0da417c 100644
--- a/drivers/xen/privcmd.c
+++ b/drivers/xen/privcmd.c
@@ -580,43 +580,44 @@ static long privcmd_ioctl_mmap_batch(
static int lock_pages(
struct privcmd_dm_op_buf kbufs[], unsigned int num,
- struct page *pages[], unsigned int nr_pages)
+ struct page *pages[], unsigned int nr_pages, int *pinned)
{
unsigned int i;
+ int errno = 0, page_count = 0;
for (i = 0; i < num; i++) {
unsigned int requested;
- int pinned;
+ *pinned += page_count;
requested = DIV_ROUND_UP(
offset_in_page(kbufs[i].uptr) + kbufs[i].size,
PAGE_SIZE);
if (requested > nr_pages)
return -ENOSPC;
- pinned = get_user_pages_fast(
+ page_count = get_user_pages_fast(
(unsigned long) kbufs[i].uptr,
requested, FOLL_WRITE, pages);
- if (pinned < 0)
- return pinned;
+ if (page_count < 0) {
+ errno = page_count;
+ return errno;
+ }
- nr_pages -= pinned;
- pages += pinned;
+ nr_pages -= page_count;
+ pages += page_count;
}
- return 0;
+ return errno;
}
static void unlock_pages(struct page *pages[], unsigned int nr_pages)
{
unsigned int i;
- if (!pages)
- return;
-
for (i = 0; i < nr_pages; i++) {
- if (pages[i])
- put_page(pages[i]);
+ if (!PageDirty(page))
+ set_page_dirty_lock(page);
+ put_page(pages[i]);
}
}
@@ -630,6 +631,7 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
struct xen_dm_op_buf *xbufs = NULL;
unsigned int i;
long rc;
+ int pinned = 0;
if (copy_from_user(, udata, sizeof(kdata)))
return -EFAULT;
@@ -683,9 +685,11 @@ static long privcmd_ioctl_dm_op(struct file *file, void
__user *udata)
goto out;
}
- rc = lock_pages(kbufs, kdata.num, pages, nr_pages);
- if (rc)
+ rc = lock_pages(kbufs, kdata.num, pages, nr_pages, );
+ if (rc < 0) {
+ nr_pages = pinned;
goto out;
+ }
for (i = 0; i < kdata.num; i++) {
set_xen_guest_handle(xbufs[i].h, kbufs[i].uptr);
--
1.9.1
Re: Migration vmdesc and xen-save-devices-state
On Wed, Jun 24, 2020 at 1:57 PM Dr. David Alan Gilbert wrote: > > * Jason Andryuk (jandr...@gmail.com) wrote: > > Hi, > > > > At some point, QEMU changed to add a json VM description (vmdesc) > > after the migration data. The vmdesc is not needed to restore the > > migration data, but qemu_loadvm_state() will read and discard the > > vmdesc to clear the stream when should_send_vmdesc() is true. > > About 5 years ago :-) :) > > xen-save-devices-state generates its migration data without a vmdesc. > > xen-load-devices-state in turn calls qemu_loadvm_state() which tries > > to load vmdesc since should_send_vmdesc is true for xen. When > > restoring from a file, this is fine since it'll return EOF, print > > "Expected vmdescription section, but got 0" and end the restore > > successfully. > > > > Linux stubdoms load their migration data over a console, so they don't > > hit the EOF and end up waiting. There does seem to be a timeout > > though and restore continues after a delay, but we'd like to eliminate > > the delay. > > > > Two options to address this are to either: > > 1) set suppress_vmdesc for the Xen machines to bypass the > > should_send_vmdesc() check. > > or > > 2) just send the vmdesc data. > > > > Since vmdesc is just discarded, maybe #1 should be followed. > > #1 does sound simple! > > > If going with #2, qemu_save_device_state() needs to generate the > > vmdesc data. Looking at qemu_save_device_state() and > > qemu_savevm_state_complete_precopy_non_iterable(), they are both very > > similar and could seemingly be merged. qmp_xen_save_devices_state() > > could even leverage the bdrv_inactivate_all() call in > > qemu_savevm_state_complete_precopy_non_iterable(). > > > > The would make qemu_save_device_state a little more heavywight, which > > could impact COLO. I'm not sure how performance sensitive the COLO > > code is, and I haven't measured anything. > > COLO snapshots are potentially quite sensitive; although we've got a > load of other things we could do with speeding up, we could do without > making them noticably heavier. qemu_savevm_state_complete_precopy_non_iterable() generates the vmdesc json and just discards it if not needed. How much overhead that adds is the question. Thanks, Jason
Re: [XEN RFC for-4.14] Re: use of "stat -"
On Wed, Jun 24, 2020 at 12:19 PM Ian Jackson wrote:
>
> Jan Beulich writes ("Re: use of "stat -""):
> > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments
> > unless you have verified the sender and know the content is safe.
> > On 14.05.2020 13:02, Ian Jackson wrote:
> > > I've read this thread. Jan, I'm sorry that this causes you
> > > inconvenience. I'm hoping it won't come down to a choice between
> > > supporting people who want to ship a dom0 without perl, and people who
> > > want a dom0 using more-than-a-decade-old coreutils.
> > >
> > > Jan, can you tell me what the output is of this on your ancient
> > > system:
> > >
> > > $ rm -f t
> > > $ >t
> > > $ exec 3 > > $ stat -L -c '%F %i' /dev/stdin <&3
> > > regular empty file 393549
> > > $ rm t
> > > $ stat -L -c '%F %i' /dev/stdin <&3
> > > regular empty file 393549
> > > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3
> > > $
> >
> > $ rm -f t
> > $ >t
> > $ exec 3 > $ stat -L -c '%F %i' /dev/stdin <&3
> > regular empty file 3380369
> > $ rm t
> > $ stat -L -c '%F %i' /dev/stdin <&3
> > regular empty file 3380369
> > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3
> > regular empty file 3380369
> >
> > > Also, the contents of the file "u" afterwards, please.
> >
> > Attached.
>
> Thanks.
>
> I think this means that `stat -' can be replaced by `stat /dev/stdin'.
>
> This script is only run on Linux where /dev/stdin has existed
> basically forever. The strace output shows
> stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
> and the transcript shows that your stat(1) behaves as I hope.
>
> Jan, will you send a patch ? It is best if someone else but me writes
> it and tests it because then I am a "clean" reviewer.
>
> Paul, supposing I review such a patch and say it is low risk, and we
> commit it by Friday, can it have a release-ack ?
I was going to just write a patch to replace - with /dev/stdin and ask
Jan to test it. When I opened the script, this comment was staring at
me:
# We can't just stat /dev/stdin or /proc/self/fd/$_lockfd or
# use bash's test -ef because those all go through what is
# actually a synthetic symlink in /proc and we aren't
# guaranteed that our stat(2) won't lose the race with an
# rm(1) between reading the synthetic link and traversing the
# file system to find the inum.
On my system:
$ ls -l /dev/stdin
lrwxrwxrwx 1 root root 15 Jun 24 21:13 /dev/stdin -> /proc/self/fd/0
$ ls -l /proc/self/fd/0 0 /home/jason/lockfile
stat /dev/stdin will work around the lack of `stat -` support, but it
doesn't address the race in the comment. Is the comment valid? How
would we prove there is no race for /dev/stdin? And as a refresher,
`stat -` does an fstat(0), so there is no symlink lookup. Or is there
no race since `stat /proc/self/fd/0` isn't a symlink lookup but just
accessing the already open fd via the proc special file? i.e.
equivalent to fstat.
I've mentioned it before, but maybe we should just use the Qubes
patch. It leaves the lockfile even when no-one is holding the lock,
but it simplifies the code and sidesteps the issues we are discussing
here.
https://github.com/QubesOS/qubes-vmm-xen/blob/xen-4.13/patch-tools-hotplug-drop-perl-usage-in-locking-mechanism.patch
Regards,
Jason
[linux-linus test] 151323: regressions - FAIL
flight 151323 linux-linus real [real] http://logs.test-lab.xenproject.org/osstest/logs/151323/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-arm64-arm64-libvirt-xsm 16 guest-start/debian.repeat fail REGR. vs. 151214 test-amd64-i386-xl-qemut-debianhvm-i386-xsm 10 debian-hvm-install fail REGR. vs. 151214 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 151214 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 151214 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 151214 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 151214 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 151214 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 151214 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 151214 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 151214 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-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-libvirt-xsm 13 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-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-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 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-i386-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-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-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-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop 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-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: linux3e08a95294a4fb3702bb3d35ed08028433c37fe6 baseline version: linux1b5044021070efa3259f3e9548dc35d1eb6aa844 Last test of basis 151214 2020-06-18 02:27:46 Z6 days Failing since151236 2020-06-19 19:10:35 Z5 days5 attempts Testing same since 151323 2020-06-24 00:10:00 Z1 days1 attempts People who touched
Re: Proposal: rename xen.git#master (to #trunk, perhaps)
On 24/06/2020 17:13, Ian Jackson wrote: > I think it would be a good idea to rename this branch name. The tech industry does use some problematic terms, and I fully agree with taking steps to reduce their use. However, I disagree that this is a problematic context. In the English language, context is paramount. Terms such as master/slave have an obvious inequality bias in the context in which they are used. There are alternatives to these terms, probably one of which is more suited to the specific scenario in question. However, the word master is a very overloaded word in terms of its different meanings. Describing someone as a "master of their trade/skill/other", is a totally different context, and it would be excessive to suggest changing the language used in this way. So too, in my opinion, is master as in "master copy", a different context and connotation to master as in master/slave. A much more meaningful use of time would be to address: xen.git$ git grep -i -e slave -e whitelist -e blacklist | wc -l 194 two thirds of which look fairly easy to address, and one third fairly complicated due to external dependencies. ~Andrew
Re: [RFC PATCH v2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()
On 6/24/20 12:41 PM, Souptick Joarder wrote:
> On Wed, Jun 24, 2020 at 9:07 PM Boris Ostrovsky
> wrote:
>> On 6/23/20 9:36 PM, Souptick Joarder wrote:
>>> On Tue, Jun 23, 2020 at 11:11 PM Boris Ostrovsky
>>> wrote:
On 6/23/20 7:58 AM, Souptick Joarder wrote:
> In 2019, we introduced pin_user_pages*() and now we are converting
> get_user_pages*() to the new API as appropriate. [1] & [2] could
> be referred for more information. This is case 5 as per document [1].
>
> As discussed, pages need to be marked as dirty before unpinned it.
>
> Previously, if lock_pages() end up partially mapping pages, it used
> to return -ERRNO due to which unlock_pages() have to go through
> each pages[i] till *nr_pages* to validate them. This can be avoided
> by passing correct number partially mapped pages & -ERRNO separately
> while returning from lock_pages() due to error.
> With this fix unlock_pages() doesn't need to validate pages[i] till
> *nr_pages* for error scenario.
This should be split into two patches please. The first one will fix the
return value bug (and will need to go to stable branches) and the second
will use new routine to pin pages.
>>> Initially I split the patches into 2 commits. But at last moment I
>>> figure out that,
>>> this bug fix ( better to call coding error, doesn't looks like lead to
>>> any runtime bug) is tightly coupled to 2nd commit for
>>> pin_user_pages*() conversion,
>>> which means we don't need the bug fix patch if we are not converting the
>>> API to
>>> pin_user_pages*()/ unpin_user_pages_dirty_lock(). That's the reason to
>>> clubbed these two
>>> commits into a single one.
>>
>> I am not sure I understand why the two issues are connected. Failure of
>> either get_user_pages_fast() or pin_user_pages() will result in the same
>> kind of overall ioctl failure, won't it?
>>
> Sorry, I think, I need to go through the bug again for my clarity.
>
> My understanding is ->
>
> First, In case of success lock_pages() returns 0, so what
> privcmd_ioctl_dm_op()
> will return to user is depend on the return value of HYPERVISOR_dm_op()
> and at last all the pages are unpinned. At present I am not clear about the
> return value of HYPERVISOR_dm_op(). But this path of code looks to be correct.
>
> second, incase of failure from lock_pages() will return either -ENOSPC /
> -ERRNO
> receive from {get|pin|_user_pages_fast() inside for loop. (at that
> point there might
> be some partially mapped pages as it is mapping inside the loop). Upon
> receiving
> -ERRNO privcmd_ioctl_dm_op() will pass the same -ERRNO to user (not the
> partial
> mapped page count). This looks to be correct behaviour from user space.
Sigh... I am sorry, you are absolutely correct. It does return -errno
on get_user_pages_fast() failure. I don't know what (or whether) I was
thinking. (And so Paul, ignore my question)
-boris
>
> The problem I was trying to address is, in the second scenario when
> lock_pages() returns
> -ERRNO and there are already existing mapped pages. In this scenario,
> when unlcok_pages()
> is called it will iterate till *nr_pages* to validate and unmap the
> pages. But it is supposed to
> unmap only the mapped pages which I am trying to address as part of bug fix.
>
> Let me know if I am able to be in sync with what you are expecting ?
>
>
>> One concern though is that we are changing how user will see this error.
> My understanding from above is user will always see right -ERRNO and
> will not be impacted.
>
> Another scenario I was thinking, if {get|pin|_user_pages_fast() return
> number of pages pinned
> which may be fewer than the number requested. Then lock_pages()
> returns 0 to caller
> and caller/user space will continue with the assumption that all pages
> are pinned correctly.
> Is this an expected behaviour as per current code ?
>
>
>> Currently Xen devicemodel (which AFAIK is the only caller) ignores it,
>> which is I think is wrong. So another option would be to fix this in Xen
>> and continue returning positive number here. I guess it's back to Paul
>> again.
>>
>>
>> -boris
>>
>>
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, Jun 24, 2020 at 02:53:54PM -0700, Stefano Stabellini wrote: > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to map the > > > > > ring: when xen_swiotlb is enabled the dma API is required. When it is > > > > > disabled, it is not required. > > > > > > > > > > Signed-off-by: Peng Fan > > > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > > > Xen was there first, but everyone else is using that now. > > > > > > Unfortunately it is complicated and it is not related to > > > VIRTIO_F_IOMMU_PLATFORM :-( > > > > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > > > foreign mappings (memory coming from other VMs) to physical addresses. > > > On x86, it also uses dma_ops to translate Linux's idea of a physical > > > address into a real physical address (this is unneeded on ARM.) > > > > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on > > > Xen/x86 > > > always and on Xen/ARM if Linux is Dom0 (because it has foreign > > > mappings.) That is why we have the if (xen_domain) return true; in > > > vring_use_dma_api. > > > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. > > > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* > > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. > > > > Unfortunately as a result Xen never got around to > > properly setting VIRTIO_F_IOMMU_PLATFORM. > > I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this because > the usage of swiotlb_xen is not a property of virtio, Basically any device without VIRTIO_F_ACCESS_PLATFORM (that is it's name in latest virtio spec, VIRTIO_F_IOMMU_PLATFORM is what linux calls it) is declared as "special, don't follow normal rules for access". So yes swiotlb_xen is not a property of virtio, but what *is* a property of virtio is that it's not special, just a regular device from DMA POV. > it is a detail of > the way Linux does Xen address translations. swiotlb-xen is used to do > these translations and it is hooked into the dma_ops framework. > > It would be possible to have a device in hardware that is > virtio-compatible and doesn't set VIRTIO_F_IOMMU_PLATFORM. That device would be basically broken, since hardware can't know whether it can access all memory or not. > The device > could be directly assigned (passthrough) to a DomU. We would still > have to use swiotlb_xen if Xen is running. > > You should think of swiotlb-xen as only internal to Linux and not > related to whether the (virtual or non-virtual) hardware comes with an > IOMMU or not. IOMMU is a misnomer here. Virtio spec now calls this bit VIRTIO_F_ACCESS_PLATFORM. We should have done the same a while ago - I'll send a patch. > > > > You might have noticed that I missed one possible case above: Xen/ARM > > > DomU :-) > > > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if > > > (xen_domain) return true; would give the wrong answer in that case. > > > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and > > > the "normal" dma_ops fail. > > > > > > > > > The solution I suggested was to make the check in vring_use_dma_api more > > > flexible by returning true if the swiotlb_xen is supposed to be used, > > > not in general for all Xen domains, because that is what the check was > > > really meant to do. > > > > Why not fix DMA ops so they DTRT (nop) on Xen/ARM DomU? What is wrong with > > that? > > swiotlb-xen is not used on Xen/ARM DomU, the default dma_ops are the > ones that are used. So you are saying, why don't we fix the default > dma_ops to work with virtio? > > It is bad that the default dma_ops crash with virtio, so yes I think it > would be good to fix that. However, even if we fixed that, the if > (xen_domain()) check in vring_use_dma_api is still a problem. Why is it a problem? It just makes virtio use DMA API. If that in turn works, problem solved. > > Alternatively we could try to work-around it from swiotlb-xen. We could > enable swiotlb-xen for Xen/ARM DomUs with a different implementation so > that we could leave the vring_use_dma_api check unmodified. > > It would be ugly because we would have to figure out from the new > swiotlb-xen functions if the device is a normal device, so we have to > call the regular dma_ops functions, or if the device is a virtio device, > in which case there is nothing to do. I think it is undesirable but > could probably be made to work.
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to map the > > > > ring: when xen_swiotlb is enabled the dma API is required. When it is > > > > disabled, it is not required. > > > > > > > > Signed-off-by: Peng Fan > > > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > > Xen was there first, but everyone else is using that now. > > > > Unfortunately it is complicated and it is not related to > > VIRTIO_F_IOMMU_PLATFORM :-( > > > > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > > foreign mappings (memory coming from other VMs) to physical addresses. > > On x86, it also uses dma_ops to translate Linux's idea of a physical > > address into a real physical address (this is unneeded on ARM.) > > > > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on Xen/x86 > > always and on Xen/ARM if Linux is Dom0 (because it has foreign > > mappings.) That is why we have the if (xen_domain) return true; in > > vring_use_dma_api. > > VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. > > Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* > forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. > > Unfortunately as a result Xen never got around to > properly setting VIRTIO_F_IOMMU_PLATFORM. I don't think VIRTIO_F_IOMMU_PLATFORM would be correct for this because the usage of swiotlb_xen is not a property of virtio, it is a detail of the way Linux does Xen address translations. swiotlb-xen is used to do these translations and it is hooked into the dma_ops framework. It would be possible to have a device in hardware that is virtio-compatible and doesn't set VIRTIO_F_IOMMU_PLATFORM. The device could be directly assigned (passthrough) to a DomU. We would still have to use swiotlb_xen if Xen is running. You should think of swiotlb-xen as only internal to Linux and not related to whether the (virtual or non-virtual) hardware comes with an IOMMU or not. > > You might have noticed that I missed one possible case above: Xen/ARM > > DomU :-) > > > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if > > (xen_domain) return true; would give the wrong answer in that case. > > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and > > the "normal" dma_ops fail. > > > > > > The solution I suggested was to make the check in vring_use_dma_api more > > flexible by returning true if the swiotlb_xen is supposed to be used, > > not in general for all Xen domains, because that is what the check was > > really meant to do. > > Why not fix DMA ops so they DTRT (nop) on Xen/ARM DomU? What is wrong with > that? swiotlb-xen is not used on Xen/ARM DomU, the default dma_ops are the ones that are used. So you are saying, why don't we fix the default dma_ops to work with virtio? It is bad that the default dma_ops crash with virtio, so yes I think it would be good to fix that. However, even if we fixed that, the if (xen_domain()) check in vring_use_dma_api is still a problem. Alternatively we could try to work-around it from swiotlb-xen. We could enable swiotlb-xen for Xen/ARM DomUs with a different implementation so that we could leave the vring_use_dma_api check unmodified. It would be ugly because we would have to figure out from the new swiotlb-xen functions if the device is a normal device, so we have to call the regular dma_ops functions, or if the device is a virtio device, in which case there is nothing to do. I think it is undesirable but could probably be made to work.
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, Jun 24, 2020 at 10:59:47AM -0700, Stefano Stabellini wrote: > On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > > Export xen_swiotlb for all platforms using xen swiotlb > > > > > > Use xen_swiotlb to determine when vring should use dma APIs to map the > > > ring: when xen_swiotlb is enabled the dma API is required. When it is > > > disabled, it is not required. > > > > > > Signed-off-by: Peng Fan > > > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > > Xen was there first, but everyone else is using that now. > > Unfortunately it is complicated and it is not related to > VIRTIO_F_IOMMU_PLATFORM :-( > > > The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate > foreign mappings (memory coming from other VMs) to physical addresses. > On x86, it also uses dma_ops to translate Linux's idea of a physical > address into a real physical address (this is unneeded on ARM.) > > > So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on Xen/x86 > always and on Xen/ARM if Linux is Dom0 (because it has foreign > mappings.) That is why we have the if (xen_domain) return true; in > vring_use_dma_api. VIRTIO_F_IOMMU_PLATFORM makes guest always use DMA ops. Xen hack predates VIRTIO_F_IOMMU_PLATFORM so it *also* forces DMA ops even if VIRTIO_F_IOMMU_PLATFORM is clear. Unfortunately as a result Xen never got around to properly setting VIRTIO_F_IOMMU_PLATFORM. I would like to make Xen do what everyone else is doing which is just set VIRTIO_F_IOMMU_PLATFORM and then put platform specific hacks inside DMA API. Then we can think about deprecating the Xen hack in a distance future, or hiding it behind a static branch, or something like this. > You might have noticed that I missed one possible case above: Xen/ARM > DomU :-) > > Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if > (xen_domain) return true; would give the wrong answer in that case. > Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and > the "normal" dma_ops fail. > > > The solution I suggested was to make the check in vring_use_dma_api more > flexible by returning true if the swiotlb_xen is supposed to be used, > not in general for all Xen domains, because that is what the check was > really meant to do. Why not fix DMA ops so they DTRT (nop) on Xen/ARM DomU? What is wrong with that? > > In this regards I have two more comments: > > - the comment on top of the check in vring_use_dma_api is still valid > - the patch looks broken on x86: it should always return true, but it > returns false instead > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index a2de775801af..768afd79f67a 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device > > > *vdev) > > >* the DMA API if we're a Xen guest, which at least allows > > >* all of the sensible Xen configurations to work correctly. > > >*/ > > > - if (xen_domain()) > > > + if (xen_vring_use_dma()) > > > return true; > > > > > > return false; > > > > > > The comment above this should probably be fixed. > > >
[xen-4.11-testing test] 151318: tolerable FAIL - PUSHED
flight 151318 xen-4.11-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/151318/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail never pass test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail never pass test-amd64-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-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-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-arm64-arm64-libvirt-xsm 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-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-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-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 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-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass version targeted for testing: xen 2b77729888fb851ab96e7f77bc854122626b4861 baseline version: xen 7dd2ac39e40f0afe1cc6d879bfe65cbf19520cab Last test of basis 150040 2020-05-05 16:06:51 Z 50 days Failing since150942 2020-06-09 17:05:43 Z 15 days 15 attempts Testing same since 151061 2020-06-12 12:25:05 Z 12 days 10 attempts People who touched revisions under test: Andrew Cooper
[ovmf test] 151320: all pass - PUSHED
flight 151320 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/151320/ Perfect :-) All tests in this flight passed as required version targeted for testing: ovmf 1a992030522622c42aa063788b3276789c56c1e1 baseline version: ovmf 00b8bf7eda00fb6f0197d3968b6078cfdb4870fa Last test of basis 151303 2020-06-23 01:55:28 Z1 days Testing same since 151320 2020-06-23 22:10:13 Z0 days1 attempts People who touched revisions under test: Chasel Chiu 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 00b8bf7eda..1a99203052 1a992030522622c42aa063788b3276789c56c1e1 -> xen-tested-master
Re: [PATCH v10 1/9] error: auto propagated local_err
On Wed, 24 Jun 2020 18:53:05 +0200 Markus Armbruster wrote: > Greg Kurz writes: > > > On Mon, 15 Jun 2020 07:21:03 +0200 > > Markus Armbruster wrote: > > > >> Greg Kurz writes: > >> > >> > On Tue, 17 Mar 2020 18:16:17 +0300 > >> > Vladimir Sementsov-Ogievskiy wrote: > >> > > >> >> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of > >> >> functions with an errp OUT parameter. > >> >> > >> >> It has three goals: > >> >> > >> >> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user > >> >> can't see this additional information, because exit() happens in > >> >> error_setg earlier than information is added. [Reported by Greg Kurz] > >> >> > >> > > >> > I have more of these coming and I'd really like to use > >> > ERRP_AUTO_PROPAGATE. > >> > > >> > It seems we have a consensus on the macro itself but this series is gated > >> > by the conversion of the existing code base. > >> > > >> > What about merging this patch separately so that people can start using > >> > it at least ? > >> > >> Please give me a few more days to finish the work I feel should go in > >> before the conversion. With any luck, Vladimir can then rebase / > >> recreate the conversion easily, and you can finally use the macro for > >> your own work. > >> > > > > Sure. Thanks. > > Just posted "[PATCH 00/46] Less clumsy error checking". The sheer size > of the thing and the length of its dependency chain explains why it took > me so long. I feel bad about delaying you all the same. Apologies! > No problem. This series of yours is impressive. Putting an end to the highjacking of the Error ** argument is really a beneficial move. > I hope we can converge quickly enough to get Vladimir's work on top > ready in time for the soft freeze. > I'll find some cycles for reviewing. Cheers, -- Greg
Re: UEFI support in ARM DomUs
On Wed, 24 Jun 2020, Oleksandr Andrushchenko wrote: > On 6/24/20 8:05 PM, Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Oleksandr Andrushchenko wrote: > >> On 6/23/20 8:31 AM, Oleksandr Andrushchenko wrote: > >>> On 6/23/20 4:20 AM, Stefano Stabellini wrote: > On Mon, 22 Jun 2020, Julien Grall wrote: > For the first part (__XEN_INTERFACE_VERSION__) I think we can > provide it > via > > CFLAGS or something. This can also be done for the location of Xen > headers. > >>> __XEN_INTERFACE_VERSION__ should work through the CFLAGS. An > >>> alternative > >>> would be to allow the user to specify through the Kconfig. > >> You mean specifying via Kconfig something like "0x00040d00"? > > Possibly yes. > > > >> And what about the headers? How will we provide their location if we > >> decide > >> not to include those > >> > >> in the tree? > > I would do through Kconfig as well. > If we specify the external location of the Xen headers via Kconfig, it > seems to me that we should be able to detect the interface version > automatically from any Makefile as part of the build. No need to ask the > user. > > However, if Oleksandr is thinking of using the Xen headers for the > hypercalls definitions, then I think we might not need external headers > at all because that is a stable interface, as Julien wrote. We could > just define our own few headers for just what you need like Linux does. > >>> This is a good idea: I'll try to get the minimal set of headers from Linux > >>> > >>> instead of Xen as those seem to be well prepared for such a use-case. This > >>> > >>> way we'll have headers in U-boot tree and guarantee that we have a minimal > >>> > >>> subset which is easier to maintain. I'll keep you updated on the progress > >> We've managed to strip the headers and remove __XEN__ and the rest > >> definitions > >> > >> we were talking about. So, these are now the minimal required set of > >> headers > >> > >> that allows U-boot to build serial and block drivers. Please take a look > >> at [1] > >> > >> Pull request for comments is at [2] > > I think this is the right approach. There is no build-dependency on Xen > > anymore, is that correct? > > No dependency Great!
Re: UEFI support in ARM DomUs
On 6/24/20 8:05 PM, Stefano Stabellini wrote: > On Wed, 24 Jun 2020, Oleksandr Andrushchenko wrote: >> On 6/23/20 8:31 AM, Oleksandr Andrushchenko wrote: >>> On 6/23/20 4:20 AM, Stefano Stabellini wrote: On Mon, 22 Jun 2020, Julien Grall wrote: For the first part (__XEN_INTERFACE_VERSION__) I think we can provide it via CFLAGS or something. This can also be done for the location of Xen headers. >>> __XEN_INTERFACE_VERSION__ should work through the CFLAGS. An alternative >>> would be to allow the user to specify through the Kconfig. >> You mean specifying via Kconfig something like "0x00040d00"? > Possibly yes. > >> And what about the headers? How will we provide their location if we >> decide >> not to include those >> >> in the tree? > I would do through Kconfig as well. If we specify the external location of the Xen headers via Kconfig, it seems to me that we should be able to detect the interface version automatically from any Makefile as part of the build. No need to ask the user. However, if Oleksandr is thinking of using the Xen headers for the hypercalls definitions, then I think we might not need external headers at all because that is a stable interface, as Julien wrote. We could just define our own few headers for just what you need like Linux does. >>> This is a good idea: I'll try to get the minimal set of headers from Linux >>> >>> instead of Xen as those seem to be well prepared for such a use-case. This >>> >>> way we'll have headers in U-boot tree and guarantee that we have a minimal >>> >>> subset which is easier to maintain. I'll keep you updated on the progress >> We've managed to strip the headers and remove __XEN__ and the rest >> definitions >> >> we were talking about. So, these are now the minimal required set of headers >> >> that allows U-boot to build serial and block drivers. Please take a look at >> [1] >> >> Pull request for comments is at [2] > I think this is the right approach. There is no build-dependency on Xen > anymore, is that correct? No dependency
Re: Proposal: rename xen.git#master (to #trunk, perhaps)
On Wed, Jun 24, 2020 at 10:39 AM Stefano Stabellini wrote: > > On Wed, 24 Jun 2020, Ian Jackson wrote: > > I think it would be a good idea to rename this branch name. This name > > has unfortunate associations[1], even if it can be argued[2] that the > > etymology is not as bad as in some uses of the word. > > > > This is relativity straightforward on a technical level and will > > involve a minimum of inconvenience. Since only osstest ever pushes to > > xen.git#master, we could easily make a new branch name and also keep > > #master for compatibility as long as we like. > > > > The effects[1] would be: > > > > Users who did "git clone https://xenbits.xen.org/git-http/xen.git"; > > would find themselves on a branch called "trunk" which tracked > > "origin/trunk", by default. (Some users with old versions of git > > using old protocols would still end up on "master".) > > > > Everyone who currently tracks "master" would be able to switch to > > tracking "trunk" at their leisure. > > > > Presumably at some future point (a year or two from now, say) we would > > abolish the name "master". > > > > Comments ? In particular, comments on: > > > > 1. What the new branch name should be called. Suggestions I have seen > > include "trunk" and "main". I suggest "trunk" because this was used > > by SVN, CVS, RCS, CSSC (and therefore probably SCCS) for this same > > purpose. > > Github seems to be about to make a similar change. I wonder if we should > wait just a couple of weeks to see what name they are going to choose. > > https://www.theregister.com/2020/06/15/github_replaces_master_with_main/ > > > Of course I don't particulalry care one way or the other, but it would > be good if we end up using the same name as everybody else. It is not > that we have to choose the name Github is going to choose, but their > user base is massive -- whatever they are going to pick is very likely > going to stick. +1 to Stefano's way of thinking here -- that's exactly what we're doing in some of the other LF projects I'm involved in. Of course all the master/slave terminology has to be addresses at the project level -- but I haven't come across much of that in my Xen hacking Thanks, Roman.
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, 24 Jun 2020, Michael S. Tsirkin wrote: > On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > > Export xen_swiotlb for all platforms using xen swiotlb > > > > Use xen_swiotlb to determine when vring should use dma APIs to map the > > ring: when xen_swiotlb is enabled the dma API is required. When it is > > disabled, it is not required. > > > > Signed-off-by: Peng Fan > > Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? > Xen was there first, but everyone else is using that now. Unfortunately it is complicated and it is not related to VIRTIO_F_IOMMU_PLATFORM :-( The Xen subsystem in Linux uses dma_ops via swiotlb_xen to translate foreign mappings (memory coming from other VMs) to physical addresses. On x86, it also uses dma_ops to translate Linux's idea of a physical address into a real physical address (this is unneeded on ARM.) So regardless of VIRTIO_F_IOMMU_PLATFORM, dma_ops should be used on Xen/x86 always and on Xen/ARM if Linux is Dom0 (because it has foreign mappings.) That is why we have the if (xen_domain) return true; in vring_use_dma_api. You might have noticed that I missed one possible case above: Xen/ARM DomU :-) Xen/ARM domUs don't need swiotlb_xen, it is not even initialized. So if (xen_domain) return true; would give the wrong answer in that case. Linux would end up calling the "normal" dma_ops, not swiotlb-xen, and the "normal" dma_ops fail. The solution I suggested was to make the check in vring_use_dma_api more flexible by returning true if the swiotlb_xen is supposed to be used, not in general for all Xen domains, because that is what the check was really meant to do. In this regards I have two more comments: - the comment on top of the check in vring_use_dma_api is still valid - the patch looks broken on x86: it should always return true, but it returns false instead > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index a2de775801af..768afd79f67a 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device > > *vdev) > > * the DMA API if we're a Xen guest, which at least allows > > * all of the sensible Xen configurations to work correctly. > > */ > > - if (xen_domain()) > > + if (xen_vring_use_dma()) > > return true; > > > > return false; > > > The comment above this should probably be fixed. >
Re: Migration vmdesc and xen-save-devices-state
* Jason Andryuk (jandr...@gmail.com) wrote: > Hi, > > At some point, QEMU changed to add a json VM description (vmdesc) > after the migration data. The vmdesc is not needed to restore the > migration data, but qemu_loadvm_state() will read and discard the > vmdesc to clear the stream when should_send_vmdesc() is true. About 5 years ago :-) > xen-save-devices-state generates its migration data without a vmdesc. > xen-load-devices-state in turn calls qemu_loadvm_state() which tries > to load vmdesc since should_send_vmdesc is true for xen. When > restoring from a file, this is fine since it'll return EOF, print > "Expected vmdescription section, but got 0" and end the restore > successfully. > > Linux stubdoms load their migration data over a console, so they don't > hit the EOF and end up waiting. There does seem to be a timeout > though and restore continues after a delay, but we'd like to eliminate > the delay. > > Two options to address this are to either: > 1) set suppress_vmdesc for the Xen machines to bypass the > should_send_vmdesc() check. > or > 2) just send the vmdesc data. > > Since vmdesc is just discarded, maybe #1 should be followed. #1 does sound simple! > If going with #2, qemu_save_device_state() needs to generate the > vmdesc data. Looking at qemu_save_device_state() and > qemu_savevm_state_complete_precopy_non_iterable(), they are both very > similar and could seemingly be merged. qmp_xen_save_devices_state() > could even leverage the bdrv_inactivate_all() call in > qemu_savevm_state_complete_precopy_non_iterable(). > > The would make qemu_save_device_state a little more heavywight, which > could impact COLO. I'm not sure how performance sensitive the COLO > code is, and I haven't measured anything. COLO snapshots are potentially quite sensitive; although we've got a load of other things we could do with speeding up, we could do without making them noticably heavier. Dave > Does anyone have thoughts or opinions on the subject? > > Thanks, > Jason > -- Dr. David Alan Gilbert / dgilb...@redhat.com / Manchester, UK
Re: Proposal: rename xen.git#master (to #trunk, perhaps)
On Wed, 24 Jun 2020, Ian Jackson wrote: > I think it would be a good idea to rename this branch name. This name > has unfortunate associations[1], even if it can be argued[2] that the > etymology is not as bad as in some uses of the word. > > This is relativity straightforward on a technical level and will > involve a minimum of inconvenience. Since only osstest ever pushes to > xen.git#master, we could easily make a new branch name and also keep > #master for compatibility as long as we like. > > The effects[1] would be: > > Users who did "git clone https://xenbits.xen.org/git-http/xen.git"; > would find themselves on a branch called "trunk" which tracked > "origin/trunk", by default. (Some users with old versions of git > using old protocols would still end up on "master".) > > Everyone who currently tracks "master" would be able to switch to > tracking "trunk" at their leisure. > > Presumably at some future point (a year or two from now, say) we would > abolish the name "master". > > Comments ? In particular, comments on: > > 1. What the new branch name should be called. Suggestions I have seen > include "trunk" and "main". I suggest "trunk" because this was used > by SVN, CVS, RCS, CSSC (and therefore probably SCCS) for this same > purpose. Github seems to be about to make a similar change. I wonder if we should wait just a couple of weeks to see what name they are going to choose. https://www.theregister.com/2020/06/15/github_replaces_master_with_main/ Of course I don't particulalry care one way or the other, but it would be good if we end up using the same name as everybody else. It is not that we have to choose the name Github is going to choose, but their user base is massive -- whatever they are going to pick is very likely going to stick. > 2. Do we want to set a time now when the old name will be removed ? > I think 12 months would be good but I am open to arguments. > > In any case in my capacity as osstest maintainer I plan to do > something like this. The starting point is rather different. There > will have to be an announcement about that, but I thought I would see > what people thought about xen.git before pressing ahead there. > > Thanks, > Ian. > > [1] It seems that for a significant number of people the word reminds > them of human slavery. This seems undesirable if we can easily avoid > it, if we can. > > [2] The precise etymology of "master" in this context is unclear. It > appears to have come from BitKeeper originally. In any case the > etymology is less important than the connotations. >
Re: Keystone Issue
On Wed, 24 Jun 2020, Bertrand Marquis wrote: > > On 23 Jun 2020, at 21:50, CodeWiz2280 wrote: > > > > Is it possible to passthrough PCI devices to domU on the 32-bit arm > > keystone? Any info is appreciated. > > > > I found some old information online that "gic-v2m" is required. I'm > > not sure if the GIC-400 on the K2E supports "gic_v2m". Based on the > > fact that there is no "gic-v2m-frame" tag in the K2E device tree i'm > > guessing that it does not. > > > > If it is possible, is there a good example for arm that I can follow? > > There is no PCI passthrough support on Arm for now in Xen. > > This is something I am working on and I will present something on this > subject at the Xen summit. > But we are not targeting arm32 for now. > > The only thing possible for now is to have PCI devices handled by Dom0 and > using xen virtual drivers to pass the functionalities (ethernet, block or > others). It should also possible to pass the entire PCI controller, together with the whole aperture and all interrupts to a domU. The domU would get all PCI devices this way, not just one. > > On Wed, Jun 17, 2020 at 7:52 PM CodeWiz2280 wrote: > >> > >> On Wed, Jun 17, 2020 at 2:46 PM Stefano Stabellini > >> wrote: > >>> > >>> On Wed, 17 Jun 2020, CodeWiz2280 wrote: > On Tue, Jun 16, 2020 at 2:23 PM Marc Zyngier wrote: > > > > On 2020-06-16 19:13, CodeWiz2280 wrote: > >> On Tue, Jun 16, 2020 at 4:11 AM Marc Zyngier wrote: > >>> > >>> On 2020-06-15 20:14, CodeWiz2280 wrote: > >>> > >>> [...] > >>> > Also, the latest linux kernel still has the X-Gene storm distributor > address as "0x7801" in the device tree, which is what the Xen > code > considers a match with the old firmware. What were the addresses for > the device tree supposed to be changed to? > >>> > >>> We usually don't care, as the GIC address is provided by the > >>> bootloader, > >>> whether via DT or ACPI (this is certainly what happens on Mustang). > >>> Whatever is still in the kernel tree is just as dead as the platform > >>> it > >>> describes. > >>> > Is my understanding > correct that there is a different base address required to access the > "non-secure" region instead of the "secure" 0x7801 region? I'm > trying to see if there are corresponding different addresses for the > keystone K2E, but haven't found them yet in the manuals. > >>> > >>> There is no such address. Think of the NS bit as an *address space* > >>> identifier. > >>> > >>> The only reason XGene presents the NS part of the GIC at a different > >>> address is because XGene is broken enough not to have EL3, hence no > >>> secure mode. To wire the GIC (and other standard ARM IPs) to the core, > >>> the designers simply used the CPU NS signal as an address bit. > >>> > >>> On your platform, the NS bit does exist. I strongly suppose that it > >>> isn't wired to the GIC. Please talk to your SoC vendor for whether iot > >>> is possible to work around this. > >>> > >> I do have a question about this out to TI, but at least this method > >> gives me something to work with in the meantime. I was just looking > >> to confirm that there wouldn't be any other undesirable side effects > >> with Dom0 or DomU when using it. Was there an actual FPGA for the > >> X-Gene that needed to be updated which controlled the GIC access? Or > >> by firmware do you mean the boot loader (e.g. uboot). Thanks for the > >> support so far to all. > > > > As I said, the specific case of XGene was just a matter of picking the > > right address, as the NS bit is used as an address bit on this platform. > > This was possible because this machine doesn't have any form of > > security. So no HW was changed, no FPGA reprogrammed. Only a firmware > > table was fixed to point to the right spot. Not even u-boot or EFI was > > changed. > Ok, thank you for clarifying. I have one more question if you don't > mind. I'm aware that dom0 can share physical memory with dom1 via > grant tables. > However, is it possible to reserve a chunk of contiguous physical > memory and directly allocate it only to dom1? > For example, if I wanted dom1 to have access to 8MB of contiguous > memory at 0x8200_ (in addition to whatever virtual memory Xen > gives it). > How would one go about doing this on ARM? Is there something in the > guest config or device tree that can be set? Thanks for you help. > >>> > >>> There isn't a "proper" way to do it, only a workaround. > >>> > >>> It is possible to do it by using the iomem option, which is meant for > >>> device MMIO regions. > >>> > >>> We have patches in the Xilinx Xen tree (not upstream) to allow for > >>> specifying the
Re: UEFI support in ARM DomUs
On Wed, 24 Jun 2020, Oleksandr Andrushchenko wrote: > On 6/23/20 8:31 AM, Oleksandr Andrushchenko wrote: > > > > On 6/23/20 4:20 AM, Stefano Stabellini wrote: > >> On Mon, 22 Jun 2020, Julien Grall wrote: > >> For the first part (__XEN_INTERFACE_VERSION__) I think we can provide > >> it > >> via > >> > >> CFLAGS or something. This can also be done for the location of Xen > >> headers. > > __XEN_INTERFACE_VERSION__ should work through the CFLAGS. An alternative > > would be to allow the user to specify through the Kconfig. > You mean specifying via Kconfig something like "0x00040d00"? > >>> Possibly yes. > >>> > And what about the headers? How will we provide their location if we > decide > not to include those > > in the tree? > >>> I would do through Kconfig as well. > >> If we specify the external location of the Xen headers via Kconfig, it > >> seems to me that we should be able to detect the interface version > >> automatically from any Makefile as part of the build. No need to ask the > >> user. > >> > >> However, if Oleksandr is thinking of using the Xen headers for the > >> hypercalls definitions, then I think we might not need external headers > >> at all because that is a stable interface, as Julien wrote. We could > >> just define our own few headers for just what you need like Linux does. > > > > This is a good idea: I'll try to get the minimal set of headers from Linux > > > > instead of Xen as those seem to be well prepared for such a use-case. This > > > > way we'll have headers in U-boot tree and guarantee that we have a minimal > > > > subset which is easier to maintain. I'll keep you updated on the progress > > We've managed to strip the headers and remove __XEN__ and the rest definitions > > we were talking about. So, these are now the minimal required set of headers > > that allows U-boot to build serial and block drivers. Please take a look at > [1] > > Pull request for comments is at [2] I think this is the right approach. There is no build-dependency on Xen anymore, is that correct?
Re: [PATCH v10 1/9] error: auto propagated local_err
Greg Kurz writes: > On Mon, 15 Jun 2020 07:21:03 +0200 > Markus Armbruster wrote: > >> Greg Kurz writes: >> >> > On Tue, 17 Mar 2020 18:16:17 +0300 >> > Vladimir Sementsov-Ogievskiy wrote: >> > >> >> Introduce a new ERRP_AUTO_PROPAGATE macro, to be used at start of >> >> functions with an errp OUT parameter. >> >> >> >> It has three goals: >> >> >> >> 1. Fix issue with error_fatal and error_prepend/error_append_hint: user >> >> can't see this additional information, because exit() happens in >> >> error_setg earlier than information is added. [Reported by Greg Kurz] >> >> >> > >> > I have more of these coming and I'd really like to use ERRP_AUTO_PROPAGATE. >> > >> > It seems we have a consensus on the macro itself but this series is gated >> > by the conversion of the existing code base. >> > >> > What about merging this patch separately so that people can start using >> > it at least ? >> >> Please give me a few more days to finish the work I feel should go in >> before the conversion. With any luck, Vladimir can then rebase / >> recreate the conversion easily, and you can finally use the macro for >> your own work. >> > > Sure. Thanks. Just posted "[PATCH 00/46] Less clumsy error checking". The sheer size of the thing and the length of its dependency chain explains why it took me so long. I feel bad about delaying you all the same. Apologies! I hope we can converge quickly enough to get Vladimir's work on top ready in time for the soft freeze.
Re: [RFC PATCH v2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()
On Wed, Jun 24, 2020 at 9:07 PM Boris Ostrovsky
wrote:
>
> On 6/23/20 9:36 PM, Souptick Joarder wrote:
> > On Tue, Jun 23, 2020 at 11:11 PM Boris Ostrovsky
> > wrote:
> >> On 6/23/20 7:58 AM, Souptick Joarder wrote:
> >>> In 2019, we introduced pin_user_pages*() and now we are converting
> >>> get_user_pages*() to the new API as appropriate. [1] & [2] could
> >>> be referred for more information. This is case 5 as per document [1].
> >>>
> >>> As discussed, pages need to be marked as dirty before unpinned it.
> >>>
> >>> Previously, if lock_pages() end up partially mapping pages, it used
> >>> to return -ERRNO due to which unlock_pages() have to go through
> >>> each pages[i] till *nr_pages* to validate them. This can be avoided
> >>> by passing correct number partially mapped pages & -ERRNO separately
> >>> while returning from lock_pages() due to error.
> >>> With this fix unlock_pages() doesn't need to validate pages[i] till
> >>> *nr_pages* for error scenario.
> >>
> >> This should be split into two patches please. The first one will fix the
> >> return value bug (and will need to go to stable branches) and the second
> >> will use new routine to pin pages.
> > Initially I split the patches into 2 commits. But at last moment I
> > figure out that,
> > this bug fix ( better to call coding error, doesn't looks like lead to
> > any runtime bug) is tightly coupled to 2nd commit for
> > pin_user_pages*() conversion,
> > which means we don't need the bug fix patch if we are not converting the
> > API to
> > pin_user_pages*()/ unpin_user_pages_dirty_lock(). That's the reason to
> > clubbed these two
> > commits into a single one.
>
>
> I am not sure I understand why the two issues are connected. Failure of
> either get_user_pages_fast() or pin_user_pages() will result in the same
> kind of overall ioctl failure, won't it?
>
Sorry, I think, I need to go through the bug again for my clarity.
My understanding is ->
First, In case of success lock_pages() returns 0, so what privcmd_ioctl_dm_op()
will return to user is depend on the return value of HYPERVISOR_dm_op()
and at last all the pages are unpinned. At present I am not clear about the
return value of HYPERVISOR_dm_op(). But this path of code looks to be correct.
second, incase of failure from lock_pages() will return either -ENOSPC / -ERRNO
receive from {get|pin|_user_pages_fast() inside for loop. (at that
point there might
be some partially mapped pages as it is mapping inside the loop). Upon
receiving
-ERRNO privcmd_ioctl_dm_op() will pass the same -ERRNO to user (not the partial
mapped page count). This looks to be correct behaviour from user space.
The problem I was trying to address is, in the second scenario when
lock_pages() returns
-ERRNO and there are already existing mapped pages. In this scenario,
when unlcok_pages()
is called it will iterate till *nr_pages* to validate and unmap the
pages. But it is supposed to
unmap only the mapped pages which I am trying to address as part of bug fix.
Let me know if I am able to be in sync with what you are expecting ?
> One concern though is that we are changing how user will see this error.
My understanding from above is user will always see right -ERRNO and
will not be impacted.
Another scenario I was thinking, if {get|pin|_user_pages_fast() return
number of pages pinned
which may be fewer than the number requested. Then lock_pages()
returns 0 to caller
and caller/user space will continue with the assumption that all pages
are pinned correctly.
Is this an expected behaviour as per current code ?
> Currently Xen devicemodel (which AFAIK is the only caller) ignores it,
> which is I think is wrong. So another option would be to fix this in Xen
> and continue returning positive number here. I guess it's back to Paul
> again.
>
>
> -boris
>
>
[xen-4.12-testing test] 151316: regressions - FAIL
flight 151316 xen-4.12-testing real [real] http://logs.test-lab.xenproject.org/osstest/logs/151316/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-i386-prev 6 xen-buildfail REGR. vs. 150176 build-amd64-prev 6 xen-buildfail REGR. vs. 150176 Tests which did not succeed, but are not blocking: test-amd64-amd64-migrupgrade 1 build-check(1) blocked n/a test-amd64-i386-migrupgrade 1 build-check(1) blocked n/a test-amd64-amd64-xl-qcow217 guest-localmigrate/x10 fail like 150176 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-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-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail 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-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-vhd 12 migrate-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-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail 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 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-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-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stop fail 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-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass version targeted for testing: xen 06760c2bf322cef4622b56bafee74fe93ae01630 baseline version: xen 09b61126b4d1e9d372fd2e24b702be10a358da9d Last test of basis 150176 2020-05-14 12:36:34 Z 41 days Failing
[XEN RFC for-4.14] Re: use of "stat -"
Jan Beulich writes ("Re: use of "stat -""):
> [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments
> unless you have verified the sender and know the content is safe.
> On 14.05.2020 13:02, Ian Jackson wrote:
> > I've read this thread. Jan, I'm sorry that this causes you
> > inconvenience. I'm hoping it won't come down to a choice between
> > supporting people who want to ship a dom0 without perl, and people who
> > want a dom0 using more-than-a-decade-old coreutils.
> >
> > Jan, can you tell me what the output is of this on your ancient
> > system:
> >
> > $ rm -f t
> > $ >t
> > $ exec 3 > $ stat -L -c '%F %i' /dev/stdin <&3
> > regular empty file 393549
> > $ rm t
> > $ stat -L -c '%F %i' /dev/stdin <&3
> > regular empty file 393549
> > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3
> > $
>
> $ rm -f t
> $ >t
> $ exec 3 $ stat -L -c '%F %i' /dev/stdin <&3
> regular empty file 3380369
> $ rm t
> $ stat -L -c '%F %i' /dev/stdin <&3
> regular empty file 3380369
> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3
> regular empty file 3380369
>
> > Also, the contents of the file "u" afterwards, please.
>
> Attached.
Thanks.
I think this means that `stat -' can be replaced by `stat /dev/stdin'.
This script is only run on Linux where /dev/stdin has existed
basically forever. The strace output shows
stat("/dev/stdin", {st_mode=S_IFREG|0644, st_size=0, ...}) = 0
and the transcript shows that your stat(1) behaves as I hope.
Jan, will you send a patch ? It is best if someone else but me writes
it and tests it because then I am a "clean" reviewer.
Paul, supposing I review such a patch and say it is low risk, and we
commit it by Friday, can it have a release-ack ?
Thanks,
Ian.
Proposal: rename xen.git#master (to #trunk, perhaps)
I think it would be a good idea to rename this branch name. This name has unfortunate associations[1], even if it can be argued[2] that the etymology is not as bad as in some uses of the word. This is relativity straightforward on a technical level and will involve a minimum of inconvenience. Since only osstest ever pushes to xen.git#master, we could easily make a new branch name and also keep #master for compatibility as long as we like. The effects[1] would be: Users who did "git clone https://xenbits.xen.org/git-http/xen.git"; would find themselves on a branch called "trunk" which tracked "origin/trunk", by default. (Some users with old versions of git using old protocols would still end up on "master".) Everyone who currently tracks "master" would be able to switch to tracking "trunk" at their leisure. Presumably at some future point (a year or two from now, say) we would abolish the name "master". Comments ? In particular, comments on: 1. What the new branch name should be called. Suggestions I have seen include "trunk" and "main". I suggest "trunk" because this was used by SVN, CVS, RCS, CSSC (and therefore probably SCCS) for this same purpose. 2. Do we want to set a time now when the old name will be removed ? I think 12 months would be good but I am open to arguments. In any case in my capacity as osstest maintainer I plan to do something like this. The starting point is rather different. There will have to be an announcement about that, but I thought I would see what people thought about xen.git before pressing ahead there. Thanks, Ian. [1] It seems that for a significant number of people the word reminds them of human slavery. This seems undesirable if we can easily avoid it, if we can. [2] The precise etymology of "master" in this context is unclear. It appears to have come from BitKeeper originally. In any case the etymology is less important than the connotations.
Ping: use of "stat -"
On 18.05.2020 12:34, Jan Beulich wrote: > On 14.05.2020 13:02, Ian Jackson wrote: >> I've read this thread. Jan, I'm sorry that this causes you >> inconvenience. I'm hoping it won't come down to a choice between >> supporting people who want to ship a dom0 without perl, and people who >> want a dom0 using more-than-a-decade-old coreutils. >> >> Jan, can you tell me what the output is of this on your ancient >> system: >> >> $ rm -f t >> $ >t >> $ exec 3> $ stat -L -c '%F %i' /dev/stdin <&3 >> regular empty file 393549 >> $ rm t >> $ stat -L -c '%F %i' /dev/stdin <&3 >> regular empty file 393549 >> $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 >> $ > > $ rm -f t > $ >t > $ exec 3 $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > $ rm t > $ stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > $ strace -ou stat -L -c '%F %i' /dev/stdin <&3 > regular empty file 3380369 > >> Also, the contents of the file "u" afterwards, please. > > Attached. > > Thanks for looking into this, Jan Is there (still) a plan to get this addressed for 4.14? Apologies if I missed something having been posted / committed... Jan
Re: [RFC PATCH v2] xen/privcmd: Convert get_user_pages*() to pin_user_pages*()
On 6/23/20 9:36 PM, Souptick Joarder wrote: > On Tue, Jun 23, 2020 at 11:11 PM Boris Ostrovsky > wrote: >> On 6/23/20 7:58 AM, Souptick Joarder wrote: >>> In 2019, we introduced pin_user_pages*() and now we are converting >>> get_user_pages*() to the new API as appropriate. [1] & [2] could >>> be referred for more information. This is case 5 as per document [1]. >>> >>> As discussed, pages need to be marked as dirty before unpinned it. >>> >>> Previously, if lock_pages() end up partially mapping pages, it used >>> to return -ERRNO due to which unlock_pages() have to go through >>> each pages[i] till *nr_pages* to validate them. This can be avoided >>> by passing correct number partially mapped pages & -ERRNO separately >>> while returning from lock_pages() due to error. >>> With this fix unlock_pages() doesn't need to validate pages[i] till >>> *nr_pages* for error scenario. >> >> This should be split into two patches please. The first one will fix the >> return value bug (and will need to go to stable branches) and the second >> will use new routine to pin pages. > Initially I split the patches into 2 commits. But at last moment I > figure out that, > this bug fix ( better to call coding error, doesn't looks like lead to > any runtime bug) is tightly coupled to 2nd commit for > pin_user_pages*() conversion, > which means we don't need the bug fix patch if we are not converting the API > to > pin_user_pages*()/ unpin_user_pages_dirty_lock(). That's the reason to > clubbed these two > commits into a single one. I am not sure I understand why the two issues are connected. Failure of either get_user_pages_fast() or pin_user_pages() will result in the same kind of overall ioctl failure, won't it? One concern though is that we are changing how user will see this error. Currently Xen devicemodel (which AFAIK is the only caller) ignores it, which is I think is wrong. So another option would be to fix this in Xen and continue returning positive number here. I guess it's back to Paul again. -boris
Ping: [PATCH] x86/CPUID: fill all fields in x86_cpuid_policy_fill_native()
(sorry, re-sending with To and Cc corrected)
On 22.06.2020 14:39, Andrew Cooper wrote:
> On 22/06/2020 13:09, Jan Beulich wrote:
>> Coverity validly complains that the new call from
>> tools/tests/cpu-policy/test-cpu-policy.c:test_cpuid_current() leaves
>> two fields uninitialized, yet they get then consumed by
>> x86_cpuid_copy_to_buffer(). (All other present callers of the function
>> pass a pointer to a static - and hence initialized - buffer.)
>>
>> Coverity-ID: 1464809
>> Fixes: c22ced93e167 ("tests/cpu-policy: Confirm that CPUID serialisation is
>> sorted")
>> Signed-off-by: Jan Beulich
>>
>> --- a/xen/lib/x86/cpuid.c
>> +++ b/xen/lib/x86/cpuid.c
>> @@ -176,6 +176,10 @@ void x86_cpuid_policy_fill_native(struct
>>ARRAY_SIZE(p->extd.raw) - 1); ++i )
>> cpuid_leaf(0x8000 + i, >extd.raw[i]);
>>
>> +/* Don't report leaves from possible lower level hypervisor. */
>
> ", for now."
>
> This will change in the future.
>
> With this, Reviewed-by: Andrew Cooper
Paul?
Thanks, Jan
Ping: [PATCH] x86/CPUID: fill all fields in x86_cpuid_policy_fill_native()
On 22.06.2020 14:39, Andrew Cooper wrote:
> On 22/06/2020 13:09, Jan Beulich wrote:
>> Coverity validly complains that the new call from
>> tools/tests/cpu-policy/test-cpu-policy.c:test_cpuid_current() leaves
>> two fields uninitialized, yet they get then consumed by
>> x86_cpuid_copy_to_buffer(). (All other present callers of the function
>> pass a pointer to a static - and hence initialized - buffer.)
>>
>> Coverity-ID: 1464809
>> Fixes: c22ced93e167 ("tests/cpu-policy: Confirm that CPUID serialisation is
>> sorted")
>> Signed-off-by: Jan Beulich
>>
>> --- a/xen/lib/x86/cpuid.c
>> +++ b/xen/lib/x86/cpuid.c
>> @@ -176,6 +176,10 @@ void x86_cpuid_policy_fill_native(struct
>>ARRAY_SIZE(p->extd.raw) - 1); ++i )
>> cpuid_leaf(0x8000 + i, >extd.raw[i]);
>>
>> +/* Don't report leaves from possible lower level hypervisor. */
>
> ", for now."
>
> This will change in the future.
>
> With this, Reviewed-by: Andrew Cooper
Paul?
Thanks, Jan
Ping: [PATCH v3 1/1] x86/acpi: Use FADT flags to determine the PMTMR width
On 22.06.2020 10:49, Jan Beulich wrote:
> On 20.06.2020 00:00, Grzegorz Uriasz wrote:
>> @@ -490,8 +497,12 @@ static int __init acpi_parse_fadt(struct
>> acpi_table_header *table)
>> */
>> if (!pmtmr_ioport) {
>> pmtmr_ioport = fadt->pm_timer_block;
>> -pmtmr_width = fadt->pm_timer_length == 4 ? 24 : 0;
>> +pmtmr_width = fadt->pm_timer_length == 4 ? 32 : 0;
>> }
>> +if (pmtmr_width < 32 && fadt->flags & ACPI_FADT_32BIT_TIMER)
>> +printk(KERN_WARNING PREFIX "PM-Timer is too short\n");
>
> Indentation is screwed up here.
>
>> --- a/xen/arch/x86/time.c
>> +++ b/xen/arch/x86/time.c
>> @@ -457,16 +457,13 @@ static u64 read_pmtimer_count(void)
>> static s64 __init init_pmtimer(struct platform_timesource *pts)
>> {
>> u64 start;
>> -u32 count, target, mask = 0xff;
>> +u32 count, target, mask;
>>
>> -if ( !pmtmr_ioport || !pmtmr_width )
>> +if ( !pmtmr_ioport || (pmtmr_width != 24 && pmtmr_width != 32) )
>> return 0;
>>
>> -if ( pmtmr_width == 32 )
>> -{
>> -pts->counter_bits = 32;
>> -mask = 0x;
>> -}
>> +pts->counter_bits = pmtmr_width;
>> +mask = (1ull << pmtmr_width) - 1;
>
> "mask" being of "u32" type, the use of 1ull is certainly a little odd
> here, and one of the reasons why I think this extra "cleanup" would
> better go in a separate patch.
>
> Seeing that besides cosmetic aspects the patch looks okay now, I'd be
> willing to leave the bigger adjustment mostly as is, albeit I'd
> prefer the 1ull to go away, by instead switching to e.g.
>
> mask = 0x >> (32 - pmtmr_width);
>
> With the adjustments (which I'd be happy to make while committing,
> provided you agree)
> Reviewed-by: Jan Beulich
>
> Also Cc-ing Paul for a possible release ack (considering this is a
> backporting candidate).
Paul?
Thanks, Jan
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
On 24.06.2020 15:41, Julien Grall wrote: > On 24/06/2020 11:12, Jan Beulich wrote: >> On 23.06.2020 19:26, Roger Pau Monné wrote: >>> I'm confused. Couldn't we switch from uint64_aligned_t to plain >>> uint64_t (like it's currently on the Linux headers), and then use the >>> compat layer in Xen to handle the size difference when called from >>> 32bit environments? >> >> And which size would we use there? The old or the new one (breaking >> future or existing callers respectively)? Meanwhile I think that if >> this indeed needs to not be tools-only (which I still question), > > I think we now agreed on a subthread that the kernel needs to know the > layout of the hypercall. > >> then our only possible route is to add explicit padding for the >> 32-bit case alongside the change you're already making. > > AFAICT Linux 32-bit doesn't have this padding. So wouldn't it make > incompatible the two incompatible? In principle yes. But they're putting the structure instance on the stack, so there's not risk from Xen reading 4 bytes too many. I'd prefer keeping the interface as is (i.e. with the previously implicit padding made explicit) to avoid risking to break other possible callers. But that's just my view on it, anyway ... Jan
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
Hi Jan,
On 24/06/2020 11:12, Jan Beulich wrote:
On 23.06.2020 19:26, Roger Pau Monné wrote:
On Tue, Jun 23, 2020 at 06:18:52PM +0200, Jan Beulich wrote:
On 23.06.2020 17:56, Roger Pau Monné wrote:
On Tue, Jun 23, 2020 at 05:02:04PM +0200, Jan Beulich wrote:
On 23.06.2020 15:52, Roger Pau Monne wrote:
XENMEM_acquire_resource and it's related structure is currently inside
a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the
hypervisor or the toolstack only. This is wrong as the hypercall is
already being used by the Linux kernel at least, and as such needs to
be public.
Also switch the usage of uint64_aligned_t to plain uint64_t, as
uint64_aligned_t is only to be used by the toolstack. Note that the
layout of the structure will be the same, as the field is already
naturally aligned to a 8 byte boundary.
I'm afraid it's more complicated, and hence ...
No functional change expected.
... this doesn't hold: As I notice only now the struct also wrongly
(if it was meant to be a tools-only interface) failed to use
XEN_GUEST_HANDLE_64(), which would in principle have been a problem
for 32-bit callers (passing garbage in the upper half of a handle).
It's not an actual problem because, unlike would typically be the
case for tools-only interfaces, there is compat translation for this
struct.
Yes, there's already compat translation for the frame_list array.
With this, however, the problem of your change becomes noticeable:
The structure's alignment for 32-bit x86, and with it the structure's
sizeof(), both change. You should be able to see this by comparing
xen/common/compat/memory.c's disassembly before and after your
change - the number of bytes to copy by the respective
copy_from_guest() ought to have changed.
Right, this is the layout with frame aligned to 8 bytes:
struct xen_mem_acquire_resource {
uint16_t domid;/* 0 2 */
uint16_t type; /* 2 2 */
uint32_t id; /* 4 4 */
uint32_t nr_frames;/* 8 4 */
uint32_t pad; /*12 4 */
uint64_t frame;/*16 8 */
long unsigned int *frame_list; /*24 4 */
/* size: 32, cachelines: 1, members: 7 */
/* padding: 4 */
/* last cacheline: 32 bytes */
};
And this is without:
struct xen_mem_acquire_resource {
uint16_t domid;/* 0 2 */
uint16_t type; /* 2 2 */
uint32_t id; /* 4 4 */
uint32_t nr_frames;/* 8 4 */
uint32_t pad; /*12 4 */
uint64_t frame;/*16 8 */
long unsigned int *frame_list; /*24 4 */
/* size: 28, cachelines: 1, members: 7 */
/* last cacheline: 28 bytes */
};
Note Xen has already been copying those extra 4 padding bytes from
32bit Linux kernels AFAICT, as the struct declaration in Linux has
dropped the aligned attribute.
The question now is whether we're willing to accept such a (marginal)
incompatibility. The chance of a 32-bit guest actually running into
it shouldn't be very high, but with the right placement of an
instance of the struct at the end of a page it would be possible to
observe afaict.
Or whether we go the alternative route and pad the struct suitably
for 32-bit.
I guess we are trapped with what Linux has on it's headers now, so we
should handle the struct size difference in Xen?
How do you mean to notice the difference in Xen? You don't know
what the caller's environment's sizeof() would yield.
I'm confused. Couldn't we switch from uint64_aligned_t to plain
uint64_t (like it's currently on the Linux headers), and then use the
compat layer in Xen to handle the size difference when called from
32bit environments?
And which size would we use there? The old or the new one (breaking
future or existing callers respectively)? Meanwhile I think that if
this indeed needs to not be tools-only (which I still question),
I think we now agreed on a subthread that the kernel needs to know the
layout of the hypercall.
then our only possible route is to add explicit padding for the
32-bit case alongside the change you're already making.
AFAICT Linux 32-bit doesn't have this padding. So wouldn't it make
incompatible the two incompatible?
Cheers,
--
Julien Grall
Migration vmdesc and xen-save-devices-state
Hi, At some point, QEMU changed to add a json VM description (vmdesc) after the migration data. The vmdesc is not needed to restore the migration data, but qemu_loadvm_state() will read and discard the vmdesc to clear the stream when should_send_vmdesc() is true. xen-save-devices-state generates its migration data without a vmdesc. xen-load-devices-state in turn calls qemu_loadvm_state() which tries to load vmdesc since should_send_vmdesc is true for xen. When restoring from a file, this is fine since it'll return EOF, print "Expected vmdescription section, but got 0" and end the restore successfully. Linux stubdoms load their migration data over a console, so they don't hit the EOF and end up waiting. There does seem to be a timeout though and restore continues after a delay, but we'd like to eliminate the delay. Two options to address this are to either: 1) set suppress_vmdesc for the Xen machines to bypass the should_send_vmdesc() check. or 2) just send the vmdesc data. Since vmdesc is just discarded, maybe #1 should be followed. If going with #2, qemu_save_device_state() needs to generate the vmdesc data. Looking at qemu_save_device_state() and qemu_savevm_state_complete_precopy_non_iterable(), they are both very similar and could seemingly be merged. qmp_xen_save_devices_state() could even leverage the bdrv_inactivate_all() call in qemu_savevm_state_complete_precopy_non_iterable(). The would make qemu_save_device_state a little more heavywight, which could impact COLO. I'm not sure how performance sensitive the COLO code is, and I haven't measured anything. Does anyone have thoughts or opinions on the subject? Thanks, Jason
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
On 24.06.2020 14:53, Paul Durrant wrote: >> -Original Message- >> From: Jan Beulich >> Sent: 24 June 2020 13:52 >> To: Julien Grall >> Cc: Roger Pau Monné ; xen-devel@lists.xenproject.org; >> p...@xen.org; Andrew >> Cooper ; George Dunlap >> ; Ian Jackson >> ; Stefano Stabellini ; >> Wei Liu >> Subject: Re: [PATCH for-4.14] mm: fix public declaration of struct >> xen_mem_acquire_resource >> >> On 24.06.2020 14:47, Julien Grall wrote: >>> Hi, >>> >>> On 24/06/2020 13:08, Jan Beulich wrote: On 24.06.2020 12:52, Julien Grall wrote: > Hi Jan, > > On 24/06/2020 11:05, Jan Beulich wrote: >> On 23.06.2020 19:32, Roger Pau Monné wrote: >>> On Tue, Jun 23, 2020 at 05:04:53PM +0200, Jan Beulich wrote: On 23.06.2020 15:52, Roger Pau Monne wrote: > XENMEM_acquire_resource and it's related structure is currently inside > a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the > hypervisor or the toolstack only. This is wrong as the hypercall is > already being used by the Linux kernel at least, and as such needs to > be public. Actually - how does this work for the Linux kernel, seeing rc = rcu_lock_remote_domain_by_id(xmar.domid, ); if ( rc ) return rc; rc = xsm_domain_resource_map(XSM_DM_PRIV, d); if ( rc ) goto out; in the function? >>> >>> It's my understanding (I haven't tried to use that hypercall yet on >>> FreeBSD, so I cannot say I've tested it), that xmar.domid is the >>> remote domain, which the functions locks and then uses >>> xsm_domain_resource_map to check whether the current domain has >>> permissions to do privileged operations against it. >> >> Yes, but that's a tool stack operation, not something the kernel >> would do all by itself. The kernel would only ever pass DOMID_SELF >> (or the actual local domain ID), I would think. > > You can't issue that hypercall directly from userspace because you need > to map the page in the physical address space of the toolstack domain. > > So the kernel has to act as the proxy for the hypercall. This is > implemented as mmap() in Linux. Oh, and there's no generic wrapping available here, unlike for dmop. >>> >>> It is not clear to me the sort of generic wrapping you are referring to. >>> Are you referring to a stable interface for an application? >>> Makes me wonder whether, for this purpose, there should be (have been) a new dmop with identical functionality, to allow such funneling. >>> >>> I am not sure how using DMOP will allow us to implement it fully in >>> userspace. Do you mind expanding it? >> >> dmop was designed so that a kernel proxying requests wouldn't need >> updating for every new request added to the interface. If the >> request here was made through a new dmop, the kernel would never >> have had a need to know of an interface structure that's of no >> interest to it, but only to the tool stack. > > How would the pages get mapped into process address space if the > kernel doesn't know what's being done? Urgh, yes, silly me. Jan
RE: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
> -Original Message- > From: Jan Beulich > Sent: 24 June 2020 13:52 > To: Julien Grall > Cc: Roger Pau Monné ; xen-devel@lists.xenproject.org; > p...@xen.org; Andrew > Cooper ; George Dunlap ; > Ian Jackson > ; Stefano Stabellini ; Wei > Liu > Subject: Re: [PATCH for-4.14] mm: fix public declaration of struct > xen_mem_acquire_resource > > On 24.06.2020 14:47, Julien Grall wrote: > > Hi, > > > > On 24/06/2020 13:08, Jan Beulich wrote: > >> On 24.06.2020 12:52, Julien Grall wrote: > >>> Hi Jan, > >>> > >>> On 24/06/2020 11:05, Jan Beulich wrote: > On 23.06.2020 19:32, Roger Pau Monné wrote: > > On Tue, Jun 23, 2020 at 05:04:53PM +0200, Jan Beulich wrote: > >> On 23.06.2020 15:52, Roger Pau Monne wrote: > >>> XENMEM_acquire_resource and it's related structure is currently inside > >>> a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the > >>> hypervisor or the toolstack only. This is wrong as the hypercall is > >>> already being used by the Linux kernel at least, and as such needs to > >>> be public. > >> > >> Actually - how does this work for the Linux kernel, seeing > >> > >> rc = rcu_lock_remote_domain_by_id(xmar.domid, ); > >> if ( rc ) > >> return rc; > >> > >> rc = xsm_domain_resource_map(XSM_DM_PRIV, d); > >> if ( rc ) > >> goto out; > >> > >> in the function? > > > > It's my understanding (I haven't tried to use that hypercall yet on > > FreeBSD, so I cannot say I've tested it), that xmar.domid is the > > remote domain, which the functions locks and then uses > > xsm_domain_resource_map to check whether the current domain has > > permissions to do privileged operations against it. > > Yes, but that's a tool stack operation, not something the kernel > would do all by itself. The kernel would only ever pass DOMID_SELF > (or the actual local domain ID), I would think. > >>> > >>> You can't issue that hypercall directly from userspace because you need > >>> to map the page in the physical address space of the toolstack domain. > >>> > >>> So the kernel has to act as the proxy for the hypercall. This is > >>> implemented as mmap() in Linux. > >> > >> Oh, and there's no generic wrapping available here, unlike for > >> dmop. > > > > It is not clear to me the sort of generic wrapping you are referring to. > > Are you referring to a stable interface for an application? > > > >> Makes me wonder whether, for this purpose, there should > >> be (have been) a new dmop with identical functionality, to > >> allow such funneling. > > > > I am not sure how using DMOP will allow us to implement it fully in > > userspace. Do you mind expanding it? > > dmop was designed so that a kernel proxying requests wouldn't need > updating for every new request added to the interface. If the > request here was made through a new dmop, the kernel would never > have had a need to know of an interface structure that's of no > interest to it, but only to the tool stack. How would the pages get mapped into process address space if the kernel doesn't know what's being done? Paul > > Jan
Re: [PATCH v2 4/7] x86/vmx: add do_vmtrace_op
On Wed, Jun 24, 2020 at 6:40 AM Andrew Cooper wrote: > > On 24/06/2020 11:03, Jan Beulich wrote: > > On 23.06.2020 19:24, Andrew Cooper wrote: > >> On 23/06/2020 09:51, Jan Beulich wrote: > >>> On 23.06.2020 03:04, Michał Leszczyński wrote: > - 22 cze 2020 o 18:16, Jan Beulich jbeul...@suse.com napisał(a): > > > On 22.06.2020 18:02, Michał Leszczyński wrote: > >> - 22 cze 2020 o 17:22, Jan Beulich jbeul...@suse.com napisał(a): > >>> On 22.06.2020 16:35, Michał Leszczyński wrote: > - 22 cze 2020 o 15:25, Jan Beulich jbeul...@suse.com napisał(a): > > Is any of what you do in this switch() actually legitimate without > > hvm_set_vmtrace_pt_size() having got called for the guest? From > > remarks elsewhere I imply you expect the param that you currently > > use to be set upon domain creation time, but at the very least the > > potentially big buffer should imo not get allocated up front, but > > only when tracing is to actually be enabled. > Wait... so you want to allocate these buffers in runtime? > Previously we were talking that there is too much runtime logic > and these enable/disable hypercalls should be stripped to absolute > minimum. > >>> Basic arrangements can be made at domain creation time. I don't > >>> think though that it would be a good use of memory if you > >>> allocated perhaps many gigabytes of memory just for possibly > >>> wanting to enable tracing on a guest. > >>> > >> From our previous conversations I thought that you want to have > >> as much logic moved to the domain creation as possible. > >> > >> Thus, a parameter "vmtrace_pt_size" was introduced. By default it's > >> zero (= disabled), if you set it to a non-zero value, then trace > >> buffers > >> of given size will be allocated for the domain and you have possibility > >> to use ipt_enable/ipt_disable at any moment. > >> > >> This way the runtime logic is as thin as possible. I assume user knows > >> in advance whether he/she would want to use external monitoring with > >> IPT > >> or not. > > Andrew - I think you requested movement to domain_create(). Could > > you clarify if indeed you mean to also allocate the big buffers > > this early? > I would like to recall what Andrew wrote few days ago: > > - 16 cze 2020 o 22:16, Andrew Cooper andrew.coop...@citrix.com wrote: > > Xen has traditionally opted for a "and turn this extra thing on > > dynamically" model, but this has caused no end of security issues and > > broken corner cases. > > > > You can see this still existing in the difference between > > XEN_DOMCTL_createdomain and XEN_DOMCTL_max_vcpus, (the latter being > > required to chose the number of vcpus for the domain) and we're making > > good progress undoing this particular wart (before 4.13, it was > > concerning easy to get Xen to fall over a NULL d->vcpu[] pointer by > > issuing other hypercalls between these two). > > > > There is a lot of settings which should be immutable for the lifetime of > > the domain, and external monitoring looks like another one of these. > > Specifying it at createdomain time allows for far better runtime > > behaviour (you are no longer in a situation where the first time you try > > to turn tracing on, you end up with -ENOMEM because another VM booted in > > the meantime and used the remaining memory), and it makes for rather > > more simple code in Xen itself (at runtime, you can rely on it having > > been set up properly, because a failure setting up will have killed the > > domain already). > > > > ... > > > > ~Andrew > according to this quote I've moved buffer allocation to the > create_domain, > the updated version was already sent to the list as patch v3. > >>> I'd still like to see an explicit confirmation by him that this > >>> use of memory is indeed what he has intended. There are much smaller > >>> amounts of memory which we allocate on demand, just to avoid > >>> allocating some without then ever using it. > >> PT is a debug/diagnostic tool. Its not something you'd run in > >> production against a production VM. > > Well, the suggested use with introspection made me assume differently. > > If this is (now and forever) truly debug/diag only, so be it then. > > At the end of the day, it is a fine grain profiling tool, meaning that > it is not used in the common case. > > The usecase presented is for fuzzing, using the execution trace > generated by the CPU, rather than the current scheme which allegedly > involves shuffling breakpoints around. That's indeed the usecase we are looking to use it for. But there is also some experimental malware analysis scenarios it is targeted for which is what I believe the CERT folks are mostly
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
On 24.06.2020 14:47, Julien Grall wrote: > Hi, > > On 24/06/2020 13:08, Jan Beulich wrote: >> On 24.06.2020 12:52, Julien Grall wrote: >>> Hi Jan, >>> >>> On 24/06/2020 11:05, Jan Beulich wrote: On 23.06.2020 19:32, Roger Pau Monné wrote: > On Tue, Jun 23, 2020 at 05:04:53PM +0200, Jan Beulich wrote: >> On 23.06.2020 15:52, Roger Pau Monne wrote: >>> XENMEM_acquire_resource and it's related structure is currently inside >>> a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the >>> hypervisor or the toolstack only. This is wrong as the hypercall is >>> already being used by the Linux kernel at least, and as such needs to >>> be public. >> >> Actually - how does this work for the Linux kernel, seeing >> >> rc = rcu_lock_remote_domain_by_id(xmar.domid, ); >> if ( rc ) >> return rc; >> >> rc = xsm_domain_resource_map(XSM_DM_PRIV, d); >> if ( rc ) >> goto out; >> >> in the function? > > It's my understanding (I haven't tried to use that hypercall yet on > FreeBSD, so I cannot say I've tested it), that xmar.domid is the > remote domain, which the functions locks and then uses > xsm_domain_resource_map to check whether the current domain has > permissions to do privileged operations against it. Yes, but that's a tool stack operation, not something the kernel would do all by itself. The kernel would only ever pass DOMID_SELF (or the actual local domain ID), I would think. >>> >>> You can't issue that hypercall directly from userspace because you need >>> to map the page in the physical address space of the toolstack domain. >>> >>> So the kernel has to act as the proxy for the hypercall. This is >>> implemented as mmap() in Linux. >> >> Oh, and there's no generic wrapping available here, unlike for >> dmop. > > It is not clear to me the sort of generic wrapping you are referring to. > Are you referring to a stable interface for an application? > >> Makes me wonder whether, for this purpose, there should >> be (have been) a new dmop with identical functionality, to >> allow such funneling. > > I am not sure how using DMOP will allow us to implement it fully in > userspace. Do you mind expanding it? dmop was designed so that a kernel proxying requests wouldn't need updating for every new request added to the interface. If the request here was made through a new dmop, the kernel would never have had a need to know of an interface structure that's of no interest to it, but only to the tool stack. Jan
Re: [PATCH] xen: Fix xen-legacy-backend qdev types
On Wed, Jun 24, 2020 at 8:30 AM Paul Durrant wrote: > > > -Original Message- > > From: Jason Andryuk > > Sent: 24 June 2020 13:20 > > To: Stefano Stabellini ; Anthony Perard > > ; Paul > > Durrant ; xen-devel@lists.xenproject.org > > Cc: Jason Andryuk ; qemu-de...@nongnu.org > > Subject: [PATCH] xen: Fix xen-legacy-backend qdev types > > > > xen-sysdev is a TYPE_SYS_BUS_DEVICE. bus_type should not be changed so > > that it can plug into the System bus. Otherwise this assert triggers: > > qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion > > `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)' > > failed. > > > > TYPE_XENBACKEND attaches to TYPE_XENSYSBUS, so its class_init needs to > > be set accordingly to attach the qdev. Otherwise the following assert > > triggers: > > qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion > > `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)' > > failed. > > > > TYPE_XENBACKEND is not a subclass of XEN_XENSYSDEV, so it's parent > > is just TYPE_DEVICE. Change that. > > > > Signed-off-by: Jason Andryuk > > Clearly we raced. This patch and my patch #1 are identical so I'm happy to > give my ack to this. Yeah, looks like you beat me by a hair :) Either way it gets fixed is fine by me. Thanks, Jason
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
Hi, On 24/06/2020 13:08, Jan Beulich wrote: On 24.06.2020 12:52, Julien Grall wrote: Hi Jan, On 24/06/2020 11:05, Jan Beulich wrote: On 23.06.2020 19:32, Roger Pau Monné wrote: On Tue, Jun 23, 2020 at 05:04:53PM +0200, Jan Beulich wrote: On 23.06.2020 15:52, Roger Pau Monne wrote: XENMEM_acquire_resource and it's related structure is currently inside a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the hypervisor or the toolstack only. This is wrong as the hypercall is already being used by the Linux kernel at least, and as such needs to be public. Actually - how does this work for the Linux kernel, seeing rc = rcu_lock_remote_domain_by_id(xmar.domid, ); if ( rc ) return rc; rc = xsm_domain_resource_map(XSM_DM_PRIV, d); if ( rc ) goto out; in the function? It's my understanding (I haven't tried to use that hypercall yet on FreeBSD, so I cannot say I've tested it), that xmar.domid is the remote domain, which the functions locks and then uses xsm_domain_resource_map to check whether the current domain has permissions to do privileged operations against it. Yes, but that's a tool stack operation, not something the kernel would do all by itself. The kernel would only ever pass DOMID_SELF (or the actual local domain ID), I would think. You can't issue that hypercall directly from userspace because you need to map the page in the physical address space of the toolstack domain. So the kernel has to act as the proxy for the hypercall. This is implemented as mmap() in Linux. Oh, and there's no generic wrapping available here, unlike for dmop. It is not clear to me the sort of generic wrapping you are referring to. Are you referring to a stable interface for an application? Makes me wonder whether, for this purpose, there should be (have been) a new dmop with identical functionality, to allow such funneling. I am not sure how using DMOP will allow us to implement it fully in userspace. Do you mind expanding it? Cheers, -- Julien Grall
[xen-unstable test] 151311: tolerable FAIL
flight 151311 xen-unstable real [real] http://logs.test-lab.xenproject.org/osstest/logs/151311/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 151290 test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 151290 test-armhf-armhf-xl-rtds 16 guest-start/debian.repeatfail like 151290 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 151290 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 151290 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 151290 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 151290 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 151290 test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stopfail like 151290 test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail like 151290 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-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 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-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-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-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-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail 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-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass version targeted for testing: xen fde76f895d0aa817a1207d844d793239c6639bc6 baseline version: xen fde76f895d0aa817a1207d844d793239c6639bc6 Last test of basis 151311 2020-06-23 10:16:25 Z1 days Testing same since (not found) 0 attempts jobs: build-amd64-xsm pass build-arm64-xsm pass build-i386-xsm pass build-amd64-xtf pass build-amd64
Re: ARM - Successful install on RockPro64
On 17/06/2020 23:28, Richard Simpson wrote: Hello Julien, Hello Richard, Apologies for the late answer. I have just tried 4.14-rc2 and it seems to work fine. Glad to hear that. Thank you for the testing! I think that the most useful page regarding the board is the one for the Ibox3399 since this refers to the RK3399 chip which the RockPro64 uses (shouldn't the page actually be called RK3399 to make it more generic). I agree with the renaming here. Perhaps I can most usefully record what I did by updating that page and making sure that the instructions work correctly. If there is additional stuff relevant to the RockPro64 over and above the generic RK3399 info then I'll give some thought to how to best record it. I will eventually be writing a fuller report on my progress on my blog at funfoodfreedom.huskydog.org.uk. Any additional content on the wiki will be greatly appreciated. By default new wiki account doesn't have write permission, but we can enable it for you if you provide us your username. I now need to finish automating the boot process (still requires manual u-boot command) and figure out how to get the console log to work. I wrote a small u-boot script in the past to try to automate the boot (see [2]). I vaguely remember some quoting issue and missing 0x in front of values depending on the U-boot configuration you use. So you may have to tweak it a bit. Currently I can either see the xen and linux kernel boot messages OR see the dom0 console, but not both. Can you provide the kernel/xen command lines you use in the two cases? As an aside, I know that on some setup Linux will try to disable the clock of the UART used by Xen. One of the symptoms is the UART is becoming completely unusable half way through Linux boot. You may want to try to pass clk_ignored_unused to see if it helps. On one more related note: I suspect that Xen would run on the PineBookPro as well as I get the impression that it uses very similar hardware. Of course that would rely on the GPU etc which I haven't tested at all as I am using the serial console. I wouldn't expect any issue to use the GPU in dom0 at least if you don't have an IOMMU on the platform. The trouble may be more with the bootloader if it doesn't drop you in hypervisor mode. Finally, when I joined this mailing list I asked for a daily digest. However I seem to be getting a new digest every hour or so. Is this right? I haven't used the digest myself. I CC Ian Jackson who may be able to help you. Cheers, [2] https://xenbits.xen.org/people/julieng/load-xen-tftp.scr.txt -- Julien Grall
Re: [PATCH v2 4/7] x86/vmx: add do_vmtrace_op
On 24/06/2020 11:03, Jan Beulich wrote: > On 23.06.2020 19:24, Andrew Cooper wrote: >> On 23/06/2020 09:51, Jan Beulich wrote: >>> On 23.06.2020 03:04, Michał Leszczyński wrote: - 22 cze 2020 o 18:16, Jan Beulich jbeul...@suse.com napisał(a): > On 22.06.2020 18:02, Michał Leszczyński wrote: >> - 22 cze 2020 o 17:22, Jan Beulich jbeul...@suse.com napisał(a): >>> On 22.06.2020 16:35, Michał Leszczyński wrote: - 22 cze 2020 o 15:25, Jan Beulich jbeul...@suse.com napisał(a): > Is any of what you do in this switch() actually legitimate without > hvm_set_vmtrace_pt_size() having got called for the guest? From > remarks elsewhere I imply you expect the param that you currently > use to be set upon domain creation time, but at the very least the > potentially big buffer should imo not get allocated up front, but > only when tracing is to actually be enabled. Wait... so you want to allocate these buffers in runtime? Previously we were talking that there is too much runtime logic and these enable/disable hypercalls should be stripped to absolute minimum. >>> Basic arrangements can be made at domain creation time. I don't >>> think though that it would be a good use of memory if you >>> allocated perhaps many gigabytes of memory just for possibly >>> wanting to enable tracing on a guest. >>> >> From our previous conversations I thought that you want to have >> as much logic moved to the domain creation as possible. >> >> Thus, a parameter "vmtrace_pt_size" was introduced. By default it's >> zero (= disabled), if you set it to a non-zero value, then trace buffers >> of given size will be allocated for the domain and you have possibility >> to use ipt_enable/ipt_disable at any moment. >> >> This way the runtime logic is as thin as possible. I assume user knows >> in advance whether he/she would want to use external monitoring with IPT >> or not. > Andrew - I think you requested movement to domain_create(). Could > you clarify if indeed you mean to also allocate the big buffers > this early? I would like to recall what Andrew wrote few days ago: - 16 cze 2020 o 22:16, Andrew Cooper andrew.coop...@citrix.com wrote: > Xen has traditionally opted for a "and turn this extra thing on > dynamically" model, but this has caused no end of security issues and > broken corner cases. > > You can see this still existing in the difference between > XEN_DOMCTL_createdomain and XEN_DOMCTL_max_vcpus, (the latter being > required to chose the number of vcpus for the domain) and we're making > good progress undoing this particular wart (before 4.13, it was > concerning easy to get Xen to fall over a NULL d->vcpu[] pointer by > issuing other hypercalls between these two). > > There is a lot of settings which should be immutable for the lifetime of > the domain, and external monitoring looks like another one of these. > Specifying it at createdomain time allows for far better runtime > behaviour (you are no longer in a situation where the first time you try > to turn tracing on, you end up with -ENOMEM because another VM booted in > the meantime and used the remaining memory), and it makes for rather > more simple code in Xen itself (at runtime, you can rely on it having > been set up properly, because a failure setting up will have killed the > domain already). > > ... > > ~Andrew according to this quote I've moved buffer allocation to the create_domain, the updated version was already sent to the list as patch v3. >>> I'd still like to see an explicit confirmation by him that this >>> use of memory is indeed what he has intended. There are much smaller >>> amounts of memory which we allocate on demand, just to avoid >>> allocating some without then ever using it. >> PT is a debug/diagnostic tool. Its not something you'd run in >> production against a production VM. > Well, the suggested use with introspection made me assume differently. > If this is (now and forever) truly debug/diag only, so be it then. At the end of the day, it is a fine grain profiling tool, meaning that it is not used in the common case. The usecase presented is for fuzzing, using the execution trace generated by the CPU, rather than the current scheme which allegedly involves shuffling breakpoints around. There is a big disclaimer with PT saying "there is a perf hit from using this". This is a consequence of the core suddenly becoming far more memory bound, and almost certainly being capable of generating trace data faster than can be written out. Even if it does find its way into some fairly custom production scenarios (fuzzing as a service?), we're still in the position where the only people who enable this will be the people intending
RE: [PATCH] xen: Fix xen-legacy-backend qdev types
> -Original Message-
> From: Jason Andryuk
> Sent: 24 June 2020 13:20
> To: Stefano Stabellini ; Anthony Perard
> ; Paul
> Durrant ; xen-devel@lists.xenproject.org
> Cc: Jason Andryuk ; qemu-de...@nongnu.org
> Subject: [PATCH] xen: Fix xen-legacy-backend qdev types
>
> xen-sysdev is a TYPE_SYS_BUS_DEVICE. bus_type should not be changed so
> that it can plug into the System bus. Otherwise this assert triggers:
> qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
> `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
> failed.
>
> TYPE_XENBACKEND attaches to TYPE_XENSYSBUS, so its class_init needs to
> be set accordingly to attach the qdev. Otherwise the following assert
> triggers:
> qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
> `dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
> failed.
>
> TYPE_XENBACKEND is not a subclass of XEN_XENSYSDEV, so it's parent
> is just TYPE_DEVICE. Change that.
>
> Signed-off-by: Jason Andryuk
Clearly we raced. This patch and my patch #1 are identical so I'm happy to give
my ack to this.
Paul
> ---
> hw/xen/xen-legacy-backend.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
> index 2335ee2e65..c5c75c0064 100644
> --- a/hw/xen/xen-legacy-backend.c
> +++ b/hw/xen/xen-legacy-backend.c
> @@ -789,11 +789,12 @@ static void xendev_class_init(ObjectClass *klass, void
> *data)
> set_bit(DEVICE_CATEGORY_MISC, dc->categories);
> /* xen-backend devices can be plugged/unplugged dynamically */
> dc->user_creatable = true;
> +dc->bus_type = TYPE_XENSYSBUS;
> }
>
> static const TypeInfo xendev_type_info = {
> .name = TYPE_XENBACKEND,
> -.parent= TYPE_XENSYSDEV,
> +.parent= TYPE_DEVICE,
> .class_init= xendev_class_init,
> .instance_size = sizeof(struct XenLegacyDevice),
> };
> @@ -824,7 +825,6 @@ static void xen_sysdev_class_init(ObjectClass *klass,
> void *data)
> DeviceClass *dc = DEVICE_CLASS(klass);
>
> device_class_set_props(dc, xen_sysdev_properties);
> -dc->bus_type = TYPE_XENSYSBUS;
> }
>
> static const TypeInfo xensysdev_info = {
> --
> 2.25.1
Re: [PATCH v2 4/7] x86/vmx: add do_vmtrace_op
- 23 cze 2020 o 19:24, Andrew Cooper andrew.coop...@citrix.com napisał(a): > On 23/06/2020 09:51, Jan Beulich wrote: >> I'd still like to see an explicit confirmation by him that this >> use of memory is indeed what he has intended. There are much smaller >> amounts of memory which we allocate on demand, just to avoid >> allocating some without then ever using it. > > PT is a debug/diagnostic tool. Its not something you'd run in > production against a production VM. > > It's off by default (by virtue of having to explicitly ask to use it in > the first place), and those who've asked for it don't want to be finding > -ENOMEM after the domain has been running for a few seconds (or midway > through the vcpus), when they inveterately want to map the rings. > > Those who request buffers in the first place and forget about them are > not semantically different from those who ask for a silly shadow memory > limit, or typo the guest memory and give it too much. Its a admin > error, not a safety/correctness issue. > > ~Andrew Absolutely +1. Assuming that somebody wants to perform some advanced scenario and is trying to run many domains (e.g. 20), it's much better to have 19 domains working fine and 1 prematurely crashing because of -ENOMEM, rather than have all 20 domains randomly crashing in runtime because it turned out there is a shortage of memory. Best regards, Michał Leszczyński CERT Polska
[PATCH] xen: Fix xen-legacy-backend qdev types
xen-sysdev is a TYPE_SYS_BUS_DEVICE. bus_type should not be changed so
that it can plug into the System bus. Otherwise this assert triggers:
qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
`dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
failed.
TYPE_XENBACKEND attaches to TYPE_XENSYSBUS, so its class_init needs to
be set accordingly to attach the qdev. Otherwise the following assert
triggers:
qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
`dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
failed.
TYPE_XENBACKEND is not a subclass of XEN_XENSYSDEV, so it's parent
is just TYPE_DEVICE. Change that.
Signed-off-by: Jason Andryuk
---
hw/xen/xen-legacy-backend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 2335ee2e65..c5c75c0064 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -789,11 +789,12 @@ static void xendev_class_init(ObjectClass *klass, void
*data)
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
/* xen-backend devices can be plugged/unplugged dynamically */
dc->user_creatable = true;
+dc->bus_type = TYPE_XENSYSBUS;
}
static const TypeInfo xendev_type_info = {
.name = TYPE_XENBACKEND,
-.parent= TYPE_XENSYSDEV,
+.parent= TYPE_DEVICE,
.class_init= xendev_class_init,
.instance_size = sizeof(struct XenLegacyDevice),
};
@@ -824,7 +825,6 @@ static void xen_sysdev_class_init(ObjectClass *klass, void
*data)
DeviceClass *dc = DEVICE_CLASS(klass);
device_class_set_props(dc, xen_sysdev_properties);
-dc->bus_type = TYPE_XENSYSBUS;
}
static const TypeInfo xensysdev_info = {
--
2.25.1
[PATCH 2/2] xen: cleanup unrealized flash devices
From: Paul Durrant
The generic pc_machine_initfn() calls pc_system_flash_create() which creates
'system.flash0' and 'system.flash1' devices. These devices are then realized
by pc_system_flash_map() which is called from pc_system_firmware_init() which
itself is called via pc_memory_init(). The latter however is not called when
xen_enable() is true and hence the following assertion fails:
qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
Assertion `dev->realized' failed
These flash devices are unneeded when using Xen so this patch avoids the
assertion by simply removing them using pc_system_flash_cleanup_unused().
Reported-by: Jason Andryuk
Fixes: ebc29e1beab0 ("pc: Support firmware configuration with -blockdev")
Signed-off-by: Paul Durrant
Tested-by: Jason Andryuk
---
Cc: Paolo Bonzini
Cc: Richard Henderson
Cc: Eduardo Habkost
Cc: "Michael S. Tsirkin"
Cc: Marcel Apfelbaum
---
hw/i386/pc_piix.c| 9 ++---
hw/i386/pc_sysfw.c | 2 +-
include/hw/i386/pc.h | 1 +
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1497d0e4ae..977d40afb8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
if (!xen_enabled()) {
pc_memory_init(pcms, system_memory,
rom_memory, _memory);
-} else if (machine->kernel_filename != NULL) {
-/* For xen HVM direct kernel boot, load linux here */
-xen_load_linux(pcms);
+} else {
+pc_system_flash_cleanup_unused(pcms);
+if (machine->kernel_filename != NULL) {
+/* For xen HVM direct kernel boot, load linux here */
+xen_load_linux(pcms);
+}
}
gsi_state = pc_gsi_create(>gsi, pcmc->pci_enabled);
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index ec2a3b3e7e..0ff47a4b59 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
}
}
-static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
+void pc_system_flash_cleanup_unused(PCMachineState *pcms)
{
char *prop_name;
int i;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e6135c34d6..497f2b7ab7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
/* pc_sysfw.c */
void pc_system_flash_create(PCMachineState *pcms);
+void pc_system_flash_cleanup_unused(PCMachineState *pcms);
void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
/* acpi-build.c */
--
2.20.1
[PATCH 1/2] xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types
From: Paul Durrant
'xen-sysdev' plugs into the 'System' bus type, not 'xen-sysbus. That bus type
is what 'xen-backend' plugs into.
'xen-sysdev' is drived form 'sys-bus-device' so the bus type need not be
overridden. 'xen-backend' is derived from 'device', which plugs into the
generic 'bus' type, so its bus type should be overridden to 'xen-sysbus'.
Without this patch, the following assertion will fail:
qemu-system-i386: hw/core/qdev.c:102: qdev_set_parent_bus: Assertion
`dc->bus_type && object_dynamic_cast(OBJECT(bus), dc->bus_type)'
failed.
Reported-by: Jason Andryuk
Fixes: 81cb05732efb ("qdev: Assert devices are plugged into a bus that can take
them")
Signed-off-by: Paul Durrant
---
Cc: Stefano Stabellini
Cc: Anthony Perard
---
hw/xen/xen-legacy-backend.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/hw/xen/xen-legacy-backend.c b/hw/xen/xen-legacy-backend.c
index 2335ee2e65..c5c75c0064 100644
--- a/hw/xen/xen-legacy-backend.c
+++ b/hw/xen/xen-legacy-backend.c
@@ -789,11 +789,12 @@ static void xendev_class_init(ObjectClass *klass, void
*data)
set_bit(DEVICE_CATEGORY_MISC, dc->categories);
/* xen-backend devices can be plugged/unplugged dynamically */
dc->user_creatable = true;
+dc->bus_type = TYPE_XENSYSBUS;
}
static const TypeInfo xendev_type_info = {
.name = TYPE_XENBACKEND,
-.parent= TYPE_XENSYSDEV,
+.parent= TYPE_DEVICE,
.class_init= xendev_class_init,
.instance_size = sizeof(struct XenLegacyDevice),
};
@@ -824,7 +825,6 @@ static void xen_sysdev_class_init(ObjectClass *klass, void
*data)
DeviceClass *dc = DEVICE_CLASS(klass);
device_class_set_props(dc, xen_sysdev_properties);
-dc->bus_type = TYPE_XENSYSBUS;
}
static const TypeInfo xensysdev_info = {
--
2.20.1
[PATCH 0/2] fix assertion failures when using Xen
From: Paul Durrant Paul Durrant (2): xen: fix legacy 'xen-sysdev' and 'xen-backend' bus types xen: cleanup unrealized flash devices hw/i386/pc_piix.c | 9 ++--- hw/i386/pc_sysfw.c | 2 +- hw/xen/xen-legacy-backend.c | 4 ++-- include/hw/i386/pc.h| 1 + 4 files changed, 10 insertions(+), 6 deletions(-) -- 2.20.1
RE: sysbus failed assert for xen_sysdev
> -Original Message-
> From: Jason Andryuk
> Sent: 24 June 2020 13:15
> To: Paul Durrant
> Cc: Markus Armbruster ; Mark Cave-Ayland
> ; Anthony
> PERARD ; xen-devel
> ; QEMU de...@nongnu.org>
> Subject: Re: sysbus failed assert for xen_sysdev
>
> On Wed, Jun 24, 2020 at 6:29 AM Paul Durrant wrote:
> >
> > > -Original Message-
> > > From: Jason Andryuk
> > > Sent: 24 June 2020 04:24
> > > To: Paul Durrant
> > > Cc: Markus Armbruster ; Mark Cave-Ayland
> > > ;
> Anthony
> > > PERARD ; xen-devel
> > > ; QEMU > > de...@nongnu.org>
> > > Subject: Re: sysbus failed assert for xen_sysdev
> > >
> > > On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant wrote:
> > > >
> > > > > -Original Message-
> > > > > From: Markus Armbruster
> > > > > Sent: 23 June 2020 09:41
> > > > > To: Jason Andryuk
> > > > > Cc: Mark Cave-Ayland ; Anthony PERARD
> ;
> > > xen-
> > > > > devel ; Paul Durrant ;
> > > > > QEMU de...@nongnu.org>
> > > > > Subject: Re: sysbus failed assert for xen_sysdev
> > > > >
> > > > > Jason Andryuk writes:
> > > > > > Then it gets farther... until
> > > > > > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > > > > > Assertion `dev->realized' failed.
> > > > > >
> > > > > > dev->id is NULL. The failing device is:
> > > > > > (gdb) p *dev.parent_obj.class.type
> > > > > > $12 = {name = 0x56207770 "cfi.pflash01",
> > > > > >
> > > >
> > > > Having commented out the call to xen_be_init() entirely (and
> > > > xen_bus_init() for good measure) I
> also
> > > get this assertion failure, so
> > > > I don't think is related.
> > >
> > > Yes, this is something different. pc_pflash_create() calls
> > > qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
> > > pc_system_flash_map()... and pc_system_flash_map() isn't called for
> > > Xen.
> > >
> > > Removing the call to pc_system_flash_create() from pc_machine_initfn()
> > > lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
> > > there since accelerators have not been initialized yes, I guess?
> >
> > Looks like it can be worked round by the following:
>
> Yes, thank you.
>
> Tested-by: Jason Andryuk
Thanks.
Paul
>
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index 1497d0e4ae..977d40afb8 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> > if (!xen_enabled()) {
> > pc_memory_init(pcms, system_memory,
> > rom_memory, _memory);
> > -} else if (machine->kernel_filename != NULL) {
> > -/* For xen HVM direct kernel boot, load linux here */
> > -xen_load_linux(pcms);
> > +} else {
> > +pc_system_flash_cleanup_unused(pcms);
> > +if (machine->kernel_filename != NULL) {
> > +/* For xen HVM direct kernel boot, load linux here */
> > +xen_load_linux(pcms);
> > +}
> > }
> >
> > gsi_state = pc_gsi_create(>gsi, pcmc->pci_enabled);
> > diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> > index ec2a3b3e7e..0ff47a4b59 100644
> > --- a/hw/i386/pc_sysfw.c
> > +++ b/hw/i386/pc_sysfw.c
> > @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
> > }
> > }
> >
> > -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> > {
> > char *prop_name;
> > int i;
> > diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> > index e6135c34d6..497f2b7ab7 100644
> > --- a/include/hw/i386/pc.h
> > +++ b/include/hw/i386/pc.h
> > @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
> >
> > /* pc_sysfw.c */
> > void pc_system_flash_create(PCMachineState *pcms);
> > +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
> > void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion
> > *rom_memory);
> >
> > /* acpi-build.c */
> >
> >
> > >
> > > Regards,
> > > Jason
> >
Re: sysbus failed assert for xen_sysdev
On Wed, Jun 24, 2020 at 6:29 AM Paul Durrant wrote:
>
> > -Original Message-
> > From: Jason Andryuk
> > Sent: 24 June 2020 04:24
> > To: Paul Durrant
> > Cc: Markus Armbruster ; Mark Cave-Ayland
> > ; Anthony
> > PERARD ; xen-devel
> > ; QEMU > de...@nongnu.org>
> > Subject: Re: sysbus failed assert for xen_sysdev
> >
> > On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant wrote:
> > >
> > > > -Original Message-
> > > > From: Markus Armbruster
> > > > Sent: 23 June 2020 09:41
> > > > To: Jason Andryuk
> > > > Cc: Mark Cave-Ayland ; Anthony PERARD
> > > > ;
> > xen-
> > > > devel ; Paul Durrant ;
> > > > QEMU
> > > > Subject: Re: sysbus failed assert for xen_sysdev
> > > >
> > > > Jason Andryuk writes:
> > > > > Then it gets farther... until
> > > > > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > > > > Assertion `dev->realized' failed.
> > > > >
> > > > > dev->id is NULL. The failing device is:
> > > > > (gdb) p *dev.parent_obj.class.type
> > > > > $12 = {name = 0x56207770 "cfi.pflash01",
> > > > >
> > >
> > > Having commented out the call to xen_be_init() entirely (and
> > > xen_bus_init() for good measure) I also
> > get this assertion failure, so
> > > I don't think is related.
> >
> > Yes, this is something different. pc_pflash_create() calls
> > qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
> > pc_system_flash_map()... and pc_system_flash_map() isn't called for
> > Xen.
> >
> > Removing the call to pc_system_flash_create() from pc_machine_initfn()
> > lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
> > there since accelerators have not been initialized yes, I guess?
>
> Looks like it can be worked round by the following:
Yes, thank you.
Tested-by: Jason Andryuk
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1497d0e4ae..977d40afb8 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
> if (!xen_enabled()) {
> pc_memory_init(pcms, system_memory,
> rom_memory, _memory);
> -} else if (machine->kernel_filename != NULL) {
> -/* For xen HVM direct kernel boot, load linux here */
> -xen_load_linux(pcms);
> +} else {
> +pc_system_flash_cleanup_unused(pcms);
> +if (machine->kernel_filename != NULL) {
> +/* For xen HVM direct kernel boot, load linux here */
> +xen_load_linux(pcms);
> +}
> }
>
> gsi_state = pc_gsi_create(>gsi, pcmc->pci_enabled);
> diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
> index ec2a3b3e7e..0ff47a4b59 100644
> --- a/hw/i386/pc_sysfw.c
> +++ b/hw/i386/pc_sysfw.c
> @@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
> }
> }
>
> -static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> +void pc_system_flash_cleanup_unused(PCMachineState *pcms)
> {
> char *prop_name;
> int i;
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index e6135c34d6..497f2b7ab7 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
>
> /* pc_sysfw.c */
> void pc_system_flash_create(PCMachineState *pcms);
> +void pc_system_flash_cleanup_unused(PCMachineState *pcms);
> void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>
> /* acpi-build.c */
>
>
> >
> > Regards,
> > Jason
>
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
On 24.06.2020 12:52, Julien Grall wrote: > Hi Jan, > > On 24/06/2020 11:05, Jan Beulich wrote: >> On 23.06.2020 19:32, Roger Pau Monné wrote: >>> On Tue, Jun 23, 2020 at 05:04:53PM +0200, Jan Beulich wrote: On 23.06.2020 15:52, Roger Pau Monne wrote: > XENMEM_acquire_resource and it's related structure is currently inside > a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the > hypervisor or the toolstack only. This is wrong as the hypercall is > already being used by the Linux kernel at least, and as such needs to > be public. Actually - how does this work for the Linux kernel, seeing rc = rcu_lock_remote_domain_by_id(xmar.domid, ); if ( rc ) return rc; rc = xsm_domain_resource_map(XSM_DM_PRIV, d); if ( rc ) goto out; in the function? >>> >>> It's my understanding (I haven't tried to use that hypercall yet on >>> FreeBSD, so I cannot say I've tested it), that xmar.domid is the >>> remote domain, which the functions locks and then uses >>> xsm_domain_resource_map to check whether the current domain has >>> permissions to do privileged operations against it. >> >> Yes, but that's a tool stack operation, not something the kernel >> would do all by itself. The kernel would only ever pass DOMID_SELF >> (or the actual local domain ID), I would think. > > You can't issue that hypercall directly from userspace because you need > to map the page in the physical address space of the toolstack domain. > > So the kernel has to act as the proxy for the hypercall. This is > implemented as mmap() in Linux. Oh, and there's no generic wrapping available here, unlike for dmop. Makes me wonder whether, for this purpose, there should be (have been) a new dmop with identical functionality, to allow such funneling. Janan
Re: [INPUT REQUESTED][PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches
Julien Grall writes ("Re: [INPUT REQUESTED][PATCH v3 for-4.14] pvcalls:
Document correctly and explicitely the padding for all arches"):
> Gentle ping. It would be good to get this resolved for Xen 4.14.
Oh and I agree that this should be changed for 4.14.
Ian.
Re: [PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches
Julien Grall writes ("Re: [PATCH v3 for-4.14] pvcalls: Document correctly and
explicitely the padding for all arches"):
> (+ Committers)
...
> As Jan and you disagree on the approach, I would like to get more input.
>
> To summarize the discussion, the document for PV calls and the public
> headers don't match when describing the padding. There is a disagreement
> on which of the two are the authority and therefore which one to fix.
>
> Does anyone else have a preference on the approach?
Hi.
>[Jan:]
>> I am leaning towards the header as authoritative because this has
>> always been the case in the past and nothing in xen.git says
>> otherwise. However I am not a user of pvcalls, so I don't really have
>> any big incentive to go either way.
I think the practice of using headers as protocol specs is not a
particularly good one. Certainly my expectations anywhere outside the
Xen Project is that if there's a doc, that is at the very least on par
with any header file. Of course there are possible compatibility
implications:
> Yeah, we are risking breaking one set of users either way :-/
> In reality, we are using pvcalls on arm64 in a new project (but it is
> still very green). I am not aware of anybody using pvcalls on x86
> (especially x86_32).
>
> I would prefer to honor the pvcalls.pandoc specification because that is
> what it was meant to be, and also leads to a better protocol
> specification.
pvcalls in Linux is Tech Preview / Experimental AFAICT ? I think that
means we can de jure change things like this.
And it seems that we don't think there are any actual users who would
experience compatibility problems.
So I don't think the compatibility concerns are a reason not to change
the header rather than the document.
So I think my conclusion is the same as Julien's: we should change the
header to match the doc.
> >> For the future, I would highly suggest writing down the support
> >> decision in xen.git. This would avoid such debate on what is the
> >> authority...
> >
> > Yes that's the way to go
Maybe it would be worth putting a note somewhere in the headers saying
the headers are provided for convenience but that the ABIs and
protocols are as specified in the docs (at least where docs exist).
Ian.
Re: [INPUT REQUESTED][PATCH v3 for-4.14] pvcalls: Document correctly and explicitely the padding for all arches
Hi Gentle ping. It would be good to get this resolved for Xen 4.14. On 18/06/2020 16:00, Julien Grall wrote: (+ Committers) On 18/06/2020 02:34, Stefano Stabellini wrote: On Tue, 16 Jun 2020, Julien Grall wrote: On Tue, 16 Jun 2020 at 21:57, Stefano Stabellini wrote: On Tue, 16 Jun 2020, Julien Grall wrote: On 16/06/2020 02:00, Stefano Stabellini wrote: On Sat, 13 Jun 2020, Julien Grall wrote: From: Julien Grall The documentation of pvcalls suggests there is padding for 32-bit x86 at the end of most the structure. However, they are not described in in the public header. Because of that all the structures would be 32-bit aligned and not 64-bit aligned for 32-bit x86. For all the other architectures supported (Arm and 64-bit x86), the structure are aligned to 64-bit because they contain uint64_t field. Therefore all the structures contain implicit padding. The paddings are now corrected for 32-bit x86 and written explicitly for all the architectures. While the structure size between 32-bit and 64-bit x86 is different, it shouldn't cause any incompatibility between a 32-bit and 64-bit frontend/backend because the commands are always 56 bits and the padding are at the end of the structure. As an aside, the padding sadly cannot be mandated to be 0 as they are already present. So it is not going to be possible to use the padding for extending a command in the future. Signed-off-by: Julien Grall --- Changes in v3: - Use __i386__ rather than CONFIG_X86_32 Changes in v2: - It is not possible to use the same padding for 32-bit x86 and all the other supported architectures. --- docs/misc/pvcalls.pandoc | 18 ++ xen/include/public/io/pvcalls.h | 14 ++ 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/docs/misc/pvcalls.pandoc b/docs/misc/pvcalls.pandoc index 665dad556c39..caa71b36d78b 100644 --- a/docs/misc/pvcalls.pandoc +++ b/docs/misc/pvcalls.pandoc @@ -246,9 +246,9 @@ The format is defined as follows: uint32_t domain; uint32_t type; uint32_t protocol; - #ifdef CONFIG_X86_32 + #ifndef __i386__ uint8_t pad[4]; - #endif + #endif Hi Julien, Thank you for doing this, and sorry for having missed v2 of this patch, I should have replied earlier. The intention of the #ifdef blocks like: #ifdef CONFIG_X86_32 uint8_t pad[4]; #endif in pvcalls.pandoc was to make sure that these structs would be 64bit aligned on x86_32 too. I realize that the public header doesn't match, but the spec is the "master copy". So far, the public headers are the defacto official ABI. So did you mark the pvcall header as just a reference? No, there is no document that says that the canonical copy of the interface is pvcalls.pandoc. However, it was clearly spelled out from the start on xen-devel (see below.) In fact, if you notice, this is the first document under docs/misc that goes into this level of details in describing a new PV protocol. Also note the title of the document which is "PV Calls Protocol version 1". While I understand this may have been the original intention, you can't expect a developer to go through the archive to check whether he/she should trust the header of the document. In reply to Jan: A public header can't be "fixed" if it may already be in use by anyone. We can only do as Andrew and you suggest (mandate textual descriptions to be "the ABI") when we do so for _new_ interfaces from the very beginning, making clear that the public header (if any) exists just for reference. What if somebody took the specification of the interface from pvcalls.pandoc and wrote their own headers and code? It is definitely possible. As it is possible for someone to have picked the headers from Xen as in the past public/ has always been the authority. We never had documents under docs/ before specifying the interfaces before pvcalls. It is not written anywhere that the headers under public/ are the authoritative interfaces either, it is just that it was the only thing available before. If you are new to the project you might go to docs/ first. At the time, it was clarified that the purpose of writing such a detailed specification document was that the document was the specification :-) At the risk of being pedantic, if it is not written in xen.git it doesn't exist ;). Anyway, no matter the decision you take here, you are going to potentially break one set of the users. I am leaning towards the header as authoritative because this has always been the case in the past and nothing in xen.git says otherwise. However I am not a user of pvcalls, so I don't really have any big incentive to go either way. Yeah, we are risking breaking one set of users either way :-/ In reality, we are
Re: [PATCH for-4.14 v2] x86/tlb: fix assisted flush usage
Hi Roger, On 23/06/2020 17:16, Roger Pau Monné wrote: On Tue, Jun 23, 2020 at 04:46:29PM +0100, Julien Grall wrote: On 23/06/2020 16:15, Roger Pau Monné wrote: On Tue, Jun 23, 2020 at 04:08:06PM +0100, Julien Grall wrote: Hi Roger, On 23/06/2020 15:50, Roger Pau Monne wrote: diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 9b62087be1..f360166f00 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -639,7 +639,8 @@ static inline void accumulate_tlbflush(bool *need_tlbflush, } } -static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp) +static inline void filtered_flush_tlb_mask(uint32_t tlbflush_timestamp, + bool sync) I read the commit message and went through the code, but it is still not clear what "sync" means in a non-architectural way. As an Arm developper, I would assume this means we don't wait for the TLB flush to complete. But I am sure this would result to some unexpected behavior. No, when we return from filtered_flush_tlb_mask the flush has been performed (either with sync or without), but I understand the confusion given the parameter name. So can you explain on non-x86 words what this really mean? sync (in this context) means to force the usage of an IPI (if built with PV or shadow paging support) in order to perform the flush. This is compare to? Using assisted flushes, like you do on Arm, where you don't send an IPI in order to achieve a TLB flush on a remote pCPU. AFAICT, the assisted flushes only happen when running a nested Xen. Is that correct? AFAICT on Arm you always avoid an IPI when performing a flush, and that's fine because you don't have PV or shadow, and then you don't require this. Arm provides instructions to broadcast TLB flush, so by the time one of instruction is completed there is all the TLB entry associated to the translation doesn't exist. I don't think using PV or shadow would change anything here in the way we do the flush. TBH, I'm not sure how this applies to Arm. There's no PV or shadow implementation, so I have no idea whether this would apply or not. Yes there is none. However, my point was that if we had to implement PV/shadow on Arm then an IPI would definitely be my last choice. I could rename to force_ipi (or require_ipi) if that's better? Hmmm, based on what I wrote above, I don't think this name would be more suitable. However, regardless the name, it is not clear to me when a caller should use false or true. Have you considered a rwlock to synchronize the two? Yes, the performance drop is huge when I tried. I could try to refine, but I think there's always going to be a performance drop, as you then require mutual exclusion when modifying the page tables (you take the lock in write mode). Right now modification of the page tables can be done concurrently. Fair enough. I will scratch that suggestion then. Thanks for the explanation! So now getting back to filtered_flush_tlb(). AFAICT, the only two callers are in common code. The two are used for allocation purpose, so may I ask why they need to use different kind of flush? FWIW Xen explicitly moved from using a lock into this model in 3203345bb13 apparently due to some deadlock situation. I'm not sure if that still holds. The old classic major change with limited explanation :/. Cheers, -- Julien Grall
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
Hi Jan, On 24/06/2020 11:05, Jan Beulich wrote: On 23.06.2020 19:32, Roger Pau Monné wrote: On Tue, Jun 23, 2020 at 05:04:53PM +0200, Jan Beulich wrote: On 23.06.2020 15:52, Roger Pau Monne wrote: XENMEM_acquire_resource and it's related structure is currently inside a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the hypervisor or the toolstack only. This is wrong as the hypercall is already being used by the Linux kernel at least, and as such needs to be public. Actually - how does this work for the Linux kernel, seeing rc = rcu_lock_remote_domain_by_id(xmar.domid, ); if ( rc ) return rc; rc = xsm_domain_resource_map(XSM_DM_PRIV, d); if ( rc ) goto out; in the function? It's my understanding (I haven't tried to use that hypercall yet on FreeBSD, so I cannot say I've tested it), that xmar.domid is the remote domain, which the functions locks and then uses xsm_domain_resource_map to check whether the current domain has permissions to do privileged operations against it. Yes, but that's a tool stack operation, not something the kernel would do all by itself. The kernel would only ever pass DOMID_SELF (or the actual local domain ID), I would think. You can't issue that hypercall directly from userspace because you need to map the page in the physical address space of the toolstack domain. So the kernel has to act as the proxy for the hypercall. This is implemented as mmap() in Linux. Cheers, -- Julien Grall
Re: [xen-4.11-testing test] 151295: regressions - FAIL
Jan Beulich writes ("Re: [xen-4.11-testing test] 151295: regressions - FAIL"):
> On 23.06.2020 20:32, osstest service owner wrote:
> > flight 151295 xen-4.11-testing real [real]
> > http://logs.test-lab.xenproject.org/osstest/logs/151295/
> >
> > Regressions :-(
> >
> > Tests which did not succeed and are blocking,
> > including tests which could not be run:
> > build-amd64-prev 6 xen-build fail in 151260 REGR. vs.
> > 150040
> > build-i386-prev 6 xen-build fail in 151260 REGR. vs.
> > 150040
> >
> > Tests which are failing intermittently (not blocking):
> > test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 10 debian-hvm-install fail
> > pass in 151260
> >
> > Tests which did not succeed, but are not blocking:
>
> I'm once again struggling to see why there was no push here: The
> latter two groups both say "not blocking", and the two build-*-prev
> didn't actually fail here, but in an earlier flight. Without
> understanding the reason here I'm hesitant to suggest a force push,
> though.
osstest is using the earlier flight (151260) to justify considering
pushing despite test-amd64-amd64-xl-qemut-debianhvm-i386-xsm but that
means it wants a justification for all the failures in 151260 too.
Broadly speaking, this logic is necessary because failed tests can
block other tests.
I think this will succeed on the next run.
Ian.
RE: sysbus failed assert for xen_sysdev
> -Original Message-
> From: Jason Andryuk
> Sent: 24 June 2020 04:24
> To: Paul Durrant
> Cc: Markus Armbruster ; Mark Cave-Ayland
> ; Anthony
> PERARD ; xen-devel
> ; QEMU de...@nongnu.org>
> Subject: Re: sysbus failed assert for xen_sysdev
>
> On Tue, Jun 23, 2020 at 7:46 AM Paul Durrant wrote:
> >
> > > -Original Message-
> > > From: Markus Armbruster
> > > Sent: 23 June 2020 09:41
> > > To: Jason Andryuk
> > > Cc: Mark Cave-Ayland ; Anthony PERARD
> > > ;
> xen-
> > > devel ; Paul Durrant ; QEMU
> > >
> > > Subject: Re: sysbus failed assert for xen_sysdev
> > >
> > > Jason Andryuk writes:
> > > > Then it gets farther... until
> > > > qemu-system-i386: hw/core/qdev.c:439: qdev_assert_realized_properly:
> > > > Assertion `dev->realized' failed.
> > > >
> > > > dev->id is NULL. The failing device is:
> > > > (gdb) p *dev.parent_obj.class.type
> > > > $12 = {name = 0x56207770 "cfi.pflash01",
> > > >
> >
> > Having commented out the call to xen_be_init() entirely (and xen_bus_init()
> > for good measure) I also
> get this assertion failure, so
> > I don't think is related.
>
> Yes, this is something different. pc_pflash_create() calls
> qdev_new(TYPE_PFLASH_CFI01), but it is only realized in
> pc_system_flash_map()... and pc_system_flash_map() isn't called for
> Xen.
>
> Removing the call to pc_system_flash_create() from pc_machine_initfn()
> lets QEMU startup and run a Xen HVM again. xen_enabled() doesn't work
> there since accelerators have not been initialized yes, I guess?
Looks like it can be worked round by the following:
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1497d0e4ae..977d40afb8 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -186,9 +186,12 @@ static void pc_init1(MachineState *machine,
if (!xen_enabled()) {
pc_memory_init(pcms, system_memory,
rom_memory, _memory);
-} else if (machine->kernel_filename != NULL) {
-/* For xen HVM direct kernel boot, load linux here */
-xen_load_linux(pcms);
+} else {
+pc_system_flash_cleanup_unused(pcms);
+if (machine->kernel_filename != NULL) {
+/* For xen HVM direct kernel boot, load linux here */
+xen_load_linux(pcms);
+}
}
gsi_state = pc_gsi_create(>gsi, pcmc->pci_enabled);
diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index ec2a3b3e7e..0ff47a4b59 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -108,7 +108,7 @@ void pc_system_flash_create(PCMachineState *pcms)
}
}
-static void pc_system_flash_cleanup_unused(PCMachineState *pcms)
+void pc_system_flash_cleanup_unused(PCMachineState *pcms)
{
char *prop_name;
int i;
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index e6135c34d6..497f2b7ab7 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -187,6 +187,7 @@ int cmos_get_fd_drive_type(FloppyDriveType fd0);
/* pc_sysfw.c */
void pc_system_flash_create(PCMachineState *pcms);
+void pc_system_flash_cleanup_unused(PCMachineState *pcms);
void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
/* acpi-build.c */
>
> Regards,
> Jason
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
On 23.06.2020 19:26, Roger Pau Monné wrote:
> On Tue, Jun 23, 2020 at 06:18:52PM +0200, Jan Beulich wrote:
>> On 23.06.2020 17:56, Roger Pau Monné wrote:
>>> On Tue, Jun 23, 2020 at 05:02:04PM +0200, Jan Beulich wrote:
On 23.06.2020 15:52, Roger Pau Monne wrote:
> XENMEM_acquire_resource and it's related structure is currently inside
> a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the
> hypervisor or the toolstack only. This is wrong as the hypercall is
> already being used by the Linux kernel at least, and as such needs to
> be public.
>
> Also switch the usage of uint64_aligned_t to plain uint64_t, as
> uint64_aligned_t is only to be used by the toolstack. Note that the
> layout of the structure will be the same, as the field is already
> naturally aligned to a 8 byte boundary.
I'm afraid it's more complicated, and hence ...
> No functional change expected.
... this doesn't hold: As I notice only now the struct also wrongly
(if it was meant to be a tools-only interface) failed to use
XEN_GUEST_HANDLE_64(), which would in principle have been a problem
for 32-bit callers (passing garbage in the upper half of a handle).
It's not an actual problem because, unlike would typically be the
case for tools-only interfaces, there is compat translation for this
struct.
>>>
>>> Yes, there's already compat translation for the frame_list array.
>>>
With this, however, the problem of your change becomes noticeable:
The structure's alignment for 32-bit x86, and with it the structure's
sizeof(), both change. You should be able to see this by comparing
xen/common/compat/memory.c's disassembly before and after your
change - the number of bytes to copy by the respective
copy_from_guest() ought to have changed.
>>>
>>> Right, this is the layout with frame aligned to 8 bytes:
>>>
>>> struct xen_mem_acquire_resource {
>>> uint16_t domid;/* 0 2 */
>>> uint16_t type; /* 2 2 */
>>> uint32_t id; /* 4 4 */
>>> uint32_t nr_frames;/* 8 4 */
>>> uint32_t pad; /*12 4 */
>>> uint64_t frame;/*16 8 */
>>> long unsigned int *frame_list; /*24 4 */
>>>
>>> /* size: 32, cachelines: 1, members: 7 */
>>> /* padding: 4 */
>>> /* last cacheline: 32 bytes */
>>> };
>>>
>>> And this is without:
>>>
>>> struct xen_mem_acquire_resource {
>>> uint16_t domid;/* 0 2 */
>>> uint16_t type; /* 2 2 */
>>> uint32_t id; /* 4 4 */
>>> uint32_t nr_frames;/* 8 4 */
>>> uint32_t pad; /*12 4 */
>>> uint64_t frame;/*16 8 */
>>> long unsigned int *frame_list; /*24 4 */
>>>
>>> /* size: 28, cachelines: 1, members: 7 */
>>> /* last cacheline: 28 bytes */
>>> };
>>>
>>> Note Xen has already been copying those extra 4 padding bytes from
>>> 32bit Linux kernels AFAICT, as the struct declaration in Linux has
>>> dropped the aligned attribute.
>>>
The question now is whether we're willing to accept such a (marginal)
incompatibility. The chance of a 32-bit guest actually running into
it shouldn't be very high, but with the right placement of an
instance of the struct at the end of a page it would be possible to
observe afaict.
Or whether we go the alternative route and pad the struct suitably
for 32-bit.
>>>
>>> I guess we are trapped with what Linux has on it's headers now, so we
>>> should handle the struct size difference in Xen?
>>
>> How do you mean to notice the difference in Xen? You don't know
>> what the caller's environment's sizeof() would yield.
>
> I'm confused. Couldn't we switch from uint64_aligned_t to plain
> uint64_t (like it's currently on the Linux headers), and then use the
> compat layer in Xen to handle the size difference when called from
> 32bit environments?
And which size would we use there? The old or the new one (breaking
future or existing callers respectively)? Meanwhile I think that if
this indeed needs to not be tools-only (which I still question),
then our only possible route is to add explicit padding for the
32-bit case alongside the change you're already making.
> This would of course assume that no toolstack has implemented direct
> calls using this interface, which seems likely because it either
> returns mfns to be mapped in the PV case or require gfns to be
> provided for HVM.
What are "direct calls" in the case here? We
Re: [PATCH for-4.14] mm: fix public declaration of struct xen_mem_acquire_resource
On 23.06.2020 19:32, Roger Pau Monné wrote: > On Tue, Jun 23, 2020 at 05:04:53PM +0200, Jan Beulich wrote: >> On 23.06.2020 15:52, Roger Pau Monne wrote: >>> XENMEM_acquire_resource and it's related structure is currently inside >>> a __XEN__ or __XEN_TOOLS__ guarded section to limit it's scope to the >>> hypervisor or the toolstack only. This is wrong as the hypercall is >>> already being used by the Linux kernel at least, and as such needs to >>> be public. >> >> Actually - how does this work for the Linux kernel, seeing >> >> rc = rcu_lock_remote_domain_by_id(xmar.domid, ); >> if ( rc ) >> return rc; >> >> rc = xsm_domain_resource_map(XSM_DM_PRIV, d); >> if ( rc ) >> goto out; >> >> in the function? > > It's my understanding (I haven't tried to use that hypercall yet on > FreeBSD, so I cannot say I've tested it), that xmar.domid is the > remote domain, which the functions locks and then uses > xsm_domain_resource_map to check whether the current domain has > permissions to do privileged operations against it. Yes, but that's a tool stack operation, not something the kernel would do all by itself. The kernel would only ever pass DOMID_SELF (or the actual local domain ID), I would think. Jan
Re: [PATCH v2 4/7] x86/vmx: add do_vmtrace_op
On 23.06.2020 19:24, Andrew Cooper wrote: > On 23/06/2020 09:51, Jan Beulich wrote: >> On 23.06.2020 03:04, Michał Leszczyński wrote: >>> - 22 cze 2020 o 18:16, Jan Beulich jbeul...@suse.com napisał(a): >>> On 22.06.2020 18:02, Michał Leszczyński wrote: > - 22 cze 2020 o 17:22, Jan Beulich jbeul...@suse.com napisał(a): >> On 22.06.2020 16:35, Michał Leszczyński wrote: >>> - 22 cze 2020 o 15:25, Jan Beulich jbeul...@suse.com napisał(a): Is any of what you do in this switch() actually legitimate without hvm_set_vmtrace_pt_size() having got called for the guest? From remarks elsewhere I imply you expect the param that you currently use to be set upon domain creation time, but at the very least the potentially big buffer should imo not get allocated up front, but only when tracing is to actually be enabled. >>> Wait... so you want to allocate these buffers in runtime? >>> Previously we were talking that there is too much runtime logic >>> and these enable/disable hypercalls should be stripped to absolute >>> minimum. >> Basic arrangements can be made at domain creation time. I don't >> think though that it would be a good use of memory if you >> allocated perhaps many gigabytes of memory just for possibly >> wanting to enable tracing on a guest. >> > From our previous conversations I thought that you want to have > as much logic moved to the domain creation as possible. > > Thus, a parameter "vmtrace_pt_size" was introduced. By default it's > zero (= disabled), if you set it to a non-zero value, then trace buffers > of given size will be allocated for the domain and you have possibility > to use ipt_enable/ipt_disable at any moment. > > This way the runtime logic is as thin as possible. I assume user knows > in advance whether he/she would want to use external monitoring with IPT > or not. Andrew - I think you requested movement to domain_create(). Could you clarify if indeed you mean to also allocate the big buffers this early? >>> I would like to recall what Andrew wrote few days ago: >>> >>> - 16 cze 2020 o 22:16, Andrew Cooper andrew.coop...@citrix.com wrote: Xen has traditionally opted for a "and turn this extra thing on dynamically" model, but this has caused no end of security issues and broken corner cases. You can see this still existing in the difference between XEN_DOMCTL_createdomain and XEN_DOMCTL_max_vcpus, (the latter being required to chose the number of vcpus for the domain) and we're making good progress undoing this particular wart (before 4.13, it was concerning easy to get Xen to fall over a NULL d->vcpu[] pointer by issuing other hypercalls between these two). There is a lot of settings which should be immutable for the lifetime of the domain, and external monitoring looks like another one of these. Specifying it at createdomain time allows for far better runtime behaviour (you are no longer in a situation where the first time you try to turn tracing on, you end up with -ENOMEM because another VM booted in the meantime and used the remaining memory), and it makes for rather more simple code in Xen itself (at runtime, you can rely on it having been set up properly, because a failure setting up will have killed the domain already). ... ~Andrew >>> according to this quote I've moved buffer allocation to the create_domain, >>> the updated version was already sent to the list as patch v3. >> I'd still like to see an explicit confirmation by him that this >> use of memory is indeed what he has intended. There are much smaller >> amounts of memory which we allocate on demand, just to avoid >> allocating some without then ever using it. > > PT is a debug/diagnostic tool. Its not something you'd run in > production against a production VM. Well, the suggested use with introspection made me assume differently. If this is (now and forever) truly debug/diag only, so be it then. Jan
Re: [xen-4.11-testing test] 151295: regressions - FAIL
On 23.06.2020 20:32, osstest service owner wrote: > flight 151295 xen-4.11-testing real [real] > http://logs.test-lab.xenproject.org/osstest/logs/151295/ > > Regressions :-( > > Tests which did not succeed and are blocking, > including tests which could not be run: > build-amd64-prev 6 xen-build fail in 151260 REGR. vs. > 150040 > build-i386-prev 6 xen-build fail in 151260 REGR. vs. > 150040 > > Tests which are failing intermittently (not blocking): > test-amd64-amd64-xl-qemut-debianhvm-i386-xsm 10 debian-hvm-install fail pass > in 151260 > > Tests which did not succeed, but are not blocking: I'm once again struggling to see why there was no push here: The latter two groups both say "not blocking", and the two build-*-prev didn't actually fail here, but in an earlier flight. Without understanding the reason here I'm hesitant to suggest a force push, though. Jan
Re: [PATCH] xen: introduce xen_vring_use_dma
On Wed, Jun 24, 2020 at 05:17:32PM +0800, Peng Fan wrote: > Export xen_swiotlb for all platforms using xen swiotlb > > Use xen_swiotlb to determine when vring should use dma APIs to map the > ring: when xen_swiotlb is enabled the dma API is required. When it is > disabled, it is not required. > > Signed-off-by: Peng Fan Isn't there some way to use VIRTIO_F_IOMMU_PLATFORM for this? Xen was there first, but everyone else is using that now. > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index a2de775801af..768afd79f67a 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev) >* the DMA API if we're a Xen guest, which at least allows >* all of the sensible Xen configurations to work correctly. >*/ > - if (xen_domain()) > + if (xen_vring_use_dma()) > return true; > > return false; The comment above this should probably be fixed. -- MST
[PATCH] xen: introduce xen_vring_use_dma
Export xen_swiotlb for all platforms using xen swiotlb
Use xen_swiotlb to determine when vring should use dma APIs to map the
ring: when xen_swiotlb is enabled the dma API is required. When it is
disabled, it is not required.
Signed-off-by: Peng Fan
---
V2:
This is a modified version from Stefano's patch
https://lore.kernel.org/patchwork/patch/1033801/#1222404
Note: This is not to address rpmsg virtio issue, this is
to let DomU virtio not using xen swiotlb could use non dma vring
on ARM64 platforms.
arch/arm/xen/mm.c | 1 +
arch/x86/include/asm/xen/swiotlb-xen.h | 2 --
arch/x86/xen/pci-swiotlb-xen.c | 2 --
drivers/virtio/virtio_ring.c | 2 +-
drivers/xen/swiotlb-xen.c | 3 +++
include/xen/swiotlb-xen.h | 6 ++
include/xen/xen.h | 6 ++
7 files changed, 17 insertions(+), 5 deletions(-)
diff --git a/arch/arm/xen/mm.c b/arch/arm/xen/mm.c
index d40e9e5fc52b..6a493ea087f0 100644
--- a/arch/arm/xen/mm.c
+++ b/arch/arm/xen/mm.c
@@ -139,6 +139,7 @@ static int __init xen_mm_init(void)
struct gnttab_cache_flush cflush;
if (!xen_initial_domain())
return 0;
+ xen_swiotlb = 1;
xen_swiotlb_init(1, false);
cflush.op = 0;
diff --git a/arch/x86/include/asm/xen/swiotlb-xen.h
b/arch/x86/include/asm/xen/swiotlb-xen.h
index 6b56d0d45d15..bb5ce02b4e20 100644
--- a/arch/x86/include/asm/xen/swiotlb-xen.h
+++ b/arch/x86/include/asm/xen/swiotlb-xen.h
@@ -3,12 +3,10 @@
#define _ASM_X86_SWIOTLB_XEN_H
#ifdef CONFIG_SWIOTLB_XEN
-extern int xen_swiotlb;
extern int __init pci_xen_swiotlb_detect(void);
extern void __init pci_xen_swiotlb_init(void);
extern int pci_xen_swiotlb_init_late(void);
#else
-#define xen_swiotlb (0)
static inline int __init pci_xen_swiotlb_detect(void) { return 0; }
static inline void __init pci_xen_swiotlb_init(void) { }
static inline int pci_xen_swiotlb_init_late(void) { return -ENXIO; }
diff --git a/arch/x86/xen/pci-swiotlb-xen.c b/arch/x86/xen/pci-swiotlb-xen.c
index 33293ce01d8d..071fbe0e1a91 100644
--- a/arch/x86/xen/pci-swiotlb-xen.c
+++ b/arch/x86/xen/pci-swiotlb-xen.c
@@ -18,8 +18,6 @@
#endif
#include
-int xen_swiotlb __read_mostly;
-
/*
* pci_xen_swiotlb_detect - set xen_swiotlb to 1 if necessary
*
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index a2de775801af..768afd79f67a 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -252,7 +252,7 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
* the DMA API if we're a Xen guest, which at least allows
* all of the sensible Xen configurations to work correctly.
*/
- if (xen_domain())
+ if (xen_vring_use_dma())
return true;
return false;
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index b6d27762c6f8..25747e72e6fe 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -40,6 +40,9 @@
#include
#define MAX_DMA_BITS 32
+
+int xen_swiotlb __read_mostly;
+
/*
* Used to do a quick range check in swiotlb_tbl_unmap_single and
* swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by
this
diff --git a/include/xen/swiotlb-xen.h b/include/xen/swiotlb-xen.h
index ffc0d3902b71..235babcde848 100644
--- a/include/xen/swiotlb-xen.h
+++ b/include/xen/swiotlb-xen.h
@@ -12,4 +12,10 @@ void xen_dma_sync_for_device(dma_addr_t handle, phys_addr_t
paddr, size_t size,
extern int xen_swiotlb_init(int verbose, bool early);
extern const struct dma_map_ops xen_swiotlb_dma_ops;
+#ifdef CONFIG_SWIOTLB_XEN
+extern int xen_swiotlb;
+#else
+#define xen_swiotlb (0)
+#endif
+
#endif /* __LINUX_SWIOTLB_XEN_H */
diff --git a/include/xen/xen.h b/include/xen/xen.h
index 19a72f591e2b..c51c46f5d739 100644
--- a/include/xen/xen.h
+++ b/include/xen/xen.h
@@ -52,4 +52,10 @@ bool xen_biovec_phys_mergeable(const struct bio_vec *vec1,
extern u64 xen_saved_max_mem_size;
#endif
+#include
+static inline int xen_vring_use_dma(void)
+{
+ return !!xen_swiotlb;
+}
+
#endif /* _XEN_XEN_H */
--
2.16.4
Re: Keystone Issue
> On 23 Jun 2020, at 21:50, CodeWiz2280 wrote: > > Is it possible to passthrough PCI devices to domU on the 32-bit arm > keystone? Any info is appreciated. > > I found some old information online that "gic-v2m" is required. I'm > not sure if the GIC-400 on the K2E supports "gic_v2m". Based on the > fact that there is no "gic-v2m-frame" tag in the K2E device tree i'm > guessing that it does not. > > If it is possible, is there a good example for arm that I can follow? There is no PCI passthrough support on Arm for now in Xen. This is something I am working on and I will present something on this subject at the Xen summit. But we are not targeting arm32 for now. The only thing possible for now is to have PCI devices handled by Dom0 and using xen virtual drivers to pass the functionalities (ethernet, block or others). Regards Bertrand > > On Wed, Jun 17, 2020 at 7:52 PM CodeWiz2280 wrote: >> >> On Wed, Jun 17, 2020 at 2:46 PM Stefano Stabellini >> wrote: >>> >>> On Wed, 17 Jun 2020, CodeWiz2280 wrote: On Tue, Jun 16, 2020 at 2:23 PM Marc Zyngier wrote: > > On 2020-06-16 19:13, CodeWiz2280 wrote: >> On Tue, Jun 16, 2020 at 4:11 AM Marc Zyngier wrote: >>> >>> On 2020-06-15 20:14, CodeWiz2280 wrote: >>> >>> [...] >>> Also, the latest linux kernel still has the X-Gene storm distributor address as "0x7801" in the device tree, which is what the Xen code considers a match with the old firmware. What were the addresses for the device tree supposed to be changed to? >>> >>> We usually don't care, as the GIC address is provided by the >>> bootloader, >>> whether via DT or ACPI (this is certainly what happens on Mustang). >>> Whatever is still in the kernel tree is just as dead as the platform >>> it >>> describes. >>> Is my understanding correct that there is a different base address required to access the "non-secure" region instead of the "secure" 0x7801 region? I'm trying to see if there are corresponding different addresses for the keystone K2E, but haven't found them yet in the manuals. >>> >>> There is no such address. Think of the NS bit as an *address space* >>> identifier. >>> >>> The only reason XGene presents the NS part of the GIC at a different >>> address is because XGene is broken enough not to have EL3, hence no >>> secure mode. To wire the GIC (and other standard ARM IPs) to the core, >>> the designers simply used the CPU NS signal as an address bit. >>> >>> On your platform, the NS bit does exist. I strongly suppose that it >>> isn't wired to the GIC. Please talk to your SoC vendor for whether iot >>> is possible to work around this. >>> >> I do have a question about this out to TI, but at least this method >> gives me something to work with in the meantime. I was just looking >> to confirm that there wouldn't be any other undesirable side effects >> with Dom0 or DomU when using it. Was there an actual FPGA for the >> X-Gene that needed to be updated which controlled the GIC access? Or >> by firmware do you mean the boot loader (e.g. uboot). Thanks for the >> support so far to all. > > As I said, the specific case of XGene was just a matter of picking the > right address, as the NS bit is used as an address bit on this platform. > This was possible because this machine doesn't have any form of > security. So no HW was changed, no FPGA reprogrammed. Only a firmware > table was fixed to point to the right spot. Not even u-boot or EFI was > changed. Ok, thank you for clarifying. I have one more question if you don't mind. I'm aware that dom0 can share physical memory with dom1 via grant tables. However, is it possible to reserve a chunk of contiguous physical memory and directly allocate it only to dom1? For example, if I wanted dom1 to have access to 8MB of contiguous memory at 0x8200_ (in addition to whatever virtual memory Xen gives it). How would one go about doing this on ARM? Is there something in the guest config or device tree that can be set? Thanks for you help. >>> >>> There isn't a "proper" way to do it, only a workaround. >>> >>> It is possible to do it by using the iomem option, which is meant for >>> device MMIO regions. >>> >>> We have patches in the Xilinx Xen tree (not upstream) to allow for >>> specifying the cacheability that you want for the iomem mapping so that >>> you can map it as normal memory. This is the latest branch: >>> >>> https://github.com/Xilinx/xen.git xilinx/release-2020.1 >>> >>> The relevant commits are the ones from: >>> https://github.com/Xilinx/xen/commit/a5c76ac1c5dc14d3e9ccedc5c1e7dd2ddc1397b6 >>> to: >>> https://github.com/Xilinx/xen/commit/b4b7e91c1524f9cf530b81b7ba95df2bf50c78e4 >>> >>> You
Re: UEFI support in ARM DomUs
On 6/24/20 10:26 AM, Peng Fan wrote: >> Subject: Re: UEFI support in ARM DomUs >> >> >> On 6/24/20 10:07 AM, Peng Fan wrote: Subject: Re: UEFI support in ARM DomUs On 6/23/20 8:31 AM, Oleksandr Andrushchenko wrote: > On 6/23/20 4:20 AM, Stefano Stabellini wrote: >> On Mon, 22 Jun 2020, Julien Grall wrote: >> For the first part (__XEN_INTERFACE_VERSION__) I think we can >> provide it via >> >> CFLAGS or something. This can also be done for the location of >> Xen headers. > __XEN_INTERFACE_VERSION__ should work through the CFLAGS. >> An > alternative would be to allow the user to specify through the >> Kconfig. You mean specifying via Kconfig something like "0x00040d00"? >>> Possibly yes. >>> And what about the headers? How will we provide their location if we decide not to include those in the tree? >>> I would do through Kconfig as well. >> If we specify the external location of the Xen headers via Kconfig, >> it seems to me that we should be able to detect the interface >> version automatically from any Makefile as part of the build. No >> need to ask the user. >> >> However, if Oleksandr is thinking of using the Xen headers for the >> hypercalls definitions, then I think we might not need external >> headers at all because that is a stable interface, as Julien wrote. >> We could just define our own few headers for just what you need >> like Linux does. > This is a good idea: I'll try to get the minimal set of headers from > Linux > > instead of Xen as those seem to be well prepared for such a use-case. > This > > way we'll have headers in U-boot tree and guarantee that we have a > minimal > > subset which is easier to maintain. I'll keep you updated on the > progress We've managed to strip the headers and remove __XEN__ and the rest definitions we were talking about. So, these are now the minimal required set of headers that allows U-boot to build serial and block drivers. Please take a look at [1] Pull request for comments is at [2] >>> The U-Boot new merge window will be open in 2020/7/1, so I'd suggest >>> the patchset goes to U-Boot mail list for discussion if you wanna the >>> patches gonna merged soon. >> We definitely want the patches to be upstreamed and merged, but before >> that >> >> we also want to make sure that Xen community is happy with what we >> upstream >> >> I believe we resolved most of the concerns such as headers, PIE etc, so this >> can >> >> be done. >> >> BTW, Peng, did you have a chance to try the pvblock driver yet? > Not yet. I could have time to give a test next Monday. I think you not > enable SPL, right? No, we decided that we can run with U-boot proper, so we can have more flexibility comparing to what we have in SPL. It seems that was the right approach: you can jump to Linux or you can jump to the U-boot provided by Android anyway, whatever your setup > So android dual bootloader feature not enabled. I think this step will depend on the use-case you have: at the moment we have a base build capable of booting Linux kernel, but this can easily be extended with specific defconfig to build in boota command or whatever else is required. > But SPL mostly not have MMU enabled.. > > Regards, > Peng. > >>> Regards, >>> Peng. >>> >> If you can do that, I think it would be better because we decouple >> the UBoot build from the Xen build completely. We don't even need >> the Xen tree checked out to build UBoot. It would be a huge >> advantage because it makes it far easier to build-test changes for >> others in the community that don't know about Xen and also it >> becomes far easier to integrate into any build system. [1] >> https://urldefense.com/v3/__https://eur01.safelinks.protection.outlook.com/?url=https*3A*2F*2Furldef__;JSUl!!GF_29dbcQIUBPA!kJhWFr5ZO_UWn2oD_I9pDfnn64pZXw1ZBtASsxS9AZwbG65093ZydtlVPy6baPy4oeF957GBNA$ >> ense.com%2Fv3%2F__https%3A%2F%2Feur01.safelinks.protection.outlook.c >> om%2F%3Furl%3Dhttps*3A*2F*2Fgithub__%3BJSUl!!GF_29dbcQIUBPA!mwi >> b3un6-vYBG68zMfurW6-0MuuER5tNmJtOitDpViICNkX0lhig8iPHmZokuM-BLQ >> YpeKYAGQ%24data=02%7C01%7Cpeng.fan%40nxp.com%7C950d9c0a >> dc514927ce8b08d8180ea4ef%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C >> 0%7C0%7C637285798460563914sdata=fMrI%2FQotknCsXV0amC4BFl >> 1Hg4vPw3zOMVdAVim2Wcs%3Dreserved=0 . >> com%2Fandr2000%2Fu-boot%2Ftree%2Fpvblock_upstream_v1data=0 >> 2%7C01%7Cpeng.fan%40nxp.com%7C407d8af24a36483fbdce08d81805ed88 %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637285761021 >> 975 >> 164sdata=5vWfBbLScICXPZWU%2BU3b7DyONcgxT8iICsxrwUbORZY% 3Dreserved=0 [2] >>
RE: [TESTDAY] Test report 4.14-rc3
> -Original Message- > From: Xen-devel On Behalf Of Tamas K > Lengyel > Sent: 24 June 2020 05:31 > To: Xen-devel > Subject: [TESTDAY] Test report 4.14-rc3 > > * Hardware: i9-9900x > > * Guest operating systems: Ubuntu 20.04 > > * Functionality tested: domain create/save/restore, altp2m, vm_event, > mem_access, mem_sharing, VM forking > > * Comments: also tested running nested in a VMware workstation VM, all > above work fine in that setup as well albeit with some expected > performance drop. > > Everything looks good from my end. > Thanks! Good to know. Paul
RE: UEFI support in ARM DomUs
> Subject: Re: UEFI support in ARM DomUs > > > On 6/24/20 10:07 AM, Peng Fan wrote: > >> Subject: Re: UEFI support in ARM DomUs > >> > >> > >> On 6/23/20 8:31 AM, Oleksandr Andrushchenko wrote: > >>> On 6/23/20 4:20 AM, Stefano Stabellini wrote: > On Mon, 22 Jun 2020, Julien Grall wrote: > For the first part (__XEN_INTERFACE_VERSION__) I think we can > provide it via > > CFLAGS or something. This can also be done for the location of > Xen headers. > >>> __XEN_INTERFACE_VERSION__ should work through the CFLAGS. > An > >>> alternative would be to allow the user to specify through the > Kconfig. > >> You mean specifying via Kconfig something like "0x00040d00"? > > Possibly yes. > > > >> And what about the headers? How will we provide their location if > >> we decide not to include those > >> > >> in the tree? > > I would do through Kconfig as well. > If we specify the external location of the Xen headers via Kconfig, > it seems to me that we should be able to detect the interface > version automatically from any Makefile as part of the build. No > need to ask the user. > > However, if Oleksandr is thinking of using the Xen headers for the > hypercalls definitions, then I think we might not need external > headers at all because that is a stable interface, as Julien wrote. > We could just define our own few headers for just what you need > like Linux > >> does. > >>> This is a good idea: I'll try to get the minimal set of headers from > >>> Linux > >>> > >>> instead of Xen as those seem to be well prepared for such a use-case. > >>> This > >>> > >>> way we'll have headers in U-boot tree and guarantee that we have a > >>> minimal > >>> > >>> subset which is easier to maintain. I'll keep you updated on the > >>> progress > >> We've managed to strip the headers and remove __XEN__ and the rest > >> definitions > >> > >> we were talking about. So, these are now the minimal required set of > >> headers > >> > >> that allows U-boot to build serial and block drivers. Please take a > >> look at [1] > >> > >> Pull request for comments is at [2] > > The U-Boot new merge window will be open in 2020/7/1, so I'd suggest > > the patchset goes to U-Boot mail list for discussion if you wanna the > > patches gonna merged soon. > > We definitely want the patches to be upstreamed and merged, but before > that > > we also want to make sure that Xen community is happy with what we > upstream > > I believe we resolved most of the concerns such as headers, PIE etc, so this > can > > be done. > > BTW, Peng, did you have a chance to try the pvblock driver yet? Not yet. I could have time to give a test next Monday. I think you not enable SPL, right? So android dual bootloader feature not enabled. But SPL mostly not have MMU enabled.. Regards, Peng. > > > > > Regards, > > Peng. > > > If you can do that, I think it would be better because we decouple > the UBoot build from the Xen build completely. We don't even need > the Xen tree checked out to build UBoot. It would be a huge > advantage because it makes it far easier to build-test changes for > others in the community that don't know about Xen and also it > becomes far easier to integrate into any build system. > >> [1] > >> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldef > ense.com%2Fv3%2F__https%3A%2F%2Feur01.safelinks.protection.outlook.c > om%2F%3Furl%3Dhttps*3A*2F*2Fgithub__%3BJSUl!!GF_29dbcQIUBPA!mwi > b3un6-vYBG68zMfurW6-0MuuER5tNmJtOitDpViICNkX0lhig8iPHmZokuM-BLQ > YpeKYAGQ%24data=02%7C01%7Cpeng.fan%40nxp.com%7C950d9c0a > dc514927ce8b08d8180ea4ef%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C > 0%7C0%7C637285798460563914sdata=fMrI%2FQotknCsXV0amC4BFl > 1Hg4vPw3zOMVdAVim2Wcs%3Dreserved=0 . > >> > com%2Fandr2000%2Fu-boot%2Ftree%2Fpvblock_upstream_v1data=0 > >> > 2%7C01%7Cpeng.fan%40nxp.com%7C407d8af24a36483fbdce08d81805ed88 > >> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637285761021 > 975 > >> > 164sdata=5vWfBbLScICXPZWU%2BU3b7DyONcgxT8iICsxrwUbORZY% > >> 3Dreserved=0 > >> > >> [2] > >> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Furldef > ense.com%2Fv3%2F__https%3A%2F%2Feur01.safelinks.protection.outlook.c > om%2F%3Furl%3Dhttps*3A*2F*2Fgithub__%3BJSUl!!GF_29dbcQIUBPA!mwi > b3un6-vYBG68zMfurW6-0MuuER5tNmJtOitDpViICNkX0lhig8iPHmZokuM-BLQ > YpeKYAGQ%24data=02%7C01%7Cpeng.fan%40nxp.com%7C950d9c0a > dc514927ce8b08d8180ea4ef%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C > 0%7C0%7C637285798460563914sdata=fMrI%2FQotknCsXV0amC4BFl > 1Hg4vPw3zOMVdAVim2Wcs%3Dreserved=0 . > >> > com%2Fxen-troops%2Fu-boot%2Fpull%2F2data=02%7C01%7Cpeng.fa > >> > n%40nxp.com%7C407d8af24a36483fbdce08d81805ed88%7C686ea1d3bc2b4 > >> > c6fa92cd99c5c301635%7C0%7C0%7C637285761021975164sdata=%2 > >> > FmXheEvKssLjjaFKsHBBbqh%2B72jH3uQnE7cpN0J3k8I%3Dreserved=0
Re: UEFI support in ARM DomUs
On 6/24/20 10:07 AM, Peng Fan wrote: >> Subject: Re: UEFI support in ARM DomUs >> >> >> On 6/23/20 8:31 AM, Oleksandr Andrushchenko wrote: >>> On 6/23/20 4:20 AM, Stefano Stabellini wrote: On Mon, 22 Jun 2020, Julien Grall wrote: For the first part (__XEN_INTERFACE_VERSION__) I think we can provide it via CFLAGS or something. This can also be done for the location of Xen headers. >>> __XEN_INTERFACE_VERSION__ should work through the CFLAGS. An >>> alternative would be to allow the user to specify through the Kconfig. >> You mean specifying via Kconfig something like "0x00040d00"? > Possibly yes. > >> And what about the headers? How will we provide their location if >> we decide not to include those >> >> in the tree? > I would do through Kconfig as well. If we specify the external location of the Xen headers via Kconfig, it seems to me that we should be able to detect the interface version automatically from any Makefile as part of the build. No need to ask the user. However, if Oleksandr is thinking of using the Xen headers for the hypercalls definitions, then I think we might not need external headers at all because that is a stable interface, as Julien wrote. We could just define our own few headers for just what you need like Linux >> does. >>> This is a good idea: I'll try to get the minimal set of headers from >>> Linux >>> >>> instead of Xen as those seem to be well prepared for such a use-case. >>> This >>> >>> way we'll have headers in U-boot tree and guarantee that we have a >>> minimal >>> >>> subset which is easier to maintain. I'll keep you updated on the >>> progress >> We've managed to strip the headers and remove __XEN__ and the rest >> definitions >> >> we were talking about. So, these are now the minimal required set of headers >> >> that allows U-boot to build serial and block drivers. Please take a look at >> [1] >> >> Pull request for comments is at [2] > The U-Boot new merge window will be open in 2020/7/1, so I'd suggest > the patchset goes to U-Boot mail list for discussion if you wanna the patches > gonna merged soon. We definitely want the patches to be upstreamed and merged, but before that we also want to make sure that Xen community is happy with what we upstream I believe we resolved most of the concerns such as headers, PIE etc, so this can be done. BTW, Peng, did you have a chance to try the pvblock driver yet? > > Regards, > Peng. > If you can do that, I think it would be better because we decouple the UBoot build from the Xen build completely. We don't even need the Xen tree checked out to build UBoot. It would be a huge advantage because it makes it far easier to build-test changes for others in the community that don't know about Xen and also it becomes far easier to integrate into any build system. >> [1] >> https://urldefense.com/v3/__https://eur01.safelinks.protection.outlook.com/?url=https*3A*2F*2Fgithub__;JSUl!!GF_29dbcQIUBPA!mwib3un6-vYBG68zMfurW6-0MuuER5tNmJtOitDpViICNkX0lhig8iPHmZokuM-BLQYpeKYAGQ$ >> . >> com%2Fandr2000%2Fu-boot%2Ftree%2Fpvblock_upstream_v1data=0 >> 2%7C01%7Cpeng.fan%40nxp.com%7C407d8af24a36483fbdce08d81805ed88 >> %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637285761021975 >> 164sdata=5vWfBbLScICXPZWU%2BU3b7DyONcgxT8iICsxrwUbORZY% >> 3Dreserved=0 >> >> [2] >> https://urldefense.com/v3/__https://eur01.safelinks.protection.outlook.com/?url=https*3A*2F*2Fgithub__;JSUl!!GF_29dbcQIUBPA!mwib3un6-vYBG68zMfurW6-0MuuER5tNmJtOitDpViICNkX0lhig8iPHmZokuM-BLQYpeKYAGQ$ >> . >> com%2Fxen-troops%2Fu-boot%2Fpull%2F2data=02%7C01%7Cpeng.fa >> n%40nxp.com%7C407d8af24a36483fbdce08d81805ed88%7C686ea1d3bc2b4 >> c6fa92cd99c5c301635%7C0%7C0%7C637285761021975164sdata=%2 >> FmXheEvKssLjjaFKsHBBbqh%2B72jH3uQnE7cpN0J3k8I%3Dreserved=0
RE: UEFI support in ARM DomUs
> Subject: Re: UEFI support in ARM DomUs > > > On 6/23/20 8:31 AM, Oleksandr Andrushchenko wrote: > > > > On 6/23/20 4:20 AM, Stefano Stabellini wrote: > >> On Mon, 22 Jun 2020, Julien Grall wrote: > >> For the first part (__XEN_INTERFACE_VERSION__) I think we can > >> provide it via > >> > >> CFLAGS or something. This can also be done for the location of > >> Xen headers. > > __XEN_INTERFACE_VERSION__ should work through the CFLAGS. An > > alternative would be to allow the user to specify through the Kconfig. > You mean specifying via Kconfig something like "0x00040d00"? > >>> Possibly yes. > >>> > And what about the headers? How will we provide their location if > we decide not to include those > > in the tree? > >>> I would do through Kconfig as well. > >> If we specify the external location of the Xen headers via Kconfig, > >> it seems to me that we should be able to detect the interface version > >> automatically from any Makefile as part of the build. No need to ask > >> the user. > >> > >> However, if Oleksandr is thinking of using the Xen headers for the > >> hypercalls definitions, then I think we might not need external > >> headers at all because that is a stable interface, as Julien wrote. > >> We could just define our own few headers for just what you need like Linux > does. > > > > This is a good idea: I'll try to get the minimal set of headers from > > Linux > > > > instead of Xen as those seem to be well prepared for such a use-case. > > This > > > > way we'll have headers in U-boot tree and guarantee that we have a > > minimal > > > > subset which is easier to maintain. I'll keep you updated on the > > progress > > We've managed to strip the headers and remove __XEN__ and the rest > definitions > > we were talking about. So, these are now the minimal required set of headers > > that allows U-boot to build serial and block drivers. Please take a look at > [1] > > Pull request for comments is at [2] The U-Boot new merge window will be open in 2020/7/1, so I'd suggest the patchset goes to U-Boot mail list for discussion if you wanna the patches gonna merged soon. Regards, Peng. > > > > >> > >> If you can do that, I think it would be better because we decouple > >> the UBoot build from the Xen build completely. We don't even need the > >> Xen tree checked out to build UBoot. It would be a huge advantage > >> because it makes it far easier to build-test changes for others in > >> the community that don't know about Xen and also it becomes far > >> easier to integrate into any build system. > > [1] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub. > com%2Fandr2000%2Fu-boot%2Ftree%2Fpvblock_upstream_v1data=0 > 2%7C01%7Cpeng.fan%40nxp.com%7C407d8af24a36483fbdce08d81805ed88 > %7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637285761021975 > 164sdata=5vWfBbLScICXPZWU%2BU3b7DyONcgxT8iICsxrwUbORZY% > 3Dreserved=0 > > [2] > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub. > com%2Fxen-troops%2Fu-boot%2Fpull%2F2data=02%7C01%7Cpeng.fa > n%40nxp.com%7C407d8af24a36483fbdce08d81805ed88%7C686ea1d3bc2b4 > c6fa92cd99c5c301635%7C0%7C0%7C637285761021975164sdata=%2 > FmXheEvKssLjjaFKsHBBbqh%2B72jH3uQnE7cpN0J3k8I%3Dreserved=0
Re: UEFI support in ARM DomUs
On 6/23/20 8:31 AM, Oleksandr Andrushchenko wrote: > > On 6/23/20 4:20 AM, Stefano Stabellini wrote: >> On Mon, 22 Jun 2020, Julien Grall wrote: >> For the first part (__XEN_INTERFACE_VERSION__) I think we can provide it >> via >> >> CFLAGS or something. This can also be done for the location of Xen >> headers. > __XEN_INTERFACE_VERSION__ should work through the CFLAGS. An alternative > would be to allow the user to specify through the Kconfig. You mean specifying via Kconfig something like "0x00040d00"? >>> Possibly yes. >>> And what about the headers? How will we provide their location if we decide not to include those in the tree? >>> I would do through Kconfig as well. >> If we specify the external location of the Xen headers via Kconfig, it >> seems to me that we should be able to detect the interface version >> automatically from any Makefile as part of the build. No need to ask the >> user. >> >> However, if Oleksandr is thinking of using the Xen headers for the >> hypercalls definitions, then I think we might not need external headers >> at all because that is a stable interface, as Julien wrote. We could >> just define our own few headers for just what you need like Linux does. > > This is a good idea: I'll try to get the minimal set of headers from Linux > > instead of Xen as those seem to be well prepared for such a use-case. This > > way we'll have headers in U-boot tree and guarantee that we have a minimal > > subset which is easier to maintain. I'll keep you updated on the progress We've managed to strip the headers and remove __XEN__ and the rest definitions we were talking about. So, these are now the minimal required set of headers that allows U-boot to build serial and block drivers. Please take a look at [1] Pull request for comments is at [2] > >> >> If you can do that, I think it would be better because we decouple the >> UBoot build from the Xen build completely. We don't even need the Xen >> tree checked out to build UBoot. It would be a huge advantage because it >> makes it far easier to build-test changes for others in the community >> that don't know about Xen and also it becomes far easier to integrate >> into any build system. [1] https://github.com/andr2000/u-boot/tree/pvblock_upstream_v1 [2] https://github.com/xen-troops/u-boot/pull/2
