Re: [Xen-devel] [PATCH v2] xen/pt: fix some pass-thru devices don't work across reboot
On Thu, Nov 15, 2018 at 11:40:39AM +0100, Roger Pau Monné wrote:
>On Thu, Nov 15, 2018 at 09:10:26AM +0800, Chao Gao wrote:
>> I find some pass-thru devices don't work any more across guest
>> reboot. Assigning it to another domain also meets the same issue. And
>> the only way to make it work again is un-binding and binding it to
>> pciback. Someone reported this issue one year ago [1].
>
>How does unbinding and binding to pciback fix the issue? Is this due
>to Dom0 using some PV or Dom0 only hypercall that can reset the
>host_maskall state?
I think it is msix_capability_init() that clear host_maskall. And this
function is called by PHYSDEVOP_prepare_msix sub-hypercall.
>
>>
>> If the device's driver doesn't disable MSI-X during shutdown or qemu is
>> killed/crashed before the domain shutdown, this domain's pirq won't be
>> unmapped. Then xen will unmap all pirq. But pciback has already disabled
>> meory decoding before xen unmapping pirq. Then when Xen is disabling a
>> MSI of the device, it has to sets the host_maskall flag and maskall bit
>> to mask a MSI rather than sets maskbit in MSI-x table. The call trace of
>> this process is:
>> ->arch_domain_destroy
>> ->free_domain_pirqs
>> ->unmap_domain_pirq (if pirq isn't unmap by qemu)
>> ->pirq_guest_force_unbind
>> ->__pirq_guest_unbind
>> ->mask_msi_irq(=desc->handler->disable())
>> ->the warning in msi_set_mask_bit()
>>
>> The host_maskall bit will prevent guests from clearing the maskall bit
>> even the device is assigned to another guest later. Guests cannot
>> receive interrupts from this device.
>>
>>
>> To fix this, host_maskall flag is cleared when all MSIs of a device are
>> freed.
>> It is definitely safely to clear it because no msi is actually set up
>> for this device. Also, 'msix->warned' is initialized to DOMID_INVALID
>> rather than 0 to avoid warnings missing for Dom0.
>>
>> [1]:
>> https://lists.xenproject.org/archives/html/xen-devel/2017-09/msg02520.html
>>
>> Signed-off-by: Chao Gao
>> ---
>> xen/arch/x86/msi.c| 18 ++
>> xen/drivers/passthrough/pci.c | 1 +
>> 2 files changed, 19 insertions(+)
>>
>> diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
>> index 5567990..cd570bc 100644
>> --- a/xen/arch/x86/msi.c
>> +++ b/xen/arch/x86/msi.c
>> @@ -637,6 +637,7 @@ int msi_free_irq(struct msi_desc *entry)
>> {
>> unsigned int nr = entry->msi_attrib.type != PCI_CAP_ID_MSIX
>>? entry->msi.nvec : 1;
>> +struct pci_dev *pdev = entry->dev;
>>
>> while ( nr-- )
>> {
>> @@ -654,6 +655,23 @@ int msi_free_irq(struct msi_desc *entry)
>>
>> list_del(>list);
>> xfree(entry);
>> +
>> +/*
>> + * In some cases, the 'host_maskall' is set for safety. Here clear
>> + * 'host_maskall' if all MSIs are freed for a msi-x capable device.
>> + * Without it, the device's msix keeps disabled even been reassigned
>
>"... even after being reassigned ..."
>
>I think it's clearer.
>
>> + * to another domain.
>> + */
>> +if ( pdev && list_empty(>msi_list) && pdev->msix )
>> +{
>> +if ( pdev->msix->host_maskall )
>> +printk(XENLOG_G_WARNING
>> + "Resetting msix status of %04x:%02x:%02x.%u\n",
>> + pdev->seg, pdev->bus, PCI_SLOT(pdev->devfn),
>> + PCI_FUNC(pdev->devfn));
>> +pdev->msix->host_maskall = false;
>> +pdev->msix->warned = DOMID_INVALID;
>> +}
>> return 0;
>
>IMO this looks quite similar to a msi_reset_state function or at least
>the start of it.
>
>Maybe it should be in a separate helper that sets all the fields to
>their initial values,
Will do.
>with the expectation that Dom0 will perform the
>device reset?
Dom0 resets devices when (de-)assignment by echo 1 to
/sys/bus/pci/devices//reset.
>
>The underlying problem here AFAICT is that the Xen internal device
>state is not the same between device assignments.
Yes, it is accurate.
>
>In any case there should be at least a note here pointing out that Xen
>expects the hardware domain to perform a device reset, so the Xen
>internal state actually matches the device state before trying to
>assign the device to another guest.
Sounds good. This issue is that Xen tries to mask msi (when unmapping pirq)
after memory decoding is disabled by pciback. If pciback can unmap all the
pirq-s related a given device before disabling memory decoding, Xen won't meet
this issue. Currently, pciback doesn't maintain the pirq information; it
isn't able to do this.
Anyway, this fix patch can serve as a sanity check.
Thanks
Chao
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130174: regressions - FAIL
flight 130174 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130174/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-amd64 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z 10 days Failing since129526 2018-11-06 20:49:26 Z9 days 90 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days 12 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/9] mm: Introduce new vm_insert_range API
On Fri, Nov 16, 2018 at 11:00:30AM +0530, Souptick Joarder wrote:
> On Thu, Nov 15, 2018 at 11:44 PM Randy Dunlap wrote:
> > On 11/15/18 7:45 AM, Souptick Joarder wrote:
> > What is the opposite of vm_insert_range() or even of vm_insert_page()?
> > or is there no need for that?
>
> There is no opposite function of vm_insert_range() / vm_insert_page().
> My understanding is, in case of any error, mmap handlers will return the
> err to user process and user space will decide the next action. So next
> time when mmap handler is getting invoked it will map from the beginning.
> Correct me if I am wrong.
The opposite function, I suppose, is unmap_region().
> > s/no./number/
>
> I didn't get it ??
This is a 'sed' expression. 's' is the 'substitute' command; the /
is a separator, 'no.' is what you wrote, and 'number' is what Randy
is recommending instead.
> > > + for (i = 0; i < page_count; i++) {
> > > + ret = vm_insert_page(vma, uaddr, pages[i]);
> > > + if (ret < 0)
> > > + return ret;
> >
> > For a non-trivial value of page_count:
> > Is it a problem if vm_insert_page() succeeds for several pages
> > and then fails?
>
> No, it will be considered as total failure and mmap handler will return
> the err to user space.
I think what Randy means is "What happens to the inserted pages?" and
the answer is that mmap_region() jumps to the 'unmap_and_free_vma'
label, which is an accurate name.
Thanks for looking, Randy.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/9] mm: Introduce new vm_insert_range API
On Thu, Nov 15, 2018 at 11:44 PM Randy Dunlap wrote:
>
> On 11/15/18 7:45 AM, Souptick Joarder wrote:
> > Previouly drivers have their own way of mapping range of
> > kernel pages/memory into user vma and this was done by
> > invoking vm_insert_page() within a loop.
> >
> > As this pattern is common across different drivers, it can
> > be generalized by creating a new function and use it across
> > the drivers.
> >
> > vm_insert_range is the new API which will be used to map a
> > range of kernel memory/pages to user vma.
> >
> > Signed-off-by: Souptick Joarder
> > Reviewed-by: Matthew Wilcox
> > ---
> > include/linux/mm_types.h | 3 +++
> > mm/memory.c | 28
> > mm/nommu.c | 7 +++
> > 3 files changed, 38 insertions(+)
>
> Hi,
>
> What is the opposite of vm_insert_range() or even of vm_insert_page()?
> or is there no need for that?
There is no opposite function of vm_insert_range() / vm_insert_page().
My understanding is, in case of any error, mmap handlers will return the
err to user process and user space will decide the next action. So next
time when mmap handler is getting invoked it will map from the beginning.
Correct me if I am wrong.
>
>
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 15c417e..da904ed 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1478,6 +1478,34 @@ static int insert_page(struct vm_area_struct *vma,
> > unsigned long addr,
> > }
> >
> > /**
> > + * vm_insert_range - insert range of kernel pages into user vma
> > + * @vma: user vma to map to
> > + * @addr: target user address of this page
> > + * @pages: pointer to array of source kernel pages
> > + * @page_count: no. of pages need to insert into user vma
>
> s/no./number/
I didn't get it ??
>
> > + *
> > + * This allows drivers to insert range of kernel pages they've allocated
> > + * into a user vma. This is a generic function which drivers can use
> > + * rather than using their own way of mapping range of kernel pages into
> > + * user vma.
> > + */
> > +int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
> > + struct page **pages, unsigned long page_count)
> > +{
> > + unsigned long uaddr = addr;
> > + int ret = 0, i;
> > +
> > + for (i = 0; i < page_count; i++) {
> > + ret = vm_insert_page(vma, uaddr, pages[i]);
> > + if (ret < 0)
> > + return ret;
>
> For a non-trivial value of page_count:
> Is it a problem if vm_insert_page() succeeds for several pages
> and then fails?
No, it will be considered as total failure and mmap handler will return
the err to user space.
>
> > + uaddr += PAGE_SIZE;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > * vm_insert_page - insert single page into user vma
> > * @vma: user vma to map to
> > * @addr: target user address of this page
>
>
> thanks.
> --
> ~Randy
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130170: regressions - FAIL
flight 130170 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130170/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-amd64 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z 10 days Failing since129526 2018-11-06 20:49:26 Z9 days 89 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days 11 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130164: regressions - FAIL
flight 130164 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130164/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-amd64 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: build-i386-libvirt1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z 10 days Failing since129526 2018-11-06 20:49:26 Z9 days 88 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days 10 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [qemu-mainline test] 129996: tolerable FAIL - PUSHED
flight 129996 qemu-mainline real [real] http://logs.test-lab.xenproject.org/osstest/logs/129996/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 129514 test-armhf-armhf-libvirt 14 saverestore-support-checkfail like 129514 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 129514 test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail like 129514 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 129514 test-amd64-i386-xl-pvshim12 guest-start fail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-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-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-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-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail 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-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-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-raw 12 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 13 migrate-support-checkfail never pass test-armhf-armhf-xl-arndale 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass version targeted for testing: qemuucb968d275c145467c8b385a3618a207ec111eab1 baseline version: qemuu31eac32a8cba966c374b39bc36afdcb2eb255ed6 Last test of basis 129514 2018-11-06 15:39:32 Z9 days Failing since129651 2018-11-08 16:05:29 Z7 days3 attempts Testing same since 129996 2018-11-13 22:49:16 Z2 days1 attempts People who touched revisions under test: Aleksandar Markovic Alex Bennée Alexander Graf Alistair Francis Chen Zhang Clement Deschamps Cornelia Huck Cédric Le Goater Daniel P. Berrangé David Gibson Dr. David Alan Gilbert Eduardo Habkost Eric Auger Eric Blake Fam Zheng Geert Uytterhoeven Gerd Hoffmann Greg Kurz Igor Mammedov Jeff Cody Kevin Wolf Laurent Vivier Li Qiang Liam Merwick Marc-André Lureau Maria Klimushenkova Mark Cave-Ayland Markus Armbruster Max Reitz Michael Roth Palmer Dabbelt Paolo Bonzini Pavel Dovgalyuk Peter Maydell Peter Xu Philippe Mathieu-Daudé Prasad J Pandit Richard Henderson Richard Henderson
[Xen-devel] [ovmf test] 130161: regressions - FAIL
flight 130161 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130161/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z 10 days Failing since129526 2018-11-06 20:49:26 Z9 days 87 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days9 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130158: regressions - FAIL
flight 130158 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130158/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z 10 days Failing since129526 2018-11-06 20:49:26 Z9 days 86 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days8 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130154: regressions - FAIL
flight 130154 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130154/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z 10 days Failing since129526 2018-11-06 20:49:26 Z9 days 85 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days7 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [linux-4.14 test] 129986: regressions - FAIL
flight 129986 linux-4.14 real [real] http://logs.test-lab.xenproject.org/osstest/logs/129986/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: test-armhf-armhf-xl-arndale 6 xen-install fail REGR. vs. 129762 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-pvshim12 guest-start fail 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-credit2 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit2 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-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-libvirt-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-libvirt-xsm 14 saverestore-support-checkfail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 14 saverestore-support-checkfail never pass test-amd64-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit1 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit1 14 saverestore-support-checkfail never pass test-armhf-armhf-libvirt 13 migrate-support-checkfail never pass test-armhf-armhf-libvirt 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemut-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-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass test-amd64-amd64-xl-qemut-win10-i386 10 windows-installfail never pass test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass version targeted for testing: linux2e390c487815669fb9bb35d7ea11883cc10a9b50 baseline version: linux0b047cbc44ae7d0cea41a99cd7ec1f009360a605 Last test of basis 129762 2018-11-10 16:18:46 Z5 days Testing same since 129986 2018-11-13 19:42:10 Z2 days1 attempts People who touched revisions under test: "Eric W. Biederman" Aaro Koskinen Al Viro Alex Stanoev Alexander Duyck Alexander Ploumistos Alexandre Belloni Alexei Starovoitov Alexey Khoroshilov Amir Goldstein Andi Kleen Andreas Kemnade Andrew Bowers Andrew Lunn Andrew Morton Antoine Tenart Ard
[Xen-devel] [ovmf test] 130151: regressions - FAIL
flight 130151 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130151/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z 10 days Failing since129526 2018-11-06 20:49:26 Z9 days 84 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days6 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130148: regressions - FAIL
flight 130148 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130148/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z 10 days Failing since129526 2018-11-06 20:49:26 Z9 days 83 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days5 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/4] x86: Drop PVRDTSCP and fix MSR_TSC_AUX handling for guests
Boris has confirmed that noone appears to be using PVRDTSCP any more, and in
the decade since it was introduced, guest kernel / hardware support has
provided a better alternative.
Andrew Cooper (4):
x86: Begin to remove TSC mode PVRDTSCP
x86/pv: Remove deferred RDTSC{,P} handling in pv_emulate_privileged_op()
x86/msr: Handle MSR_TSC_AUX consistently for PV and HVM guests
x86/pv: Expose RDTSCP to PV guests
xen/arch/x86/domain.c | 3 +--
xen/arch/x86/domctl.c | 5 -
xen/arch/x86/hvm/hvm.c | 18 +--
xen/arch/x86/hvm/svm/svm.c | 4 ++--
xen/arch/x86/hvm/vmx/vmx.c | 4 ++--
xen/arch/x86/msr.c | 18 +++
xen/arch/x86/pv/emul-inv-op.c | 27 +-
xen/arch/x86/pv/emul-priv-op.c | 32 +-
xen/arch/x86/time.c | 35 -
xen/include/asm-x86/hvm/hvm.h | 6 -
xen/include/asm-x86/hvm/vcpu.h | 1 -
xen/include/asm-x86/msr.h | 8 +++
xen/include/asm-x86/time.h | 6 +
xen/include/public/arch-x86/cpufeatureset.h | 2 +-
14 files changed, 44 insertions(+), 125 deletions(-)
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 3/4] x86/msr: Handle MSR_TSC_AUX consistently for PV and HVM guests
With PVRDTSCP mode removed, handling of MSR_TSC_AUX can move into the common
code. Move its storage into struct vcpu_msrs (dropping the HVM-specific
msr_tsc_aux), and add an RDPID feature check as this bit also enumerates the
presence of the MSR.
Drop hvm_msr_tsc_aux() entirely, and use v->arch.msrs->tsc_aux directly.
Update hvm_load_cpu_ctxt() to check that the incoming ctxt.msr_tsc_aux isn't
out of range. In practice, no previous version of Xen ever wrote an
out-of-range value. Add MSR_TSC_AUX to the list of MSRs migrated for PV
guests.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Konrad Rzeszutek Wilk
CC: Jun Nakajima
CC: Kevin Tian
CC: Boris Ostrovsky
CC: Suravee Suthikulpanit
CC: Brian Woods
---
xen/arch/x86/domain.c | 3 +--
xen/arch/x86/domctl.c | 2 ++
xen/arch/x86/hvm/hvm.c | 18 +-
xen/arch/x86/hvm/svm/svm.c | 4 ++--
xen/arch/x86/hvm/vmx/vmx.c | 4 ++--
xen/arch/x86/msr.c | 18 ++
xen/arch/x86/pv/emul-priv-op.c | 4
xen/include/asm-x86/hvm/hvm.h | 6 --
xen/include/asm-x86/hvm/vcpu.h | 1 -
xen/include/asm-x86/msr.h | 8
10 files changed, 38 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 295b10c..2067a0c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1593,8 +1593,7 @@ void paravirt_ctxt_switch_to(struct vcpu *v)
activate_debugregs(v);
if ( cpu_has_rdtscp )
-wrmsr_tsc_aux(v->domain->arch.tsc_mode == TSC_MODE_PVRDTSCP
- ? v->domain->arch.incarnation : 0);
+wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
}
/* Update per-VCPU guest runstate shared memory area (if registered). */
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 97ea5d8..b8d0796 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -1275,6 +1275,7 @@ long arch_do_domctl(
static const uint32_t msrs_to_send[] = {
MSR_SPEC_CTRL,
MSR_INTEL_MISC_FEATURES_ENABLES,
+MSR_TSC_AUX,
};
uint32_t nr_msrs = ARRAY_SIZE(msrs_to_send);
@@ -1399,6 +1400,7 @@ long arch_do_domctl(
{
case MSR_SPEC_CTRL:
case MSR_INTEL_MISC_FEATURES_ENABLES:
+case MSR_TSC_AUX:
if ( guest_wrmsr(v, msr.index, msr.value) != X86EMUL_OKAY )
break;
continue;
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0bc676c..1e4fc7d 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -774,7 +774,7 @@ static int hvm_save_cpu_ctxt(struct vcpu *v,
hvm_domain_context_t *h)
struct segment_register seg;
struct hvm_hw_cpu ctxt = {
.tsc = hvm_get_guest_tsc_fixed(v, v->domain->arch.hvm.sync_tsc),
-.msr_tsc_aux = hvm_msr_tsc_aux(v),
+.msr_tsc_aux = v->arch.msrs->tsc_aux,
.rax = v->arch.user_regs.rax,
.rbx = v->arch.user_regs.rbx,
.rcx = v->arch.user_regs.rcx,
@@ -1040,7 +1040,10 @@ static int hvm_load_cpu_ctxt(struct domain *d,
hvm_domain_context_t *h)
if ( hvm_funcs.tsc_scaling.setup )
hvm_funcs.tsc_scaling.setup(v);
-v->arch.hvm.msr_tsc_aux = ctxt.msr_tsc_aux;
+if ( ctxt.msr_tsc_aux != (uint32_t)ctxt.msr_tsc_aux )
+return -EINVAL;
+
+v->arch.msrs->tsc_aux = ctxt.msr_tsc_aux;
hvm_set_guest_tsc_fixed(v, ctxt.tsc, d->arch.hvm.sync_tsc);
@@ -3400,10 +3403,6 @@ int hvm_msr_read_intercept(unsigned int msr, uint64_t
*msr_content)
*msr_content = v->arch.hvm.msr_tsc_adjust;
break;
-case MSR_TSC_AUX:
-*msr_content = hvm_msr_tsc_aux(v);
-break;
-
case MSR_IA32_APICBASE:
*msr_content = vcpu_vlapic(v)->hw.apic_base_msr;
break;
@@ -3556,13 +3555,6 @@ int hvm_msr_write_intercept(unsigned int msr, uint64_t
msr_content,
hvm_set_guest_tsc_adjust(v, msr_content);
break;
-case MSR_TSC_AUX:
-v->arch.hvm.msr_tsc_aux = (uint32_t)msr_content;
-if ( cpu_has_rdtscp
- && (v->domain->arch.tsc_mode != TSC_MODE_PVRDTSCP) )
-wrmsr_tsc_aux(msr_content);
-break;
-
case MSR_IA32_APICBASE:
if ( !vlapic_msr_set(vcpu_vlapic(v), msr_content) )
goto gp_fault;
diff --git a/xen/arch/x86/hvm/svm/svm.c b/xen/arch/x86/hvm/svm/svm.c
index 396ee4a..e42e152 100644
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -1136,7 +1136,7 @@ static void svm_ctxt_switch_to(struct vcpu *v)
svm_tsc_ratio_load(v);
if ( cpu_has_rdtscp )
-wrmsr_tsc_aux(hvm_msr_tsc_aux(v));
+wrmsr_tsc_aux(v->arch.msrs->tsc_aux);
}
static void noreturn svm_do_resume(struct vcpu *v)
@@ -3063,7 +3063,7 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
break;
case VMEXIT_RDTSCP:
-regs->rcx =
[Xen-devel] [PATCH 2/4] x86/pv: Remove deferred RDTSC{, P} handling in pv_emulate_privileged_op()
As noted in c/s 4999bf3e8b "x86/PV: use generic emulator for privileged
instruction handling", these hoops are jumped through to retain the older
behaviour, along with a note suggesting that we should reconsider things.
It does not matter exactly when pv_soft_rdtsc() is called, as Xen's behaviour
is an opaque atomic action from the guests point of view. Furthermore, even
with PVRDTSCP mode, the TSC_AUX value constant while the domain is executing.
Drop all the deferral logic, and leave TSC_AUX uniformly at 0 as PVRDTSCP mode
is being removed. Later changes will make this behave architecturally.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Konrad Rzeszutek Wilk
CC: Boris Ostrovsky
---
xen/arch/x86/pv/emul-priv-op.c | 30 ++
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 83441b6..3641d31 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -51,9 +51,6 @@ struct priv_op_ctxt {
} cs;
char *io_emul_stub;
unsigned int bpmatch;
-unsigned int tsc;
-#define TSC_BASE 1
-#define TSC_AUX 2
};
/* I/O emulation support. Helper routines for, and type of, the stack stub. */
@@ -810,7 +807,6 @@ static inline bool is_cpufreq_controller(const struct
domain *d)
static int read_msr(unsigned int reg, uint64_t *val,
struct x86_emulate_ctxt *ctxt)
{
-struct priv_op_ctxt *poc = container_of(ctxt, struct priv_op_ctxt, ctxt);
const struct vcpu *curr = current;
const struct domain *currd = curr->domain;
bool vpmu_msr = false;
@@ -847,19 +843,11 @@ static int read_msr(unsigned int reg, uint64_t *val,
*val = curr->arch.pv.gs_base_user;
return X86EMUL_OKAY;
-/*
- * In order to fully retain original behavior, defer calling
- * pv_soft_rdtsc() until after emulation. This may want/need to be
- * reconsidered.
- */
case MSR_IA32_TSC:
-poc->tsc |= TSC_BASE;
-goto normal;
+*val = currd->arch.vtsc ? pv_soft_rdtsc(curr, ctxt->regs) : rdtsc();
+return X86EMUL_OKAY;
case MSR_TSC_AUX:
-poc->tsc |= TSC_AUX;
-if ( cpu_has_rdtscp )
-goto normal;
*val = 0;
return X86EMUL_OKAY;
@@ -1341,20 +1329,6 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs)
switch ( rc )
{
case X86EMUL_OKAY:
-if ( ctxt.tsc & TSC_BASE )
-{
-if ( currd->arch.vtsc || (ctxt.tsc & TSC_AUX) )
-{
-msr_split(regs, pv_soft_rdtsc(curr, regs));
-
-if ( ctxt.tsc & TSC_AUX )
-regs->rcx = (currd->arch.tsc_mode == TSC_MODE_PVRDTSCP
- ? currd->arch.incarnation : 0);
-}
-else
-msr_split(regs, rdtsc());
-}
-
if ( ctxt.ctxt.retire.singlestep )
ctxt.bpmatch |= DR_STEP;
if ( ctxt.bpmatch )
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/4] x86: Begin to remove TSC mode PVRDTSCP
For more historical context, see
c/s c17b36d5dc792cfdf59b6de0213b168bec0af8e8
c/s 04656384a1b9714e43db850c51431008e23450d8
PVRDTSCP was an attempt to provide Xen-aware userspace with a stable monotonic
clock, and enough information for said userspace to cope with frequency
changes across migrate. However, the PVRDTSCP infrastructure has resulted in
very tangled code, and non-architectural behaviour even in non-PVRDTSCP cases.
Seeing as the functionality has been replaced entirely by improvements in PV
clocks (including being plumbed into the VDSO nowadays), or alternatively by
hardware TSC scaling features, and no-one is aware of any remaining users of
this mode, take the opportunity to remove it.
For now, introduce an upper range check on the toolstack setting to
XEN_DOMCTL_settscinfo, and exclude TSC_MODE_PVRDTSCP from selection.
(Arguably, its a bug that this hypercall previously accepted any value and
turned into a nop). This will catch and cleanly reject attempts to migrate in
a VM configured to use PVRDTSCP, rather than letting it run and have the wrong
timing mode.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Konrad Rzeszutek Wilk
CC: Boris Ostrovsky
---
xen/arch/x86/domctl.c | 3 ++-
xen/arch/x86/time.c| 35 ---
xen/include/asm-x86/time.h | 5 +
3 files changed, 3 insertions(+), 40 deletions(-)
diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c
index 175a0c9..97ea5d8 100644
--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -970,7 +970,8 @@ long arch_do_domctl(
break;
case XEN_DOMCTL_settscinfo:
-if ( d == currd ) /* no domain_pause() */
+if ( d == currd || /* no domain_pause() */
+ domctl->u.tsc_info.tsc_mode > TSC_MODE_NEVER_EMULATE )
ret = -EINVAL;
else
{
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 24d4c27..3f095a2 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -2165,21 +2165,6 @@ void tsc_get_info(struct domain *d, uint32_t *tsc_mode,
*elapsed_nsec = scale_delta(tsc, >arch.vtsc_to_ns);
*gtsc_khz = enable_tsc_scaling ? d->arch.tsc_khz : cpu_khz;
break;
-case TSC_MODE_PVRDTSCP:
-if ( d->arch.vtsc )
-{
-*elapsed_nsec = get_s_time() - d->arch.vtsc_offset;
-*gtsc_khz = cpu_khz;
-}
-else
-{
-tsc = rdtsc();
-*elapsed_nsec = scale_delta(tsc, _cpu(cpu_time).tsc_scale) -
-d->arch.vtsc_offset;
-*gtsc_khz = enable_tsc_scaling ? d->arch.tsc_khz
- : 0 /* ignored by tsc_set_info */;
-}
-break;
}
if ( (int64_t)*elapsed_nsec < 0 )
@@ -2208,8 +2193,6 @@ void tsc_set_info(struct domain *d,
switch ( d->arch.tsc_mode = tsc_mode )
{
-bool enable_tsc_scaling;
-
case TSC_MODE_DEFAULT:
case TSC_MODE_ALWAYS_EMULATE:
d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
@@ -2235,24 +2218,6 @@ void tsc_set_info(struct domain *d,
d->arch.vtsc = 1;
d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
break;
-case TSC_MODE_PVRDTSCP:
-d->arch.vtsc = !boot_cpu_has(X86_FEATURE_RDTSCP) ||
- !host_tsc_is_safe();
-enable_tsc_scaling = is_hvm_domain(d) && !d->arch.vtsc &&
- hvm_get_tsc_scaling_ratio(gtsc_khz ?: cpu_khz);
-d->arch.tsc_khz = (enable_tsc_scaling && gtsc_khz) ? gtsc_khz :
cpu_khz;
-set_time_scale(>arch.vtsc_to_ns, d->arch.tsc_khz * 1000 );
-d->arch.ns_to_vtsc = scale_reciprocal(d->arch.vtsc_to_ns);
-if ( d->arch.vtsc )
-d->arch.vtsc_offset = get_s_time() - elapsed_nsec;
-else {
-/* when using native TSC, offset is nsec relative to power-on
- * of physical machine */
-d->arch.vtsc_offset = scale_delta(rdtsc(),
- _cpu(cpu_time).tsc_scale) -
- elapsed_nsec;
-}
-break;
}
d->arch.incarnation = incarnation + 1;
if ( is_hvm_domain(d) )
diff --git a/xen/include/asm-x86/time.h b/xen/include/asm-x86/time.h
index ce96ec9..070cdef 100644
--- a/xen/include/asm-x86/time.h
+++ b/xen/include/asm-x86/time.h
@@ -12,10 +12,7 @@
*2 = guest rdtsc always executed natively (no monotonicity/frequency
* guarantees); guest rdtscp emulated at native frequency if
* unsupported by h/w, else executed natively
- *3 = same as 2, except xen manages TSC_AUX register so guest can
- * determine when a restore/migration has occurred and assumes
- * guest obtains/uses pvclock-like mechanism to adjust for
- * monotonicity and frequency changes
+ *3 = Removed, was PVRDTSCP.
*/
#define TSC_MODE_DEFAULT 0
[Xen-devel] [PATCH 4/4] x86/pv: Expose RDTSCP to PV guests
The final remnanat of PVRDTSCP is that we would emulate RDTSCP even on
hardware which lacked the instruction. RDTSCP is available on almost all
64-bit x86 hardware.
Remove this emulation, drop the TSC_MODE_PVRDTSCP constant, and allow RDTSCP
in a PV guest's CPUID policy.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Konrad Rzeszutek Wilk
CC: Boris Ostrovsky
---
xen/arch/x86/pv/emul-inv-op.c | 27 +--
xen/include/asm-x86/time.h | 1 -
xen/include/public/arch-x86/cpufeatureset.h | 2 +-
3 files changed, 2 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/pv/emul-inv-op.c b/xen/arch/x86/pv/emul-inv-op.c
index 56f5a45..91d0579 100644
--- a/xen/arch/x86/pv/emul-inv-op.c
+++ b/xen/arch/x86/pv/emul-inv-op.c
@@ -41,31 +41,6 @@
#include "emulate.h"
-static int emulate_invalid_rdtscp(struct cpu_user_regs *regs)
-{
-char opcode[3];
-unsigned long eip, rc;
-struct vcpu *v = current;
-const struct domain *currd = v->domain;
-
-eip = regs->rip;
-if ( (rc = copy_from_user(opcode, (char *)eip, sizeof(opcode))) != 0 )
-{
-pv_inject_page_fault(0, eip + sizeof(opcode) - rc);
-return EXCRET_fault_fixed;
-}
-if ( memcmp(opcode, "\xf\x1\xf9", sizeof(opcode)) )
-return 0;
-eip += sizeof(opcode);
-
-msr_split(regs, pv_soft_rdtsc(v, regs));
-regs->rcx = (currd->arch.tsc_mode == TSC_MODE_PVRDTSCP
- ? currd->arch.incarnation : 0);
-
-pv_emul_instruction_done(regs, eip);
-return EXCRET_fault_fixed;
-}
-
static int emulate_forced_invalid_op(struct cpu_user_regs *regs)
{
char sig[5], instr[2];
@@ -121,7 +96,7 @@ static int emulate_forced_invalid_op(struct cpu_user_regs
*regs)
bool pv_emulate_invalid_op(struct cpu_user_regs *regs)
{
-return !emulate_invalid_rdtscp(regs) && !emulate_forced_invalid_op(regs);
+return !emulate_forced_invalid_op(regs);
}
/*
diff --git a/xen/include/asm-x86/time.h b/xen/include/asm-x86/time.h
index 070cdef..d95814e 100644
--- a/xen/include/asm-x86/time.h
+++ b/xen/include/asm-x86/time.h
@@ -17,7 +17,6 @@
#define TSC_MODE_DEFAULT 0
#define TSC_MODE_ALWAYS_EMULATE 1
#define TSC_MODE_NEVER_EMULATE2
-#define TSC_MODE_PVRDTSCP 3
typedef u64 cycles_t;
diff --git a/xen/include/public/arch-x86/cpufeatureset.h
b/xen/include/public/arch-x86/cpufeatureset.h
index 6c82816..fbc68fa 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -156,7 +156,7 @@ XEN_CPUFEATURE(NX,2*32+20) /*A Execute Disable
*/
XEN_CPUFEATURE(MMXEXT,2*32+22) /*A AMD MMX extensions */
XEN_CPUFEATURE(FFXSR, 2*32+25) /*A FFXSR instruction optimizations */
XEN_CPUFEATURE(PAGE1GB, 2*32+26) /*H 1Gb large page support */
-XEN_CPUFEATURE(RDTSCP,2*32+27) /*S RDTSCP */
+XEN_CPUFEATURE(RDTSCP,2*32+27) /*A RDTSCP */
XEN_CPUFEATURE(LM,2*32+29) /*A Long Mode (x86-64) */
XEN_CPUFEATURE(3DNOWEXT, 2*32+30) /*A AMD 3DNow! extensions */
XEN_CPUFEATURE(3DNOW, 2*32+31) /*A 3DNow! */
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 13/18] xen/arm: Implement PSCI SYSTEM_SUSPEND call (physical interface)
Hi,
On 11/12/18 11:30 AM, Mirela Simonovic wrote:
PSCI system suspend function shall be invoked to finalize Xen suspend
procedure. Resume entry point, which needs to be passed via 1st argument
of PSCI system suspend call to the EL3, is hyp_resume. For now, hyp_resume
is just a placeholder that will be implemented in assembly. Context ID,
which is 2nd argument of system suspend PSCI call, is unused, as in Linux.
Signed-off-by: Mirela Simonovic
Signed-off-by: Saeed Nowshadi
---
Changes in v2:
-The commit message was stale - referring to the do_suspend function
that has been renamed long time ago. Fixed commit message
---
xen/arch/arm/arm64/entry.S| 3 +++
xen/arch/arm/psci.c | 16
xen/arch/arm/suspend.c| 4
xen/include/asm-arm/psci.h| 1 +
xen/include/asm-arm/suspend.h | 1 +
5 files changed, 25 insertions(+)
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 97b05f53ea..dbc4717903 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -533,6 +533,9 @@ ENTRY(__context_switch)
mov sp, x9
ret
+ENTRY(hyp_resume)
+b .
+
/*
* Local variables:
* mode: ASM
diff --git a/xen/arch/arm/psci.c b/xen/arch/arm/psci.c
index a93121f43b..b100bd8ad2 100644
--- a/xen/arch/arm/psci.c
+++ b/xen/arch/arm/psci.c
@@ -24,6 +24,7 @@
#include
#include
#include
+#include
/*
* While a 64-bit OS can make calls with SMC32 calling conventions, for
@@ -67,6 +68,21 @@ void call_psci_cpu_off(void)
}
}
+int call_psci_system_suspend(void)
+{
SYSTEM_SUSPEND was introduced by PSCI 1.0 and optional. So you need to
check the PSCI version and use PSCI_FEATURES to check if it was implemented.
+#ifdef CONFIG_ARM_64
+struct arm_smccc_res res;
+
+/* 2nd argument (context ID) is not used */
It still needs to be defined to some known values rather than whatever
is in x2 at that time.
But I would suggest to make good use of it to catch implementation not
doing the right thing. We could define it to 0xdeadbeef and shout at
anyone not preserving the value.
+arm_smccc_smc(PSCI_1_0_FN64_SYSTEM_SUSPEND, __pa(hyp_resume), );
+
+return PSCI_RET(res);
+#else
+/* not supported */
+return 1;
+#endif
+}
+
void call_psci_system_off(void)
{
if ( psci_ver > PSCI_VERSION(0, 1) )
diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
index d1b48c339a..37926374bc 100644
--- a/xen/arch/arm/suspend.c
+++ b/xen/arch/arm/suspend.c
@@ -141,6 +141,10 @@ static long system_suspend(void *data)
goto resume_irqs;
}
+status = call_psci_system_suspend();
Some platform may not support PSCI at all. So this need to be check.
+if ( status )
+dprintk(XENLOG_ERR, "PSCI system suspend failed, err=%d\n", status);
+
system_state = SYS_STATE_resume;
gic_resume();
diff --git a/xen/include/asm-arm/psci.h b/xen/include/asm-arm/psci.h
index 26462d0c47..9f6116a224 100644
--- a/xen/include/asm-arm/psci.h
+++ b/xen/include/asm-arm/psci.h
@@ -20,6 +20,7 @@ extern uint32_t psci_ver;
int psci_init(void);
int call_psci_cpu_on(int cpu);
+int call_psci_system_suspend(void);
void call_psci_cpu_off(void);
void call_psci_system_off(void);
void call_psci_system_reset(void);
diff --git a/xen/include/asm-arm/suspend.h b/xen/include/asm-arm/suspend.h
index de787d296a..7604e2e2e2 100644
--- a/xen/include/asm-arm/suspend.h
+++ b/xen/include/asm-arm/suspend.h
@@ -2,6 +2,7 @@
#define __ASM_ARM_SUSPEND_H__
int32_t domain_suspend(register_t epoint, register_t cid);
+void hyp_resume(void);
#endif
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume
On 11/15/18 6:57 PM, Stefano Stabellini wrote: On Wed, 14 Nov 2018, Julien Grall wrote: On 14/11/2018 23:04, Stefano Stabellini wrote: On Wed, 14 Nov 2018, Julien Grall wrote: - return -ENOSYS; Why do you return -ENOSYS until this patch? Should not have it be 0? To distinguish that Xen suspend wasn't supported until we at least do something useful in suspend procedure. This is not so important, we can return 0 but needs to be fixed in previous patches. If you return 0 before hand you can more easily bisect this series and know where it suspend/resume breaks. Why 0 improves bisectability? 0 prevents other checks from figuring out that there was an error. But this code is exactly replacing -ENOSYS by state (that would be 0 in success. So what's wrong to return 0 rather than -ENOSYS in that patch implement the dummy system_state? Also, the feature is not bisectable until the full series is applied. Really? I was under the impression you can still do some sort of suspend/resume patch by patch. Although, you would not do a full suspend/resume. You are saying that we could call the function and return successfully even if the function does nothing, simply by returning 0. That would make suspend bisectable within this series, patch by patch. I think it's impressive that Mirela managed to write the series this way, and if suspend is actually bisectable patch by patch simply by returning 0 here, it would be amazing, and certainly worth doing. However, if it is not the case, I wouldn't ask Mirela to make the effort to make suspend bisectable patch by patch beyond returning 0 here, it would be good to have but not required. This wasn't my intention :). Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130144: regressions - FAIL
flight 130144 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130144/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z9 days Failing since129526 2018-11-06 20:49:26 Z8 days 82 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days4 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6 4/4] x86/altp2m: fix display frozen when switching to a new view early
On 11/15/18 10:26 PM, George Dunlap wrote: > > >> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru >> wrote: >> >> When an new altp2m view is created very early on guest boot, the >> display will freeze (although the guest will run normally). This >> may also happen on resizing the display. The reason is the way >> Xen currently (mis)handles logdirty VGA: it intentionally >> misconfigures VGA pages so that they will fault. >> >> The problem is that it only does this in the host p2m. Once we >> switch to a new altp2m, the misconfigured entries will no longer >> fault, so the display will not be updated. >> >> This patch: >> * updates ept_handle_misconfig() to use the active altp2m instead >> of the hostp2m; >> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed >> and p2m_change_type_range() to propagate their changes to all >> valid altp2ms. >> >> With the introduction of altp2m fields in p2m_memory_type_changed() >> the whole function has been put under CONFIG_HVM. > > Sorry — actually, I think there’s yet another issue lurking here: > p2m_finish_type_change(). I’ll poke around a bit more tomorrow. OK, I'll wait for your recommendation before working on the last patch of the series. Thanks, Razvan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 05/18] xen/arm: Trigger Xen suspend when Dom0 completes suspend
On 11/15/18 7:17 PM, Mirela Simonovic wrote:
Hi Julien,
On Thu, Nov 15, 2018 at 7:23 PM Julien Grall wrote:
Hi Mirela,
On 11/12/18 11:30 AM, Mirela Simonovic wrote:
When Dom0 finalizes its suspend procedure the suspend of Xen is
triggered by calling system_suspend(). Dom0 finalizes the suspend from
its boot core (VCPU#0), which could be mapped to any physical CPU,
i.e. the system_suspend() function could be executed by any physical
CPU. Since Xen suspend procedure has to be run by the boot CPU
(non-boot CPUs will be disabled at some point in suspend procedure),
system_suspend() execution has to continue on CPU#0.
When the system_suspend() returns 0, it means that the system was
suspended and it is coming out of the resume procedure. Regardless
of the system_suspend() return value, after this function returns
Xen is fully functional, and its state, including all devices and data
structures, matches the state prior to calling system_suspend().
The status is returned by system_suspend() for debugging/logging
purposes and function prototype compatibility.
Signed-off-by: Mirela Simonovic
Signed-off-by: Saeed Nowshadi
---
xen/arch/arm/suspend.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
index f2338e41db..21b45f8248 100644
--- a/xen/arch/arm/suspend.c
+++ b/xen/arch/arm/suspend.c
@@ -112,11 +112,20 @@ static void vcpu_suspend(register_t epoint, register_t
cid)
_arch_set_info_guest(v, );
}
+/* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
+static long system_suspend(void *data)
+{
+BUG_ON(system_state != SYS_STATE_active);
+
+return -ENOSYS;
+}
+
int32_t domain_suspend(register_t epoint, register_t cid)
{
struct vcpu *v;
struct domain *d = current->domain;
bool is_thumb = epoint & 1;
+int status;
dprintk(XENLOG_DEBUG,
"Dom%d suspend: epoint=0x%"PRIregister", cid=0x%"PRIregister"\n",
@@ -156,6 +165,31 @@ int32_t domain_suspend(register_t epoint, register_t cid)
*/
vcpu_block_unless_event_pending(current);
+/* If this was dom0 the whole system should suspend: trigger Xen suspend */
+if ( is_hardware_domain(d) )
+{
+/*
+ * system_suspend should be called when Dom0 finalizes the suspend
+ * procedure from its boot core (VCPU#0). However, Dom0's VCPU#0 could
+ * be mapped to any PCPU (this function could be executed by any PCPU).
+ * The suspend procedure has to be finalized by the PCPU#0 (non-boot
+ * PCPUs will be disabled during the suspend).
+ */
+status = continue_hypercall_on_cpu(0, system_suspend, NULL);
+/*
+ * If an error happened, there is nothing that needs to be done here
+ * because the system_suspend always returns in fully functional state
+ * no matter what the outcome of suspend procedure is. If the system
+ * suspended successfully the function will return 0 after the resume.
+ * Otherwise, if an error is returned it means Xen did not suspended,
+ * but it is still in the same state as if the system_suspend was never
+ * called. We dump a debug message in case of an error for debugging/
+ * logging purpose.
+ */
+if ( status )
+dprintk(XENLOG_ERR, "Failed to suspend, errno=%d\n", status);
+}
After discussing we Stefano today, I have one more question regarding
Dom0 suspend.
From my understanding, the host may resume because of an event
targeting a guest. This means that Dom0 would still be blocked. As Dom0
would contain PV backend, how do you expect this to work?
Is there any potential dependency between frontend and backend? Or would
Dom0 be resume when the PV frontend probe the backend?
We have assumed that Dom0 has to resume whenever Xen resume. So if the
wake-up interrupt was targeted to DomU, the Dom0 would resume
regarless. Vice versa does not hold.
This is done in patch "[PATCH 17/18] xen/arm: Resume Dom0 after Xen
resumes" - the Dom0 is simply unblocked at the end of Xen's resume.
How about the disaggregated case where backend may live in a different
guest. How is this going to happen?
I have heard Stefano suggesting to resume all the domains but that seem
to be a massive hammer to solve it.
I am wondering whether we can rely on the frontend sending an event (i.e
event channel) in part of resuming the PV protocols.
Did you test it the PV protocols in the suspend/resume? I know that
Linux does not have everything for suspend/resume Xen Arm today. There
are some patches inflight (see [1]).
[1]
https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg00823.html
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 17/18] xen/arm: Resume Dom0 after Xen resumes
Hi, On 11/12/18 11:30 AM, Mirela Simonovic wrote: The resume of Dom0 should always follow Xen's resume. This is done by unblocking the first vCPU of Dom0. Please explain why you always need to resume Dom0 afterwards. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 03/18] xen/arm: Save GIC and virtual timer context when the domain suspends
Hi, On 11/15/18 12:35 AM, Stefano Stabellini wrote: On Wed, 14 Nov 2018, Julien Grall wrote: On 14/11/2018 22:45, Stefano Stabellini wrote: On Wed, 14 Nov 2018, Julien Grall wrote: Hi, On 13/11/2018 20:44, Stefano Stabellini wrote: On Mon, 12 Nov 2018, Julien Grall wrote: (+ Andre) On 11/12/18 5:42 PM, Mirela Simonovic wrote: Hi Julien, On Mon, Nov 12, 2018 at 6:00 PM Julien Grall wrote: On 11/12/18 4:52 PM, Mirela Simonovic wrote: Hi Julien, Hi, Thanks for the feedback. On Mon, Nov 12, 2018 at 4:36 PM Julien Grall wrote: Hi Mirela, On 11/12/18 11:30 AM, Mirela Simonovic wrote: GIC and virtual timer context must be saved when the domain suspends. Please provide the rationale for this. Is it required by the spec? This is required for GIC because a guest leaves enabled interrupts in the GIC that could wake it up, and on resume it should be able to detect which interrupt woke it up. Without saving/restoring the state of GIC this would not be possible. I am confused. In patch #10, you save the GIC host because the GIC can be powered-down. Linux is also saving the GIC state. So how the interrupt can come up if the GIC is powered down? After Xen (or Linux in the config without Xen) hands over the control to EL3 on suspend (calls system suspend PSCI to EL3), it leaves enabled interrupts that are its wake-up sources. Saving a GIC state only means that its current configuration will be remembered somewhere in data structures, but the configuration is not changed on suspend. Everything that happens with interrupt configuration from this point on is platform specific. Now there are 2 options: 1) EL3 will never allow CPU0 to be powered down and the wake-up interrupt will indeed propagate via GIC; or 2) CPU0 will be powered down and the GIC may be powered down as well, so an external help is needed to deal with wake-up and resume (e.g. in order to react to a wake-up interrupt while the GIC is down, and power up CPU0). This external help is provided by some low-power processor outside of the Cortex-A cluster. So the platform firmware is responsible for properly configuring a wake-up path if GIC goes down. This is commonly handled by EL3 communicating with low-power processor. When the GIC power comes up, the interrupt triggered by a peripheral is still active and the software on Cortex-A cluster should be able to observe it once the GIC state is restored, i.e. interrupts get enabled at GIC. Thank you for the explanation. Now the question is why can't we reset at least the GIC CPU interface? AFAIU, the guest should restore them in any case. The only things we need to know is the interrupt was received for a given guest. We can then queue it and wake-up the domain. This seems to fit with the description on top of gic_dist_save() in Linux GICv2 driver. Can we rely on all PSCI compliant OSes to restore their own GIC again at resume? The PSCI spec is not very clear in that regard (at the the version I am looking at...) I am just asking so that we don't come up with a solution that only works with Linux. See PSCI 1.1 (DEN0022D) section 6.8. Each level should save its own context because the PSCI implementations is allowed to shutdown the GIC. Great, in that case we should be able to skip saving some of the GICD registers too. We do need to save GICD_ISACTIVER, and GICD_ICFGR, but we should be able to skip the others (GICD_ISENABLER, GICD_IPRIORITYR, GICD_ITARGETSR). If we do, we still need to re-initialize them as we do in gicv2_dist_init. You are assuming a domain will handle properly the suspend/resume. I don't think we can promise that as we call freeze/thaw. Dho! That would break every single guest that has been forcefully suspended :-/ Right, we can't do that (FYI I tested today the series with an unaware domU and it all resumed correctly.) But given that we only suspend/resume GICC_CTLR, GICC_PMR, GICC_BPR of the GICC interface, it should be fine to re-initialize that. We do need to be careful because the current implementation of gicv2_cpu_init touches a bunch of GICD registers that we'll have to save separately for suspend/resume. See my review on patch #10. I suggested to move out GICC_CTLR, GICC_PMR, GICC_BPR in a separate helpers that could be called by gicv2_cpu_init and the new function. A good name for that helper would be gicv2_cpu_if_up(). Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6 4/4] x86/altp2m: fix display frozen when switching to a new view early
> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru > wrote: > > When an new altp2m view is created very early on guest boot, the > display will freeze (although the guest will run normally). This > may also happen on resizing the display. The reason is the way > Xen currently (mis)handles logdirty VGA: it intentionally > misconfigures VGA pages so that they will fault. > > The problem is that it only does this in the host p2m. Once we > switch to a new altp2m, the misconfigured entries will no longer > fault, so the display will not be updated. > > This patch: > * updates ept_handle_misconfig() to use the active altp2m instead > of the hostp2m; > * modifies p2m_change_entry_type_global(), p2m_memory_type_changed > and p2m_change_type_range() to propagate their changes to all > valid altp2ms. > > With the introduction of altp2m fields in p2m_memory_type_changed() > the whole function has been put under CONFIG_HVM. Sorry — actually, I think there’s yet another issue lurking here: p2m_finish_type_change(). I’ll poke around a bit more tomorrow. -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
Hi Mirela,
On 11/15/18 11:42 AM, Mirela Simonovic wrote:
Hi Julien,
On Thu, Nov 15, 2018 at 12:38 PM Julien Grall wrote:
Hi,
On 11/15/18 11:10 AM, Mirela Simonovic wrote:
Hi Julien,
On Thu, Nov 15, 2018 at 11:59 AM Julien Grall wrote:
Hi Mirela,
On 11/15/18 10:33 AM, Mirela Simonovic wrote:
On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
wrote:
On 15/11/2018 10:13, Julien Grall wrote:
(+ Andre)
On 11/15/18 12:47 AM, Andrew Cooper wrote:
On 14/11/2018 12:49, Julien Grall wrote:
Hi Mirela,
On 14/11/2018 12:08, Mirela Simonovic wrote:
On 11/13/2018 09:32 AM, Andrew Cooper wrote:
On 12/11/2018 19:56, Julien Grall wrote:
Hi Andrew,
On 11/12/18 4:41 PM, Andrew Cooper wrote:
On 12/11/18 16:35, Mirela Simonovic wrote:
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index e594b48d81..7f8105465c 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu *p)
if ( is_idle_vcpu(p) )
return;
+/* VCPU's context should not be saved if its domain is
suspended */
+if ( p->domain->is_shut_down &&
+(p->domain->shutdown_code == SHUTDOWN_suspend) )
+return;
SHUTDOWN_suspend is used in Xen for other purpose (see
SCHEDOP_shutdown). The other user of that code relies on all the
state
to be saved on suspend.
We just need a flag to mark a domain as suspended, and I do
believe
SHUTDOWN_suspend is not used anywhere else.
Let's come back on this.
SHUTDOWN_suspend is used for migration. Grep for it through the
Xen
tree and you'll find several pieces of documentation, including the
description of what this shutdown code means>>>
What you are introducing here is not a shutdown - it is a suspend
with
the intent to resume executing later. As such, it shouldn't use
Xen's
shutdown infrastructure, which exists mainly to communicate with
the
toolstack.
Would domain pause/unpause be a better solution?
Actually yes - that sounds like a very neat solution.
I believe domain pause will not work here - a domain cannot pause
itself, i.e. current domain cannot be paused. This functionality
seems to assume that a domain is pausing another domain. Or I missed
the point.
Yes domain pause/unpause will not work. However, you can introduce a
boolean to tell you whether the domain was suspend.
I actually quite like how suspend work for x86 HVM. This is based on
pause/unpause. Have a look at hvm_s3_{suspend/resume}.
That only exists because the ACPI controller is/was implemented in
QEMU. I wouldn't recommend copying it.
May I ask why? I don't think the properties are very different from
Arm here.
If you observe, that can only be actioned by a hypercall from qemu. It
can't be actioned by an ACPI controller emulated by Xen.
Having spent some more time thinking about this problem, what properties
does the PSCI call have?
I gather from other parts of this thread that there may be a partial
reset of state? Beyond that, what else?
I ask, because conceptually, domU suspend to RAM is literally just
"de-schedule until we want to wake up", and in this case, it appears to
be "wake up on any of the still-active interrupts". We've already got a
scheduler API for that, and its called vcpu_block(). This already
exists with "wait until a new event occurs" semantics.
Is there anything else I've missed?
That's correct, and I agree. BTW that is what's implemented in this series.
All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
only events on that vCPU can trigger a resume. All the other event
should not be taken into account.
What other events are talking about here?
vcpu_unblock is not only called when you receive interrupts. It can be
called in other place when you receive an events.
From the Arm Arm, an event can be anything. So do we really want to
wake-up on any events?
My worry with vcpu_block() is we don't prevent the other vCPUs to run.
Technically they should be off, but I would like some safety to avoid
any potential corner case (i.e other way to turn a vCPU on).
Other vCPUs are hot-unplugged (offlined) by the guest. If that is not
the case, SYSTEM_SUSPEND will return an error.
Could you please clarify what a potential corner case would be?
PSCI CPU_ON is not the only way to online a vCPU. I merely want to
prevent other path to play with the vCPU when it is not necessary.
[...]
If instead of waiting for any event, you need to wait for a specific
event, there is also vcpu_poll() which is a related scheduler API.
~Andrew
Some content disappeared, so I'll write here to avoid thread branching.
The semantic of vCPU block and domain_pause is not the same. When
guest suspends the domain is not (and should not be) paused, instead
its last online vCPU is blocked waiting on an interrupt. That's it.
Well no, you will block until you receive an event. Interrupts are one
of them.
Do we want to consider all events as wakeup event?
I
Re: [Xen-devel] [PATCH 4/8] xen/arm: Add support for read-only foreign mappings
Hi,
On 11/15/18 7:48 PM, Stefano Stabellini wrote:
On Thu, 15 Nov 2018, Julien Grall wrote:
Hi Stefano,
On 11/15/18 6:44 PM, Stefano Stabellini wrote:
On Thu, 15 Nov 2018, Julien Grall wrote:
Hi,
On 11/15/18 1:19 PM, Andrii Anisov wrote:
So I would prefer to stick with _t which is quite common within the
p2m
code base so far.
I've found a similar code only in one place - p2m_get_entry()
function. And it is, at least, somehow commented there:
...
/* Allow t to be NULL */
t = t ?: &_t;
*t = p2m_invalid;
...
And in x86 code...
But IMO, it is really confusing to write a code to calculate and store
a value which clearly is not needed by a caller.
I disagree, you directly know that t can be NULL. Although, I agree that a
comment would help to understand.
From another hand, I'm not sure if a compiler would be intelligent
enough to factor out the odd code from execution pass on the incoming
null pointer.
You can't assume the compiler will inline even with the inline keyword.
I'm sorry, but I can't pass my RB for `_t`.
I think this request is unreasonable because this is a matter of taste.
While this is a nice feature to have in Xen 4.12 and address a long
standing
open issue on Arm. I don't require it and I am not inclined to address
bikeshedding.
So I will keep aside for now until someone cares about it.
Let's try to be reasonable and have constructive conversations. If we do
have to disagree, let's disagree on meaningful design decisions, not
silly code style issues :-)
Andrii, when something can be done equally well in two different ways,
especially when it is a matter of style, I think it is best to express
our preference, but not to force a contributor in a particular
direction, unless specifically stated in CODING_STYLE or equivalent
docs. We don't have written rules about code reviews, but if we had,
I think this should be one of them. (I would love to have written rulesAs
about code reviews.)
Julien, usually in situations like this, it is quicker to change the
code than to argue. I personally don't care and wouldn't ask you to
change it, but if a member of the community reads the code and has an
adverse reaction, it is always worth reconsidering the approach, because
others might find it antagonizing too. Given the choice, it is best to
go for something obvious to most, so if a new contributor sends a patch
to change, it is more likely to be correct from the start.
I agree with your point here but this is also valid in the other way. If the
suggested alternative provokes an adverse reaction to the contributor, then
why should he use it?
As I wrote earlier one trying to use ternary condition split over 2 lines is
not making the code more obvious. So I am not entirely sure why I should be
forced to write such code?
I don't think you should be. You can find another way. For instance, the
whole thing could be reduce to one more "if" condition:
if ( t != NULL )
{
*t = p2m_invalid;
if ( page->u.inuse.type_info & PGT_writable_page )
*t = p2m_ram_rw;
else
*t = p2m_ram_ro;
}
be creative :-)
Well the code suggested is different from what I intended :). t should
be set to invalid before the check mfn_valid/get_page. So this needs at
least 2 checks. 2 places were t != NULL needs to be explained.
As you said this is a matter of taste. If someone disagree on the coding
style, then he should suggest an alternative way fitting the requirement.
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable-smoke test] 130136: tolerable all pass - PUSHED
flight 130136 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/130136/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 2190c8160a802560029d5d260d5f6979302b53d0 baseline version: xen 2262e808f4665bee820b5bb536aff47e560bdcc3 Last test of basis 130122 2018-11-15 15:00:24 Z0 days Testing same since 130136 2018-11-15 18:00:46 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Ian Jackson Jan Beulich Tim Deegan Wei Liu jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-i386 pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 2262e808f4..2190c8160a 2190c8160a802560029d5d260d5f6979302b53d0 -> smoke ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/8] xen/arm: Add support for read-only foreign mappings
On Thu, 15 Nov 2018, Julien Grall wrote:
> Hi Stefano,
>
> On 11/15/18 6:44 PM, Stefano Stabellini wrote:
> > On Thu, 15 Nov 2018, Julien Grall wrote:
> > > Hi,
> > >
> > > On 11/15/18 1:19 PM, Andrii Anisov wrote:
> > > > > So I would prefer to stick with _t which is quite common within the
> > > > > p2m
> > > > > code base so far.
> > > >
> > > > I've found a similar code only in one place - p2m_get_entry()
> > > > function. And it is, at least, somehow commented there:
> > > > ...
> > > > /* Allow t to be NULL */
> > > > t = t ?: &_t;
> > > >
> > > > *t = p2m_invalid;
> > > > ...
> > >
> > > And in x86 code...
> > >
> > > >
> > > > But IMO, it is really confusing to write a code to calculate and store
> > > > a value which clearly is not needed by a caller.
> > >
> > > I disagree, you directly know that t can be NULL. Although, I agree that a
> > > comment would help to understand.
> > >
> > > > From another hand, I'm not sure if a compiler would be intelligent
> > > > enough to factor out the odd code from execution pass on the incoming
> > > > null pointer.
> > >
> > > You can't assume the compiler will inline even with the inline keyword.
> > >
> > > >
> > > > I'm sorry, but I can't pass my RB for `_t`.
> > >
> > > I think this request is unreasonable because this is a matter of taste.
> > >
> > > While this is a nice feature to have in Xen 4.12 and address a long
> > > standing
> > > open issue on Arm. I don't require it and I am not inclined to address
> > > bikeshedding.
> > >
> > > So I will keep aside for now until someone cares about it.
> >
> > Let's try to be reasonable and have constructive conversations. If we do
> > have to disagree, let's disagree on meaningful design decisions, not
> > silly code style issues :-)
> >
> > Andrii, when something can be done equally well in two different ways,
> > especially when it is a matter of style, I think it is best to express
> > our preference, but not to force a contributor in a particular
> > direction, unless specifically stated in CODING_STYLE or equivalent
> > docs. We don't have written rules about code reviews, but if we had,
> > I think this should be one of them. (I would love to have written rules
> > about code reviews.)
> >
> > Julien, usually in situations like this, it is quicker to change the
> > code than to argue. I personally don't care and wouldn't ask you to
> > change it, but if a member of the community reads the code and has an
> > adverse reaction, it is always worth reconsidering the approach, because
> > others might find it antagonizing too. Given the choice, it is best to
> > go for something obvious to most, so if a new contributor sends a patch
> > to change, it is more likely to be correct from the start.
>
> I agree with your point here but this is also valid in the other way. If the
> suggested alternative provokes an adverse reaction to the contributor, then
> why should he use it?
>
> As I wrote earlier one trying to use ternary condition split over 2 lines is
> not making the code more obvious. So I am not entirely sure why I should be
> forced to write such code?
I don't think you should be. You can find another way. For instance, the
whole thing could be reduce to one more "if" condition:
if ( t != NULL )
{
*t = p2m_invalid;
if ( page->u.inuse.type_info & PGT_writable_page )
*t = p2m_ram_rw;
else
*t = p2m_ram_ro;
}
be creative :-)
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 02/18] xen/arm: Implement PSCI system suspend call (virtual interface)
On Thu, 15 Nov 2018, Mirela Simonovic wrote:
> Hi Julien,
>
> On Thu, Nov 15, 2018 at 12:38 PM Julien Grall wrote:
> >
> > Hi,
> >
> > On 11/15/18 11:10 AM, Mirela Simonovic wrote:
> > > Hi Julien,
> > >
> > > On Thu, Nov 15, 2018 at 11:59 AM Julien Grall
> > > wrote:
> > >>
> > >> Hi Mirela,
> > >>
> > >> On 11/15/18 10:33 AM, Mirela Simonovic wrote:
> > >>> On Thu, Nov 15, 2018 at 11:26 AM Andrew Cooper
> > >>> wrote:
> >
> > On 15/11/2018 10:13, Julien Grall wrote:
> > > (+ Andre)
> > >
> > > On 11/15/18 12:47 AM, Andrew Cooper wrote:
> > >> On 14/11/2018 12:49, Julien Grall wrote:
> > >>> Hi Mirela,
> > >>>
> > >>> On 14/11/2018 12:08, Mirela Simonovic wrote:
> >
> >
> > On 11/13/2018 09:32 AM, Andrew Cooper wrote:
> > > On 12/11/2018 19:56, Julien Grall wrote:
> > >> Hi Andrew,
> > >>
> > >> On 11/12/18 4:41 PM, Andrew Cooper wrote:
> > >>> On 12/11/18 16:35, Mirela Simonovic wrote:
> > >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > >> index e594b48d81..7f8105465c 100644
> > >> --- a/xen/arch/arm/domain.c
> > >> +++ b/xen/arch/arm/domain.c
> > >> @@ -97,6 +97,11 @@ static void ctxt_switch_from(struct vcpu
> > >> *p)
> > >>if ( is_idle_vcpu(p) )
> > >>return;
> > >>
> > >> +/* VCPU's context should not be saved if its domain is
> > >> suspended */
> > >> +if ( p->domain->is_shut_down &&
> > >> +(p->domain->shutdown_code == SHUTDOWN_suspend) )
> > >> +return;
> > > SHUTDOWN_suspend is used in Xen for other purpose (see
> > > SCHEDOP_shutdown). The other user of that code relies on all
> > > the
> > > state
> > > to be saved on suspend.
> > >
> > We just need a flag to mark a domain as suspended, and I do
> > believe
> > SHUTDOWN_suspend is not used anywhere else.
> > Let's come back on this.
> > >>> SHUTDOWN_suspend is used for migration. Grep for it through the
> > >>> Xen
> > >>> tree and you'll find several pieces of documentation, including
> > >>> the
> > >>> description of what this shutdown code means>>>
> > >>> What you are introducing here is not a shutdown - it is a
> > >>> suspend
> > >>> with
> > >>> the intent to resume executing later. As such, it shouldn't use
> > >>> Xen's
> > >>> shutdown infrastructure, which exists mainly to communicate with
> > >>> the
> > >>> toolstack.
> > >> Would domain pause/unpause be a better solution?
> > > Actually yes - that sounds like a very neat solution.
> >
> > I believe domain pause will not work here - a domain cannot pause
> > itself, i.e. current domain cannot be paused. This functionality
> > seems to assume that a domain is pausing another domain. Or I
> > missed
> > the point.
> > >>>
> > >>> Yes domain pause/unpause will not work. However, you can introduce a
> > >>> boolean to tell you whether the domain was suspend.
> > >>>
> > >>> I actually quite like how suspend work for x86 HVM. This is based on
> > >>> pause/unpause. Have a look at hvm_s3_{suspend/resume}.
> > >>
> > >> That only exists because the ACPI controller is/was implemented in
> > >> QEMU. I wouldn't recommend copying it.
> > >
> > > May I ask why? I don't think the properties are very different from
> > > Arm here.
> >
> > If you observe, that can only be actioned by a hypercall from qemu. It
> > can't be actioned by an ACPI controller emulated by Xen.
> >
> > >
> > >>
> > >> Having spent some more time thinking about this problem, what
> > >> properties
> > >> does the PSCI call have?
> > >>
> > >> I gather from other parts of this thread that there may be a partial
> > >> reset of state? Beyond that, what else?
> > >>
> > >> I ask, because conceptually, domU suspend to RAM is literally just
> > >> "de-schedule until we want to wake up", and in this case, it appears
> > >> to
> > >> be "wake up on any of the still-active interrupts". We've already
> > >> got a
> > >> scheduler API for that, and its called vcpu_block(). This already
> > >> exists with "wait until a new event occurs" semantics.
> > >>
> > >> Is there anything else I've missed?
> > >
> > >>>
> > >>> That's correct, and I agree. BTW that is what's implemented in this
> > >>> series.
> > >>>
> > > All vCPUs but the vCPU calling SYSTEM_SUSPEND should be off. Also,
> > > only events on that vCPU can trigger a
[Xen-devel] [ovmf test] 130134: regressions - FAIL
flight 130134 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130134/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z9 days Failing since129526 2018-11-06 20:49:26 Z8 days 81 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days3 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 05/18] xen/arm: Trigger Xen suspend when Dom0 completes suspend
Hi Julien,
On Thu, Nov 15, 2018 at 7:23 PM Julien Grall wrote:
>
> Hi Mirela,
>
> On 11/12/18 11:30 AM, Mirela Simonovic wrote:
> > When Dom0 finalizes its suspend procedure the suspend of Xen is
> > triggered by calling system_suspend(). Dom0 finalizes the suspend from
> > its boot core (VCPU#0), which could be mapped to any physical CPU,
> > i.e. the system_suspend() function could be executed by any physical
> > CPU. Since Xen suspend procedure has to be run by the boot CPU
> > (non-boot CPUs will be disabled at some point in suspend procedure),
> > system_suspend() execution has to continue on CPU#0.
> >
> > When the system_suspend() returns 0, it means that the system was
> > suspended and it is coming out of the resume procedure. Regardless
> > of the system_suspend() return value, after this function returns
> > Xen is fully functional, and its state, including all devices and data
> > structures, matches the state prior to calling system_suspend().
> > The status is returned by system_suspend() for debugging/logging
> > purposes and function prototype compatibility.
> >
> > Signed-off-by: Mirela Simonovic
> > Signed-off-by: Saeed Nowshadi
> > ---
> > xen/arch/arm/suspend.c | 34 ++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > index f2338e41db..21b45f8248 100644
> > --- a/xen/arch/arm/suspend.c
> > +++ b/xen/arch/arm/suspend.c
> > @@ -112,11 +112,20 @@ static void vcpu_suspend(register_t epoint,
> > register_t cid)
> > _arch_set_info_guest(v, );
> > }
> >
> > +/* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
> > +static long system_suspend(void *data)
> > +{
> > +BUG_ON(system_state != SYS_STATE_active);
> > +
> > +return -ENOSYS;
> > +}
> > +
> > int32_t domain_suspend(register_t epoint, register_t cid)
> > {
> > struct vcpu *v;
> > struct domain *d = current->domain;
> > bool is_thumb = epoint & 1;
> > +int status;
> >
> > dprintk(XENLOG_DEBUG,
> > "Dom%d suspend: epoint=0x%"PRIregister",
> > cid=0x%"PRIregister"\n",
> > @@ -156,6 +165,31 @@ int32_t domain_suspend(register_t epoint, register_t
> > cid)
> >*/
> > vcpu_block_unless_event_pending(current);
> >
> > +/* If this was dom0 the whole system should suspend: trigger Xen
> > suspend */
> > +if ( is_hardware_domain(d) )
> > +{
> > +/*
> > + * system_suspend should be called when Dom0 finalizes the suspend
> > + * procedure from its boot core (VCPU#0). However, Dom0's VCPU#0
> > could
> > + * be mapped to any PCPU (this function could be executed by any
> > PCPU).
> > + * The suspend procedure has to be finalized by the PCPU#0
> > (non-boot
> > + * PCPUs will be disabled during the suspend).
> > + */
> > +status = continue_hypercall_on_cpu(0, system_suspend, NULL);
> > +/*
> > + * If an error happened, there is nothing that needs to be done
> > here
> > + * because the system_suspend always returns in fully functional
> > state
> > + * no matter what the outcome of suspend procedure is. If the
> > system
> > + * suspended successfully the function will return 0 after the
> > resume.
> > + * Otherwise, if an error is returned it means Xen did not
> > suspended,
> > + * but it is still in the same state as if the system_suspend was
> > never
> > + * called. We dump a debug message in case of an error for
> > debugging/
> > + * logging purpose.
> > + */
> > +if ( status )
> > +dprintk(XENLOG_ERR, "Failed to suspend, errno=%d\n", status);
> > +}
>
> After discussing we Stefano today, I have one more question regarding
> Dom0 suspend.
>
> From my understanding, the host may resume because of an event
> targeting a guest. This means that Dom0 would still be blocked. As Dom0
> would contain PV backend, how do you expect this to work?
>
> Is there any potential dependency between frontend and backend? Or would
> Dom0 be resume when the PV frontend probe the backend?
>
We have assumed that Dom0 has to resume whenever Xen resume. So if the
wake-up interrupt was targeted to DomU, the Dom0 would resume
regarless. Vice versa does not hold.
This is done in patch "[PATCH 17/18] xen/arm: Resume Dom0 after Xen
resumes" - the Dom0 is simply unblocked at the end of Xen's resume.
> Cheers,
>
> --
> Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/8] xen/arm: Add support for read-only foreign mappings
Hi Stefano, On 11/15/18 6:44 PM, Stefano Stabellini wrote: On Thu, 15 Nov 2018, Julien Grall wrote: Hi, On 11/15/18 1:19 PM, Andrii Anisov wrote: So I would prefer to stick with _t which is quite common within the p2m code base so far. I've found a similar code only in one place - p2m_get_entry() function. And it is, at least, somehow commented there: ... /* Allow t to be NULL */ t = t ?: &_t; *t = p2m_invalid; ... And in x86 code... But IMO, it is really confusing to write a code to calculate and store a value which clearly is not needed by a caller. I disagree, you directly know that t can be NULL. Although, I agree that a comment would help to understand. From another hand, I'm not sure if a compiler would be intelligent enough to factor out the odd code from execution pass on the incoming null pointer. You can't assume the compiler will inline even with the inline keyword. I'm sorry, but I can't pass my RB for `_t`. I think this request is unreasonable because this is a matter of taste. While this is a nice feature to have in Xen 4.12 and address a long standing open issue on Arm. I don't require it and I am not inclined to address bikeshedding. So I will keep aside for now until someone cares about it. Let's try to be reasonable and have constructive conversations. If we do have to disagree, let's disagree on meaningful design decisions, not silly code style issues :-) Andrii, when something can be done equally well in two different ways, especially when it is a matter of style, I think it is best to express our preference, but not to force a contributor in a particular direction, unless specifically stated in CODING_STYLE or equivalent docs. We don't have written rules about code reviews, but if we had, I think this should be one of them. (I would love to have written rules about code reviews.) Julien, usually in situations like this, it is quicker to change the code than to argue. I personally don't care and wouldn't ask you to change it, but if a member of the community reads the code and has an adverse reaction, it is always worth reconsidering the approach, because others might find it antagonizing too. Given the choice, it is best to go for something obvious to most, so if a new contributor sends a patch to change, it is more likely to be correct from the start. I agree with your point here but this is also valid in the other way. If the suggested alternative provokes an adverse reaction to the contributor, then why should he use it? As I wrote earlier one trying to use ternary condition split over 2 lines is not making the code more obvious. So I am not entirely sure why I should be forced to write such code? Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [linux-4.9 test] 129966: tolerable FAIL - PUSHED
flight 129966 linux-4.9 real [real] http://logs.test-lab.xenproject.org/osstest/logs/129966/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemut-win7-amd64 17 guest-stopfail like 128925 test-amd64-i386-xl-qemut-win7-amd64 17 guest-stop fail like 128925 test-amd64-amd64-xl-qemuu-win7-amd64 17 guest-stopfail like 128925 test-amd64-i386-xl-qemuu-win7-amd64 17 guest-stop fail like 128925 test-amd64-amd64-xl-qemuu-ws16-amd64 17 guest-stopfail like 128925 test-amd64-amd64-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail never pass test-amd64-i386-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-amd64-xl-pvhv2-intel 12 guest-start fail never pass test-amd64-amd64-libvirt-xsm 13 migrate-support-checkfail never pass test-amd64-i386-xl-qemuu-dmrestrict-amd64-dmrestrict 10 debian-hvm-install fail never pass test-amd64-amd64-xl-pvhv2-amd 12 guest-start fail never pass test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-amd64-i386-xl-pvshim12 guest-start fail never pass test-arm64-arm64-xl-credit1 13 migrate-support-checkfail never pass test-arm64-arm64-xl-credit1 14 saverestore-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-arm64-arm64-xl 13 migrate-support-checkfail never pass test-arm64-arm64-xl 14 saverestore-support-checkfail never pass test-arm64-arm64-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-i386-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-amd64-amd64-libvirt-qemuu-debianhvm-amd64-xsm 11 migrate-support-check fail never pass test-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-amd64-amd64-qemuu-nested-amd 17 debian-hvm-install/l1/l2 fail never pass test-amd64-amd64-libvirt-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 13 migrate-support-checkfail never pass test-armhf-armhf-xl-rtds 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-credit2 13 migrate-support-checkfail never pass test-armhf-armhf-xl-credit2 14 saverestore-support-checkfail never pass test-armhf-armhf-xl-cubietruck 13 migrate-support-checkfail never pass test-armhf-armhf-xl-multivcpu 13 migrate-support-checkfail never pass test-armhf-armhf-xl-cubietruck 14 saverestore-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-qemut-ws16-amd64 17 guest-stop fail never pass test-amd64-i386-xl-qemut-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-libvirt-raw 12 migrate-support-checkfail never pass test-armhf-armhf-libvirt-raw 13 saverestore-support-checkfail never pass test-amd64-i386-xl-qemuu-ws16-amd64 17 guest-stop fail never pass test-armhf-armhf-xl-vhd 12 migrate-support-checkfail never pass test-armhf-armhf-xl-vhd 13 saverestore-support-checkfail never pass 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-xl-qemut-win10-i386 10 windows-installfail never pass test-amd64-amd64-xl-qemuu-win10-i386 10 windows-installfail never pass test-amd64-i386-xl-qemuu-win10-i386 10 windows-install fail never pass test-amd64-i386-xl-qemut-win10-i386 10 windows-install fail never pass version targeted for testing: linux0bb1a5e5aa1728a2c501cdd916923ef44fc07e2f baseline version: linuxb24c9962b179803dc1d51f17cf1acc58be8bbb2e Last test of basis 128925 2018-10-22 07:16:34 Z 24 days Testing same since 129763 2018-11-10 16:18:59 Z5 days2 attempts
Re: [Xen-devel] [PATCH 08/18] xen/arm: Disable/enable non-boot physical CPUs on suspend/resume
On Wed, 14 Nov 2018, Julien Grall wrote:
> On 14/11/2018 23:04, Stefano Stabellini wrote:
> > On Wed, 14 Nov 2018, Julien Grall wrote:
> >> Hi Mirela,
> >>
> >> On 14/11/2018 13:00, Mirela Simonovic wrote:
> >>>
> >>>
> >>> On 11/14/2018 11:52 AM, Julien Grall wrote:
> Hi Mirela,
>
> On 12/11/2018 11:30, Mirela Simonovic wrote:
> > Non-boot CPUs have to be disabled on suspend and enabled on resume
> > (hotplug-based mechanism). Disabling non-boot CPUs will lead to PSCI
> > CPU_OFF to be called by each non-boot CPU. Depending on the underlying
> > platform capabilities, this may lead to the physical powering down of
> > CPUs. Tested on Xilinx Zynq Ultrascale+ MPSoC (including power down of
> > each non-boot CPU).
> >
> > Signed-off-by: Mirela Simonovic
> > Signed-off-by: Saeed Nowshadi
> > ---
> > xen/arch/arm/suspend.c | 15 ++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
> > index 575afd5eb8..dae1b1f7d6 100644
> > --- a/xen/arch/arm/suspend.c
> > +++ b/xen/arch/arm/suspend.c
> > @@ -1,4 +1,5 @@
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -115,17 +116,29 @@ static void vcpu_suspend(register_t epoint,
> > register_t cid)
> > /* Xen suspend. Note: data is not used (suspend is the suspend to
> > RAM)
> > */
> > static long system_suspend(void *data)
> > {
> > + int status;
> > +
> > BUG_ON(system_state != SYS_STATE_active);
> > system_state = SYS_STATE_suspend;
> > freeze_domains();
> > + status = disable_nonboot_cpus();
> > + if ( status )
> > + {
> > + system_state = SYS_STATE_resume;
> > + goto resume_nonboot_cpus;
> > + }
> > +
> > system_state = SYS_STATE_resume;
> > +resume_nonboot_cpus:
> > + enable_nonboot_cpus();
> > thaw_domains();
> > system_state = SYS_STATE_active;
> > + dsb(sy);
>
> Why do you need a dsb(sy) here?
>
> >>>
> >>> Updated value of system_state variable needs to be visible to other CPUs
> >>> before we move on
> >>
> >> We tend to write the reason on top of barrier why they are necessary. But
> >> I am
> >> still unsure to understand why this is important. What would happen if
> >> move on
> >> without it?
> >
> > That is a good question. I went through the code and it seems that the
> > only effect could be potentially taking the wrong path in
> > cpupool_cpu_add, but since that's called from a .notifier_call I don't
> > think it can happen in practice. It is always difficult to prove that
> > we don't need a barrier, it is easier to prove when we need a barrier,
> > but in this case it does look like we do not need it after all.
>
> It is also very easy to add barrier everywhere so we are sure what to do
> ;). If you need a barrier, then you need to give plausible explanation.
>
> In that case, if you need barrier here for system_state. Then what
> wouldn't you need it in other places where system_state is updated/read?
Right, no plausible explanation here, so no barrier.
> >
> > - return -ENOSYS;
>
> Why do you return -ENOSYS until this patch? Should not have it be 0?
>
> >>>
> >>> To distinguish that Xen suspend wasn't supported until we at least do
> >>> something useful in suspend procedure. This is not so important, we can
> >>> return 0 but needs to be fixed in previous patches.
> >>
> >> If you return 0 before hand you can more easily bisect this series and know
> >> where it suspend/resume breaks.
> >
> > Why 0 improves bisectability? 0 prevents other checks from figuring out
> > that there was an error.
>
> But this code is exactly replacing -ENOSYS by state (that would be 0 in
> success. So what's wrong to return 0 rather than -ENOSYS in that patch
> implement the dummy system_state?
>
> > Also, the feature is not bisectable until the
> > full series is applied.
>
> Really? I was under the impression you can still do some sort of
> suspend/resume patch by patch. Although, you would not do a full
> suspend/resume.
You are saying that we could call the function and return successfully
even if the function does nothing, simply by returning 0. That would
make suspend bisectable within this series, patch by patch.
I think it's impressive that Mirela managed to write the series this
way, and if suspend is actually bisectable patch by patch simply by
returning 0 here, it would be amazing, and certainly worth doing.
However, if it is not the case, I wouldn't ask Mirela to make the effort
to make suspend bisectable patch by patch beyond returning 0 here, it
would be good to have but not required.___
Xen-devel mailing list
Re: [Xen-devel] [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.
On Thu, Nov 15, 2018 at 05:41:44PM +, Anthony PERARD wrote:
> On Thu, Nov 01, 2018 at 06:32:07PM +0100, Marek Marczykowski-Górecki wrote:
> > On Thu, Nov 01, 2018 at 04:57:18PM +, Ian Jackson wrote:
> > > Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support
> > > for qemu-xen runnning in a Linux-based stubdomain."):
> > > > 2. pv console
> > > > pros:
> > > >- no qemu modifications
> > > >- same read()/write() on libxl side
> > > > cons:
> > > >- no out of band reset, needs libxl handling for that (skipping
> > > > negotiation)
> > >
> > > Doesn't this potentially mean that the qmp console connection can
> > > become irrecoverably desynchronised ? I don't know how you would
> > > recover from the situation where another libxl process had got halfway
> > > through some qmp stuff and been terminated (for whatever reason; maybe
> > > the calling toolstack crashed).
> >
> > That's right, it could result in irrecoverably desynchronised
> > connection. So, we need out of band reset.
>
> Actually, it looks like we can recover that situation without out of
> band reset. It's even in the spec[1]:
That's interesting. And it mention serial console explicitly as the use
case for this. Does it apply to monitor socket too, or guest agent only?
I'd much prefer to use console, as the code would be much simpler (the
same handling for local and stubdomain qemu).
Also, this doesn't cover capabilities (re-)negotiation. While actual
capabilities are probably not a problem, libxl do store qemu version
from the server greeting (is it used anywhere?).
(...)
> previous section:
> 2.6 Forcing the JSON parser into known-good state
> -
>
> Incomplete or invalid input can leave the server's JSON parser in a
> state where it can't parse additional commands. To get it back into
> known-good state, the client should provoke a lexical error.
>
> The cleanest way to do that is sending an ASCII control character
> other than '\t' (horizontal tab), '\r' (carriage return), or '\n' (new
> line).
>
> Sadly, older versions of QEMU can fail to flag this as an error. If a
> client needs to deal with them, it should send a 0xFF byte.
That's weird as guest-sync-delimiter documentation (still?) mention
0xFF. In both directions. Does it mean the new recommendation is to use
ASCII control character in one direction, but expect 0xFF in the other?
> [1]
> https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/qmp-spec.txt;h=8f7da0245d51447be7df2b3d4b105bad9fbec0b3;hb=HEAD#l231
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
signature.asc
Description: PGP signature
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/8] xen/arm: Add support for read-only foreign mappings
On Thu, 15 Nov 2018, Julien Grall wrote: > Hi, > > On 11/15/18 1:19 PM, Andrii Anisov wrote: > > > So I would prefer to stick with _t which is quite common within the p2m > > > code base so far. > > > > I've found a similar code only in one place - p2m_get_entry() > > function. And it is, at least, somehow commented there: > > ... > > /* Allow t to be NULL */ > > t = t ?: &_t; > > > > *t = p2m_invalid; > > ... > > And in x86 code... > > > > > But IMO, it is really confusing to write a code to calculate and store > > a value which clearly is not needed by a caller. > > I disagree, you directly know that t can be NULL. Although, I agree that a > comment would help to understand. > > > From another hand, I'm not sure if a compiler would be intelligent > > enough to factor out the odd code from execution pass on the incoming > > null pointer. > > You can't assume the compiler will inline even with the inline keyword. > > > > > I'm sorry, but I can't pass my RB for `_t`. > > I think this request is unreasonable because this is a matter of taste. > > While this is a nice feature to have in Xen 4.12 and address a long standing > open issue on Arm. I don't require it and I am not inclined to address > bikeshedding. > > So I will keep aside for now until someone cares about it. Let's try to be reasonable and have constructive conversations. If we do have to disagree, let's disagree on meaningful design decisions, not silly code style issues :-) Andrii, when something can be done equally well in two different ways, especially when it is a matter of style, I think it is best to express our preference, but not to force a contributor in a particular direction, unless specifically stated in CODING_STYLE or equivalent docs. We don't have written rules about code reviews, but if we had, I think this should be one of them. (I would love to have written rules about code reviews.) Julien, usually in situations like this, it is quicker to change the code than to argue. I personally don't care and wouldn't ask you to change it, but if a member of the community reads the code and has an adverse reaction, it is always worth reconsidering the approach, because others might find it antagonizing too. Given the choice, it is best to go for something obvious to most, so if a new contributor sends a patch to change, it is more likely to be correct from the start. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v6.2 05/11] libxl_qmp: Implementation of libxl__ev_qmp_*
Anthony PERARD writes ("[PATCH v6.2 05/11] libxl_qmp: Implementation of
libxl__ev_qmp_*"):
> Signed-off-by: Anthony PERARD
Thanks for the additional comments. I got thoroughly stuck in.
Overall the structure looks broadly right and my comments are
generally about details. As might be expected, some of them are
stylistic or matters of taste.
Please feel free to push back if you disagree with me on anything.
> struct libxl__ev_qmp {
> /* caller should include this in their own struct */
> /* caller must fill these in, and they must all remain valid */
> libxl_domid domid;
> libxl__ev_qmp_callback *callback;
> int fd; /* set to send a fd with the command, -1 otherwise */
> +
This name `fd' becomes very confusing, because most of the time with
qmp the fd in question is our socket to qemu. Can you rename it ?
payload_fd maybe ?
> +/*
> + * remaining fields are private to libxl_ev_qmp_*
> + */
> +
> +int id;
> +libxl__carefd *qmp_cfd;
> +libxl__ev_fd qmp_efd;
> +libxl__qmp_state qmp_state;
What purpose do the qmp_ prefixes on these serve ? I think if it were
me I would drop them and simply call these `cfd' and `efd' and `state'.
> +/* Implementation of libxl__ev_qmp */
> +
> +/*
> + * Possible internal state compared to qmp_state:
> + *
> + * qmp_state External qmp_cfd qmp_efd id rx_buf* tx_buf* msg*
> + * disconnected Idle closeIdle reset freefreefree
^
YM `0' or `NULL'.
Maybe this table would be easier to read if the headings were offset
from the values. Eg:
+ * qmp_state External qmp_cfd qmp_efd id rx_buf* tx_buf* msg*
+ * disconnected Idle closeIdle reset freefreefree
?
Up to you.
> + * connecting Activeopen Active set usedfreeset
You say Active for the qmp_efd but I think it would be better to say
what you are waiting for ? Looking at qmp_ev_connect I think
connecting has only POLLIN. You could write IN, IN[|OUT], etc., maybe.
While reading the rest of the code I found that this was often a
critical piece of state you are managing, so I think some improvement
here is warranted.
> + * capability_negotiation
> + *Activeopen Active set usedany set
I would abbreviate this state name here, because it spoils the table.
`cap.nego.' ? Or maybe you could just rename it to capab_nego or
something.
> + * connected Connected open Active prev[1] usedfreeany
What msg might there be in state `connected' ? According to the
public interface there is no message yet.
In general the movement of the caller's intended qmp command from msg
to rx_buf through to qemu could perhaps do with some exposition
(commentary).
> + * waiting_reply Activeopen Active set usedany free
I am a bit confused about the semantics of tx_buf being free in state
waiting_reply. Does that mean the caller's wanted command has been
sent ? This seems like part of the same question as I just asked...
Maybe you want to split this into two rows:
+ * waiting_reply Activeopen IN|OUT set useduser's free
+ * waiting_reply Activeopen IN set usedfreefree
?
> + * broken[2] none[3] any Active any any any any
...
> + * [1] id used on the previously sent command
It would be better if the id column stated something more useful than
`set'. `next' maybe, where applicable ?
AIUI it is intended (important?) that ids should not be reused.
> + * Possible buffers states:
> + * - receiving buffer:
> + *free used
> + * rx_buf NULL allocated
> + * rx_buf_size 0 allocation size of `rx_buf`
> + * rx_buf_off 0 <= rx_buf_size
> + * - transmiting buffer:
Typo `transmitting'.
A minor point, but indenting the rx_* things here would avoid them
lining up with `transmitting buffer' and misleading the eye.
Better still, have you considered moving this table into the struct
itself ? You could put the table to the RHS of the actual member
definitions. That gives slightly fewer places to look, although it
would involve a cross-reference from this wider state description to
the field's state tables.
> + *free used
> + * tx_buf NULL contain data
> + * tx_buf_len 0 size of data
> + * tx_buf_off 0 <= tx_buf_len
You should explain the semantics of _off in both cases. It points
into the buffer, but what is the meaning of the data (or the space)
before and after it ?
... oh, I see for rx_buf_used this is documented in the struct
itself. But not for tx_buf_off.
Which leads me to say: the struct contains rx_buf_used but the comment
here talks about _off.
> + * - queued user command:
> + *free set
> + * msg NULL contain
Re: [Xen-devel] [PATCH 10/18] xen/arm: Implement GIC suspend/resume functions (gicv2 only)
On Wed, 14 Nov 2018, Julien Grall wrote:
> Hi,
>
> On 14/11/2018 22:18, Stefano Stabellini wrote:
> > On Wed, 14 Nov 2018, Julien Grall wrote:
> > @@ -1319,6 +1341,129 @@ static void gicv2_do_LPI(unsigned int lpi)
> > BUG();
> > }
> > +static void gicv2_alloc_context(struct gicv2_context *gc)
> > +{
>
> Is it necessary to allocate them at boot? Can we make them static or
> allocate them when we suspend?
>
> >>>
> >>> We need to allocate dynamically because the size of allocated data depends
> >>> on the number of irq lines, which is not known at the compile time.
> >>
> >> Well you know the upper bound. Why can't you use the upper bound?
> >>
> >>> Alternative is to allocate on suspend, but I believe it is better to do
> >>> this
> >>> when the system boots.
> >>
> >> Why is it better?
> >
> > I'll reply here also to your other point because they are related:
> >
> >> Suspend/resume is not a critical feature in common case. So I would
> >> prefer if we disable it when we can't alloc memory.
> >
> >
> > It is true that suspend/resume is not a critical feature for the common
> > case, but proceeding as "normal" when a memory allocation fails is not a
> > good idea: if the hypervisor is so low in memory as to fail in an
> > allocation like this one, it is not going to be able to work right. In
> > no other cases in Xen we continue on memory allocation failures, even
> > for less-critical features.
> >
> > I suggest that we either allocate statically using the upper bound as
> > you suggested, although it leads to some memory being wasted.
>
> We are speaking of at most 2KB of memory. I don't think it is going to
> be waste given of the number of interrupts GIC usually supports.
>
> The more that we already statically allocate irq_desc.
OK___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 05/18] xen/arm: Trigger Xen suspend when Dom0 completes suspend
Hi Mirela,
On 11/12/18 11:30 AM, Mirela Simonovic wrote:
When Dom0 finalizes its suspend procedure the suspend of Xen is
triggered by calling system_suspend(). Dom0 finalizes the suspend from
its boot core (VCPU#0), which could be mapped to any physical CPU,
i.e. the system_suspend() function could be executed by any physical
CPU. Since Xen suspend procedure has to be run by the boot CPU
(non-boot CPUs will be disabled at some point in suspend procedure),
system_suspend() execution has to continue on CPU#0.
When the system_suspend() returns 0, it means that the system was
suspended and it is coming out of the resume procedure. Regardless
of the system_suspend() return value, after this function returns
Xen is fully functional, and its state, including all devices and data
structures, matches the state prior to calling system_suspend().
The status is returned by system_suspend() for debugging/logging
purposes and function prototype compatibility.
Signed-off-by: Mirela Simonovic
Signed-off-by: Saeed Nowshadi
---
xen/arch/arm/suspend.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/xen/arch/arm/suspend.c b/xen/arch/arm/suspend.c
index f2338e41db..21b45f8248 100644
--- a/xen/arch/arm/suspend.c
+++ b/xen/arch/arm/suspend.c
@@ -112,11 +112,20 @@ static void vcpu_suspend(register_t epoint, register_t
cid)
_arch_set_info_guest(v, );
}
+/* Xen suspend. Note: data is not used (suspend is the suspend to RAM) */
+static long system_suspend(void *data)
+{
+BUG_ON(system_state != SYS_STATE_active);
+
+return -ENOSYS;
+}
+
int32_t domain_suspend(register_t epoint, register_t cid)
{
struct vcpu *v;
struct domain *d = current->domain;
bool is_thumb = epoint & 1;
+int status;
dprintk(XENLOG_DEBUG,
"Dom%d suspend: epoint=0x%"PRIregister", cid=0x%"PRIregister"\n",
@@ -156,6 +165,31 @@ int32_t domain_suspend(register_t epoint, register_t cid)
*/
vcpu_block_unless_event_pending(current);
+/* If this was dom0 the whole system should suspend: trigger Xen suspend */
+if ( is_hardware_domain(d) )
+{
+/*
+ * system_suspend should be called when Dom0 finalizes the suspend
+ * procedure from its boot core (VCPU#0). However, Dom0's VCPU#0 could
+ * be mapped to any PCPU (this function could be executed by any PCPU).
+ * The suspend procedure has to be finalized by the PCPU#0 (non-boot
+ * PCPUs will be disabled during the suspend).
+ */
+status = continue_hypercall_on_cpu(0, system_suspend, NULL);
+/*
+ * If an error happened, there is nothing that needs to be done here
+ * because the system_suspend always returns in fully functional state
+ * no matter what the outcome of suspend procedure is. If the system
+ * suspended successfully the function will return 0 after the resume.
+ * Otherwise, if an error is returned it means Xen did not suspended,
+ * but it is still in the same state as if the system_suspend was never
+ * called. We dump a debug message in case of an error for debugging/
+ * logging purpose.
+ */
+if ( status )
+dprintk(XENLOG_ERR, "Failed to suspend, errno=%d\n", status);
+}
After discussing we Stefano today, I have one more question regarding
Dom0 suspend.
From my understanding, the host may resume because of an event
targeting a guest. This means that Dom0 would still be blocked. As Dom0
would contain PV backend, how do you expect this to work?
Is there any potential dependency between frontend and backend? Or would
Dom0 be resume when the PV frontend probe the backend?
Cheers,
--
Julien Grall
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/9] mm: Introduce new vm_insert_range API
On 11/15/18 7:45 AM, Souptick Joarder wrote:
> Previouly drivers have their own way of mapping range of
> kernel pages/memory into user vma and this was done by
> invoking vm_insert_page() within a loop.
>
> As this pattern is common across different drivers, it can
> be generalized by creating a new function and use it across
> the drivers.
>
> vm_insert_range is the new API which will be used to map a
> range of kernel memory/pages to user vma.
>
> Signed-off-by: Souptick Joarder
> Reviewed-by: Matthew Wilcox
> ---
> include/linux/mm_types.h | 3 +++
> mm/memory.c | 28
> mm/nommu.c | 7 +++
> 3 files changed, 38 insertions(+)
Hi,
What is the opposite of vm_insert_range() or even of vm_insert_page()?
or is there no need for that?
> diff --git a/mm/memory.c b/mm/memory.c
> index 15c417e..da904ed 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1478,6 +1478,34 @@ static int insert_page(struct vm_area_struct *vma,
> unsigned long addr,
> }
>
> /**
> + * vm_insert_range - insert range of kernel pages into user vma
> + * @vma: user vma to map to
> + * @addr: target user address of this page
> + * @pages: pointer to array of source kernel pages
> + * @page_count: no. of pages need to insert into user vma
s/no./number/
> + *
> + * This allows drivers to insert range of kernel pages they've allocated
> + * into a user vma. This is a generic function which drivers can use
> + * rather than using their own way of mapping range of kernel pages into
> + * user vma.
> + */
> +int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
> + struct page **pages, unsigned long page_count)
> +{
> + unsigned long uaddr = addr;
> + int ret = 0, i;
> +
> + for (i = 0; i < page_count; i++) {
> + ret = vm_insert_page(vma, uaddr, pages[i]);
> + if (ret < 0)
> + return ret;
For a non-trivial value of page_count:
Is it a problem if vm_insert_page() succeeds for several pages
and then fails?
> + uaddr += PAGE_SIZE;
> + }
> +
> + return ret;
> +}
> +
> +/**
> * vm_insert_page - insert single page into user vma
> * @vma: user vma to map to
> * @addr: target user address of this page
thanks.
--
~Randy
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC 3/6] kexec: export PG_offline to VMCOREINFO
On Thu, Nov 15, 2018 at 01:01:17PM +0100, David Hildenbrand wrote:
> Just saying that "I'm not the first to do it, don't hit me with a stick" :)
:-)
> Indeed. And we still have without makedumpfile. I think you are aware of
> this, but I'll explain it just for consistency: PG_hwpoison
No, I appreciate an explanation very much! So thanks for that. :)
> At some point we detect a HW error and mask a page as PG_hwpoison.
>
> makedumpfile knows how to treat that flag and can exclude it from the
> dump (== not access it). No crash.
>
> kdump itself has no clue about old "struct pages". Especially:
> a) Where they are located in memory (e.g. SPARSE)
> b) What their format is ("where are the flags")
> c) What the meaning of flags is ("what does bit X mean")
>
> In order to know such information, we would have to do parsing of quite
> some information inside the kernel in kdump. Basically what makedumpfile
> does just now. Is this feasible? I don't think so.
>
> So we would need another approach to communicate such information as you
> said. I can't think of any, but if anybody reading this has an idea,
> please speak up. I am interested.
Yeah but that ship has sailed. And even if we had a great idea, we'd
have to support kdump before and after the idea. And that would be a
serious mess.
And if you have a huge box with gazillion piles of memory and an alpha
particle passes through a bunch of them on its way down to the earth's
core, and while doing so, flips a bunch of bits, you need to go and
collect all those regions and update some list which you then need to
shove into the second kernel.
And you probably need to do all that through perhaps a piece of memory
which is used for communication between first and second kernel and that
list better fit in there, or you need to realloc. And that piece of
memory's layout needs to be properly defined so that the second kernel
can parse it correctly.
And so on...
> The *only* way right now we would have to handle such scenarios:
>
> 1. While dumping memory and we get a machine check, fake reading a zero
> page instead of crashing.
> 2. While dumping memory and we get a fault, fake reading a zero page
> instead of crashing.
Yap.
> Indeed, and the basic design is to export these flags. (let's say
> "unfortunately", being able to handle such stuff in kdump directly would
> be the dream).
Well, AFAICT, the minimum work you need to always do before starting the
dumping is somehow generate that list of pages or ranges to not dump.
And that work needs to be done by the first or the second kernel, I'd
say.
If the first kernel would do it, then you'd have to probably have
callbacks to certain operations which go and add ranges or pages to
exclude, to a list which is then readily accessible to the second
kernel. Which means, when you reserve memory for the second kernel,
you'd have to reserve memory also for such a list.
But then what do you do when that memory gets filled up...?
So I guess exporting those things in vmcoreinfo is probably the only
thing we *can* do in the end.
Oh well, enough rambling... :)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6 4/4] x86/altp2m: fix display frozen when switching to a new view early
On 11/15/18 7:34 PM, George Dunlap wrote:
>
>
>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru
>> wrote:
>>
>> When an new altp2m view is created very early on guest boot, the
>> display will freeze (although the guest will run normally). This
>> may also happen on resizing the display. The reason is the way
>> Xen currently (mis)handles logdirty VGA: it intentionally
>> misconfigures VGA pages so that they will fault.
>>
>> The problem is that it only does this in the host p2m. Once we
>> switch to a new altp2m, the misconfigured entries will no longer
>> fault, so the display will not be updated.
>>
>> This patch:
>> * updates ept_handle_misconfig() to use the active altp2m instead
>> of the hostp2m;
>> * modifies p2m_change_entry_type_global(), p2m_memory_type_changed
>> and p2m_change_type_range() to propagate their changes to all
>> valid altp2ms.
>>
>> With the introduction of altp2m fields in p2m_memory_type_changed()
>> the whole function has been put under CONFIG_HVM.
>>
>> Signed-off-by: Razvan Cojocaru
>> Suggested-by: George Dunlap
>> Reviewed-by: Kevin Tian
>
> Just one more question...
>
>>
>> ---
>> CC: Jun Nakajima
>> CC: Kevin Tian
>> CC: George Dunlap
>> CC: Jan Beulich
>> CC: Andrew Cooper
>> CC: Wei Liu
>>
>> ---
>> Changes since V5:
>> - Added Kevin's Reviewed-by.
>> - Added a note on p2m_memory_type_changed() being under CONFIG_HVM.
>> ---
>> xen/arch/x86/mm/p2m-ept.c | 8
>> xen/arch/x86/mm/p2m-pt.c | 8
>> xen/arch/x86/mm/p2m.c | 115
>> ++
>> xen/include/asm-x86/p2m.h | 6 +--
>> 4 files changed, 114 insertions(+), 23 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
>> index fabcd06..e6fa85f 100644
>> --- a/xen/arch/x86/mm/p2m-ept.c
>> +++ b/xen/arch/x86/mm/p2m-ept.c
>> @@ -657,6 +657,9 @@ bool_t ept_handle_misconfig(uint64_t gpa)
>> bool_t spurious;
>> int rc;
>>
>> +if ( altp2m_active(curr->domain) )
>> +p2m = p2m_get_altp2m(curr);
>> +
>> p2m_lock(p2m);
>>
>> spurious = curr->arch.hvm.vmx.ept_spurious_misconfig;
>> @@ -1440,6 +1443,11 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned
>> int i)
>> struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
>> struct ept_data *ept;
>>
>> +p2m->max_mapped_pfn = hostp2m->max_mapped_pfn;
>
> Why we need to copy this value?
>
> I’ve just spent the afternoon tracing around the source code, and while I’m
> pretty sure it won’t hurt, I’m also not sure why it’s necessary.
I think I did that because I assumed that it would matter for
ept_get_entry() and misconfigurations, when:
954 /* This pfn is higher than the highest the p2m map currently
holds */
955 if ( gfn > p2m->max_mapped_pfn )
956 {
957 for ( i = ept->wl; i > 0; --i )
958 if ( (gfn & ~((1UL << (i * EPT_TABLE_ORDER)) - 1)) >
959 p2m->max_mapped_pfn )
960 break;
961 goto out;
962 }
but I am not certain it is required either. I can certainly remove this
assignment and see if anything breaks.
> Everything else looks good!
Thanks,
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [RFC PATCH v2 00/17] Add support for qemu-xen runnning in a Linux-based stubdomain.
On Thu, Nov 01, 2018 at 06:32:07PM +0100, Marek Marczykowski-Górecki wrote:
> On Thu, Nov 01, 2018 at 04:57:18PM +, Ian Jackson wrote:
> > Marek Marczykowski-Górecki writes ("Re: [RFC PATCH v2 00/17] Add support
> > for qemu-xen runnning in a Linux-based stubdomain."):
> > > 2. pv console
> > > pros:
> > >- no qemu modifications
> > >- same read()/write() on libxl side
> > > cons:
> > >- no out of band reset, needs libxl handling for that (skipping
> > > negotiation)
> >
> > Doesn't this potentially mean that the qmp console connection can
> > become irrecoverably desynchronised ? I don't know how you would
> > recover from the situation where another libxl process had got halfway
> > through some qmp stuff and been terminated (for whatever reason; maybe
> > the calling toolstack crashed).
>
> That's right, it could result in irrecoverably desynchronised
> connection. So, we need out of band reset.
Actually, it looks like we can recover that situation without out of
band reset. It's even in the spec[1]:
2.7 QGA Synchronization
---
When a client connects to QGA over a transport lacking proper
connection semantics such as virtio-serial, QGA may have read partial
input from a previous client. The client needs to force QGA's parser
into known-good state using the previous section's technique.
Moreover, the client may receive output a previous client didn't read.
To help with skipping that output, QGA provides the
'guest-sync-delimited' command. Refer to its documentation for
details.
previous section:
2.6 Forcing the JSON parser into known-good state
-
Incomplete or invalid input can leave the server's JSON parser in a
state where it can't parse additional commands. To get it back into
known-good state, the client should provoke a lexical error.
The cleanest way to do that is sending an ASCII control character
other than '\t' (horizontal tab), '\r' (carriage return), or '\n' (new
line).
Sadly, older versions of QEMU can fail to flag this as an error. If a
client needs to deal with them, it should send a 0xFF byte.
[1]
https://git.qemu.org/?p=qemu.git;a=blob;f=docs/interop/qmp-spec.txt;h=8f7da0245d51447be7df2b3d4b105bad9fbec0b3;hb=HEAD#l231
--
Anthony PERARD
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6 4/4] x86/altp2m: fix display frozen when switching to a new view early
> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru > wrote: > > When an new altp2m view is created very early on guest boot, the > display will freeze (although the guest will run normally). This > may also happen on resizing the display. The reason is the way > Xen currently (mis)handles logdirty VGA: it intentionally > misconfigures VGA pages so that they will fault. > > The problem is that it only does this in the host p2m. Once we > switch to a new altp2m, the misconfigured entries will no longer > fault, so the display will not be updated. > > This patch: > * updates ept_handle_misconfig() to use the active altp2m instead > of the hostp2m; > * modifies p2m_change_entry_type_global(), p2m_memory_type_changed > and p2m_change_type_range() to propagate their changes to all > valid altp2ms. > > With the introduction of altp2m fields in p2m_memory_type_changed() > the whole function has been put under CONFIG_HVM. > > Signed-off-by: Razvan Cojocaru > Suggested-by: George Dunlap > Reviewed-by: Kevin Tian Just one more question... > > --- > CC: Jun Nakajima > CC: Kevin Tian > CC: George Dunlap > CC: Jan Beulich > CC: Andrew Cooper > CC: Wei Liu > > --- > Changes since V5: > - Added Kevin's Reviewed-by. > - Added a note on p2m_memory_type_changed() being under CONFIG_HVM. > --- > xen/arch/x86/mm/p2m-ept.c | 8 > xen/arch/x86/mm/p2m-pt.c | 8 > xen/arch/x86/mm/p2m.c | 115 ++ > xen/include/asm-x86/p2m.h | 6 +-- > 4 files changed, 114 insertions(+), 23 deletions(-) > > diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c > index fabcd06..e6fa85f 100644 > --- a/xen/arch/x86/mm/p2m-ept.c > +++ b/xen/arch/x86/mm/p2m-ept.c > @@ -657,6 +657,9 @@ bool_t ept_handle_misconfig(uint64_t gpa) > bool_t spurious; > int rc; > > +if ( altp2m_active(curr->domain) ) > +p2m = p2m_get_altp2m(curr); > + > p2m_lock(p2m); > > spurious = curr->arch.hvm.vmx.ept_spurious_misconfig; > @@ -1440,6 +1443,11 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned > int i) > struct p2m_domain *hostp2m = p2m_get_hostp2m(d); > struct ept_data *ept; > > +p2m->max_mapped_pfn = hostp2m->max_mapped_pfn; Why we need to copy this value? I’ve just spent the afternoon tracing around the source code, and while I’m pretty sure it won’t hurt, I’m also not sure why it’s necessary. Everything else looks good! -George ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2] libxl/arm: Fix build on arm64 + acpi w/ gcc 8.2
From: Christopher Clark
[modified for Xen 4.11 to add required: #include ]
Add zero-padding to #defined ACPI table strings that are copied.
Provides sufficient characters to satisfy the length required to
fully populate the destination and prevent array-bounds warnings.
Add BUILD_BUG_ON sizeof checks for compile-time length checking.
Signed-off-by: Christopher Clark
Reviewed-by: Stefano Stabellini
Acked-by: Wei Liu
Signed-off-by: Matt Weber
---
v2: add BUILD_BUG_ON length checks, requested by Wei.
v1: Please add this patch to the backport list for the next minor
4.11 release.
Prior to this: gcc 8.2 objects to memcpy past bounds:
| libxl_arm_acpi.c: In function 'make_acpi_header':
| libxl_arm_acpi.c:208:5: error: 'memcpy' forming offset [5, 6] is out
of the bounds [0, 4] [-Werror=array-bounds]
| memcpy(h->oem_id, ACPI_OEM_ID, sizeof(h->oem_id));
| ^
| libxl_arm_acpi.c:209:5: error: 'memcpy' forming offset [5, 8] is out
of the bounds [0, 4] [-Werror=array-bounds]
| memcpy(h->oem_table_id, ACPI_OEM_TABLE_ID,
sizeof(h->oem_table_id));
|
^~~
| libxl_arm_acpi.c:211:5: error: 'memcpy' forming offset 4 is out of the
bounds [0, 3] [-Werror=array-bounds]
| memcpy(h->asl_compiler_id, ACPI_ASL_COMPILER_ID,
| ^~~~
| sizeof(h->asl_compiler_id));
| ~~~
| In function 'make_acpi_rsdp.isra.4',
| inlined from 'libxl__prepare_acpi' at libxl_arm_acpi.c:389:5:
| libxl_arm_acpi.c:193:5: error: 'memcpy' forming offset [5, 6] is out
of the bounds [0, 4] [-Werror=array-bounds]
| memcpy(rsdp->oem_id, ACPI_OEM_ID, sizeof(rsdp->oem_id));
| ^~~
tools/libxl/libxl_arm_acpi.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/tools/libxl/libxl_arm_acpi.c b/tools/libxl/libxl_arm_acpi.c
index 636f724..8924396 100644
--- a/tools/libxl/libxl_arm_acpi.c
+++ b/tools/libxl/libxl_arm_acpi.c
@@ -29,6 +29,7 @@ typedef int64_t s64;
#include
#include
+#include
#ifndef BITS_PER_LONG
#ifdef _LP64
@@ -48,9 +49,9 @@ extern const unsigned char dsdt_anycpu_arm[];
_hidden
extern const int dsdt_anycpu_arm_len;
-#define ACPI_OEM_ID "Xen"
-#define ACPI_OEM_TABLE_ID "ARM"
-#define ACPI_ASL_COMPILER_ID "XL"
+#define ACPI_OEM_ID "Xen\0\0"
+#define ACPI_OEM_TABLE_ID "ARM\0\0\0\0"
+#define ACPI_ASL_COMPILER_ID "XL\0"
enum {
RSDP,
@@ -190,6 +191,7 @@ static void make_acpi_rsdp(libxl__gc *gc, struct
xc_dom_image *dom,
struct acpi_table_rsdp *rsdp = (void *)dom->acpi_modules[0].data + offset;
memcpy(rsdp->signature, "RSD PTR ", sizeof(rsdp->signature));
+BUILD_BUG_ON(sizeof(ACPI_OEM_ID) != sizeof(rsdp->oem_id));
memcpy(rsdp->oem_id, ACPI_OEM_ID, sizeof(rsdp->oem_id));
rsdp->length = acpitables[RSDP].size;
rsdp->revision = 0x02;
@@ -205,9 +207,12 @@ static void make_acpi_header(struct acpi_table_header *h,
const char *sig,
memcpy(h->signature, sig, 4);
h->length = len;
h->revision = rev;
+BUILD_BUG_ON(sizeof(ACPI_OEM_ID) != sizeof(h->oem_id));
memcpy(h->oem_id, ACPI_OEM_ID, sizeof(h->oem_id));
+BUILD_BUG_ON(sizeof(ACPI_OEM_TABLE_ID) != sizeof(h->oem_table_id));
memcpy(h->oem_table_id, ACPI_OEM_TABLE_ID, sizeof(h->oem_table_id));
h->oem_revision = 0;
+BUILD_BUG_ON(sizeof(ACPI_ASL_COMPILER_ID) != sizeof(h->asl_compiler_id));
memcpy(h->asl_compiler_id, ACPI_ASL_COMPILER_ID,
sizeof(h->asl_compiler_id));
h->asl_compiler_revision = 0;
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130125: regressions - FAIL
flight 130125 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130125/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 build-i386-pvops 6 kernel-build fail REGR. vs. 129475 Tests which did not succeed, but are not blocking: test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z9 days Failing since129526 2018-11-06 20:49:26 Z8 days 80 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days2 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops fail test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable-smoke test] 130122: tolerable all pass - PUSHED
flight 130122 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/130122/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 2262e808f4665bee820b5bb536aff47e560bdcc3 baseline version: xen 2a8922cff38403a9be7b0e38e09668dae0c6d9f6 Last test of basis 130110 2018-11-15 12:00:33 Z0 days Testing same since 130122 2018-11-15 15:00:24 Z0 days1 attempts People who touched revisions under test: Brian Woods George Dunlap Jan Beulich Julien Grall Razvan Cojocaru Tamas K Lengyel jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-i386 pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 2a8922cff3..2262e808f4 2262e808f4665bee820b5bb536aff47e560bdcc3 -> smoke ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH] x86/time: Identify the platform timer before attempting to probe it
>>> On 15.11.18 at 17:22, wrote: > When platform timer initialisation fails due to missing interrupts, hardware > tends to livelock without identifying the timer in use. Have you observed this on non-flawed hardware? Is there anything that needs fixing? > Leave a trace around to help debugging. > > Signed-off-by: Andrew Cooper Acked-by: Jan Beulich ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH] x86/time: Identify the platform timer before attempting to probe it
When platform timer initialisation fails due to missing interrupts, hardware
tends to livelock without identifying the timer in use.
Leave a trace around to help debugging.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Brian Woods
---
xen/arch/x86/time.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index 24d4c27..992540a 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -795,6 +795,7 @@ static u64 __init init_platform_timer(void)
for ( i = 0; i < ARRAY_SIZE(plt_timers); i++ )
{
pts = plt_timers[i];
+printk(XENLOG_DEBUG "Probing platform timer %s\n", pts->name);
if ( (rc = try_platform_timer(pts)) > 0 )
break;
}
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms
On 11/15/18 5:52 PM, George Dunlap wrote:
>
>
>> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru
>> wrote:
>>
>> This patch is a pre-requisite for the one fixing VGA logdirty
>> freezes when using altp2m. It only concerns itself with the
>> ranges allocation / deallocation / initialization part. While
>> touching the code, I've switched global_logdirty from bool_t
>> to bool.
>>
>> P2m_reset_altp2m() has been refactored to reduce code
>> repetition, and it now takes the p2m lock. Similar
>> refactoring has been done with p2m_activate_altp2m().
>
> Thanks! I think this is almost there. I have a couple of comments about the
> code below; so since we have to do another round, let me comment on the
> changelog.
>
> It doesn’t make much sense to say you’re “refactoring” a function that you
> are just now introducing in this patch.
Right, the term doesn't apply to p2m_activate_altp2m() - I got carried
away with p2m_reset_altp2m(). :)
> I think I’d say something more like this:
>
> ---
> For now, only do allocation/deallocation; keeping them in sync will be done
> in subsequent patches.
>
> Logdirty synchronization will only be done for active altp2ms; so allocate
> logdirty rangesets (copying the host logdirty rangeset) when an altp2m is
> activated, and free it when deactivated.
>
> Write a helper function to do altp2m activiation (appropriately handling
> failures). Also, refactor p2m_reset_altp2m() so that it can be used to
> remove redundant codepaths, fixing the locking while we’re at it.
>
> While we’re here, switch global_logdirty from bool_t to bool.
> ---
Thanks, I'll use the new description.
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 418ff85..abdf443 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t
>> gpa,
>> return 1;
>> }
>>
>> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
>> + bool reset_remapped, bool free_logdirty_ranges)
>
> As Jan says, passing in (…true, false) makes it more difficult to follow the
> code and see what’s going on. You could use a ‘flags’ structure, as he
> mentions; or alternately, you could pass in an argument that was either
> DEACTIVATE or RESET, and then use that to decide which things to do.
Will do.
>> +{
>> +struct p2m_domain *p2m;
>> +
>> +ASSERT(idx < MAX_ALTP2M);
>> +p2m = d->arch.altp2m_p2m[idx];
>> +
>> +p2m_lock(p2m);
>> +
>> +p2m_flush_table_locked(p2m);
>> +/* Uninit and reinit ept to force TLB shootdown */
>> +
>> +if ( free_logdirty_ranges )
>> +p2m_free_logdirty(p2m);
>> +
>> +ept_p2m_uninit(p2m);
>> +ept_p2m_init(p2m);
>
> Nit: The comment about uninit/reinit should be just before the uninit/reinit.
> :-)
Will move it.
>> +
>> +if ( reset_remapped )
>> +{
>> +p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
>> +p2m->max_remapped_gfn = 0;
>> +}
>
> I don’t think there’s a need to do this conditionally. In fact, the only
> reason it’s correct *not* to do this for the other two cases is because in
> those cases the p2m will go through p2m_init_altp2m_ept() before being used
> again. Resetting these is redundant, but harmless, and not worth an extra
> function argument and conditional to avoid.
I'll remove the if.
>> +static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
>> +{
>> +struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
>> +int rc = p2m_init_logdirty(p2m);
>> +
>> +if ( rc )
>> +return rc;
>> +
>> +/* The following is really just a rangeset copy. */
>> +return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
>> +}
>> +
>> +static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
>> +{
>> +int rc;
>> +
>> +ASSERT(idx < MAX_ALTP2M);
>> +rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
>> +
>> +if ( rc )
>> +return rc;
>> +
>> +p2m_init_altp2m_ept(d, idx);
>
> Is there any reason to make these separate functions? I had in mind a single
> p2m_activate_altp2m() function which would do all three things
> (p2m_init_logdirty, rangeset_merge, and init_altp2m_ept).
Nope, no reason, in fact I did think about just that but wasn't sure it
wasn't deviating from what I thought was requested. I'll make that code
a single function.
> Also, I realize it’s _probably_ not necessary to grab the lock here for the
> p2m in question (since it shouldn’t be in use, because altp2m_eptp[] is
> INVALID_MFN); but since it’s not on the hot path, it seems like we might as
> well grab it just to be safe.
>
> Everything else looks good, thanks.
Thanks for the review!
Razvan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6 2/4] x86/mm: introduce p2m_{init, free}_logdirty()
> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru > wrote: > > Add logdirty_ranges allocator / deallocator helpers. > p2m_init_logdirty() will not re-allocate if > p2m->logdirty ranges has already been allocated. > > Move the rangeset deallocation call from p2m_teardown_hostp2m() > to p2m_free_one() - we will want this to apply to altp2ms > as well. > > Signed-off-by: Razvan Cojocaru Reviewed-by: George Dunlap ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 5/5] amd/iommu: skip bridge devices when updating IOMMU page tables
>>> On 15.11.18 at 16:48, wrote:
> On Thu, Nov 15, 2018 at 08:40:11AM -0700, Jan Beulich wrote:
>> >>> On 14.11.18 at 12:57, wrote:
>> > Bridges are not behind an IOMMU, and are already special cased and
>> > silently skipped in amd_iommu_add_device. Apply the same special
>> > casing when updating page tables.
>>
>> But bridges also don't issue I/O on their own if I'm not mistaken. So
>> what I'm missing here is a word on the benefit of this change. I also
>> question the "silently" in your wording, seeing the AMD_IOMMU_DEBUG()
>> there.
>
> I see, by silently I meant without throwing an error, but I think just
> using 'skipped' would be clearer.
>
> The benefit is that update_paging_mode doesn't return an error when it
> finds a bridge attached to Dom0, which would cause the caller of
> update_paging_mode (amd_iommu_{un}map_page) to crash the domain.
>
> Ie: without this change a PVH Dom0 running on AMD hardware crashes
> when the IOMMU page table is expanded.
You want to say so in the description then.
>> The code change itself looks fine to me, albeit personally I'd prefer
>> if it fully matched the other conditional (i.e. if you flipped the
>> operands of && ). Of course the special casing of the hardware
>> domain here is somewhat odd anyway.
>
> Do you mean because bridges would only be ever assigned to the
> hardware domain?
No, because even if they were assigned to another domain they still
should be skipped.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 4/5] amd/iommu: assign iommu devices to Xen
On Thu, Nov 15, 2018 at 08:34:44AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, wrote:
> > --- a/xen/drivers/passthrough/amd/iommu_init.c
> > +++ b/xen/drivers/passthrough/amd/iommu_init.c
> > @@ -993,6 +993,16 @@ static void * __init allocate_ppr_log(struct amd_iommu
> > *iommu)
> >
> > static int __init amd_iommu_init_one(struct amd_iommu *iommu)
> > {
> > +struct pci_dev *pdev;
> > +
> > +pcidevs_lock();
> > +pdev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
> > +PCI_DEVFN2(iommu->bdf));
> > +if ( pdev )
> > +/* Assign the IOMMU PCI device to Xen */
> > +pdev->domain = dom_xen;
> > +pcidevs_unlock();
>
> Why do you kind of open-code pci_hide_device()? It would need
> extending to cope with a non-zero segment number, but I'd much
> prefer if there could be one central place where the logic lives.
> That way list addition would also not be omitted, like you do.
Sure, expanding pci_hide_device is better, I just didn't realize this
function existed in the first place.
> As to the hiding in general, also considering Andrew's objection:
> Are these devices representing the IOMMU and nothing else? As
> mentioned by Andrew something similar would be needed on the
> VT-d side, but iirc there's less clear of a relationship there in any
> event (which causes me to wonder about the situation on the
> AMD side). I'm asking not the least because iirc at the time
> pci_hide_device() was introduced I think it was considered to
> hide the AMD IOMMU devices; I don't recall why we didn't in the
> end, though.
I think this would be easier if I give more context about the issue
I'm hitting here, which is very similar to the issue patch 5/5
attempts to address.
The problem is that the IOMMU PCI device itself is obviously not
behind an IOMMU, and update_paging_mode will return an error if it
finds any such device in the domain list when attempting to expand the
IOMMU page tables:
...
iommu = find_iommu_for_device(pdev->seg, bdf);
if ( !iommu )
{
AMD_IOMMU_DEBUG("%s Fail to find iommu.\n", __func__);
return -ENODEV;
}
...
Another option is to allow the hardware domain to have assigned
devices that a not behind an IOMMU, but I would consider this more
like a workaround rather than a real fix.
I'm not sure whether the IOMMU AMD PCI device could also represent
something else, Maybe Brian can provide more insight on whether there
might be other platform devices encompassed in the same PCI device as
the IOMMU.
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [ovmf test] 130120: regressions - FAIL
flight 130120 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130120/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 build-i386-pvops 6 kernel-build fail REGR. vs. 129475 Tests which did not succeed, but are not blocking: build-amd64-libvirt 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-i386-libvirt1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a version targeted for testing: ovmf 66127011a544b90e800eb3619e84c2f94a354903 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z9 days Failing since129526 2018-11-06 20:49:26 Z8 days 79 attempts Testing same since 130120 2018-11-15 14:41:03 Z0 days1 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops fail test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 911 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH V6 3/4] x86/mm: allocate logdirty_ranges for altp2ms
> On Nov 14, 2018, at 8:40 PM, Razvan Cojocaru
> wrote:
>
> This patch is a pre-requisite for the one fixing VGA logdirty
> freezes when using altp2m. It only concerns itself with the
> ranges allocation / deallocation / initialization part. While
> touching the code, I've switched global_logdirty from bool_t
> to bool.
>
> P2m_reset_altp2m() has been refactored to reduce code
> repetition, and it now takes the p2m lock. Similar
> refactoring has been done with p2m_activate_altp2m().
Thanks! I think this is almost there. I have a couple of comments about the
code below; so since we have to do another round, let me comment on the
changelog.
It doesn’t make much sense to say you’re “refactoring” a function that you are
just now introducing in this patch.
I think I’d say something more like this:
---
For now, only do allocation/deallocation; keeping them in sync will be done in
subsequent patches.
Logdirty synchronization will only be done for active altp2ms; so allocate
logdirty rangesets (copying the host logdirty rangeset) when an altp2m is
activated, and free it when deactivated.
Write a helper function to do altp2m activiation (appropriately handling
failures). Also, refactor p2m_reset_altp2m() so that it can be used to remove
redundant codepaths, fixing the locking while we’re at it.
While we’re here, switch global_logdirty from bool_t to bool.
---
>
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 418ff85..abdf443 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2282,6 +2282,34 @@ bool_t p2m_altp2m_lazy_copy(struct vcpu *v, paddr_t
> gpa,
> return 1;
> }
>
> +static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
> + bool reset_remapped, bool free_logdirty_ranges)
As Jan says, passing in (…true, false) makes it more difficult to follow the
code and see what’s going on. You could use a ‘flags’ structure, as he
mentions; or alternately, you could pass in an argument that was either
DEACTIVATE or RESET, and then use that to decide which things to do.
> +{
> +struct p2m_domain *p2m;
> +
> +ASSERT(idx < MAX_ALTP2M);
> +p2m = d->arch.altp2m_p2m[idx];
> +
> +p2m_lock(p2m);
> +
> +p2m_flush_table_locked(p2m);
> +/* Uninit and reinit ept to force TLB shootdown */
> +
> +if ( free_logdirty_ranges )
> +p2m_free_logdirty(p2m);
> +
> +ept_p2m_uninit(p2m);
> +ept_p2m_init(p2m);
Nit: The comment about uninit/reinit should be just before the uninit/reinit.
:-)
> +
> +if ( reset_remapped )
> +{
> +p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
> +p2m->max_remapped_gfn = 0;
> +}
I don’t think there’s a need to do this conditionally. In fact, the only
reason it’s correct *not* to do this for the other two cases is because in
those cases the p2m will go through p2m_init_altp2m_ept() before being used
again. Resetting these is redundant, but harmless, and not worth an extra
function argument and conditional to avoid.
> +static int p2m_init_altp2m_logdirty(struct p2m_domain *p2m)
> +{
> +struct p2m_domain *hostp2m = p2m_get_hostp2m(p2m->domain);
> +int rc = p2m_init_logdirty(p2m);
> +
> +if ( rc )
> +return rc;
> +
> +/* The following is really just a rangeset copy. */
> +return rangeset_merge(p2m->logdirty_ranges, hostp2m->logdirty_ranges);
> +}
> +
> +static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
> +{
> +int rc;
> +
> +ASSERT(idx < MAX_ALTP2M);
> +rc = p2m_init_altp2m_logdirty(d->arch.altp2m_p2m[idx]);
> +
> +if ( rc )
> +return rc;
> +
> +p2m_init_altp2m_ept(d, idx);
Is there any reason to make these separate functions? I had in mind a single
p2m_activate_altp2m() function which would do all three things
(p2m_init_logdirty, rangeset_merge, and init_altp2m_ept).
Also, I realize it’s _probably_ not necessary to grab the lock here for the p2m
in question (since it shouldn’t be in use, because altp2m_eptp[] is
INVALID_MFN); but since it’s not on the hot path, it seems like we might as
well grab it just to be safe.
Everything else looks good, thanks.
-George
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 5/5] amd/iommu: skip bridge devices when updating IOMMU page tables
On Thu, Nov 15, 2018 at 08:40:11AM -0700, Jan Beulich wrote:
> >>> On 14.11.18 at 12:57, wrote:
> > Bridges are not behind an IOMMU, and are already special cased and
> > silently skipped in amd_iommu_add_device. Apply the same special
> > casing when updating page tables.
>
> But bridges also don't issue I/O on their own if I'm not mistaken. So
> what I'm missing here is a word on the benefit of this change. I also
> question the "silently" in your wording, seeing the AMD_IOMMU_DEBUG()
> there.
I see, by silently I meant without throwing an error, but I think just
using 'skipped' would be clearer.
The benefit is that update_paging_mode doesn't return an error when it
finds a bridge attached to Dom0, which would cause the caller of
update_paging_mode (amd_iommu_{un}map_page) to crash the domain.
Ie: without this change a PVH Dom0 running on AMD hardware crashes
when the IOMMU page table is expanded.
> The code change itself looks fine to me, albeit personally I'd prefer
> if it fully matched the other conditional (i.e. if you flipped the
> operands of && ). Of course the special casing of the hardware
> domain here is somewhat odd anyway.
Do you mean because bridges would only be ever assigned to the
hardware domain?
Thanks, Roger.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 8/9] xen/gntdev.c: Convert to use vm_insert_range
Convert to use vm_insert_range() to map range of kernel
memory to user vma.
Signed-off-by: Souptick Joarder
Reviewed-by: Matthew Wilcox
---
drivers/xen/gntdev.c | 11 ---
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index b0b02a5..430d4cb 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -1084,7 +1084,7 @@ static int gntdev_mmap(struct file *flip, struct
vm_area_struct *vma)
int index = vma->vm_pgoff;
int count = vma_pages(vma);
struct gntdev_grant_map *map;
- int i, err = -EINVAL;
+ int err = -EINVAL;
if ((vma->vm_flags & VM_WRITE) && !(vma->vm_flags & VM_SHARED))
return -EINVAL;
@@ -1145,12 +1145,9 @@ static int gntdev_mmap(struct file *flip, struct
vm_area_struct *vma)
goto out_put_map;
if (!use_ptemod) {
- for (i = 0; i < count; i++) {
- err = vm_insert_page(vma, vma->vm_start + i*PAGE_SIZE,
- map->pages[i]);
- if (err)
- goto out_put_map;
- }
+ err = vm_insert_range(vma, vma->vm_start, map->pages, count);
+ if (err)
+ goto out_put_map;
} else {
#ifdef CONFIG_X86
/*
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 9/9] xen/privcmd-buf.c: Convert to use vm_insert_range
Convert to use vm_insert_range() to map range of kernel
memory to user vma.
Signed-off-by: Souptick Joarder
Reviewed-by: Matthew Wilcox
---
drivers/xen/privcmd-buf.c | 8 ++--
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/xen/privcmd-buf.c b/drivers/xen/privcmd-buf.c
index df1ed37..8d8255b 100644
--- a/drivers/xen/privcmd-buf.c
+++ b/drivers/xen/privcmd-buf.c
@@ -180,12 +180,8 @@ static int privcmd_buf_mmap(struct file *file, struct
vm_area_struct *vma)
if (vma_priv->n_pages != count)
ret = -ENOMEM;
else
- for (i = 0; i < vma_priv->n_pages; i++) {
- ret = vm_insert_page(vma, vma->vm_start + i * PAGE_SIZE,
-vma_priv->pages[i]);
- if (ret)
- break;
- }
+ ret = vm_insert_range(vma, vma->vm_start, vma_priv->pages,
+ vma_priv->n_pages);
if (ret)
privcmd_buf_vmapriv_free(vma_priv);
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 5/9] drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range
Convert to use vm_insert_range() to map range of kernel
memory to user vma.
Signed-off-by: Souptick Joarder
Reviewed-by: Matthew Wilcox
---
drivers/gpu/drm/xen/xen_drm_front_gem.c | 20 ++--
1 file changed, 6 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/xen/xen_drm_front_gem.c
b/drivers/gpu/drm/xen/xen_drm_front_gem.c
index 47ff019..a3eade6 100644
--- a/drivers/gpu/drm/xen/xen_drm_front_gem.c
+++ b/drivers/gpu/drm/xen/xen_drm_front_gem.c
@@ -225,8 +225,7 @@ struct drm_gem_object *
static int gem_mmap_obj(struct xen_gem_object *xen_obj,
struct vm_area_struct *vma)
{
- unsigned long addr = vma->vm_start;
- int i;
+ int err;
/*
* clear the VM_PFNMAP flag that was set by drm_gem_mmap(), and set the
@@ -247,18 +246,11 @@ static int gem_mmap_obj(struct xen_gem_object *xen_obj,
* FIXME: as we insert all the pages now then no .fault handler must
* be called, so don't provide one
*/
- for (i = 0; i < xen_obj->num_pages; i++) {
- int ret;
-
- ret = vm_insert_page(vma, addr, xen_obj->pages[i]);
- if (ret < 0) {
- DRM_ERROR("Failed to insert pages into vma: %d\n", ret);
- return ret;
- }
-
- addr += PAGE_SIZE;
- }
- return 0;
+ err = vm_insert_range(vma, vma->vm_start, xen_obj->pages,
+ xen_obj->num_pages);
+ if (err < 0)
+ DRM_ERROR("Failed to insert pages into vma: %d\n", err);
+ return err;
}
int xen_drm_front_gem_mmap(struct file *filp, struct vm_area_struct *vma)
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 0/4] x86/vvmx: Misc fixes
On 15/11/2018 13:52, Andrew Cooper wrote:
> All from code inspection
>
> Andrew Cooper (4):
> x86/vvmx: Drop unused CASE_{GET,SET}_REG() macros
> x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR
> x86/vvmx: Fixes to VMWRITE emulation
> x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()
Reviewed-by: Sergey Dyasli
--
Thanks,
Sergey
> xen/arch/x86/hvm/vmx/vvmx.c | 22 +++---
> 1 file changed, 7 insertions(+), 15 deletions(-)
>
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/9] mm: Introduce new vm_insert_range API
Previouly drivers have their own way of mapping range of
kernel pages/memory into user vma and this was done by
invoking vm_insert_page() within a loop.
As this pattern is common across different drivers, it can
be generalized by creating a new function and use it across
the drivers.
vm_insert_range is the new API which will be used to map a
range of kernel memory/pages to user vma.
Signed-off-by: Souptick Joarder
Reviewed-by: Matthew Wilcox
---
include/linux/mm_types.h | 3 +++
mm/memory.c | 28
mm/nommu.c | 7 +++
3 files changed, 38 insertions(+)
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 5ed8f62..15ae24f 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -523,6 +523,9 @@ extern void tlb_gather_mmu(struct mmu_gather *tlb, struct
mm_struct *mm,
extern void tlb_finish_mmu(struct mmu_gather *tlb,
unsigned long start, unsigned long end);
+int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
+ struct page **pages, unsigned long page_count);
+
static inline void init_tlb_flush_pending(struct mm_struct *mm)
{
atomic_set(>tlb_flush_pending, 0);
diff --git a/mm/memory.c b/mm/memory.c
index 15c417e..da904ed 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1478,6 +1478,34 @@ static int insert_page(struct vm_area_struct *vma,
unsigned long addr,
}
/**
+ * vm_insert_range - insert range of kernel pages into user vma
+ * @vma: user vma to map to
+ * @addr: target user address of this page
+ * @pages: pointer to array of source kernel pages
+ * @page_count: no. of pages need to insert into user vma
+ *
+ * This allows drivers to insert range of kernel pages they've allocated
+ * into a user vma. This is a generic function which drivers can use
+ * rather than using their own way of mapping range of kernel pages into
+ * user vma.
+ */
+int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
+ struct page **pages, unsigned long page_count)
+{
+ unsigned long uaddr = addr;
+ int ret = 0, i;
+
+ for (i = 0; i < page_count; i++) {
+ ret = vm_insert_page(vma, uaddr, pages[i]);
+ if (ret < 0)
+ return ret;
+ uaddr += PAGE_SIZE;
+ }
+
+ return ret;
+}
+
+/**
* vm_insert_page - insert single page into user vma
* @vma: user vma to map to
* @addr: target user address of this page
diff --git a/mm/nommu.c b/mm/nommu.c
index 749276b..d6ef5c7 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -473,6 +473,13 @@ int vm_insert_page(struct vm_area_struct *vma, unsigned
long addr,
}
EXPORT_SYMBOL(vm_insert_page);
+int vm_insert_range(struct vm_area_struct *vma, unsigned long addr,
+ struct page **pages, unsigned long page_count)
+{
+ return -EINVAL;
+}
+EXPORT_SYMBOL(vm_insert_range);
+
/*
* sys_brk() for the most part doesn't need the global kernel
* lock, except when an application is doing something nasty
--
1.9.1
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 5/5] amd/iommu: skip bridge devices when updating IOMMU page tables
>>> On 14.11.18 at 12:57, wrote: > Bridges are not behind an IOMMU, and are already special cased and > silently skipped in amd_iommu_add_device. Apply the same special > casing when updating page tables. But bridges also don't issue I/O on their own if I'm not mistaken. So what I'm missing here is a word on the benefit of this change. I also question the "silently" in your wording, seeing the AMD_IOMMU_DEBUG() there. The code change itself looks fine to me, albeit personally I'd prefer if it fully matched the other conditional (i.e. if you flipped the operands of && ). Of course the special casing of the hardware domain here is somewhat odd anyway. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/9] Use vm_insert_range
Previouly drivers have their own way of mapping range of kernel pages/memory into user vma and this was done by invoking vm_insert_page() within a loop. As this pattern is common across different drivers, it can be generalized by creating a new function and use it across the drivers. vm_insert_range is the new API which will be used to map a range of kernel memory/pages to user vma. All the applicable places are converted to use new vm_insert_range in this patch series. Souptick Joarder (9): mm: Introduce new vm_insert_range API arch/arm/mm/dma-mapping.c: Convert to use vm_insert_range drivers/firewire/core-iso.c: Convert to use vm_insert_range drm/rockchip/rockchip_drm_gem.c: Convert to use vm_insert_range drm/xen/xen_drm_front_gem.c: Convert to use vm_insert_range iommu/dma-iommu.c: Convert to use vm_insert_range videobuf2/videobuf2-dma-sg.c: Convert to use vm_insert_range xen/gntdev.c: Convert to use vm_insert_range xen/privcmd-buf.c: Convert to use vm_insert_range arch/arm/mm/dma-mapping.c | 21 ++--- drivers/firewire/core-iso.c | 15 ++-- drivers/gpu/drm/rockchip/rockchip_drm_gem.c | 20 ++-- drivers/gpu/drm/xen/xen_drm_front_gem.c | 20 +--- drivers/iommu/dma-iommu.c | 12 ++ drivers/media/common/videobuf2/videobuf2-dma-sg.c | 23 ++- drivers/xen/gntdev.c | 11 - drivers/xen/privcmd-buf.c | 8 ++- include/linux/mm_types.h | 3 +++ mm/memory.c | 28 +++ mm/nommu.c| 7 ++ 11 files changed, 70 insertions(+), 98 deletions(-) -- 1.9.1 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v4 4/5] amd/iommu: assign iommu devices to Xen
>>> On 14.11.18 at 12:57, wrote:
> --- a/xen/drivers/passthrough/amd/iommu_init.c
> +++ b/xen/drivers/passthrough/amd/iommu_init.c
> @@ -993,6 +993,16 @@ static void * __init allocate_ppr_log(struct amd_iommu
> *iommu)
>
> static int __init amd_iommu_init_one(struct amd_iommu *iommu)
> {
> +struct pci_dev *pdev;
> +
> +pcidevs_lock();
> +pdev = pci_get_pdev(iommu->seg, PCI_BUS(iommu->bdf),
> +PCI_DEVFN2(iommu->bdf));
> +if ( pdev )
> +/* Assign the IOMMU PCI device to Xen */
> +pdev->domain = dom_xen;
> +pcidevs_unlock();
Why do you kind of open-code pci_hide_device()? It would need
extending to cope with a non-zero segment number, but I'd much
prefer if there could be one central place where the logic lives.
That way list addition would also not be omitted, like you do.
As to the hiding in general, also considering Andrew's objection:
Are these devices representing the IOMMU and nothing else? As
mentioned by Andrew something similar would be needed on the
VT-d side, but iirc there's less clear of a relationship there in any
event (which causes me to wonder about the situation on the
AMD side). I'm asking not the least because iirc at the time
pci_hide_device() was introduced I think it was considered to
hide the AMD IOMMU devices; I don't recall why we didn't in the
end, though.
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()
On 15/11/2018 15:28, Sergey Dyasli wrote: > On 15/11/2018 13:52, Andrew Cooper wrote: >> This ends up corrupting L1's view of RFLAGS by setting ZF. The correct value >> is established earlier in the function. > vmsucceed() doesn't set any flags, only clears some. And in this function it's > just redundant. ZF is set by VMfailValid. So I think the description must be > changed. Oh - so it does. Yes - I was confusing it with the failvalid case. I'll fix the wording to "The correct RFLAGS value is established earlier in the function, and this call has no net effect." ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()
On 15/11/2018 13:52, Andrew Cooper wrote: > This ends up corrupting L1's view of RFLAGS by setting ZF. The correct value > is established earlier in the function. vmsucceed() doesn't set any flags, only clears some. And in this function it's just redundant. ZF is set by VMfailValid. So I think the description must be changed. -- Sergey > Signed-off-by: Andrew Cooper > --- > CC: Jan Beulich > CC: Wei Liu > CC: Roger Pau Monné > CC: Sergey Dyasli > CC: Jun Nakajima > CC: Kevin Tian > --- > xen/arch/x86/hvm/vmx/vvmx.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index 41c4e2f..a72b519 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1371,7 +1371,6 @@ static void virtual_vmexit(struct cpu_user_regs *regs) > nvmx_update_apicv(v); > > nvcpu->nv_vmswitch_in_progress = 0; > -vmsucceed(regs); > } > > static void nvmx_eptp_update(void) > ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] x86/shadow: emulate_gva_to_mfn() should respect p2m_ioreq_server
At 05:51 -0700 on 15 Nov (1542261108), Jan Beulich wrote: > Writes to such pages need to be handed to the emulator. > > Signed-off-by: Jan Beulich Acked-by: Tim Deegan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/8] xen/arm: p2m: Introduce p2m_get_page_from_gfn
On 11/15/18 1:31 PM, Andrii Anisov wrote: Hello Julien, Hi, вт, 6 лист. 2018 о 21:16 Julien Grall пише: In a follow-up patches, we will need to handle get_page_from_gfn differently for DOMID_XEN. To keep the code simple move the current content in a new separate helper p2m_get_page_from_gfn. Note the new helper is a not anymore a static inline function as the helper is quite complex. In the patch "[PATCH 4/8] xen/arm: Add support for read-only foreign mappings" you make p2m_get_page_from_gfn() comparingly complex, but still keep it as static inline. Could you please be consistent (making both of those functions inline or non-inline) The reason I didn't move the other one in p2m.c is because so far p2m.c is only dealing with auto-translated guest. I didn't want to add function related with non-auto translated guest in it. I also don't think introduce a new file for one 10 line function is really useful. So that was the best solution. I am open to other suggestion. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()
On Thu, Nov 15, 2018 at 01:52:50PM +, Andrew Cooper wrote: > This ends up corrupting L1's view of RFLAGS by setting ZF. The correct value > is established earlier in the function. > > Signed-off-by: Andrew Cooper Reviewed-by: Roger Pau Monné ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 3/3] xen-init-dom0: set Dom0 UUID if requested
On 15/11/2018 14:30, Wei Liu wrote: > Read from XEN_CONFIG_DIR/dom0-uuid. If it contains a valid UUID, set > it for Dom0. > > Signed-off-by: Wei Liu > --- > v2: > 1. add missing "goto out" > 2. print file names more > 3. also print errno in xc_interface_open error message > 4. take care of short-read > --- > tools/examples/Makefile | 1 + > tools/examples/README | 2 ++ > tools/examples/dom0-uuid | 0 > tools/helpers/Makefile| 3 +- > tools/helpers/xen-init-dom0.c | 65 > +-- > 5 files changed, 67 insertions(+), 4 deletions(-) > create mode 100644 tools/examples/dom0-uuid I can't spot anything wrong here. Acked-by: Edwin Török Best regards, --Edwin ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
>>> On 15.11.18 at 15:23, wrote: > On 09/11/2018 17:16, Andrew Cooper wrote: >> >>> However, one >>> issue already might be that in order for bits in a (sub)leaf above >>> (guest) limits to come out all clear, it is guest_cpuid() which cuts >>> things off. Neither cpuid_featureset_to_policy() nor its inverse >>> nor sanitise_featureset() look to zap fields above limits, in case >>> they've been previously set to non-zero values. Am I overlooking >>> something? >> No - that is an aspect I'd overlooked, because the >> DOMCTL_set_cpuid_policy work (which does this correctly) hasn't gone in yet. >> >> I think I'll tweak recalculate_misc() to zero out beyond the max_subleaf >> settings, because the intention was always that a flat cpuid_policy was >> safe to use in this manner. I think there is an existing corner case >> when trying to level basic.max_leaf to < 7, or extd.max_leaf to < 0x807. > > Actually, I remember now that I tried fixing this once before. It's not > possible without DOMCTL_set_cpu_policy because the current API with > DOMCTL_set_cpuid provides leaves piecewise and there is no point at > which Xen can safely zero the out-of-range leaves without loosing > information later if max_leaf is increased. There could be such a point if an arch call was added at the point where d->creation_finished gets set the first time. Looking at the domctl side flow I'm wondering what the domain pausing is good for there, now that we don't allow changes once d->creation_finished is set. Was not dropping this an oversight of 3d0cab7b5d? > The content of cpuid_policy will be safe for the emulator to use, > because it will be bounded by real hardware support. Except that it voids all respective uses of vcpu_has_*() - just cpu_has_* would then be sufficient. And overall behavior is inconsistent between bit-wise leveling and full leaf hiding. > As levelling like > this is an extreme corner case which doesn't work at the moment for > other reasons, I'd like to take this series now and fix up the behaviour > later, to reduce the dependencies in the remaining CPUID/MSR work - its > already complicated enough. Hmm, okay, as long as a respective remark gets added at least to the commit message, I could live with that. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] xen: report PV capability in sysctl and use it in toolstack
Wei Liu writes ("[PATCH v2] xen: report PV capability in sysctl and use it in
toolstack"):
> 0e2c886ef ("xen: decouple HVM and IOMMU capabilities") provided a
> truth table for what `xl info` would report. In order to make the
> table work xen will need to report its PV capability.
Acked-by: Ian Jackson
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 4/8] xen/arm: Add support for read-only foreign mappings
Hi, On 11/15/18 1:19 PM, Andrii Anisov wrote: So I would prefer to stick with _t which is quite common within the p2m code base so far. I've found a similar code only in one place - p2m_get_entry() function. And it is, at least, somehow commented there: ... /* Allow t to be NULL */ t = t ?: &_t; *t = p2m_invalid; ... And in x86 code... But IMO, it is really confusing to write a code to calculate and store a value which clearly is not needed by a caller. I disagree, you directly know that t can be NULL. Although, I agree that a comment would help to understand. From another hand, I'm not sure if a compiler would be intelligent enough to factor out the odd code from execution pass on the incoming null pointer. You can't assume the compiler will inline even with the inline keyword. I'm sorry, but I can't pass my RB for `_t`. I think this request is unreasonable because this is a matter of taste. While this is a nice feature to have in Xen 4.12 and address a long standing open issue on Arm. I don't require it and I am not inclined to address bikeshedding. So I will keep aside for now until someone cares about it. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/4] x86/vvmx: Fixes to VMWRITE emulation
On Thu, Nov 15, 2018 at 01:52:49PM +, Andrew Cooper wrote: > * Don't assume that decode_vmx_inst() always returns X86EMUL_EXCEPTION. > * The okay boolean is never written, making the else case dead. > > Signed-off-by: Andrew Cooper Reviewed-by: Roger Pau Monné ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/4] x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR
On Thu, Nov 15, 2018 at 01:52:48PM +, Andrew Cooper wrote: > The referenced addresses also need checking against MAXPHYSADDR. > > Signed-off-by: Andrew Cooper Reviewed-by: Roger Pau Monné > --- > CC: Jan Beulich > CC: Wei Liu > CC: Roger Pau Monné > CC: Sergey Dyasli > CC: Jun Nakajima > CC: Kevin Tian > --- > xen/arch/x86/hvm/vmx/vvmx.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c > index c296660..5daab82 100644 > --- a/xen/arch/x86/hvm/vmx/vvmx.c > +++ b/xen/arch/x86/hvm/vmx/vvmx.c > @@ -1672,7 +1672,7 @@ static int nvmx_handle_vmptrld(struct cpu_user_regs > *regs) > if ( rc != X86EMUL_OKAY ) > return rc; > > -if ( gpa & 0xfff ) > +if ( (gpa & ~PAGE_MASK) || !gfn_valid(v->domain, gaddr_to_gfn(gpa)) ) Seeing this is repeated in several places, it might be helpful to introduce a helper? Thanks, Roger. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/4] x86/vvmx: Drop unused CASE_{GET, SET}_REG() macros
On Thu, Nov 15, 2018 at 01:52:47PM +, Andrew Cooper wrote: > These have been obsolete since c/s 053ae230 "x86/vvmx: Remove enum > vmx_regs_enc". > > Signed-off-by: Andrew Cooper Reviewed-by: Roger Pau Monné ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 11/11] tools/libvchan: libxenvchan_client_init: use ENOENT for no server
On Thu, Nov 08, 2018 at 05:08:05PM +, Ian Jackson wrote: > * Promise that we will set errno to ENOENT if the server is not > yet set up. > * Arrange that all ENOENT returns other than from the read of ring-ref > are turned into EIO, logging when we do so. > > Signed-off-by: Ian Jackson Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 03/11] tools/libs/*: Rely on the default logger
On Thu, Nov 08, 2018 at 05:07:57PM +, Ian Jackson wrote: > Delete 11 entirely formulaic conditional calls to > xtl_createlogger_stdiostream(stderr, XTL_PROGRESS, 0); > and associated logger_tofree variables, error handling, etc. > > No overall functional change, although some memory allocation errors > may no longer occur. > > After this there are still several calls to > xtl_createlogger_stdiostream in tree, but they almost all have > non-default message level etc. so it is not obvious that they should > be replaced. > > The exception is in xc_private.c where xch->error_handler is > initialised using a copy of the default initialisation boilerplate > (ant there is the associated xch->error_handler_tofree). However, > there is also xch->dombuild_logger, and xch->dombuild_logger_tofree > which is handled differently and so must be retained. It seems better > to keep the xch code internally consistent, and decoupled from the > general default. > > Signed-off-by: Ian Jackson Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 02/11] tools/libs/toollog: Use the default logger
On Thu, Nov 08, 2018 at 05:07:56PM +, Ian Jackson wrote: > Previously xtl_log, xtl_logv and xtl_progress would all crash if > passed logger=NULL. Have the use the default logger instead. > This is more convenient. > > Signed-off-by: Ian Jackson Acked-by: Wei Liu ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/3] tools/helpers: make gen_stub_json_config accept an UUID argument
On 14/11/2018 18:17, Wei Liu wrote:
> If that's set, the stub is going to contain that UUID.
>
> No functional change.
>
> Signed-off-by: Wei Liu
> ---
> tools/helpers/init-dom-json.c| 5 -
> tools/helpers/init-dom-json.h| 3 ++-
> tools/helpers/init-xenstore-domain.c | 2 +-
> tools/helpers/xen-init-dom0.c| 2 +-
> 4 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/tools/helpers/init-dom-json.c b/tools/helpers/init-dom-json.c
> index 704e7cb4f0..9514b3ceb6 100644
> --- a/tools/helpers/init-dom-json.c
> +++ b/tools/helpers/init-dom-json.c
> @@ -7,7 +7,7 @@
> #include
> #include
>
> -int gen_stub_json_config(uint32_t domid)
> +int gen_stub_json_config(uint32_t domid, libxl_uuid *uuid)
> {
> int rc = 1;
> xentoollog_logger_stdiostream *logger;
> @@ -40,6 +40,9 @@ int gen_stub_json_config(uint32_t domid)
> libxl_domain_build_info_init_type(_config.b_info,
>dom_config.c_info.type);
>
> +if (uuid && !libxl_uuid_is_nil(uuid))
> +libxl_uuid_copy(ctx, _config.c_info.uuid, uuid);
Is the nil check necessary, given that nil is the default value for
dom_config.c_info.uuid ?
Either way, Acked-by: Andrew Cooper
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/3] tools: update examples/README
On Thu, Nov 15, 2018 at 02:28:15PM +, Andrew Cooper wrote: > On 14/11/2018 18:17, Wei Liu wrote: > > This file gets installed to the host system. > > > > This patch cleans it up: 1. remove things that don't exist anymore; 2. > > change xm to xl; 3. fix xen-devel list address; 4. add things that are > > missing. > > > > Signed-off-by: Wei Liu > > Is this file actually worth keeping? Considering how obsolete its > information is, I severely doubt anyone uses it. Somebody will probably want to know what is what. > > If we do want to keep it, can you strip trailing whitespace in this > patch as well please? Sure. Wei. > > ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v2 3/3] xen-init-dom0: set Dom0 UUID if requested
Read from XEN_CONFIG_DIR/dom0-uuid. If it contains a valid UUID, set
it for Dom0.
Signed-off-by: Wei Liu
---
v2:
1. add missing "goto out"
2. print file names more
3. also print errno in xc_interface_open error message
4. take care of short-read
---
tools/examples/Makefile | 1 +
tools/examples/README | 2 ++
tools/examples/dom0-uuid | 0
tools/helpers/Makefile| 3 +-
tools/helpers/xen-init-dom0.c | 65 +--
5 files changed, 67 insertions(+), 4 deletions(-)
create mode 100644 tools/examples/dom0-uuid
diff --git a/tools/examples/Makefile b/tools/examples/Makefile
index f86ed3a271..f8492462db 100644
--- a/tools/examples/Makefile
+++ b/tools/examples/Makefile
@@ -9,6 +9,7 @@ XEN_CONFIGS += xlexample.hvm
XEN_CONFIGS += xlexample.pvlinux
XEN_CONFIGS += xl.conf
XEN_CONFIGS += cpupool
+XEN_CONFIGS += dom0-uuid
XEN_CONFIGS += $(XEN_CONFIGS-y)
diff --git a/tools/examples/README b/tools/examples/README
index 80a4652b06..8f940a55c1 100644
--- a/tools/examples/README
+++ b/tools/examples/README
@@ -14,6 +14,8 @@ block-common.sh - sourced by block, block-*
block-enbd - binds/unbinds network block devices
block-nbd - binds/unbinds network block devices
cpupool - example configuration script for 'xl cpupool-create'
+dom0-uuid - stores the UUID in canonical form for Dom0, will be
+ read by xen-init-dom0
external-device-migrate - called by xend for migrating external devices
locking.sh - locking functions to prevent concurrent access to
critical sections inside script files
diff --git a/tools/examples/dom0-uuid b/tools/examples/dom0-uuid
new file mode 100644
index 00..e69de29bb2
diff --git a/tools/helpers/Makefile b/tools/helpers/Makefile
index 4f3bbe6a7d..f759528322 100644
--- a/tools/helpers/Makefile
+++ b/tools/helpers/Makefile
@@ -14,6 +14,7 @@ XEN_INIT_DOM0_OBJS = xen-init-dom0.o init-dom-json.o
$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxentoollog)
$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenstore)
$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenlight)
+$(XEN_INIT_DOM0_OBJS): CFLAGS += $(CFLAGS_libxenctrl)
INIT_XENSTORE_DOMAIN_OBJS = init-xenstore-domain.o init-dom-json.o
$(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxentoollog)
@@ -26,7 +27,7 @@ $(INIT_XENSTORE_DOMAIN_OBJS): CFLAGS += $(CFLAGS_libxenlight)
all: $(PROGS)
xen-init-dom0: $(XEN_INIT_DOM0_OBJS)
- $(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS) $(LDLIBS_libxentoollog)
$(LDLIBS_libxenstore) $(LDLIBS_libxenlight) $(APPEND_LDFLAGS)
+ $(CC) $(LDFLAGS) -o $@ $(XEN_INIT_DOM0_OBJS) $(LDLIBS_libxenctrl)
$(LDLIBS_libxentoollog) $(LDLIBS_libxenstore) $(LDLIBS_libxenlight)
$(APPEND_LDFLAGS)
$(INIT_XENSTORE_DOMAIN_OBJS): _paths.h
diff --git a/tools/helpers/xen-init-dom0.c b/tools/helpers/xen-init-dom0.c
index 09bc0027f9..e826da57b4 100644
--- a/tools/helpers/xen-init-dom0.c
+++ b/tools/helpers/xen-init-dom0.c
@@ -3,23 +3,72 @@
#include
#include
+#include
#include
+#include
#include "init-dom-json.h"
+#include "_paths.h"
#define DOMNAME_PATH "/local/domain/0/name"
#define DOMID_PATH "/local/domain/0/domid"
+#define DOM0_UUID_PATH XEN_CONFIG_DIR "/dom0-uuid"
+
+static void get_dom0_uuid(libxl_uuid *uuid)
+{
+FILE *f = NULL;
+size_t r;
+char uuid_buf[LIBXL_UUID_FMTLEN+1];
+bool ok = false;
+
+f = fopen(DOM0_UUID_PATH, "r");
+if (!f) {
+fprintf(stderr, "failed to open %s, errno %d\n",
+DOM0_UUID_PATH, errno);
+goto out;
+}
+
+r = fread(uuid_buf, 1, LIBXL_UUID_FMTLEN, f);
+if (r != LIBXL_UUID_FMTLEN) {
+fprintf(stderr, "failed to read %s, read %zu, errno %d\n",
+DOM0_UUID_PATH, r, ferror(f));
+goto out;
+}
+
+uuid_buf[LIBXL_UUID_FMTLEN] = 0;
+
+if (libxl_uuid_from_string(uuid, uuid_buf)) {
+fprintf(stderr, "failed to parse UUID in %s\n", DOM0_UUID_PATH);
+goto out;
+}
+
+ok = true;
+out:
+if (f) fclose(f);
+if (!ok) libxl_uuid_clear(uuid);
+}
+
int main(int argc, char **argv)
{
int rc;
-struct xs_handle *xsh;
+struct xs_handle *xsh = NULL;
+xc_interface *xch = NULL;
char *domname_string = NULL, *domid_string = NULL;
+libxl_uuid uuid;
xsh = xs_open(0);
if (!xsh) {
fprintf(stderr, "cannot open xenstore connection\n");
-exit(1);
+rc = 1;
+goto out;
+}
+
+xch = xc_interface_open(NULL, NULL, 0);
+if (!xch) {
+fprintf(stderr, "xc_interface_open() failed\n");
+rc = 1;
+goto out;
}
/* Sanity check: this program can only be run once. */
@@ -31,7 +80,16 @@ int main(int argc, char **argv)
goto out;
}
-rc = gen_stub_json_config(0, NULL);
+get_dom0_uuid();
+
+if (!libxl_uuid_is_nil() &&
+xc_domain_sethandle(xch, 0,
[Xen-devel] [ovmf test] 130112: regressions - FAIL
flight 130112 ovmf real [real] http://logs.test-lab.xenproject.org/osstest/logs/130112/ Regressions :-( Tests which did not succeed and are blocking, including tests which could not be run: build-amd64 6 xen-buildfail REGR. vs. 129475 build-amd64-xsm 6 xen-buildfail REGR. vs. 129475 build-i3866 xen-buildfail REGR. vs. 129475 build-i386-xsm6 xen-buildfail REGR. vs. 129475 Tests which did not succeed, but are not blocking: build-i386-libvirt1 build-check(1) blocked n/a test-amd64-i386-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a test-amd64-amd64-xl-qemuu-ovmf-amd64 1 build-check(1) blocked n/a build-amd64-libvirt 1 build-check(1) blocked n/a version targeted for testing: ovmf 85588389222a3636baf0f9ed8227f2434af4c3f9 baseline version: ovmf 5ae3184d8c59f7bbb84bad482df6b8020ba58188 Last test of basis 129475 2018-11-05 21:13:11 Z9 days Failing since129526 2018-11-06 20:49:26 Z8 days 78 attempts Testing same since 130014 2018-11-14 03:43:46 Z1 days 18 attempts People who touched revisions under test: Ard Biesheuvel Dandan Bi Eric Dong Fu Siyuan Hao Wu Jian J Wang Jiaxin Wu Jiewen Yao Laszlo Ersek Liming Gao Marc Zyngier Ming Huang Ruiyu Ni Star Zeng Wu Jiaxin yuchenlin jobs: build-amd64-xsm fail build-i386-xsm fail build-amd64 fail build-i386 fail build-amd64-libvirt blocked build-i386-libvirt blocked build-amd64-pvopspass build-i386-pvops pass test-amd64-amd64-xl-qemuu-ovmf-amd64 blocked test-amd64-i386-xl-qemuu-ovmf-amd64 blocked sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Not pushing. (No revision log; it would be 871 lines long.) ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 1/3] tools: update examples/README
On 14/11/2018 18:17, Wei Liu wrote: > This file gets installed to the host system. > > This patch cleans it up: 1. remove things that don't exist anymore; 2. > change xm to xl; 3. fix xen-devel list address; 4. add things that are > missing. > > Signed-off-by: Wei Liu Is this file actually worth keeping? Considering how obsolete its information is, I severely doubt anyone uses it. If we do want to keep it, can you strip trailing whitespace in this patch as well please? ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [xen-unstable-smoke test] 130110: tolerable all pass - PUSHED
flight 130110 xen-unstable-smoke real [real] http://logs.test-lab.xenproject.org/osstest/logs/130110/ Failures :-/ but no regressions. Tests which did not succeed, but are not blocking: test-amd64-amd64-libvirt 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 13 migrate-support-checkfail never pass test-arm64-arm64-xl-xsm 14 saverestore-support-checkfail never pass test-armhf-armhf-xl 13 migrate-support-checkfail never pass test-armhf-armhf-xl 14 saverestore-support-checkfail never pass version targeted for testing: xen 2a8922cff38403a9be7b0e38e09668dae0c6d9f6 baseline version: xen 5d797ee199b32e4a789b55ae2aed69561df1bdf4 Last test of basis 130072 2018-11-14 22:04:51 Z0 days Testing same since 130110 2018-11-15 12:00:33 Z0 days1 attempts People who touched revisions under test: Andrew Cooper Jan Beulich Julien Grall jobs: build-arm64-xsm pass build-amd64 pass build-armhf pass build-amd64-libvirt pass test-armhf-armhf-xl pass test-arm64-arm64-xl-xsm pass test-amd64-amd64-xl-qemuu-debianhvm-i386 pass test-amd64-amd64-libvirt pass sg-report-flight on osstest.test-lab.xenproject.org logs: /home/logs/logs images: /home/logs/images Logs, config files, etc. are available at http://logs.test-lab.xenproject.org/osstest/logs Explanation of these reports, and of osstest in general, is at http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README.email;hb=master http://xenbits.xen.org/gitweb/?p=osstest.git;a=blob;f=README;hb=master Test harness code can be found at http://xenbits.xen.org/gitweb?p=osstest.git;a=summary Pushing revision : To xenbits.xen.org:/home/xen/git/xen.git 5d797ee199..2a8922cff3 2a8922cff38403a9be7b0e38e09668dae0c6d9f6 -> smoke ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
On 09/11/2018 17:16, Andrew Cooper wrote: > >> However, one >> issue already might be that in order for bits in a (sub)leaf above >> (guest) limits to come out all clear, it is guest_cpuid() which cuts >> things off. Neither cpuid_featureset_to_policy() nor its inverse >> nor sanitise_featureset() look to zap fields above limits, in case >> they've been previously set to non-zero values. Am I overlooking >> something? > No - that is an aspect I'd overlooked, because the > DOMCTL_set_cpuid_policy work (which does this correctly) hasn't gone in yet. > > I think I'll tweak recalculate_misc() to zero out beyond the max_subleaf > settings, because the intention was always that a flat cpuid_policy was > safe to use in this manner. I think there is an existing corner case > when trying to level basic.max_leaf to < 7, or extd.max_leaf to < 0x807. Actually, I remember now that I tried fixing this once before. It's not possible without DOMCTL_set_cpu_policy because the current API with DOMCTL_set_cpuid provides leaves piecewise and there is no point at which Xen can safely zero the out-of-range leaves without loosing information later if max_leaf is increased. The content of cpuid_policy will be safe for the emulator to use, because it will be bounded by real hardware support. As levelling like this is an extreme corner case which doesn't work at the moment for other reasons, I'd like to take this series now and fix up the behaviour later, to reduce the dependencies in the remaining CPUID/MSR work - its already complicated enough. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH RFC 3/6] kexec: export PG_offline to VMCOREINFO
On Thu, Nov 15, 2018 at 01:11:02PM +0100, Michal Hocko wrote: > I am not familiar with kexec to judge this particular patch but we > cannot simply define any range for these pages (same as for hwpoison > ones) because they can be almost anywhere in the available memory range. > Then there can be countless of them. There is no other way to rule them > out but to check the page state. I guess, especially if it is a monster box with a lot of memory in it. > I am not really sure what is the concern here exactly. Kdump is so > closly tight to the specific kernel version that the api exported > specifically for its purpose cannot be seriously considered an ABI. > Kdump has to adopt all the time. Right... Except, when people start ogling vmcoreinfo for other things and start exporting all kinds of kernel internals in there, my alarm bells start ringing. But ok, kdump *is* special and I guess that's fine. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2 0/3] x86/HVM: honor p2m_ioreq_server type
On 15/11/2018 12:45, Jan Beulich wrote: > 1: __hvm_copy() should not write to p2m_ioreq_server pages > 2: hvm_map_guest_frame_rw() should respect p2m_ioreq_server > 3: emulate_gva_to_mfn() should respect p2m_ioreq_server Acked-by: Andrew Cooper ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 0/4] x86/vvmx: Misc fixes
All from code inspection
Andrew Cooper (4):
x86/vvmx: Drop unused CASE_{GET,SET}_REG() macros
x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR
x86/vvmx: Fixes to VMWRITE emulation
x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()
xen/arch/x86/hvm/vmx/vvmx.c | 22 +++---
1 file changed, 7 insertions(+), 15 deletions(-)
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 3/4] x86/vvmx: Fixes to VMWRITE emulation
* Don't assume that decode_vmx_inst() always returns X86EMUL_EXCEPTION.
* The okay boolean is never written, making the else case dead.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Sergey Dyasli
CC: Jun Nakajima
CC: Kevin Tian
---
xen/arch/x86/hvm/vmx/vvmx.c | 12 +---
1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 5daab82..41c4e2f 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1872,11 +1872,12 @@ static int nvmx_handle_vmwrite(struct cpu_user_regs
*regs)
struct vmx_inst_decoded decode;
unsigned long operand;
u64 vmcs_encoding;
-bool_t okay = 1;
enum vmx_insn_errno err;
+int rc;
-if ( decode_vmx_inst(regs, , ) != X86EMUL_OKAY )
-return X86EMUL_EXCEPTION;
+rc = decode_vmx_inst(regs, , );
+if ( rc != X86EMUL_OKAY )
+return rc;
if ( !vvmcx_valid(v) )
{
@@ -1905,10 +1906,7 @@ static int nvmx_handle_vmwrite(struct cpu_user_regs
*regs)
break;
}
-if ( okay )
-vmsucceed(regs);
-else
-vmfail_valid(regs, VMX_INSN_UNSUPPORTED_VMCS_COMPONENT);
+vmsucceed(regs);
return X86EMUL_OKAY;
}
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 4/4] x86/vvmx: Don't call vmsucceed() at the end of virtual_vmexit()
This ends up corrupting L1's view of RFLAGS by setting ZF. The correct value is established earlier in the function. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Wei Liu CC: Roger Pau Monné CC: Sergey Dyasli CC: Jun Nakajima CC: Kevin Tian --- xen/arch/x86/hvm/vmx/vvmx.c | 1 - 1 file changed, 1 deletion(-) diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c index 41c4e2f..a72b519 100644 --- a/xen/arch/x86/hvm/vmx/vvmx.c +++ b/xen/arch/x86/hvm/vmx/vvmx.c @@ -1371,7 +1371,6 @@ static void virtual_vmexit(struct cpu_user_regs *regs) nvmx_update_apicv(v); nvcpu->nv_vmswitch_in_progress = 0; -vmsucceed(regs); } static void nvmx_eptp_update(void) -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 1/4] x86/vvmx: Drop unused CASE_{GET, SET}_REG() macros
These have been obsolete since c/s 053ae230 "x86/vvmx: Remove enum
vmx_regs_enc".
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Sergey Dyasli
CC: Jun Nakajima
CC: Kevin Tian
---
xen/arch/x86/hvm/vmx/vvmx.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index 88021af..c296660 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -207,11 +207,6 @@ struct vmx_inst_decoded {
unsigned int reg2;
};
-#define CASE_SET_REG(REG, reg) \
-case VMX_REG_ ## REG: regs->reg = value; break
-#define CASE_GET_REG(REG, reg) \
-case VMX_REG_ ## REG: value = regs->reg; break
-
static int vvmcs_offset(u32 width, u32 type, u32 index)
{
int offset;
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/4] x86/vvmx: Correct the INVALID_PADDR checks for VMPTRLD/VMCLEAR
The referenced addresses also need checking against MAXPHYSADDR.
Signed-off-by: Andrew Cooper
---
CC: Jan Beulich
CC: Wei Liu
CC: Roger Pau Monné
CC: Sergey Dyasli
CC: Jun Nakajima
CC: Kevin Tian
---
xen/arch/x86/hvm/vmx/vvmx.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/vmx/vvmx.c b/xen/arch/x86/hvm/vmx/vvmx.c
index c296660..5daab82 100644
--- a/xen/arch/x86/hvm/vmx/vvmx.c
+++ b/xen/arch/x86/hvm/vmx/vvmx.c
@@ -1672,7 +1672,7 @@ static int nvmx_handle_vmptrld(struct cpu_user_regs *regs)
if ( rc != X86EMUL_OKAY )
return rc;
-if ( gpa & 0xfff )
+if ( (gpa & ~PAGE_MASK) || !gfn_valid(v->domain, gaddr_to_gfn(gpa)) )
{
vmfail(regs, VMX_INSN_VMPTRLD_INVALID_PHYADDR);
goto out;
@@ -1780,7 +1780,7 @@ static int nvmx_handle_vmclear(struct cpu_user_regs *regs)
goto out;
}
-if ( gpa & 0xfff )
+if ( (gpa & ~PAGE_MASK) || !gfn_valid(v->domain, gaddr_to_gfn(gpa)) )
{
vmfail(regs, VMX_INSN_VMCLEAR_INVALID_PHYADDR);
goto out;
--
2.1.4
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 6/8] xen/arm: Initialize trace buffer
Reviewed-by: Andrii Anisov Sincerely, Andrii Anisov. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 5/8] xen/arm: Allow a privileged domain to map foreign page from DOMID_XEN
Reviewed-by: Andrii Anisov Sincerely, Andrii Anisov. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen-init-dom0: set Dom0 UUID if requested
On 15/11/2018 13:35, Wei Liu wrote:
> On Thu, Nov 15, 2018 at 11:20:37AM +, Wei Liu wrote:
>> On Thu, Nov 15, 2018 at 10:45:52AM +, Edwin Török wrote:
>>> On 14/11/2018 18:17, Wei Liu wrote:
Read from XEN_CONFIG_DIR/dom0-uuid. If it contains a valid UUID, set
it for Dom0.
Signed-off-by: Wei Liu
>>> [snip]
>>> In general this looks good, however I am not familiar with libxl
>>> conventions, so just some generic comments below.
>>>
+static void get_dom0_uuid(libxl_uuid *uuid)
+{
+int fd = -1;
+ssize_t r;
+char uuid_buf[LIBXL_UUID_FMTLEN+1];
+
+libxl_uuid_clear(uuid);
+
+fd = open(DOM0_UUID_PATH, O_RDONLY);
+if (fd < 0) {
+fprintf(stderr, "failed to open %s\n", DOM0_UUID_PATH);
+goto out;
+}
+
+r = read(fd, uuid_buf, LIBXL_UUID_FMTLEN);
>>> Could this be a short read? I'm not familiar with libxl conventions, but
>>> would there be a utility function that repeats the read if it is short,
>>> or would fread be better?
>> I can use libxl_read_exactly instead. That saves me from writing some
>> code to handle every corner case.
>>
> On second thought, this requires code to allocating and destroying libxl
> ctx. I will write a loop here to handle short-read instead.
fopen()/fread() will get you the correct behaviour for far less code.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 3/3] xen-init-dom0: set Dom0 UUID if requested
On Thu, Nov 15, 2018 at 11:20:37AM +, Wei Liu wrote:
> On Thu, Nov 15, 2018 at 10:45:52AM +, Edwin Török wrote:
> > On 14/11/2018 18:17, Wei Liu wrote:
> > > Read from XEN_CONFIG_DIR/dom0-uuid. If it contains a valid UUID, set
> > > it for Dom0.
> > >
> > > Signed-off-by: Wei Liu
> >
> > [snip]
> > In general this looks good, however I am not familiar with libxl
> > conventions, so just some generic comments below.
> >
> > > +static void get_dom0_uuid(libxl_uuid *uuid)
> > > +{
> > > +int fd = -1;
> > > +ssize_t r;
> > > +char uuid_buf[LIBXL_UUID_FMTLEN+1];
> > > +
> > > +libxl_uuid_clear(uuid);
> > > +
> > > +fd = open(DOM0_UUID_PATH, O_RDONLY);
> > > +if (fd < 0) {
> > > +fprintf(stderr, "failed to open %s\n", DOM0_UUID_PATH);
> > > +goto out;
> > > +}
> > > +
> > > +r = read(fd, uuid_buf, LIBXL_UUID_FMTLEN);
> >
> > Could this be a short read? I'm not familiar with libxl conventions, but
> > would there be a utility function that repeats the read if it is short,
> > or would fread be better?
>
> I can use libxl_read_exactly instead. That saves me from writing some
> code to handle every corner case.
>
On second thought, this requires code to allocating and destroying libxl
ctx. I will write a loop here to handle short-read instead.
Wei.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/8] xen/arm: p2m: Introduce p2m_get_page_from_gfn
Sorry, Not "comparingly complex", but "equally complex". ;) Sincerely, Andrii Anisov. чт, 15 лист. 2018 о 15:31 Andrii Anisov пише: > > Hello Julien, > > вт, 6 лист. 2018 о 21:16 Julien Grall пише: > > > > In a follow-up patches, we will need to handle get_page_from_gfn > > differently for DOMID_XEN. To keep the code simple move the current > > content in a new separate helper p2m_get_page_from_gfn. > > > > Note the new helper is a not anymore a static inline function as the helper > > is quite complex. > > In the patch "[PATCH 4/8] xen/arm: Add support for read-only foreign > mappings" you make p2m_get_page_from_gfn() comparingly complex, but > still keep it as static inline. Could you please be consistent (making > both of those functions inline or non-inline) > > Sincerely, > Andrii Anisov. ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
