* [xen-unstable test] 56456: regressions - FAIL
@ 2015-05-16 8:51 osstest service user
2015-05-16 11:45 ` Roger Pau Monné
0 siblings, 1 reply; 15+ messages in thread
From: osstest service user @ 2015-05-16 8:51 UTC (permalink / raw)
To: xen-devel; +Cc: ian.jackson
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 14212 bytes --]
flight 56456 xen-unstable real [real]
http://logs.test-lab.xenproject.org/osstest/logs/56456/
Regressions :-(
Tests which did not succeed and are blocking,
including tests which could not be run:
test-amd64-i386-rumpuserxen-i386 15 rumpuserxen-demo-xenstorels/xenstorels.repeat fail REGR. vs. 56375
test-amd64-amd64-rumpuserxen-amd64 15 rumpuserxen-demo-xenstorels/xenstorels.repeat fail REGR. vs. 56375
test-amd64-amd64-xl-multivcpu 14 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl 14 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-credit2 14 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-qemut-win7-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-xl 14 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-xl-qemut-win7-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-pair 21 guest-migrate/src_host/dst_host fail REGR. vs. 56375
test-amd64-amd64-xl-qemut-debianhvm-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-pair 21 guest-migrate/src_host/dst_host fail REGR. vs. 56375
test-amd64-i386-xl-qemut-debianhvm-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-qemut-winxpsp3 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-xl-qemut-winxpsp3-vcpus1 12 guest-localmigrate fail REGR. vs. 56375
test-armhf-armhf-xl-multivcpu 17 leak-check/check fail REGR. vs. 56375
test-amd64-i386-xl-qemut-winxpsp3 12 guest-localmigrate fail REGR. vs. 56375
Regressions which are regarded as allowable (not blocking):
test-amd64-i386-freebsd10-amd64 13 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-sedf 14 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-xl-qemuu-winxpsp3 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-sedf-pin 14 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-qemuu-debianhvm-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-xl-qemuu-win7-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-qemuu-winxpsp3 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-xl-qemuu-debianhvm-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-qemuu-ovmf-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-xl-qemuu-ovmf-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-amd64-xl-qemuu-win7-amd64 12 guest-localmigrate fail REGR. vs. 56375
test-amd64-i386-freebsd10-i386 13 guest-localmigrate fail like 56375
test-amd64-i386-libvirt 11 guest-start fail like 56375
test-armhf-armhf-libvirt 11 guest-start fail like 56375
Tests which did not succeed, but are not blocking:
test-amd64-i386-xl-qemut-debianhvm-amd64-xsm 9 debian-hvm-install fail never pass
test-amd64-amd64-xl-xsm 11 guest-start fail never pass
test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm 9 debian-hvm-install fail never pass
test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm 9 debian-hvm-install fail never pass
test-amd64-amd64-libvirt-xsm 11 guest-start fail never pass
test-amd64-i386-xl-xsm 11 guest-start fail never pass
test-amd64-amd64-xl-pvh-intel 11 guest-start fail never pass
test-amd64-i386-libvirt-xsm 11 guest-start fail never pass
test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm 9 debian-hvm-install fail never pass
test-armhf-armhf-libvirt-xsm 6 xen-boot fail never pass
test-amd64-amd64-xl-pvh-amd 11 guest-start fail never pass
test-armhf-armhf-xl-xsm 6 xen-boot fail never pass
test-armhf-armhf-xl-arndale 12 migrate-support-check fail never pass
test-amd64-amd64-libvirt 12 migrate-support-check fail never pass
test-armhf-armhf-xl-sedf-pin 12 migrate-support-check fail never pass
test-armhf-armhf-xl-cubietruck 12 migrate-support-check fail never pass
test-armhf-armhf-xl 12 migrate-support-check fail never pass
test-armhf-armhf-xl-sedf 12 migrate-support-check fail never pass
test-armhf-armhf-xl-credit2 12 migrate-support-check fail never pass
test-armhf-armhf-xl-multivcpu 12 migrate-support-check fail never pass
version targeted for testing:
xen a809eeea06d20b115d78f12e473502bcb6209844
baseline version:
xen e13013dbf1d5997915548a3b5f1c39594d8c1d7b
------------------------------------------------------------
People who touched revisions under test:
David Vrabel <david.vrabel@citrix.com>
Ian Campbell <ian.campbell@citrix.com>
Jan Beulich <jbeulich@suse.com>
Julien Grall <julien.grall@citrix.com> (ARM)
Roger Pau Monné <roger.pau@citrix.com>
------------------------------------------------------------
jobs:
build-amd64-xsm pass
build-armhf-xsm pass
build-i386-xsm pass
build-amd64 pass
build-armhf pass
build-i386 pass
build-amd64-libvirt pass
build-armhf-libvirt pass
build-i386-libvirt pass
build-amd64-oldkern pass
build-i386-oldkern pass
build-amd64-pvops pass
build-armhf-pvops pass
build-i386-pvops pass
build-amd64-rumpuserxen pass
build-i386-rumpuserxen pass
test-amd64-amd64-xl fail
test-armhf-armhf-xl pass
test-amd64-i386-xl fail
test-amd64-amd64-xl-qemut-debianhvm-amd64-xsm fail
test-amd64-i386-xl-qemut-debianhvm-amd64-xsm fail
test-amd64-amd64-xl-qemuu-debianhvm-amd64-xsm fail
test-amd64-i386-xl-qemuu-debianhvm-amd64-xsm fail
test-amd64-amd64-libvirt-xsm fail
test-armhf-armhf-libvirt-xsm fail
test-amd64-i386-libvirt-xsm fail
test-amd64-amd64-xl-xsm fail
test-armhf-armhf-xl-xsm fail
test-amd64-i386-xl-xsm fail
test-amd64-amd64-xl-pvh-amd fail
test-amd64-i386-qemut-rhel6hvm-amd pass
test-amd64-i386-qemuu-rhel6hvm-amd pass
test-amd64-amd64-xl-qemut-debianhvm-amd64 fail
test-amd64-i386-xl-qemut-debianhvm-amd64 fail
test-amd64-amd64-xl-qemuu-debianhvm-amd64 fail
test-amd64-i386-xl-qemuu-debianhvm-amd64 fail
test-amd64-i386-freebsd10-amd64 fail
test-amd64-amd64-xl-qemuu-ovmf-amd64 fail
test-amd64-i386-xl-qemuu-ovmf-amd64 fail
test-amd64-amd64-rumpuserxen-amd64 fail
test-amd64-amd64-xl-qemut-win7-amd64 fail
test-amd64-i386-xl-qemut-win7-amd64 fail
test-amd64-amd64-xl-qemuu-win7-amd64 fail
test-amd64-i386-xl-qemuu-win7-amd64 fail
test-armhf-armhf-xl-arndale pass
test-amd64-amd64-xl-credit2 fail
test-armhf-armhf-xl-credit2 pass
test-armhf-armhf-xl-cubietruck pass
test-amd64-i386-freebsd10-i386 fail
test-amd64-i386-rumpuserxen-i386 fail
test-amd64-amd64-xl-pvh-intel fail
test-amd64-i386-qemut-rhel6hvm-intel pass
test-amd64-i386-qemuu-rhel6hvm-intel pass
test-amd64-amd64-libvirt pass
test-armhf-armhf-libvirt fail
test-amd64-i386-libvirt fail
test-amd64-amd64-xl-multivcpu fail
test-armhf-armhf-xl-multivcpu fail
test-amd64-amd64-pair fail
test-amd64-i386-pair fail
test-amd64-amd64-xl-sedf-pin fail
test-armhf-armhf-xl-sedf-pin pass
test-amd64-amd64-xl-sedf fail
test-armhf-armhf-xl-sedf pass
test-amd64-i386-xl-qemut-winxpsp3-vcpus1 fail
test-amd64-i386-xl-qemuu-winxpsp3-vcpus1 fail
test-amd64-amd64-xl-qemut-winxpsp3 fail
test-amd64-i386-xl-qemut-winxpsp3 fail
test-amd64-amd64-xl-qemuu-winxpsp3 fail
test-amd64-i386-xl-qemuu-winxpsp3 fail
------------------------------------------------------------
sg-report-flight on osstest.test-lab.xenproject.org
logs: /home/osstest/pub/logs
images: /home/osstest/pub/images
Logs, config files, etc. are available at
http://logs.test-lab.xenproject.org/osstest/logs
Test harness code can be found at
http://xenbits.xen.org/gitweb?p=osstest.git;a=summary
Not pushing.
------------------------------------------------------------
commit a809eeea06d20b115d78f12e473502bcb6209844
Author: Roger Pau Monné <roger.pau@citrix.com>
Date: Fri May 15 10:08:33 2015 +0200
x86: rework paging_log_dirty_op to work with hvm guests
When the caller of paging_log_dirty_op is a hvm guest Xen would choke when
trying to copy the dirty bitmap to the guest because the paging lock is
already held.
Fix this by independently mapping each page of the guest bitmap as needed
without the paging lock held.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
commit acc0899ef41e763c665c542beca6809049fac11c
Author: Roger Pau Monné <roger.pau@citrix.com>
Date: Fri May 15 10:07:50 2015 +0200
x86/hap: make hap_track_dirty_vram use non-contiguous memory for temporary map
Just like it's done for shadow_track_dirty_vram allocate the temporary
buffer using non-contiguous memory.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
commit bd1b4a71b325933a08099676515a7cc8235d7144
Author: Roger Pau Monné <roger.pau@citrix.com>
Date: Fri May 15 10:07:20 2015 +0200
x86/shadow: fix shadow_track_dirty_vram to work on hvm guests
Modify shadow_track_dirty_vram to use a local buffer and then flush to the
guest without the paging_lock held. This is modeled after
hap_track_dirty_vram.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
commit f278fcf19ce15f7b7ee69181560b5884a5e12b66
Author: Roger Pau Monné <roger.pau@citrix.com>
Date: Fri May 15 10:06:04 2015 +0200
introduce a helper to allocate non-contiguous memory
The allocator uses independent calls to alloc_domheap_pages in order to get
the desired amount of memory and then maps all the independent physical
addresses into a contiguous virtual address space.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Tested-by: Julien Grall <julien.grall@citrix.com> (ARM)
Reviewed-by: Tim Deegan <tim@xen.org>
commit e62e49e6d5d4e8d22f3df0b75443ede65a812435
Author: David Vrabel <david.vrabel@citrix.com>
Date: Fri May 15 09:52:25 2015 +0200
x86,arm: remove asm/spinlock.h from all architectures
Now that all architecture use a common ticket lock implementation for
spinlocks, remove the architecture specific byte lock implementations.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Acked-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
commit 45fcc4568c5162b00fb3907fb158af82dd484a3d
Author: David Vrabel <david.vrabel@citrix.com>
Date: Fri May 15 09:49:12 2015 +0200
use ticket locks for spin locks
Replace the byte locks with ticket locks. Ticket locks are: a) fair;
and b) peform better when contented since they spin without an atomic
operation.
The lock is split into two ticket values: head and tail. A locker
acquires a ticket by (atomically) increasing tail and using the
previous tail value. A CPU holds the lock if its ticket == head. The
lock is released by increasing head.
spin_lock_irq() and spin_lock_irqsave() now spin with irqs disabled
(previously, they would spin with irqs enabled if possible). This is
required to prevent deadlocks when the irq handler tries to take the
same lock with a higher ticket.
Architectures need only provide arch_fetch_and_add() and two barriers:
arch_lock_acquire_barrier() and arch_lock_release_barrier().
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
(qemu changes not included)
[-- Attachment #2: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-16 8:51 [xen-unstable test] 56456: regressions - FAIL osstest service user
@ 2015-05-16 11:45 ` Roger Pau Monné
2015-05-18 8:34 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2015-05-16 11:45 UTC (permalink / raw)
To: osstest service user, xen-devel
Cc: Andrew Cooper, ian.jackson, Jan Beulich, Tim Deegan
El 16/05/15 a les 10.51, osstest service user ha escrit:
> flight 56456 xen-unstable real [real]
> http://logs.test-lab.xenproject.org/osstest/logs/56456/
>
> Regressions :-(
This is my fault, paging_gva_to_gfn cannot be used to translate a PV
guest VA to a GFN. The patch above restores the previous path for PV
callers.
---
commit 4cfaf46cbb116ce8dd0124bbbea319489db4b068
Author: Roger Pau Monne <roger.pau@citrix.com>
Date: Sat May 16 13:42:39 2015 +0200
x86: restore PV path on paging_log_dirty_op
Restore previous path for PV domains calling paging_log_dirty_op. This fixes
the fallout from a809eeea06d20b115d78f12e473502bcb6209844.
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
index 5eee88c..2f51126 100644
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -416,6 +416,7 @@ static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
unsigned long gfn;
p2m_type_t p2mt;
+ ASSERT(paging_mode_enabled(current->domain));
gfn = paging_gva_to_gfn(current,
(unsigned long)(dirty_bitmap.p + (pages >> 3)),
&pfec);
@@ -446,6 +447,7 @@ static inline void *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
static inline void unmap_dirty_bitmap(void *addr, struct page_info *page)
{
+ ASSERT(paging_mode_enabled(current->domain));
if ( addr != NULL )
{
unmap_domain_page(addr);
@@ -465,9 +467,9 @@ static int paging_log_dirty_op(struct domain *d,
mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
unsigned long *l1 = NULL;
int i4, i3, i2;
- uint8_t *dirty_bitmap;
- struct page_info *page;
- unsigned long index_mapped;
+ uint8_t *dirty_bitmap = NULL;
+ struct page_info *page = NULL;
+ unsigned long index_mapped = 0;
again:
if ( !resuming )
@@ -482,12 +484,15 @@ static int paging_log_dirty_op(struct domain *d,
p2m_flush_hardware_cached_dirty(d);
}
- index_mapped = resuming ? d->arch.paging.preempt.log_dirty.done : 0;
- dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, index_mapped, &page);
- if ( dirty_bitmap == NULL )
+ if ( paging_mode_enabled(current->domain) )
{
- domain_unpause(d);
- return -EFAULT;
+ index_mapped = resuming ? d->arch.paging.preempt.log_dirty.done : 0;
+ dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, index_mapped, &page);
+ if ( dirty_bitmap == NULL )
+ {
+ domain_unpause(d);
+ return -EFAULT;
+ }
}
paging_lock(d);
@@ -549,7 +554,8 @@ static int paging_log_dirty_op(struct domain *d,
bytes = (unsigned int)((sc->pages - pages + 7) >> 3);
if ( likely(peek) )
{
- if ( pages >> (3 + PAGE_SHIFT) !=
+ if ( paging_mode_enabled(current->domain) &&
+ pages >> (3 + PAGE_SHIFT) !=
index_mapped >> (3 + PAGE_SHIFT) )
{
/* We need to map next page */
@@ -564,13 +570,30 @@ static int paging_log_dirty_op(struct domain *d,
unmap_dirty_bitmap(dirty_bitmap, page);
goto again;
}
- ASSERT(((pages >> 3) % PAGE_SIZE) + bytes <= PAGE_SIZE);
- if ( l1 )
- memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), l1,
- bytes);
+
+ if ( paging_mode_enabled(current->domain) )
+ {
+ ASSERT(((pages >> 3) % PAGE_SIZE) + bytes <= PAGE_SIZE);
+ if ( l1 )
+ memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
+ l1, bytes);
+ else
+ memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
+ 0, bytes);
+ }
else
- memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), 0,
- bytes);
+ {
+ if ( (l1 ? copy_to_guest_offset(sc->dirty_bitmap,
+ pages >> 3,
+ (uint8_t *)l1,
+ bytes)
+ : clear_guest_offset(sc->dirty_bitmap,
+ pages >> 3, bytes)) != 0 )
+ {
+ rv = -EFAULT;
+ goto out;
+ }
+ }
}
pages += bytes << 3;
if ( l1 )
@@ -630,7 +653,8 @@ static int paging_log_dirty_op(struct domain *d,
if ( rv )
{
/* Never leave the domain paused on real errors. */
- unmap_dirty_bitmap(dirty_bitmap, page);
+ if ( paging_mode_enabled(current->domain) )
+ unmap_dirty_bitmap(dirty_bitmap, page);
ASSERT(rv == -ERESTART);
return rv;
}
@@ -643,14 +667,16 @@ static int paging_log_dirty_op(struct domain *d,
* paging modes (shadow or hap). Safe because the domain is paused. */
d->arch.paging.log_dirty.clean_dirty_bitmap(d);
}
- unmap_dirty_bitmap(dirty_bitmap, page);
+ if ( paging_mode_enabled(current->domain) )
+ unmap_dirty_bitmap(dirty_bitmap, page);
domain_unpause(d);
return rv;
out:
d->arch.paging.preempt.dom = NULL;
paging_unlock(d);
- unmap_dirty_bitmap(dirty_bitmap, page);
+ if ( paging_mode_enabled(current->domain) )
+ unmap_dirty_bitmap(dirty_bitmap, page);
domain_unpause(d);
if ( l1 )
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-16 11:45 ` Roger Pau Monné
@ 2015-05-18 8:34 ` Jan Beulich
2015-05-18 10:17 ` Tim Deegan
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2015-05-18 8:34 UTC (permalink / raw)
To: Roger Pau Monné, Tim Deegan; +Cc: AndrewCooper, ian.jackson, xen-devel
>>> On 16.05.15 at 13:45, <roger.pau@citrix.com> wrote:
> El 16/05/15 a les 10.51, osstest service user ha escrit:
>> flight 56456 xen-unstable real [real]
>> http://logs.test-lab.xenproject.org/osstest/logs/56456/
>>
>> Regressions :-(
>
> This is my fault, paging_gva_to_gfn cannot be used to translate a PV
> guest VA to a GFN. The patch above restores the previous path for PV
> callers.
While Tim would have the final say, I certainly would prefer to revert
the offending patch and then apply a correct new version in its stead
in this case (where the fix is not a simple, few lines change).
Jan
> ---
> commit 4cfaf46cbb116ce8dd0124bbbea319489db4b068
> Author: Roger Pau Monne <roger.pau@citrix.com>
> Date: Sat May 16 13:42:39 2015 +0200
>
> x86: restore PV path on paging_log_dirty_op
>
> Restore previous path for PV domains calling paging_log_dirty_op. This
> fixes
> the fallout from a809eeea06d20b115d78f12e473502bcb6209844.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>
> diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> index 5eee88c..2f51126 100644
> --- a/xen/arch/x86/mm/paging.c
> +++ b/xen/arch/x86/mm/paging.c
> @@ -416,6 +416,7 @@ static inline void
> *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
> unsigned long gfn;
> p2m_type_t p2mt;
>
> + ASSERT(paging_mode_enabled(current->domain));
> gfn = paging_gva_to_gfn(current,
> (unsigned long)(dirty_bitmap.p + (pages >> 3)),
> &pfec);
> @@ -446,6 +447,7 @@ static inline void
> *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
>
> static inline void unmap_dirty_bitmap(void *addr, struct page_info *page)
> {
> + ASSERT(paging_mode_enabled(current->domain));
> if ( addr != NULL )
> {
> unmap_domain_page(addr);
> @@ -465,9 +467,9 @@ static int paging_log_dirty_op(struct domain *d,
> mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
> unsigned long *l1 = NULL;
> int i4, i3, i2;
> - uint8_t *dirty_bitmap;
> - struct page_info *page;
> - unsigned long index_mapped;
> + uint8_t *dirty_bitmap = NULL;
> + struct page_info *page = NULL;
> + unsigned long index_mapped = 0;
>
> again:
> if ( !resuming )
> @@ -482,12 +484,15 @@ static int paging_log_dirty_op(struct domain *d,
> p2m_flush_hardware_cached_dirty(d);
> }
>
> - index_mapped = resuming ? d->arch.paging.preempt.log_dirty.done : 0;
> - dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, index_mapped, &page);
> - if ( dirty_bitmap == NULL )
> + if ( paging_mode_enabled(current->domain) )
> {
> - domain_unpause(d);
> - return -EFAULT;
> + index_mapped = resuming ? d->arch.paging.preempt.log_dirty.done : 0;
> + dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, index_mapped,
> &page);
> + if ( dirty_bitmap == NULL )
> + {
> + domain_unpause(d);
> + return -EFAULT;
> + }
> }
>
> paging_lock(d);
> @@ -549,7 +554,8 @@ static int paging_log_dirty_op(struct domain *d,
> bytes = (unsigned int)((sc->pages - pages + 7) >> 3);
> if ( likely(peek) )
> {
> - if ( pages >> (3 + PAGE_SHIFT) !=
> + if ( paging_mode_enabled(current->domain) &&
> + pages >> (3 + PAGE_SHIFT) !=
> index_mapped >> (3 + PAGE_SHIFT) )
> {
> /* We need to map next page */
> @@ -564,13 +570,30 @@ static int paging_log_dirty_op(struct domain *d,
> unmap_dirty_bitmap(dirty_bitmap, page);
> goto again;
> }
> - ASSERT(((pages >> 3) % PAGE_SIZE) + bytes <= PAGE_SIZE);
> - if ( l1 )
> - memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), l1,
> - bytes);
> +
> + if ( paging_mode_enabled(current->domain) )
> + {
> + ASSERT(((pages >> 3) % PAGE_SIZE) + bytes <=
> PAGE_SIZE);
> + if ( l1 )
> + memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> + l1, bytes);
> + else
> + memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> + 0, bytes);
> + }
> else
> - memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), 0,
> - bytes);
> + {
> + if ( (l1 ? copy_to_guest_offset(sc->dirty_bitmap,
> + pages >> 3,
> + (uint8_t *)l1,
> + bytes)
> + : clear_guest_offset(sc->dirty_bitmap,
> + pages >> 3, bytes)) !=
> 0 )
> + {
> + rv = -EFAULT;
> + goto out;
> + }
> + }
> }
> pages += bytes << 3;
> if ( l1 )
> @@ -630,7 +653,8 @@ static int paging_log_dirty_op(struct domain *d,
> if ( rv )
> {
> /* Never leave the domain paused on real errors. */
> - unmap_dirty_bitmap(dirty_bitmap, page);
> + if ( paging_mode_enabled(current->domain) )
> + unmap_dirty_bitmap(dirty_bitmap, page);
> ASSERT(rv == -ERESTART);
> return rv;
> }
> @@ -643,14 +667,16 @@ static int paging_log_dirty_op(struct domain *d,
> * paging modes (shadow or hap). Safe because the domain is
> paused. */
> d->arch.paging.log_dirty.clean_dirty_bitmap(d);
> }
> - unmap_dirty_bitmap(dirty_bitmap, page);
> + if ( paging_mode_enabled(current->domain) )
> + unmap_dirty_bitmap(dirty_bitmap, page);
> domain_unpause(d);
> return rv;
>
> out:
> d->arch.paging.preempt.dom = NULL;
> paging_unlock(d);
> - unmap_dirty_bitmap(dirty_bitmap, page);
> + if ( paging_mode_enabled(current->domain) )
> + unmap_dirty_bitmap(dirty_bitmap, page);
> domain_unpause(d);
>
> if ( l1 )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-18 8:34 ` Jan Beulich
@ 2015-05-18 10:17 ` Tim Deegan
2015-05-18 10:36 ` Jan Beulich
2015-05-18 10:50 ` Roger Pau Monné
0 siblings, 2 replies; 15+ messages in thread
From: Tim Deegan @ 2015-05-18 10:17 UTC (permalink / raw)
To: Jan Beulich; +Cc: AndrewCooper, ian.jackson, xen-devel, Roger Pau Monné
At 09:34 +0100 on 18 May (1431941676), Jan Beulich wrote:
> >>> On 16.05.15 at 13:45, <roger.pau@citrix.com> wrote:
> > El 16/05/15 a les 10.51, osstest service user ha escrit:
> >> flight 56456 xen-unstable real [real]
> >> http://logs.test-lab.xenproject.org/osstest/logs/56456/
> >>
> >> Regressions :-(
> >
> > This is my fault, paging_gva_to_gfn cannot be used to translate a PV
> > guest VA to a GFN. The patch above restores the previous path for PV
> > callers.
>
> While Tim would have the final say, I certainly would prefer to revert
> the offending patch and then apply a correct new version in its stead
> in this case (where the fix is not a simple, few lines change).
I would be OK with a follow-up fix here, but I'm not convinced that
this is it.
In particular, paging_mode_enabled() should be true for any PV domain
that's in log-dirty mode, so presumably the failure is only for lgd
ops on VMs that don't have lgd enabled. So maybe we can either:
- return an error for that case (but we'd want to understand how we
got there first); or
- have map_dirty_bitmap() DTRT, with something like access_ok() +
a linear-pagetable lookup to find the frame.
In any case, this is a regression so we should revert while we figure
it out.
Tim.
> > ---
> > commit 4cfaf46cbb116ce8dd0124bbbea319489db4b068
> > Author: Roger Pau Monne <roger.pau@citrix.com>
> > Date: Sat May 16 13:42:39 2015 +0200
> >
> > x86: restore PV path on paging_log_dirty_op
> >
> > Restore previous path for PV domains calling paging_log_dirty_op. This
> > fixes
> > the fallout from a809eeea06d20b115d78f12e473502bcb6209844.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> > diff --git a/xen/arch/x86/mm/paging.c b/xen/arch/x86/mm/paging.c
> > index 5eee88c..2f51126 100644
> > --- a/xen/arch/x86/mm/paging.c
> > +++ b/xen/arch/x86/mm/paging.c
> > @@ -416,6 +416,7 @@ static inline void
> > *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
> > unsigned long gfn;
> > p2m_type_t p2mt;
> >
> > + ASSERT(paging_mode_enabled(current->domain));
> > gfn = paging_gva_to_gfn(current,
> > (unsigned long)(dirty_bitmap.p + (pages >> 3)),
> > &pfec);
> > @@ -446,6 +447,7 @@ static inline void
> > *map_dirty_bitmap(XEN_GUEST_HANDLE_64(uint8) dirty_bitmap,
> >
> > static inline void unmap_dirty_bitmap(void *addr, struct page_info *page)
> > {
> > + ASSERT(paging_mode_enabled(current->domain));
> > if ( addr != NULL )
> > {
> > unmap_domain_page(addr);
> > @@ -465,9 +467,9 @@ static int paging_log_dirty_op(struct domain *d,
> > mfn_t *l4 = NULL, *l3 = NULL, *l2 = NULL;
> > unsigned long *l1 = NULL;
> > int i4, i3, i2;
> > - uint8_t *dirty_bitmap;
> > - struct page_info *page;
> > - unsigned long index_mapped;
> > + uint8_t *dirty_bitmap = NULL;
> > + struct page_info *page = NULL;
> > + unsigned long index_mapped = 0;
> >
> > again:
> > if ( !resuming )
> > @@ -482,12 +484,15 @@ static int paging_log_dirty_op(struct domain *d,
> > p2m_flush_hardware_cached_dirty(d);
> > }
> >
> > - index_mapped = resuming ? d->arch.paging.preempt.log_dirty.done : 0;
> > - dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, index_mapped, &page);
> > - if ( dirty_bitmap == NULL )
> > + if ( paging_mode_enabled(current->domain) )
> > {
> > - domain_unpause(d);
> > - return -EFAULT;
> > + index_mapped = resuming ? d->arch.paging.preempt.log_dirty.done : 0;
> > + dirty_bitmap = map_dirty_bitmap(sc->dirty_bitmap, index_mapped,
> > &page);
> > + if ( dirty_bitmap == NULL )
> > + {
> > + domain_unpause(d);
> > + return -EFAULT;
> > + }
> > }
> >
> > paging_lock(d);
> > @@ -549,7 +554,8 @@ static int paging_log_dirty_op(struct domain *d,
> > bytes = (unsigned int)((sc->pages - pages + 7) >> 3);
> > if ( likely(peek) )
> > {
> > - if ( pages >> (3 + PAGE_SHIFT) !=
> > + if ( paging_mode_enabled(current->domain) &&
> > + pages >> (3 + PAGE_SHIFT) !=
> > index_mapped >> (3 + PAGE_SHIFT) )
> > {
> > /* We need to map next page */
> > @@ -564,13 +570,30 @@ static int paging_log_dirty_op(struct domain *d,
> > unmap_dirty_bitmap(dirty_bitmap, page);
> > goto again;
> > }
> > - ASSERT(((pages >> 3) % PAGE_SIZE) + bytes <= PAGE_SIZE);
> > - if ( l1 )
> > - memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), l1,
> > - bytes);
> > +
> > + if ( paging_mode_enabled(current->domain) )
> > + {
> > + ASSERT(((pages >> 3) % PAGE_SIZE) + bytes <=
> > PAGE_SIZE);
> > + if ( l1 )
> > + memcpy(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> > + l1, bytes);
> > + else
> > + memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE),
> > + 0, bytes);
> > + }
> > else
> > - memset(dirty_bitmap + ((pages >> 3) % PAGE_SIZE), 0,
> > - bytes);
> > + {
> > + if ( (l1 ? copy_to_guest_offset(sc->dirty_bitmap,
> > + pages >> 3,
> > + (uint8_t *)l1,
> > + bytes)
> > + : clear_guest_offset(sc->dirty_bitmap,
> > + pages >> 3, bytes)) !=
> > 0 )
> > + {
> > + rv = -EFAULT;
> > + goto out;
> > + }
> > + }
> > }
> > pages += bytes << 3;
> > if ( l1 )
> > @@ -630,7 +653,8 @@ static int paging_log_dirty_op(struct domain *d,
> > if ( rv )
> > {
> > /* Never leave the domain paused on real errors. */
> > - unmap_dirty_bitmap(dirty_bitmap, page);
> > + if ( paging_mode_enabled(current->domain) )
> > + unmap_dirty_bitmap(dirty_bitmap, page);
> > ASSERT(rv == -ERESTART);
> > return rv;
> > }
> > @@ -643,14 +667,16 @@ static int paging_log_dirty_op(struct domain *d,
> > * paging modes (shadow or hap). Safe because the domain is
> > paused. */
> > d->arch.paging.log_dirty.clean_dirty_bitmap(d);
> > }
> > - unmap_dirty_bitmap(dirty_bitmap, page);
> > + if ( paging_mode_enabled(current->domain) )
> > + unmap_dirty_bitmap(dirty_bitmap, page);
> > domain_unpause(d);
> > return rv;
> >
> > out:
> > d->arch.paging.preempt.dom = NULL;
> > paging_unlock(d);
> > - unmap_dirty_bitmap(dirty_bitmap, page);
> > + if ( paging_mode_enabled(current->domain) )
> > + unmap_dirty_bitmap(dirty_bitmap, page);
> > domain_unpause(d);
> >
> > if ( l1 )
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-18 10:17 ` Tim Deegan
@ 2015-05-18 10:36 ` Jan Beulich
2015-05-18 10:50 ` Roger Pau Monné
1 sibling, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2015-05-18 10:36 UTC (permalink / raw)
To: Tim Deegan; +Cc: AndrewCooper, ian.jackson, xen-devel, roger.pau
>>> On 18.05.15 at 12:17, <tim@xen.org> wrote:
> At 09:34 +0100 on 18 May (1431941676), Jan Beulich wrote:
>> >>> On 16.05.15 at 13:45, <roger.pau@citrix.com> wrote:
>> > El 16/05/15 a les 10.51, osstest service user ha escrit:
>> >> flight 56456 xen-unstable real [real]
>> >> http://logs.test-lab.xenproject.org/osstest/logs/56456/
>> >>
>> >> Regressions :-(
>> >
>> > This is my fault, paging_gva_to_gfn cannot be used to translate a PV
>> > guest VA to a GFN. The patch above restores the previous path for PV
>> > callers.
>>
>> While Tim would have the final say, I certainly would prefer to revert
>> the offending patch and then apply a correct new version in its stead
>> in this case (where the fix is not a simple, few lines change).
>
> I would be OK with a follow-up fix here, but I'm not convinced that
> this is it.
>
> In particular, paging_mode_enabled() should be true for any PV domain
> that's in log-dirty mode, so presumably the failure is only for lgd
> ops on VMs that don't have lgd enabled. So maybe we can either:
> - return an error for that case (but we'd want to understand how we
> got there first); or
> - have map_dirty_bitmap() DTRT, with something like access_ok() +
> a linear-pagetable lookup to find the frame.
>
> In any case, this is a regression so we should revert while we figure
> it out.
Done.
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-18 10:17 ` Tim Deegan
2015-05-18 10:36 ` Jan Beulich
@ 2015-05-18 10:50 ` Roger Pau Monné
2015-05-18 11:00 ` Tim Deegan
2015-05-18 11:19 ` Jan Beulich
1 sibling, 2 replies; 15+ messages in thread
From: Roger Pau Monné @ 2015-05-18 10:50 UTC (permalink / raw)
To: Tim Deegan, Jan Beulich; +Cc: AndrewCooper, ian.jackson, xen-devel
El 18/05/15 a les 12.17, Tim Deegan ha escrit:
> At 09:34 +0100 on 18 May (1431941676), Jan Beulich wrote:
>>>>> On 16.05.15 at 13:45, <roger.pau@citrix.com> wrote:
>>> El 16/05/15 a les 10.51, osstest service user ha escrit:
>>>> flight 56456 xen-unstable real [real]
>>>> http://logs.test-lab.xenproject.org/osstest/logs/56456/
>>>>
>>>> Regressions :-(
>>>
>>> This is my fault, paging_gva_to_gfn cannot be used to translate a PV
>>> guest VA to a GFN. The patch above restores the previous path for PV
>>> callers.
>>
>> While Tim would have the final say, I certainly would prefer to revert
>> the offending patch and then apply a correct new version in its stead
>> in this case (where the fix is not a simple, few lines change).
>
> I would be OK with a follow-up fix here, but I'm not convinced that
> this is it.
>
> In particular, paging_mode_enabled() should be true for any PV domain
> that's in log-dirty mode, so presumably the failure is only for lgd
> ops on VMs that don't have lgd enabled. So maybe we can either:
> - return an error for that case (but we'd want to understand how we
> got there first); or
The error is caused because we are trying to use paging_gva_to_gfn
against a Dom0 VA, which is a PV guest.
gva_to_gfn is set to sh_gva_to_gfn for a PV Dom0, but calling
vtlb_lookup against a PV guest crashes Xen because the paging structures
are not populated.
> - have map_dirty_bitmap() DTRT, with something like access_ok() +
> a linear-pagetable lookup to find the frame.
That was my first intention, but AFAICT we have no function in tree to
resolve a PV guest VA into a GFN/MFN. The closest thing I could find was
using guest_walk_tables + guest_walk_to_gfn in order to obtain the gfn.
Should I send a patch to introduce a pv_gva_to_gfn function based on that?
Thanks, Roger.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-18 10:50 ` Roger Pau Monné
@ 2015-05-18 11:00 ` Tim Deegan
2015-05-18 11:19 ` Jan Beulich
1 sibling, 0 replies; 15+ messages in thread
From: Tim Deegan @ 2015-05-18 11:00 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: AndrewCooper, ian.jackson, Jan Beulich, xen-devel
At 12:50 +0200 on 18 May (1431953450), Roger Pau Monné wrote:
> El 18/05/15 a les 12.17, Tim Deegan ha escrit:
> > At 09:34 +0100 on 18 May (1431941676), Jan Beulich wrote:
> >>>>> On 16.05.15 at 13:45, <roger.pau@citrix.com> wrote:
> >>> El 16/05/15 a les 10.51, osstest service user ha escrit:
> >>>> flight 56456 xen-unstable real [real]
> >>>> http://logs.test-lab.xenproject.org/osstest/logs/56456/
> >>>>
> >>>> Regressions :-(
> >>>
> >>> This is my fault, paging_gva_to_gfn cannot be used to translate a PV
> >>> guest VA to a GFN. The patch above restores the previous path for PV
> >>> callers.
> >>
> >> While Tim would have the final say, I certainly would prefer to revert
> >> the offending patch and then apply a correct new version in its stead
> >> in this case (where the fix is not a simple, few lines change).
> >
> > I would be OK with a follow-up fix here, but I'm not convinced that
> > this is it.
> >
> > In particular, paging_mode_enabled() should be true for any PV domain
> > that's in log-dirty mode, so presumably the failure is only for lgd
> > ops on VMs that don't have lgd enabled. So maybe we can either:
> > - return an error for that case (but we'd want to understand how we
> > got there first); or
>
> The error is caused because we are trying to use paging_gva_to_gfn
> against a Dom0 VA, which is a PV guest.
Oh right; I had got confused.
> > - have map_dirty_bitmap() DTRT, with something like access_ok() +
> > a linear-pagetable lookup to find the frame.
>
> That was my first intention, but AFAICT we have no function in tree to
> resolve a PV guest VA into a GFN/MFN. The closest thing I could find was
> using guest_walk_tables + guest_walk_to_gfn in order to obtain the gfn.
> Should I send a patch to introduce a pv_gva_to_gfn function based on that?
PV guest addresses need a bit more care: we need to at least call
access_ok() on them to make sure we're not going to write to a Xen
address. For the translation itself, we could use guest_walk_tables(),
though I wouldn't be surprised if that only works on
paging_mode_enabled() guests too. Worth a try though. Otherwise,
guest_get_eff_l1e() will get the gfn but doesn't include a check for
write permissions, so might introduce an in-guest issue.
Tim.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-18 10:50 ` Roger Pau Monné
2015-05-18 11:00 ` Tim Deegan
@ 2015-05-18 11:19 ` Jan Beulich
2015-05-19 10:20 ` Tim Deegan
1 sibling, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2015-05-18 11:19 UTC (permalink / raw)
To: Roger Pau Monné, Tim Deegan; +Cc: AndrewCooper, ian.jackson, xen-devel
>>> On 18.05.15 at 12:50, <roger.pau@citrix.com> wrote:
> El 18/05/15 a les 12.17, Tim Deegan ha escrit:
>> - have map_dirty_bitmap() DTRT, with something like access_ok() +
>> a linear-pagetable lookup to find the frame.
>
> That was my first intention, but AFAICT we have no function in tree to
> resolve a PV guest VA into a GFN/MFN. The closest thing I could find was
> using guest_walk_tables + guest_walk_to_gfn in order to obtain the gfn.
> Should I send a patch to introduce a pv_gva_to_gfn function based on that?
Isn't that what we have the linear page table and guest_map_l1e()
for?
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-18 11:19 ` Jan Beulich
@ 2015-05-19 10:20 ` Tim Deegan
2015-05-19 10:29 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Tim Deegan @ 2015-05-19 10:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: AndrewCooper, ian.jackson, xen-devel, Roger Pau Monné
At 12:19 +0100 on 18 May (1431951570), Jan Beulich wrote:
> >>> On 18.05.15 at 12:50, <roger.pau@citrix.com> wrote:
> > El 18/05/15 a les 12.17, Tim Deegan ha escrit:
> >> - have map_dirty_bitmap() DTRT, with something like access_ok() +
> >> a linear-pagetable lookup to find the frame.
> >
> > That was my first intention, but AFAICT we have no function in tree to
> > resolve a PV guest VA into a GFN/MFN. The closest thing I could find was
> > using guest_walk_tables + guest_walk_to_gfn in order to obtain the gfn.
> > Should I send a patch to introduce a pv_gva_to_gfn function based on that?
>
> Isn't that what we have the linear page table and guest_map_l1e()
> for?
Yes, or in this case guest_get_eff_l1e(). We'd want to make sure we
get_page() the underlying page as well to guard against it being freed
and reused while we have a mapping.
That won't check user/supervisor or write permissions in the upper
levels of the tree. OTOH, __copy_to_user() doesn't either, so maybe
we don't care. Jan, if you're happy with that then so am I.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-19 10:20 ` Tim Deegan
@ 2015-05-19 10:29 ` Jan Beulich
2015-05-19 15:07 ` Roger Pau Monné
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2015-05-19 10:29 UTC (permalink / raw)
To: Tim Deegan; +Cc: AndrewCooper, ian.jackson, xen-devel, roger.pau
>>> On 19.05.15 at 12:20, <tim@xen.org> wrote:
> At 12:19 +0100 on 18 May (1431951570), Jan Beulich wrote:
>> >>> On 18.05.15 at 12:50, <roger.pau@citrix.com> wrote:
>> > El 18/05/15 a les 12.17, Tim Deegan ha escrit:
>> >> - have map_dirty_bitmap() DTRT, with something like access_ok() +
>> >> a linear-pagetable lookup to find the frame.
>> >
>> > That was my first intention, but AFAICT we have no function in tree to
>> > resolve a PV guest VA into a GFN/MFN. The closest thing I could find was
>> > using guest_walk_tables + guest_walk_to_gfn in order to obtain the gfn.
>> > Should I send a patch to introduce a pv_gva_to_gfn function based on that?
>>
>> Isn't that what we have the linear page table and guest_map_l1e()
>> for?
>
> Yes, or in this case guest_get_eff_l1e(). We'd want to make sure we
> get_page() the underlying page as well to guard against it being freed
> and reused while we have a mapping.
>
> That won't check user/supervisor or write permissions in the upper
> levels of the tree. OTOH, __copy_to_user() doesn't either, so maybe
> we don't care.
Hmm, permissions are being checked by __copy_to_user() afaict
(due to us using the actual page tables), so that being bypassed
here would seem wrong then.
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-19 10:29 ` Jan Beulich
@ 2015-05-19 15:07 ` Roger Pau Monné
2015-05-20 8:58 ` Roger Pau Monné
0 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2015-05-19 15:07 UTC (permalink / raw)
To: Jan Beulich, Tim Deegan; +Cc: AndrewCooper, ian.jackson, xen-devel
El 19/05/15 a les 12.29, Jan Beulich ha escrit:
>>>> On 19.05.15 at 12:20, <tim@xen.org> wrote:
>> At 12:19 +0100 on 18 May (1431951570), Jan Beulich wrote:
>>>>>> On 18.05.15 at 12:50, <roger.pau@citrix.com> wrote:
>>>> El 18/05/15 a les 12.17, Tim Deegan ha escrit:
>>>>> - have map_dirty_bitmap() DTRT, with something like access_ok() +
>>>>> a linear-pagetable lookup to find the frame.
>>>>
>>>> That was my first intention, but AFAICT we have no function in tree to
>>>> resolve a PV guest VA into a GFN/MFN. The closest thing I could find was
>>>> using guest_walk_tables + guest_walk_to_gfn in order to obtain the gfn.
>>>> Should I send a patch to introduce a pv_gva_to_gfn function based on that?
>>>
>>> Isn't that what we have the linear page table and guest_map_l1e()
>>> for?
>>
>> Yes, or in this case guest_get_eff_l1e(). We'd want to make sure we
>> get_page() the underlying page as well to guard against it being freed
>> and reused while we have a mapping.
>>
>> That won't check user/supervisor or write permissions in the upper
>> levels of the tree. OTOH, __copy_to_user() doesn't either, so maybe
>> we don't care.
>
> Hmm, permissions are being checked by __copy_to_user() afaict
> (due to us using the actual page tables), so that being bypassed
> here would seem wrong then.
The only way I see to check for permissions of all levels is to use
guest_walk_tables instead of guest_get_eff_l1e, but that's going to make
this quite slow (as compared to the previous implementation).
Roger.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-19 15:07 ` Roger Pau Monné
@ 2015-05-20 8:58 ` Roger Pau Monné
2015-05-20 9:12 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Roger Pau Monné @ 2015-05-20 8:58 UTC (permalink / raw)
To: Jan Beulich, Tim Deegan; +Cc: AndrewCooper, ian.jackson, xen-devel
El 19/05/15 a les 17.07, Roger Pau Monné ha escrit:
> El 19/05/15 a les 12.29, Jan Beulich ha escrit:
>>>>> On 19.05.15 at 12:20, <tim@xen.org> wrote:
>>> At 12:19 +0100 on 18 May (1431951570), Jan Beulich wrote:
>>>>>>> On 18.05.15 at 12:50, <roger.pau@citrix.com> wrote:
>>>>> El 18/05/15 a les 12.17, Tim Deegan ha escrit:
>>>>>> - have map_dirty_bitmap() DTRT, with something like access_ok() +
>>>>>> a linear-pagetable lookup to find the frame.
>>>>>
>>>>> That was my first intention, but AFAICT we have no function in tree to
>>>>> resolve a PV guest VA into a GFN/MFN. The closest thing I could find was
>>>>> using guest_walk_tables + guest_walk_to_gfn in order to obtain the gfn.
>>>>> Should I send a patch to introduce a pv_gva_to_gfn function based on that?
>>>>
>>>> Isn't that what we have the linear page table and guest_map_l1e()
>>>> for?
>>>
>>> Yes, or in this case guest_get_eff_l1e(). We'd want to make sure we
>>> get_page() the underlying page as well to guard against it being freed
>>> and reused while we have a mapping.
>>>
>>> That won't check user/supervisor or write permissions in the upper
>>> levels of the tree. OTOH, __copy_to_user() doesn't either, so maybe
>>> we don't care.
>>
>> Hmm, permissions are being checked by __copy_to_user() afaict
>> (due to us using the actual page tables), so that being bypassed
>> here would seem wrong then.
>
> The only way I see to check for permissions of all levels is to use
> guest_walk_tables instead of guest_get_eff_l1e, but that's going to make
> this quite slow (as compared to the previous implementation).
After looking into this a little bit more, I'm afraid I don't see a
straight forward way to check for the permissions of all paging levels.
Here are the options I've found in order to deal with this:
- Use guest_get_eff_l1e and only check for the permissions of the L1
entry. Is it possible that the guest places an invalid entry in the
linear l1 table without Xen realizing?
- Add a new function hook somewhere (pv_domain maybe?) that can be
used to translate GVA to PFN for PV guests (mimicking what
paging_gva_to_gfn does). This would be implemented using
guest_walk_X_level, where X is the paging levels of the guest.
- Use some glue to be able to call guest_walk_{3/4}_level from
paging.c directly, and correctly choose which one to use based on
the guest bitness. IMHO this looks quite wacky, and I'm not even
sure if it's possible given the amount of preprocessor foo in
guest_pt.h.
I have the first option already implemented, but I would appreciate some
advice regarding the security implications of it.
Thanks, Roger.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-20 8:58 ` Roger Pau Monné
@ 2015-05-20 9:12 ` Jan Beulich
2015-05-20 9:43 ` Tim Deegan
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2015-05-20 9:12 UTC (permalink / raw)
To: Roger Pau Monné, Tim Deegan; +Cc: AndrewCooper, ian.jackson, xen-devel
>>> On 20.05.15 at 10:58, <roger.pau@citrix.com> wrote:
> After looking into this a little bit more, I'm afraid I don't see a
> straight forward way to check for the permissions of all paging levels.
> Here are the options I've found in order to deal with this:
>
> - Use guest_get_eff_l1e and only check for the permissions of the L1
> entry. Is it possible that the guest places an invalid entry in the
> linear l1 table without Xen realizing?
No - all page table changes are being validated by Xen.
> - Add a new function hook somewhere (pv_domain maybe?) that can be
> used to translate GVA to PFN for PV guests (mimicking what
> paging_gva_to_gfn does). This would be implemented using
> guest_walk_X_level, where X is the paging levels of the guest.
>
> - Use some glue to be able to call guest_walk_{3/4}_level from
> paging.c directly, and correctly choose which one to use based on
> the guest bitness. IMHO this looks quite wacky, and I'm not even
> sure if it's possible given the amount of preprocessor foo in
> guest_pt.h.
>
> I have the first option already implemented, but I would appreciate some
> advice regarding the security implications of it.
I think with all of the options here being unsatisfactory we should
reconsider your original option of restoring previous behavior
(without any mapping) for the PV case. Tim?
Jan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-20 9:12 ` Jan Beulich
@ 2015-05-20 9:43 ` Tim Deegan
2015-05-20 9:55 ` Roger Pau Monné
0 siblings, 1 reply; 15+ messages in thread
From: Tim Deegan @ 2015-05-20 9:43 UTC (permalink / raw)
To: Jan Beulich; +Cc: AndrewCooper, ian.jackson, xen-devel, Roger Pau Monné
At 10:12 +0100 on 20 May (1432116766), Jan Beulich wrote:
> >>> On 20.05.15 at 10:58, <roger.pau@citrix.com> wrote:
> > After looking into this a little bit more, I'm afraid I don't see a
> > straight forward way to check for the permissions of all paging levels.
> > Here are the options I've found in order to deal with this:
> >
> > - Use guest_get_eff_l1e and only check for the permissions of the L1
> > entry. Is it possible that the guest places an invalid entry in the
> > linear l1 table without Xen realizing?
>
> No - all page table changes are being validated by Xen.
Yes, using guest_get_eff_l1e() is safe for Xen. The only concern is
whether it's safe for the guest -- Xen might not honour an upper-level
read-only mark (which copy_to_guest() would) or a supervisor-mode-only
mark (which it wouldn't).
> > - Add a new function hook somewhere (pv_domain maybe?) that can be
> > used to translate GVA to PFN for PV guests (mimicking what
> > paging_gva_to_gfn does). This would be implemented using
> > guest_walk_X_level, where X is the paging levels of the guest.
> >
> > - Use some glue to be able to call guest_walk_{3/4}_level from
> > paging.c directly, and correctly choose which one to use based on
> > the guest bitness. IMHO this looks quite wacky, and I'm not even
> > sure if it's possible given the amount of preprocessor foo in
> > guest_pt.h.
> >
> > I have the first option already implemented, but I would appreciate some
> > advice regarding the security implications of it.
>
> I think with all of the options here being unsatisfactory we should
> reconsider your original option of restoring previous behavior
> (without any mapping) for the PV case. Tim?
Yeah, I don't think it's worth adding a bunch mode pagetable-walk
machinery just to keep this function clean. So I suppose we have to
have two paths. in this code.
Cheers,
Tim.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [xen-unstable test] 56456: regressions - FAIL
2015-05-20 9:43 ` Tim Deegan
@ 2015-05-20 9:55 ` Roger Pau Monné
0 siblings, 0 replies; 15+ messages in thread
From: Roger Pau Monné @ 2015-05-20 9:55 UTC (permalink / raw)
To: Tim Deegan, Jan Beulich; +Cc: AndrewCooper, ian.jackson, xen-devel
El 20/05/15 a les 11.43, Tim Deegan ha escrit:
> At 10:12 +0100 on 20 May (1432116766), Jan Beulich wrote:
>>>>> On 20.05.15 at 10:58, <roger.pau@citrix.com> wrote:
>>> After looking into this a little bit more, I'm afraid I don't see a
>>> straight forward way to check for the permissions of all paging levels.
>>> Here are the options I've found in order to deal with this:
>>>
>>> - Use guest_get_eff_l1e and only check for the permissions of the L1
>>> entry. Is it possible that the guest places an invalid entry in the
>>> linear l1 table without Xen realizing?
>>
>> No - all page table changes are being validated by Xen.
>
> Yes, using guest_get_eff_l1e() is safe for Xen. The only concern is
> whether it's safe for the guest -- Xen might not honour an upper-level
> read-only mark (which copy_to_guest() would) or a supervisor-mode-only
> mark (which it wouldn't).
>
>>> - Add a new function hook somewhere (pv_domain maybe?) that can be
>>> used to translate GVA to PFN for PV guests (mimicking what
>>> paging_gva_to_gfn does). This would be implemented using
>>> guest_walk_X_level, where X is the paging levels of the guest.
>>>
>>> - Use some glue to be able to call guest_walk_{3/4}_level from
>>> paging.c directly, and correctly choose which one to use based on
>>> the guest bitness. IMHO this looks quite wacky, and I'm not even
>>> sure if it's possible given the amount of preprocessor foo in
>>> guest_pt.h.
>>>
>>> I have the first option already implemented, but I would appreciate some
>>> advice regarding the security implications of it.
>>
>> I think with all of the options here being unsatisfactory we should
>> reconsider your original option of restoring previous behavior
>> (without any mapping) for the PV case. Tim?
>
> Yeah, I don't think it's worth adding a bunch mode pagetable-walk
> machinery just to keep this function clean. So I suppose we have to
> have two paths. in this code.
FWIW there's also the option of taking the callers p2m lock if it's a
HVM guest:
http://lists.xen.org/archives/html/xen-devel/2014-10/msg01769.html
And avoid doing any modifications of the code paths.
Roger.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-05-20 9:55 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-05-16 8:51 [xen-unstable test] 56456: regressions - FAIL osstest service user
2015-05-16 11:45 ` Roger Pau Monné
2015-05-18 8:34 ` Jan Beulich
2015-05-18 10:17 ` Tim Deegan
2015-05-18 10:36 ` Jan Beulich
2015-05-18 10:50 ` Roger Pau Monné
2015-05-18 11:00 ` Tim Deegan
2015-05-18 11:19 ` Jan Beulich
2015-05-19 10:20 ` Tim Deegan
2015-05-19 10:29 ` Jan Beulich
2015-05-19 15:07 ` Roger Pau Monné
2015-05-20 8:58 ` Roger Pau Monné
2015-05-20 9:12 ` Jan Beulich
2015-05-20 9:43 ` Tim Deegan
2015-05-20 9:55 ` Roger Pau Monné
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).