Re: [Xen-ia64-devel] [PATCH 1/3] [read-only mapping] GNTMAP_readonly support xen part

[Alex Williamson <alex.williamson@xxxxxx>]

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