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 <jbeul...@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.coop...@citrix.com>
> 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
