Xen devel - May 2007 - RFC: Using grant table to give iomem permission

It used to be possible to use domctl to programatically set up
permissions for other domains to map iomem regions.  However, domctl is
no longer accessible from drivers (only from tools).  After some
discussion with Keir about how to achieve this functionality, it was
suggested that it might be possible to use the grant table operations to
do it.  

Attached is a patch that has a go at this and seems to work.  However,
it is quite primitive and I would welcome the comments of others who
know more about the grant tables than I do!

The patch adds a new type of grant (GNTMAP_iomem_map) to complement
GNTMAP_device_map and GNTMAP_host_map.

The granting domain would do a normal grant operation to specify the
region that can be used as iomem:
        err = gnttab_grant_foreign_access(dev->otherend_id, mfn, 0)
and then pass the grant ref to the mapping domain.

The mapping domain would then do a map op, along the lines of:
  	struct gnttab_map_grant_ref op;
	gnttab_set_map_op(&op, 0, GNTMAP_iomem_map, gnt_ref, dev->otherend_id);
	if (HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref, &op, 1))
		BUG();

When the hypervisor tries to do this grant in xen/common/grant_table.c
it (i) notices that it is an iomem_map rather than device_map or
host_map; (ii) it is from dom0; and (iii) it is not a RAM page.  It then
attempts to call iomem_permit_access().

If successful, on return, the mapping domain should then be able to call
ioremap() to access the page in question.

When finished, the mapping domain can similarly unmap the grant, which
removes its ability to ioremap() the page:
  	struct gnttab_unmap_grant_ref op;
	gnttab_set_unmap_op(&op, 0, GNTMAP_iomem_map, handle);
	if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
		BUG();

Some questions from me:
 - does this approach seem sane?

 - any problem with adding the GNTTAB_iomem_map type?

 - how about the current test for it being a RAM page: !mfn_valid
(frame).  I''ve seen this return "valid" on iomem pages on a
machine with
4GB of RAM, so it may not be a good/sufficient test.

 - the current patch doesn''t actually do the ioremap() as part of the
grant operation, it just sets up the permissions.  I can see that others
might prefer it to do both.  Any thoughts?

 - is there some gross omission from what I''ve done?

Thanks

Kieran


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 18/5/07 15:01, "Kieran Mansley" <kmansley@solarflare.com>
wrote:
> When finished, the mapping domain can similarly unmap the grant, which
> removes its ability to ioremap() the page:
> struct gnttab_unmap_grant_ref op;
> gnttab_set_unmap_op(&op, 0, GNTMAP_iomem_map, handle);
> if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
> BUG();
> 
> Some questions from me:
>  - does this approach seem sane?
There''s no reason you shouldn''t be able to use GNTMAP_host_map
as usual, and
do refcounting in the active grant entry, also as usual.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2007-05-18 at 15:05 +0100, Keir Fraser wrote:
> On 18/5/07 15:01, "Kieran Mansley"
<kmansley@solarflare.com> wrote:
> 
> > When finished, the mapping domain can similarly unmap the grant, which
> > removes its ability to ioremap() the page:
> > struct gnttab_unmap_grant_ref op;
> > gnttab_set_unmap_op(&op, 0, GNTMAP_iomem_map, handle);
> > if (HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref, &op, 1))
> > BUG();
> > 
> > Some questions from me:
> >  - does this approach seem sane?
> 
> There''s no reason you shouldn''t be able to use
GNTMAP_host_map as usual, and
> do refcounting in the active grant entry, also as usual.
OK.  My reluctance to do that was simply that I wasn''t sure if the
operations that take place when doing a GNTMAP_host_map would conflict
with those when doing an iomem_map.  If you think they shouldn''t,
I''ll
give it a go.

That raises one of my other questions which is how to test for it being
a valid RAM page, as in the absence of it GNTMAP_iomem_map that''s
pretty
much the only indicator that it''s iomem we''re dealing with. 
As I said,
mfn_valid() doesn''t seem to be sufficient.

Kieran 


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 18/5/07 15:14, "Kieran Mansley" <kmansley@solarflare.com>
wrote:
>>> Some questions from me:
>>>  - does this approach seem sane?
>> 
>> There''s no reason you shouldn''t be able to use
GNTMAP_host_map as usual, and
>> do refcounting in the active grant entry, also as usual.
> 
> OK.  My reluctance to do that was simply that I wasn''t sure if the
> operations that take place when doing a GNTMAP_host_map would conflict
> with those when doing an iomem_map.  If you think they shouldn''t,
I''ll
> give it a go.
I''m not sure what you mean.
> That raises one of my other questions which is how to test for it being
> a valid RAM page, as in the absence of it GNTMAP_iomem_map that''s
pretty
> much the only indicator that it''s iomem we''re dealing
with.  As I said,
> mfn_valid() doesn''t seem to be sufficient.
Yes, I misled you here. You want the middle bit of get_page_from_l1e(). It
checks for !mfn_valid()||page_get_owner()==dom_io. Then you''d do an
iomem_access_permitted check passing in a pointer to the granting domain.

You should perhaps pull the core of get_page_from_l1e() into a supporting
function which you can then make available for common/grant_table.c to call
into.

 -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2007-05-18 at 15:45 +0100, Keir Fraser wrote:
> On 18/5/07 15:14, "Kieran Mansley"
<kmansley@solarflare.com> wrote:
> 
> >>> Some questions from me:
> >>>  - does this approach seem sane?
> >> 
> >> There''s no reason you shouldn''t be able to use
GNTMAP_host_map as usual, and
> >> do refcounting in the active grant entry, also as usual.
> > 
> > OK.  My reluctance to do that was simply that I wasn''t sure
if the
> > operations that take place when doing a GNTMAP_host_map would conflict
> > with those when doing an iomem_map.  If you think they
shouldn''t, I''ll
> > give it a go.
> 
> I''m not sure what you mean.
That if GNTMAP_host_map is specified, the map/unmap operations do a
number of things that might not be appropriate.  eg.

    if ( op->flags & GNTMAP_host_map )
    {
        rc = create_grant_host_mapping(op->host_addr, frame, op->flags);
        if ( rc != GNTST_okay )
        {
            if ( !(op->flags & GNTMAP_readonly) )
                put_page_type(mfn_to_page(frame));
            put_page(mfn_to_page(frame));
            goto undo_out;
        }

I could prevent it doing these things in the case of it being iomem, but
in that case I felt that it was no longer really doing a host_map
operation, and so to avoid confusion (e.g. others maintaining the code
might not realise that GNTMAP_host_map can mean two different things)
thought it better to give it a separate type.
> > That raises one of my other questions which is how to test for it
being
> > a valid RAM page, as in the absence of it GNTMAP_iomem_map
that''s pretty
> > much the only indicator that it''s iomem we''re
dealing with.  As I said,
> > mfn_valid() doesn''t seem to be sufficient.
> 
> Yes, I misled you here. You want the middle bit of get_page_from_l1e(). It
> checks for !mfn_valid()||page_get_owner()==dom_io. Then you''d do
an
> iomem_access_permitted check passing in a pointer to the granting domain.
> 
> You should perhaps pull the core of get_page_from_l1e() into a supporting
> function which you can then make available for common/grant_table.c to call
> into.
Very helpful, thanks

Kieran


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 18/5/07 15:50, "Kieran Mansley" <kmansley@solarflare.com>
wrote:
> I could prevent it doing these things in the case of it being iomem, but
> in that case I felt that it was no longer really doing a host_map
> operation, and so to avoid confusion (e.g. others maintaining the code
> might not realise that GNTMAP_host_map can mean two different things)
> thought it better to give it a separate type.
At the interface it still makes sense to call it a host mapping, because it
is mapped into host page tables. But yes, you''ll end up doing things a
bit
differently where there''s no struct page_info.

 -- Keir


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> When the hypervisor tries to do this grant in xen/common/grant_table.c
> it (i) notices that it is an iomem_map rather than device_map or
> host_map; (ii) it is from dom0; and (iii) it is not a RAM page.  It then
> attempts to call iomem_permit_access().
Hi Karen, 
Thanks for the patch, I find it useful... but I am wondering why you need to
check it is from a dom0 ... I maybe not getting the reason, but iomem_map should
be available for page sharing among all domains, shouldn''t it ?

Thank you,
-- Lamia Youseff






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2007-05-18 at 12:11 -0700, Lamia M. Youseff
wrote:
> > When the hypervisor tries to do this grant in xen/common/grant_table.c
> > it (i) notices that it is an iomem_map rather than device_map or
> > host_map; (ii) it is from dom0; and (iii) it is not a RAM page.  It
then
> > attempts to call iomem_permit_access().
> 
> Hi Karen, 
> Thanks for the patch, I find it useful... but I am wondering why you
> need to check it is from a dom0 ... I maybe not getting the reason,
> but iomem_map should be available for page sharing among all domains,
> shouldn''t it ?
There''s no absolute requirement for that check to be there, but it is
something that came out of discussion with Keir.  It seems sensible to
me to only allow dom0 to programatically give access to iomem, as
typically this is used to access physical hardware.  I think most guest
domains wouldn''t have any iomem regions available to them that they
would want to share.  If they did, access to this could be currently
configured using the Xen tools (I think - not actually tried this).

Kieran


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel