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

[Alex Williamson <alex.williamson@xxxxxx>]

On Fri, 2006-05-19 at 17:31 +0900, Isaku Yamahata wrote:
>      page = mfn_to_page(mfn);
>      ret = get_page(page, page_get_owner(page));
>      BUG_ON(ret == 0);
> -    assign_domain_page_replace(d, gpaddr, mfn, flags);
> -
> +
> +    arflags = _PAGE_AR_RWX;
> +    if (flags & GNTMAP_readonly) {
> +        arflags = _PAGE_AR_R;
> +    }
> +    assign_domain_page_replace(d, gpaddr, mfn, arflags);
>      return GNTST_okay;
>  }

   I think we're still parsing flags too high up in the stack.  Couldn't
we pass flags "as is" to assign_domain_page_replace(), then do

arflags = (flags & GNTMAP_readonly) ? _PAGE_AR_R : _PAGE_AR_RWX;

in that end function?  It also seems more logical to call
__assign_domain_page() as:

__assign_domain_page(d, j, io_ranges[i].type, GNTMAP_readonly /* or 0 for RWX 
*/)

than to pass in explicit page flags.  Doing these, we could get rid of
the BUG_ON checks.  For example:

__assign_domain_page(struct domain *d,
                     unsigned long mpaddr, unsigned long physaddr,
                     unsigned long flags)
{
    pte_t *pte;
    unsigned long arflags = (flags & GNTMAP_readonly) ? _PAGE_AR_R :
                                                        _PAGE_AR_RWX;

Thoughts?  I think there are only a couple minor places in patches 2 and
3 that would need to be updated to reflect this.  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