[Xen-devel] RE: [PATCH 5/6] Handler m2p table and frametable fault in page faulthandler

["Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx>]

Jan Beulich wrote:
>>>> "Jiang, Yunhong" <yunhong.jiang@xxxxxxxxx> 28.06.09 11:27 >>>
>> ...
>> +int handle_memadd_fault(unsigned long addr, struct cpu_user_regs
>> *regs) +{ +    l3_pgentry_t  *pl3e;
>> +    l3_pgentry_t l3e;
>> +    l2_pgentry_t *pl2e;
>> +    l2_pgentry_t l2e, idle_l2e;
>> +    unsigned long mfn, cr3;
>> +
>> +    idle_l2e = idle_pg_table_l2[l2_linear_offset(addr)];
>> +    if (!(l2e_get_flags(idle_l2e) & _PAGE_PRESENT)) +        return
>> 0; +
>> +    cr3 = read_cr3();
>> +    mfn = cr3 >> PAGE_SHIFT;
>> +
>> +    /*
>> +     * No need get page type here and validate checking for xen
>> mapping +     */ +    pl3e = map_domain_page(mfn);
>> +    pl3e += (cr3 & 0xFE0UL) >> 3;
>> +    l3e = pl3e[3];
>> +
>> +    if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) )
>> +        return 0;
>> +
>> +    mfn = l3e_get_pfn(l3e);
>> +    pl2e = map_domain_page(mfn);
>> +
>> +    l2e = pl2e[l2_table_offset(addr)];
>> +
>> +    if (l2e_get_flags(l2e) & _PAGE_PRESENT)
>> +        return 0;
> 
> You'd also need to check that idle_page_table_l2's entry has
> _PAGE_PRESENT set here, otherwise you'd risk live locking the domain
> on an out-of-current- bounds M2P access.

Seems I forgot check it in 32bit, (it is checked in 64 bit version).  will add 
that back.

> 
> Furthermore, I'm unsure forwarding the page fault in case you found
> the domain's L2 entry to already have _PAGE_PRESENT set is a good way
> to handle things: A racing access on another vCPU of the same
> guest may just
> have managed to install that entry.

Good catch. 
Considering these mapping should only be set by Xen HV, if the PAGE_PRESENT is 
set, we can assume it has been handled by other vCPU and return successfully, 
since only non_present fault will come here. (an ASSERT to check the content of 
the guest l2e is same as idel_page_table will be enough).

> Bottom line is, you probably need to exclusively check
> idle_page_table_l2 here.
> 
>> +
>> +    memcpy(&pl2e[l2_table_offset(addr)],
>> +            &idle_pg_table_l2[l2_linear_offset(addr)],
>> +            sizeof(l2_pgentry_t));
> 
> Using memcpy for a single page table entry seems odd - why not
> use a direct
> assignment? However, perhaps using memcpy() here is the right
> thing - to
> avoid future faults, you could update the full M2P related
> part of the L2
> directory in a single step. This ought to be safe, since the
> M2P table can only
> be extended, but never shrunk.
> 
>> +
>> +    return EXCRET_fault_fixed;
>> +}
> 
> You're leaking map_domain_page()-es here (and at the earlier
> return points).

:$ , Will add that back.

> 
> (All the comments likewise apply to the subsequent 64-bit variant.)
> 
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel