Hi Isaku,
A few comments/questions on this patch came up as I tried to merge it
in...
On Wed, 2006-05-17 at 14:09 +0900, Isaku Yamahata wrote:
> +static void
> +assign_pte(pte_t* pte, unsigned long maddr, unsigned long readonly)
> +{
> + unsigned long arflags = _PAGE_AR_RWX;
> + if (readonly)
> + arflags = _PAGE_AR_R;
> + set_pte(pte, pfn_pte(maddr >> PAGE_SHIFT,
> + __pgprot(__DIRTY_BITS | _PAGE_PL_2 |
> arflags)));
> +}
> +
This function seems unnecessary. In the end, it's really only doing
(readonly ? _PAGE_AR_R : _PAGE_AR_RWX) which could be done in the caller
or maybe simplified in the caller with a macro.
> @@ -804,16 +813,15 @@ assign_new_domain0_page(struct domain *d
> /* map a physical address to the specified metaphysical addr */
> void
> __assign_domain_page(struct domain *d,
> - unsigned long mpaddr, unsigned long physaddr)
> + unsigned long mpaddr, unsigned long physaddr,
> + unsigned long readonly)
unsigned long for a boolean value? See comment on flags vs readonly
below.
> @@ -1083,7 +1090,7 @@ out:
> // caller must call set_gpfn_from_mfn().
> static void
> assign_domain_page_replace(struct domain *d, unsigned long mpaddr,
> - unsigned long mfn, unsigned int flags)
> + unsigned long mfn, unsigned int readonly)
I'm not sure I see the benefit of replacing a generic "flags" parameter
with a very specific "readonly" for all of these. Wouldn't it be more
consistent to continue using GNTMAP_readonly? There's actually one
place in this patch that still calls this function with (flags &
GNTMAP_readonly).
> {
> struct mm_struct *mm = d->arch.mm;
> pte_t* pte;
> @@ -1093,8 +1100,7 @@ assign_domain_page_replace(struct domain
>
> // update pte
> old_pte = ptep_get_and_clear(mm, mpaddr, pte);
> - set_pte(pte, pfn_pte(mfn,
> - __pgprot(__DIRTY_BITS | _PAGE_PL_2 |
> _PAGE_AR_RWX)));
> + assign_pte(pte, mfn << PAGE_SHIFT, readonly);
This chunk conflicts with Tristan's pte_xchg patch. With the current
code, I think the cleanest way to add the readonly attribute is to
simply do a (readonly ? _PAGE_AR_R : _PAGE_AR_RWX) in the __pgprot,
which again makes the extra step of having an assign_pte function seem
unnecessary.
Thanks,
Alex
--
Alex Williamson HP Linux & Open Source Lab
_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel
|
Home