| Message ID | 59DF596E020000780018555D@prv-mh.provo.novell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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)) ) > >
>>> 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
--- 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)) )
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).