* [PATCH 0/3] x86: adjustments to page table updates
@ 2017-10-12 9:38 Jan Beulich
2017-10-12 10:00 ` [PATCH 1/3] x86: request page table page-in for the correct domain Jan Beulich
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Jan Beulich @ 2017-10-12 9:38 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Julien Grall
The first two patches are bug fixes and hence candidates for 4.10.
The 3rd is mostly cleanup, and hence intended only for after 4.10.
1: request page table page-in for the correct domain
2: fix do_update_va_mapping_otherdomain() wrt translated domains
3: tighten MMU_*PT_UPDATE* check and combine error paths
Signed-off-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] x86: request page table page-in for the correct domain
2017-10-12 9:38 [PATCH 0/3] x86: adjustments to page table updates Jan Beulich
@ 2017-10-12 10:00 ` Jan Beulich
2017-10-12 10:02 ` Andrew Cooper
2017-10-12 10:00 ` [PATCH 2/3] x86: fix do_update_va_mapping_otherdomain() wrt translated domains Jan Beulich
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2017-10-12 10:00 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Julien Grall
The domain passed to p2m_mem_paging_populate() should match the one
passed to the corresponding get_page_from_gfn().
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3287,7 +3287,7 @@ long do_mmu_update(
if ( p2m_is_paged(p2mt) )
{
ASSERT(!page);
- p2m_mem_paging_populate(pg_owner, gmfn);
+ p2m_mem_paging_populate(pt_owner, gmfn);
rc = -ENOENT;
break;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/3] x86: fix do_update_va_mapping_otherdomain() wrt translated domains
2017-10-12 9:38 [PATCH 0/3] x86: adjustments to page table updates Jan Beulich
2017-10-12 10:00 ` [PATCH 1/3] x86: request page table page-in for the correct domain Jan Beulich
@ 2017-10-12 10:00 ` Jan Beulich
2017-10-12 11:18 ` Andrew Cooper
2017-10-12 10:01 ` [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths Jan Beulich
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2017-10-12 10:00 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Julien Grall
While I can't seem to find any users of this hypercall (being a likely
explanation of why the problem wasn't noticed so far), just like for
do_mmu_update() paged-out and shared page handling is needed here. Move
all this logic into mod_l1_entry(), which then also results in no
longer
- doing any of this handling for non-present PTEs,
- acquiring two temporary page references when one is already more than
enough.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Now that L1 entry handling in do_mmu_update() is sufficiently similar
again to that of L2/L3/L4 entries, I wonder whether it wouldn't it be
better for the function to refuse pg_owner != pt_owner for L2/L3/L4
updates. Right now the passed in foreign domain ID is simply ignored
in that case (except for the XSM check).
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1632,7 +1632,6 @@ static int mod_l1_entry(l1_pgentry_t *pl
if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
{
- /* Translate foreign guest addresses. */
struct page_info *page = NULL;
if ( unlikely(l1e_get_flags(nl1e) & l1_disallow_mask(pt_dom)) )
@@ -1642,9 +1641,35 @@ static int mod_l1_entry(l1_pgentry_t *pl
return -EINVAL;
}
+ /* Translate foreign guest address. */
if ( paging_mode_translate(pg_dom) )
{
- page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), NULL, P2M_ALLOC);
+ p2m_type_t p2mt;
+ p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
+ P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;
+
+ page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), &p2mt, q);
+
+ if ( p2m_is_paged(p2mt) )
+ {
+ if ( page )
+ put_page(page);
+ p2m_mem_paging_populate(pg_dom, l1e_get_pfn(nl1e));
+ return -ENOENT;
+ }
+
+ if ( p2mt == p2m_ram_paging_in && !page )
+ return -ENOENT;
+
+ /* Did our attempt to unshare fail? */
+ if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
+ {
+ /* We could not have obtained a page ref. */
+ ASSERT(!page);
+ /* And mem_sharing_notify has already been called. */
+ return -ENOMEM;
+ }
+
if ( !page )
return -EINVAL;
nl1e = l1e_from_page(page, l1e_get_flags(nl1e));
@@ -3315,47 +3340,10 @@ long do_mmu_update(
switch ( page->u.inuse.type_info & PGT_type_mask )
{
case PGT_l1_page_table:
- {
- l1_pgentry_t l1e = l1e_from_intpte(req.val);
- p2m_type_t l1e_p2mt = p2m_ram_rw;
- struct page_info *target = NULL;
- p2m_query_t q = (l1e_get_flags(l1e) & _PAGE_RW) ?
- P2M_UNSHARE : P2M_ALLOC;
-
- if ( paging_mode_translate(pg_owner) )
- target = get_page_from_gfn(pg_owner, l1e_get_pfn(l1e),
- &l1e_p2mt, q);
-
- if ( p2m_is_paged(l1e_p2mt) )
- {
- if ( target )
- put_page(target);
- p2m_mem_paging_populate(pg_owner, l1e_get_pfn(l1e));
- rc = -ENOENT;
- break;
- }
- else if ( p2m_ram_paging_in == l1e_p2mt && !target )
- {
- rc = -ENOENT;
- break;
- }
- /* If we tried to unshare and failed */
- else if ( (q & P2M_UNSHARE) && p2m_is_shared(l1e_p2mt) )
- {
- /* We could not have obtained a page ref. */
- ASSERT(target == NULL);
- /* And mem_sharing_notify has already been called. */
- rc = -ENOMEM;
- break;
- }
-
- rc = mod_l1_entry(va, l1e, mfn,
+ rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
pg_owner);
- if ( target )
- put_page(target);
- }
- break;
+ break;
case PGT_l2_page_table:
rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
@@ -3367,7 +3355,7 @@ long do_mmu_update(
case PGT_l4_page_table:
rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
- break;
+ break;
case PGT_writable_page:
perfc_incr(writable_mmu_updates);
if ( paging_write_guest_entry(v, va, req.val, _mfn(mfn)) )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths
2017-10-12 9:38 [PATCH 0/3] x86: adjustments to page table updates Jan Beulich
2017-10-12 10:00 ` [PATCH 1/3] x86: request page table page-in for the correct domain Jan Beulich
2017-10-12 10:00 ` [PATCH 2/3] x86: fix do_update_va_mapping_otherdomain() wrt translated domains Jan Beulich
@ 2017-10-12 10:01 ` Jan Beulich
2017-10-12 11:31 ` Andrew Cooper
2017-12-04 10:41 ` Ping: " Jan Beulich
2017-10-12 12:24 ` [PATCH 4/3] x86: don't ignore foreigndom on L2/L3/L4 page table updates Jan Beulich
2017-10-13 9:43 ` [PATCH 0/3] x86: adjustments to " Julien Grall
4 siblings, 2 replies; 15+ messages in thread
From: Jan Beulich @ 2017-10-12 10:01 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
Don't accept anything other than r/w RAM pages and move the paged-out
check into the (unlikely) error path following that check.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3507,18 +3507,18 @@ long do_mmu_update(
gmfn = req.ptr >> PAGE_SHIFT;
page = get_page_from_gfn(pt_owner, gmfn, &p2mt, P2M_ALLOC);
- if ( p2m_is_paged(p2mt) )
+ if ( unlikely(!page) || p2mt != p2m_ram_rw )
{
- ASSERT(!page);
- p2m_mem_paging_populate(pt_owner, gmfn);
- rc = -ENOENT;
- break;
- }
-
- if ( unlikely(!page) )
- {
- gdprintk(XENLOG_WARNING,
- "Could not get page for normal update\n");
+ if ( page )
+ put_page(page);
+ if ( p2m_is_paged(p2mt) )
+ {
+ p2m_mem_paging_populate(pt_owner, gmfn);
+ rc = -ENOENT;
+ }
+ else
+ gdprintk(XENLOG_WARNING,
+ "Could not get page for normal update\n");
break;
}
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] x86: request page table page-in for the correct domain
2017-10-12 10:00 ` [PATCH 1/3] x86: request page table page-in for the correct domain Jan Beulich
@ 2017-10-12 10:02 ` Andrew Cooper
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2017-10-12 10:02 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Julien Grall
On 12/10/17 11:00, Jan Beulich wrote:
> The domain passed to p2m_mem_paging_populate() should match the one
> passed to the corresponding get_page_from_gfn().
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3287,7 +3287,7 @@ long do_mmu_update(
> if ( p2m_is_paged(p2mt) )
> {
> ASSERT(!page);
> - p2m_mem_paging_populate(pg_owner, gmfn);
> + p2m_mem_paging_populate(pt_owner, gmfn);
> rc = -ENOENT;
> break;
> }
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] x86: fix do_update_va_mapping_otherdomain() wrt translated domains
2017-10-12 10:00 ` [PATCH 2/3] x86: fix do_update_va_mapping_otherdomain() wrt translated domains Jan Beulich
@ 2017-10-12 11:18 ` Andrew Cooper
2017-10-12 11:27 ` Jan Beulich
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2017-10-12 11:18 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Julien Grall
[-- Attachment #1.1: Type: text/plain, Size: 7350 bytes --]
On 12/10/17 11:00, Jan Beulich wrote:
> While I can't seem to find any users of this hypercall (being a likely
> explanation of why the problem wasn't noticed so far), just like for
Judging by c/s a51ed685b which shifted
__HYPERVISOR_update_va_mapping_otherdomain's hypercall number to make
space for __HYPERVISOR_grant_table_op, I'd have said the chance of it
being used was slim. However,
andrewcoop@andrewcoop:/local/xen.git/xen$ git checkout a51ed685
andrewcoop@andrewcoop:/local/xen.git/xen$ git grep update_va_mapping_otherdomain -- :/
../linux-2.6.7-xen-sparse/drivers/xen/blkback/blkback.c:320: if ( HYPERVISOR_update_va_mapping_otherdomain(
../linux-2.6.7-xen-sparse/drivers/xen/blkback/blkback.c:404: mcl[i].op = __HYPERVISOR_update_va_mapping_otherdomain;
../linux-2.6.7-xen-sparse/drivers/xen/netback/netback.c:516: mcl[0].op = __HYPERVISOR_update_va_mapping_otherdomain;
../linux-2.6.7-xen-sparse/include/asm-xen/hypervisor.h:458:static inline int HYPERVISOR_update_va_mapping_otherdomain(
../linux-2.6.7-xen-sparse/include/asm-xen/hypervisor.h:464: : "=a" (ret) : "0" (__HYPERVISOR_update_va_mapping_otherdomain),
arch/x86/memory.c:1264:int do_update_va_mapping_otherdomain(unsigned long page_nr,
arch/x86/x86_32/entry.S:723: .long SYMBOL_NAME(do_update_va_mapping_otherdomain)
include/hypervisor-ifs/hypervisor-if.h:50:#define __HYPERVISOR_update_va_mapping_otherdomain 22
It certainly was used at that point in history. If none of that code
has survived into more recent version {blk,net}back, it is probably that
the hypercall isn't used any more.
> do_mmu_update() paged-out and shared page handling is needed here. Move
> all this logic into mod_l1_entry(), which then also results in no
> longer
> - doing any of this handling for non-present PTEs,
> - acquiring two temporary page references when one is already more than
> enough.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Now that L1 entry handling in do_mmu_update() is sufficiently similar
> again to that of L2/L3/L4 entries, I wonder whether it wouldn't it be
> better for the function to refuse pg_owner != pt_owner for L2/L3/L4
> updates. Right now the passed in foreign domain ID is simply ignored
> in that case (except for the XSM check).
I can't see anything good coming from having pg_owner != pt_owner in non
L1 pagetables. Explicit rejection is certainly better than doing the
wrong thing silently under the hood.
Do you want to do a separate patch for that, or fold it into this one?
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -1632,7 +1632,6 @@ static int mod_l1_entry(l1_pgentry_t *pl
>
> if ( l1e_get_flags(nl1e) & _PAGE_PRESENT )
> {
> - /* Translate foreign guest addresses. */
> struct page_info *page = NULL;
>
> if ( unlikely(l1e_get_flags(nl1e) & l1_disallow_mask(pt_dom)) )
> @@ -1642,9 +1641,35 @@ static int mod_l1_entry(l1_pgentry_t *pl
> return -EINVAL;
> }
>
> + /* Translate foreign guest address. */
> if ( paging_mode_translate(pg_dom) )
> {
> - page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), NULL, P2M_ALLOC);
> + p2m_type_t p2mt;
> + p2m_query_t q = l1e_get_flags(nl1e) & _PAGE_RW ?
> + P2M_ALLOC | P2M_UNSHARE : P2M_ALLOC;
> +
> + page = get_page_from_gfn(pg_dom, l1e_get_pfn(nl1e), &p2mt, q);
> +
> + if ( p2m_is_paged(p2mt) )
> + {
> + if ( page )
> + put_page(page);
> + p2m_mem_paging_populate(pg_dom, l1e_get_pfn(nl1e));
> + return -ENOENT;
> + }
> +
> + if ( p2mt == p2m_ram_paging_in && !page )
> + return -ENOENT;
> +
> + /* Did our attempt to unshare fail? */
> + if ( (q & P2M_UNSHARE) && p2m_is_shared(p2mt) )
> + {
> + /* We could not have obtained a page ref. */
> + ASSERT(!page);
> + /* And mem_sharing_notify has already been called. */
> + return -ENOMEM;
> + }
> +
> if ( !page )
> return -EINVAL;
> nl1e = l1e_from_page(page, l1e_get_flags(nl1e));
> @@ -3315,47 +3340,10 @@ long do_mmu_update(
> switch ( page->u.inuse.type_info & PGT_type_mask )
> {
> case PGT_l1_page_table:
> - {
> - l1_pgentry_t l1e = l1e_from_intpte(req.val);
> - p2m_type_t l1e_p2mt = p2m_ram_rw;
> - struct page_info *target = NULL;
> - p2m_query_t q = (l1e_get_flags(l1e) & _PAGE_RW) ?
> - P2M_UNSHARE : P2M_ALLOC;
> -
> - if ( paging_mode_translate(pg_owner) )
> - target = get_page_from_gfn(pg_owner, l1e_get_pfn(l1e),
> - &l1e_p2mt, q);
> -
> - if ( p2m_is_paged(l1e_p2mt) )
> - {
> - if ( target )
> - put_page(target);
> - p2m_mem_paging_populate(pg_owner, l1e_get_pfn(l1e));
> - rc = -ENOENT;
> - break;
> - }
> - else if ( p2m_ram_paging_in == l1e_p2mt && !target )
> - {
> - rc = -ENOENT;
> - break;
> - }
> - /* If we tried to unshare and failed */
> - else if ( (q & P2M_UNSHARE) && p2m_is_shared(l1e_p2mt) )
> - {
> - /* We could not have obtained a page ref. */
> - ASSERT(target == NULL);
> - /* And mem_sharing_notify has already been called. */
> - rc = -ENOMEM;
> - break;
> - }
> -
> - rc = mod_l1_entry(va, l1e, mfn,
> + rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
> pg_owner);
> - if ( target )
> - put_page(target);
> - }
> - break;
> + break;
> case PGT_l2_page_table:
> rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> @@ -3367,7 +3355,7 @@ long do_mmu_update(
> case PGT_l4_page_table:
> rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> - break;
> + break;
If we are tidying up the style, could we also get some newlines between
break and case?
Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> case PGT_writable_page:
> perfc_incr(writable_mmu_updates);
> if ( paging_write_guest_entry(v, va, req.val, _mfn(mfn)) )
>
>
[-- Attachment #1.2: Type: text/html, Size: 8477 bytes --]
[-- Attachment #2: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] x86: fix do_update_va_mapping_otherdomain() wrt translated domains
2017-10-12 11:18 ` Andrew Cooper
@ 2017-10-12 11:27 ` Jan Beulich
0 siblings, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2017-10-12 11:27 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Julien Grall
>>> On 12.10.17 at 13:18, <andrew.cooper3@citrix.com> wrote:
> On 12/10/17 11:00, Jan Beulich wrote:
>> While I can't seem to find any users of this hypercall (being a likely
>> explanation of why the problem wasn't noticed so far), just like for
>
> Judging by c/s a51ed685b which shifted
> __HYPERVISOR_update_va_mapping_otherdomain's hypercall number to make
> space for __HYPERVISOR_grant_table_op, I'd have said the chance of it
> being used was slim. However,
>
> andrewcoop@andrewcoop:/local/xen.git/xen$ git checkout a51ed685
> andrewcoop@andrewcoop:/local/xen.git/xen$ git grep
> update_va_mapping_otherdomain -- :/
> ../linux-2.6.7-xen-sparse/drivers/xen/blkback/blkback.c:320: if (
> HYPERVISOR_update_va_mapping_otherdomain(
> ../linux-2.6.7-xen-sparse/drivers/xen/blkback/blkback.c:404:
> mcl[i].op = __HYPERVISOR_update_va_mapping_otherdomain;
> ../linux-2.6.7-xen-sparse/drivers/xen/netback/netback.c:516:
> mcl[0].op = __HYPERVISOR_update_va_mapping_otherdomain;
> ../linux-2.6.7-xen-sparse/include/asm-xen/hypervisor.h:458:static inline int
> HYPERVISOR_update_va_mapping_otherdomain(
> ../linux-2.6.7-xen-sparse/include/asm-xen/hypervisor.h:464: : "=a"
> (ret) : "0" (__HYPERVISOR_update_va_mapping_otherdomain),
> arch/x86/memory.c:1264:int do_update_va_mapping_otherdomain(unsigned long
> page_nr,
> arch/x86/x86_32/entry.S:723: .long
> SYMBOL_NAME(do_update_va_mapping_otherdomain)
> include/hypervisor-ifs/hypervisor-if.h:50:#define
> __HYPERVISOR_update_va_mapping_otherdomain 22
>
>
> It certainly was used at that point in history. If none of that code
> has survived into more recent version {blk,net}back, it is probably that
> the hypercall isn't used any more.
I did my check on Linux 4.4.88 (plus tool stack and qemu),
without finding anything.
>> do_mmu_update() paged-out and shared page handling is needed here. Move
>> all this logic into mod_l1_entry(), which then also results in no
>> longer
>> - doing any of this handling for non-present PTEs,
>> - acquiring two temporary page references when one is already more than
>> enough.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Now that L1 entry handling in do_mmu_update() is sufficiently similar
>> again to that of L2/L3/L4 entries, I wonder whether it wouldn't it be
>> better for the function to refuse pg_owner != pt_owner for L2/L3/L4
>> updates. Right now the passed in foreign domain ID is simply ignored
>> in that case (except for the XSM check).
>
> I can't see anything good coming from having pg_owner != pt_owner in non
> L1 pagetables. Explicit rejection is certainly better than doing the
> wrong thing silently under the hood.
>
> Do you want to do a separate patch for that, or fold it into this one?
I'll do it separately - this again wouldn't really qualify for 4.10.
>> @@ -3315,47 +3340,10 @@ long do_mmu_update(
>> switch ( page->u.inuse.type_info & PGT_type_mask )
>> {
>> case PGT_l1_page_table:
>> - {
>> - l1_pgentry_t l1e = l1e_from_intpte(req.val);
>> - p2m_type_t l1e_p2mt = p2m_ram_rw;
>> - struct page_info *target = NULL;
>> - p2m_query_t q = (l1e_get_flags(l1e) & _PAGE_RW) ?
>> - P2M_UNSHARE : P2M_ALLOC;
>> -
>> - if ( paging_mode_translate(pg_owner) )
>> - target = get_page_from_gfn(pg_owner, l1e_get_pfn(l1e),
>> - &l1e_p2mt, q);
>> -
>> - if ( p2m_is_paged(l1e_p2mt) )
>> - {
>> - if ( target )
>> - put_page(target);
>> - p2m_mem_paging_populate(pg_owner, l1e_get_pfn(l1e));
>> - rc = -ENOENT;
>> - break;
>> - }
>> - else if ( p2m_ram_paging_in == l1e_p2mt && !target )
>> - {
>> - rc = -ENOENT;
>> - break;
>> - }
>> - /* If we tried to unshare and failed */
>> - else if ( (q & P2M_UNSHARE) && p2m_is_shared(l1e_p2mt) )
>> - {
>> - /* We could not have obtained a page ref. */
>> - ASSERT(target == NULL);
>> - /* And mem_sharing_notify has already been called. */
>> - rc = -ENOMEM;
>> - break;
>> - }
>> -
>> - rc = mod_l1_entry(va, l1e, mfn,
>> + rc = mod_l1_entry(va, l1e_from_intpte(req.val), mfn,
>> cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
>> pg_owner);
>> - if ( target )
>> - put_page(target);
>> - }
>> - break;
>> + break;
>> case PGT_l2_page_table:
>> rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
>> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
>> @@ -3367,7 +3355,7 @@ long do_mmu_update(
>> case PGT_l4_page_table:
>> rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
>> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
>> - break;
>> + break;
>
> If we are tidying up the style, could we also get some newlines between
> break and case?
I had considered that, but then discarded the idea for the switch
as whole not being all that large, yet the diff becoming quite a bit
larger if I did.
> Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks, Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths
2017-10-12 10:01 ` [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths Jan Beulich
@ 2017-10-12 11:31 ` Andrew Cooper
2017-10-12 12:14 ` Jan Beulich
2017-12-04 10:41 ` Ping: " Jan Beulich
1 sibling, 1 reply; 15+ messages in thread
From: Andrew Cooper @ 2017-10-12 11:31 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 12/10/17 11:01, Jan Beulich wrote:
> Don't accept anything other than r/w RAM pages and move the paged-out
> check into the (unlikely) error path following that check.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
How does dom0 boot with this change in place? You appear to have
prohibited mapping MMIO frames.
~Andrew
>
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3507,18 +3507,18 @@ long do_mmu_update(
> gmfn = req.ptr >> PAGE_SHIFT;
> page = get_page_from_gfn(pt_owner, gmfn, &p2mt, P2M_ALLOC);
>
> - if ( p2m_is_paged(p2mt) )
> + if ( unlikely(!page) || p2mt != p2m_ram_rw )
> {
> - ASSERT(!page);
> - p2m_mem_paging_populate(pt_owner, gmfn);
> - rc = -ENOENT;
> - break;
> - }
> -
> - if ( unlikely(!page) )
> - {
> - gdprintk(XENLOG_WARNING,
> - "Could not get page for normal update\n");
> + if ( page )
> + put_page(page);
> + if ( p2m_is_paged(p2mt) )
> + {
> + p2m_mem_paging_populate(pt_owner, gmfn);
> + rc = -ENOENT;
> + }
> + else
> + gdprintk(XENLOG_WARNING,
> + "Could not get page for normal update\n");
> break;
> }
>
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths
2017-10-12 11:31 ` Andrew Cooper
@ 2017-10-12 12:14 ` Jan Beulich
2017-12-04 16:26 ` Andrew Cooper
0 siblings, 1 reply; 15+ messages in thread
From: Jan Beulich @ 2017-10-12 12:14 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 12.10.17 at 13:31, <andrew.cooper3@citrix.com> wrote:
> On 12/10/17 11:01, Jan Beulich wrote:
>> Don't accept anything other than r/w RAM pages and move the paged-out
>> check into the (unlikely) error path following that check.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> How does dom0 boot with this change in place? You appear to have
> prohibited mapping MMIO frames.
The page in question is a page table one, which can't be MMIO.
Dom0 is booting fine.
Jan
>> --- a/xen/arch/x86/mm.c
>> +++ b/xen/arch/x86/mm.c
>> @@ -3507,18 +3507,18 @@ long do_mmu_update(
>> gmfn = req.ptr >> PAGE_SHIFT;
>> page = get_page_from_gfn(pt_owner, gmfn, &p2mt, P2M_ALLOC);
>>
>> - if ( p2m_is_paged(p2mt) )
>> + if ( unlikely(!page) || p2mt != p2m_ram_rw )
>> {
>> - ASSERT(!page);
>> - p2m_mem_paging_populate(pt_owner, gmfn);
>> - rc = -ENOENT;
>> - break;
>> - }
>> -
>> - if ( unlikely(!page) )
>> - {
>> - gdprintk(XENLOG_WARNING,
>> - "Could not get page for normal update\n");
>> + if ( page )
>> + put_page(page);
>> + if ( p2m_is_paged(p2mt) )
>> + {
>> + p2m_mem_paging_populate(pt_owner, gmfn);
>> + rc = -ENOENT;
>> + }
>> + else
>> + gdprintk(XENLOG_WARNING,
>> + "Could not get page for normal update\n");
>> break;
>> }
>>
>>
>>
>>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/3] x86: don't ignore foreigndom on L2/L3/L4 page table updates
2017-10-12 9:38 [PATCH 0/3] x86: adjustments to page table updates Jan Beulich
` (2 preceding siblings ...)
2017-10-12 10:01 ` [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths Jan Beulich
@ 2017-10-12 12:24 ` Jan Beulich
2017-12-04 10:42 ` Ping: " Jan Beulich
2017-12-04 16:28 ` Andrew Cooper
2017-10-13 9:43 ` [PATCH 0/3] x86: adjustments to " Julien Grall
4 siblings, 2 replies; 15+ messages in thread
From: Jan Beulich @ 2017-10-12 12:24 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper
Silently assuming DOMID_SELF is unlikely to be a good idea for page
table updates. For PGT_writable pages, though, it seems better to allow
the writes, so the same check isn't being applied there.
Also add blank lines between the individual case blocks.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3542,18 +3542,28 @@ long do_mmu_update(
cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
pg_owner);
break;
+
case PGT_l2_page_table:
+ if ( unlikely(pg_owner != pt_owner) )
+ break;
rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
break;
+
case PGT_l3_page_table:
+ if ( unlikely(pg_owner != pt_owner) )
+ break;
rc = mod_l3_entry(va, l3e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
break;
+
case PGT_l4_page_table:
+ if ( unlikely(pg_owner != pt_owner) )
+ break;
rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
break;
+
case PGT_writable_page:
perfc_incr(writable_mmu_updates);
if ( paging_write_guest_entry(v, va, req.val, _mfn(mfn)) )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] x86: adjustments to page table updates
2017-10-12 9:38 [PATCH 0/3] x86: adjustments to page table updates Jan Beulich
` (3 preceding siblings ...)
2017-10-12 12:24 ` [PATCH 4/3] x86: don't ignore foreigndom on L2/L3/L4 page table updates Jan Beulich
@ 2017-10-13 9:43 ` Julien Grall
4 siblings, 0 replies; 15+ messages in thread
From: Julien Grall @ 2017-10-13 9:43 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Andrew Cooper, Julien Grall
Hi Jan,
On 12/10/17 10:38, Jan Beulich wrote:
> The first two patches are bug fixes and hence candidates for 4.10.
> The 3rd is mostly cleanup, and hence intended only for after 4.10.
>
> 1: request page table page-in for the correct domain
> 2: fix do_update_va_mapping_otherdomain() wrt translated domains
Release-acked-by: Julien Grall <julien.grall@linaro.org>
Cheers,
> 3: tighten MMU_*PT_UPDATE* check and combine error paths
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
>
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Ping: [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths
2017-10-12 10:01 ` [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths Jan Beulich
2017-10-12 11:31 ` Andrew Cooper
@ 2017-12-04 10:41 ` Jan Beulich
1 sibling, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2017-12-04 10:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 12.10.17 at 12:01, wrote:
> Don't accept anything other than r/w RAM pages and move the paged-out
> check into the (unlikely) error path following that check.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Ping?
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3507,18 +3507,18 @@ long do_mmu_update(
> gmfn = req.ptr >> PAGE_SHIFT;
> page = get_page_from_gfn(pt_owner, gmfn, &p2mt, P2M_ALLOC);
>
> - if ( p2m_is_paged(p2mt) )
> + if ( unlikely(!page) || p2mt != p2m_ram_rw )
> {
> - ASSERT(!page);
> - p2m_mem_paging_populate(pt_owner, gmfn);
> - rc = -ENOENT;
> - break;
> - }
> -
> - if ( unlikely(!page) )
> - {
> - gdprintk(XENLOG_WARNING,
> - "Could not get page for normal update\n");
> + if ( page )
> + put_page(page);
> + if ( p2m_is_paged(p2mt) )
> + {
> + p2m_mem_paging_populate(pt_owner, gmfn);
> + rc = -ENOENT;
> + }
> + else
> + gdprintk(XENLOG_WARNING,
> + "Could not get page for normal update\n");
> break;
> }
>
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Ping: [PATCH 4/3] x86: don't ignore foreigndom on L2/L3/L4 page table updates
2017-10-12 12:24 ` [PATCH 4/3] x86: don't ignore foreigndom on L2/L3/L4 page table updates Jan Beulich
@ 2017-12-04 10:42 ` Jan Beulich
2017-12-04 16:28 ` Andrew Cooper
1 sibling, 0 replies; 15+ messages in thread
From: Jan Beulich @ 2017-12-04 10:42 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel
>>> On 12.10.17 at 14:24, wrote:
> Silently assuming DOMID_SELF is unlikely to be a good idea for page
> table updates. For PGT_writable pages, though, it seems better to allow
> the writes, so the same check isn't being applied there.
>
> Also add blank lines between the individual case blocks.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Ping?
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -3542,18 +3542,28 @@ long do_mmu_update(
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v,
> pg_owner);
> break;
> +
> case PGT_l2_page_table:
> + if ( unlikely(pg_owner != pt_owner) )
> + break;
> rc = mod_l2_entry(va, l2e_from_intpte(req.val), mfn,
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> break;
> +
> case PGT_l3_page_table:
> + if ( unlikely(pg_owner != pt_owner) )
> + break;
> rc = mod_l3_entry(va, l3e_from_intpte(req.val), mfn,
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> break;
> +
> case PGT_l4_page_table:
> + if ( unlikely(pg_owner != pt_owner) )
> + break;
> rc = mod_l4_entry(va, l4e_from_intpte(req.val), mfn,
> cmd == MMU_PT_UPDATE_PRESERVE_AD, v);
> break;
> +
> case PGT_writable_page:
> perfc_incr(writable_mmu_updates);
> if ( paging_write_guest_entry(v, va, req.val,
> _mfn(mfn)) )
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths
2017-10-12 12:14 ` Jan Beulich
@ 2017-12-04 16:26 ` Andrew Cooper
0 siblings, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2017-12-04 16:26 UTC (permalink / raw)
To: Jan Beulich; +Cc: xen-devel
On 12/10/17 13:14, Jan Beulich wrote:
>>>> On 12.10.17 at 13:31, <andrew.cooper3@citrix.com> wrote:
>> On 12/10/17 11:01, Jan Beulich wrote:
>>> Don't accept anything other than r/w RAM pages and move the paged-out
>>> check into the (unlikely) error path following that check.
>>>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> How does dom0 boot with this change in place? You appear to have
>> prohibited mapping MMIO frames.
> The page in question is a page table one, which can't be MMIO.
> Dom0 is booting fine.
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/3] x86: don't ignore foreigndom on L2/L3/L4 page table updates
2017-10-12 12:24 ` [PATCH 4/3] x86: don't ignore foreigndom on L2/L3/L4 page table updates Jan Beulich
2017-12-04 10:42 ` Ping: " Jan Beulich
@ 2017-12-04 16:28 ` Andrew Cooper
1 sibling, 0 replies; 15+ messages in thread
From: Andrew Cooper @ 2017-12-04 16:28 UTC (permalink / raw)
To: Jan Beulich, xen-devel
On 12/10/17 13:24, Jan Beulich wrote:
> Silently assuming DOMID_SELF is unlikely to be a good idea for page
> table updates. For PGT_writable pages, though, it seems better to allow
> the writes, so the same check isn't being applied there.
>
> Also add blank lines between the individual case blocks.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-12-04 16:28 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-12 9:38 [PATCH 0/3] x86: adjustments to page table updates Jan Beulich
2017-10-12 10:00 ` [PATCH 1/3] x86: request page table page-in for the correct domain Jan Beulich
2017-10-12 10:02 ` Andrew Cooper
2017-10-12 10:00 ` [PATCH 2/3] x86: fix do_update_va_mapping_otherdomain() wrt translated domains Jan Beulich
2017-10-12 11:18 ` Andrew Cooper
2017-10-12 11:27 ` Jan Beulich
2017-10-12 10:01 ` [PATCH 3/3] x86: tighten MMU_*PT_UPDATE* check and combine error paths Jan Beulich
2017-10-12 11:31 ` Andrew Cooper
2017-10-12 12:14 ` Jan Beulich
2017-12-04 16:26 ` Andrew Cooper
2017-12-04 10:41 ` Ping: " Jan Beulich
2017-10-12 12:24 ` [PATCH 4/3] x86: don't ignore foreigndom on L2/L3/L4 page table updates Jan Beulich
2017-12-04 10:42 ` Ping: " Jan Beulich
2017-12-04 16:28 ` Andrew Cooper
2017-10-13 9:43 ` [PATCH 0/3] x86: adjustments to " Julien Grall
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).