>>> On 1/7/2008 at 8:03 AM, in message <47824D56.76E4.0078.0@xxxxxxxxxx>, "Jan
Beulich" <jbeulich@xxxxxxxxxx> wrote:
> On the Xen patch:
>
> Both added case blocks in do_mmu_update() have a nested switch
> statement that seems redundant (i.e. the outer switch already only
> handle PGT_l1_page_table pages, but the inner switch check this same
> value again.
Right - a silly oversight - will fix.
>
> The same two case block access addresses obtained from
> map_domain_page_with_cache() through __copy_from_user(). Is there
> any particular reason to not use direct accesses here? (Note that
> mod_l1_entry() has to use __copy_from_user() as it may be called from
> do_update_va_mapping()). Likewise I would think that there's no strict
> need for update_intpte_sync() to use paging_cmpxchg_guest_entry(),
> but here I would agree that it's easier to re-use the existing function
> than to create and use a new one.
I'll change to use direct access.
>
> An additional piece of concern regarding the bit assignments of
> MMU_FLAG_RANGE_UPDATE's val parameter (Keir, maybe you need to
> comment on this one): The whole mmu_update interface, being
> defined in public/xen.h, is supposed to be sufficiently architecture
> neutral, which it won't be anymore in the way it currently is being
> modified. But maybe I'm mistaken and the interface's declaration is
> just badly placed (would apply to the mmuext interface then, too)?
I should point out that the MMU_FLAG_RANGE_UPDATE hypercall didn't end up being
needed for improving the SAP test, but does get invoked by other uses of
mprotect() and feel that it is useful. As it stands I've only implemented x86
version of this, other architecture owners would need to do the same.
Perhaps as far as what is presented in public/xen.h, we should just have a more
generic description (not pointing out the format) and leave it up to each
architecture as to what specific format for the val member makes sense.
>
> In the Linux patch, I'd just like to see the abstraction to be less Xen-
> specific, i.e. something like
>
> (perhaps in include/asm-generic/pgtable.h)
>
> #ifndef arch_change_pte_range
> # define arch_change_pte_range(mm, pmd, addr, end, newprot) 0
> #endif
>
> and then in change_pmd_range():
>
> ...
> if (pmd_none_or_clear_bad(pmd))
> continue;
> if (arch_change_pte_range(mm, pmd, addr, next, newprot))
> continue;
> change_pte_range(mm, pmd, addr, next, newprot);
> } while (pmd++, addr = next, addr != end);
> ...
>
> The BUG() (which really can be BUG_ON() here) would go into the actual
> function then.
Yes, that approach does look better. I'll respin the patch.
>
> Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxxxxxxxx
> http://lists.xensource.com/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home