* [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
* 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
* [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
* 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
* [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 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
* 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
* 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
* [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
* 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 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
* 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
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).