On Tue, May 23, 2006 at 01:08:48PM -0600, Alex Williamson wrote:
> 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,
Hmm. __assign_domain_page_replace() isn't specific for grant table.
and it is used by __assign_domain_page() and dom0vp add physmap hypercall.
So I don't think GNTMAP_readonly is good flag name for read only.
How about ASSIGN_readonly? Or can you suggest better name?
--
yamahata
_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ia64-devel
|
Home