Kieran Mansley
2007-Jul-09 15:23 UTC
[Xen-devel] [PATCH] Allow programatic iomem permissions
This patch addresses the problem of how to give iomem access to guests
from dom0. At the moment, these permissions can only be set up using
the xen tools, which pretty much rules out doing so programmatically
from a backend driver for example.
After some discussion with Keir it was decided that the best approach
was to use the grant table to provide these permissions. This patch
adds this facility.
In use:
- backend in dom0 grants (using the normal grant table ops) access to
the region of iomemory and passes a grant handle to the guest.
- frontend in guest tries to map this grant, and the hypervisor spots
that it''s for a region of iomemory rather than RAM.
- hypervisor sets up the relevant iomem access permissions
- when finished with, frontend in guest unmaps the grant, and
hypervisor similarly removes the iomem access permission.
I''ve posted this before, and have been using it myself for some time,
so
would welcome comments, code review and/or applying to xen-unstable
While this patch solves the problem above, there is a chicken-and-egg
situation that results. Currently, grant table operations are only
permitted if you have some iomem permissions. This is because of the
following in iocap.h:
/*
* Until TLB flushing issues are sorted out we consider it unsafe for
* domains with no hardware-access privileges to perform grant
map/transfer
* operations.
*/
#define grant_operation_permitted(d) \
(!rangeset_is_empty((d)->iomem_caps))
This means that you can''t do the grant table operation to provide the
iomemory capability because you don''t yet have the iomemory capability.
There are a couple of obvious solutions to this:
- make grant_operation_permitted more sophisticated so it can realise
that iomem grant operations should be allowed.
- replace grant_operation_permitted with something like the Xen
Security Policy stuff that is being proposed.
- solve the TLB issues alluded to in the above comment so we don''t
have
to have this restriction.
As the definition of grant_operation_permitted sounds like a bit of a
temporary hack I''ve done nothing to address this yet, but I would
favour
the Xen Security Policy approach as being the most useful. I''d welcome
suggestions on this.
Thanks
Kieran
Allow iomem permissions to be set up through grant table ops
Signed-off-by Kieran Mansley <kmansley@solarflare.com>
diff -r 76781ee87984 xen/arch/x86/mm.c
--- a/xen/arch/x86/mm.c Mon Jul 09 12:49:31 2007 +0100
+++ b/xen/arch/x86/mm.c Mon Jul 09 16:05:43 2007 +0100
@@ -594,6 +594,14 @@ get_##level##_linear_pagetable(
return 1; \
}
+
+int iomem_page_test(unsigned long mfn, struct page_info *page)
+{
+ return unlikely(!mfn_valid(mfn)) ||
+ unlikely(page_get_owner(page) == dom_io);
+}
+
+
int
get_page_from_l1e(
l1_pgentry_t l1e, struct domain *d)
@@ -611,8 +619,7 @@ get_page_from_l1e(
return 0;
}
- if ( unlikely(!mfn_valid(mfn)) ||
- unlikely(page_get_owner(page) == dom_io) )
+ if ( iomem_page_test(mfn, page) )
{
/* DOMID_IO reverts to caller for privilege checks. */
if ( d == dom_io )
diff -r 76781ee87984 xen/common/grant_table.c
--- a/xen/common/grant_table.c Mon Jul 09 12:49:31 2007 +0100
+++ b/xen/common/grant_table.c Mon Jul 09 16:06:36 2007 +0100
@@ -187,6 +187,7 @@ __gnttab_map_grant_ref(
int handle;
unsigned long frame = 0;
int rc = GNTST_okay;
+ int is_iomem = 0;
struct active_grant_entry *act;
struct grant_mapping *mt;
grant_entry_t *sha;
@@ -315,34 +316,52 @@ __gnttab_map_grant_ref(
spin_unlock(&rd->grant_table->lock);
- if ( unlikely(!mfn_valid(frame)) ||
- unlikely(!((op->flags & GNTMAP_readonly) ?
- get_page(mfn_to_page(frame), rd) :
- get_page_and_type(mfn_to_page(frame), rd,
- PGT_writable_page))) )
- {
- if ( !rd->is_dying )
- gdprintk(XENLOG_WARNING, "Could not pin grant frame
%lx\n", frame);
- rc = GNTST_general_error;
- goto undo_out;
- }
-
- 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));
+ if ( op->flags & GNTMAP_host_map )
+ {
+ /* Could be an iomem page for setting up permission */
+ if( iomem_page_test(frame, mfn_to_page(frame)) ) {
+ is_iomem = 1;
+ if ( iomem_permit_access(ld, frame, frame) ) {
+ gdprintk(XENLOG_WARNING,
+ "Could not permit access to grant frame %lx as
iomem\n",
+ frame);
+ rc = GNTST_general_error;
+ goto undo_out;
+ }
+ }
+ }
+
+ if (!is_iomem )
+ {
+ if ( unlikely(!mfn_valid(frame)) ||
+ unlikely(!((op->flags & GNTMAP_readonly) ?
+ get_page(mfn_to_page(frame), rd) :
+ get_page_and_type(mfn_to_page(frame), rd,
+ PGT_writable_page))))
+ {
+ if ( !rd->is_dying )
+ gdprintk(XENLOG_WARNING, "Could not pin grant frame
%lx\n", frame);
+ rc = GNTST_general_error;
goto undo_out;
}
-
- if ( op->flags & GNTMAP_device_map )
- {
- (void)get_page(mfn_to_page(frame), rd);
- if ( !(op->flags & GNTMAP_readonly) )
- get_page_type(mfn_to_page(frame), PGT_writable_page);
+
+ 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;
+ }
+
+ if ( op->flags & GNTMAP_device_map )
+ {
+ (void)get_page(mfn_to_page(frame), rd);
+ if ( !(op->flags & GNTMAP_readonly) )
+ get_page_type(mfn_to_page(frame), PGT_writable_page);
+ }
}
}
@@ -485,23 +504,39 @@ __gnttab_unmap_common(
}
}
- if ( (op->host_addr != 0) && (flags & GNTMAP_host_map) )
- {
- if ( (rc = replace_grant_host_mapping(op->host_addr,
- frame, op->new_addr, flags))
< 0 )
- goto unmap_out;
-
- ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
- map->flags &= ~GNTMAP_host_map;
- if ( flags & GNTMAP_readonly )
- {
- act->pin -= GNTPIN_hstr_inc;
- put_page(mfn_to_page(frame));
- }
- else
- {
- act->pin -= GNTPIN_hstw_inc;
- put_page_and_type(mfn_to_page(frame));
+ if ( flags & GNTMAP_host_map )
+ {
+ if ( op->host_addr != 0 )
+ {
+ if ( (rc = replace_grant_host_mapping(op->host_addr,
+ frame, op->new_addr,
+ flags)) < 0 )
+ goto unmap_out;
+
+ ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
+ map->flags &= ~GNTMAP_host_map;
+ if ( flags & GNTMAP_readonly )
+ {
+ act->pin -= GNTPIN_hstr_inc;
+ put_page(mfn_to_page(frame));
+ }
+ else
+ {
+ act->pin -= GNTPIN_hstw_inc;
+ put_page_and_type(mfn_to_page(frame));
+ }
+ } else if ( iomem_page_test(frame, mfn_to_page(frame)) &&
+ iomem_access_permitted(ld, frame, frame) ){
+
+ if ( (rc = iomem_deny_access(ld, frame, frame)) < 0 )
+ goto unmap_out;
+
+ ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
+ map->flags &= ~GNTMAP_host_map;
+ if ( flags & GNTMAP_readonly )
+ act->pin -= GNTPIN_hstr_inc;
+ else
+ act->pin -= GNTPIN_hstw_inc;
}
}
@@ -1429,6 +1464,7 @@ gnttab_release_mappings(
struct domain *rd;
struct active_grant_entry *act;
struct grant_entry *sha;
+ int rc;
BUG_ON(!d->is_dying);
@@ -1486,7 +1522,12 @@ gnttab_release_mappings(
{
BUG_ON(!(act->pin & GNTPIN_hstw_mask));
act->pin -= GNTPIN_hstw_inc;
- gnttab_release_put_page_and_type(mfn_to_page(act->frame));
+
+ if ( iomem_page_test(act->frame, mfn_to_page(act->frame))
&&
+ iomem_access_permitted(rd, act->frame, act->frame) )
+ rc = iomem_deny_access(rd, act->frame, act->frame);
+ else
+
gnttab_release_put_page_and_type(mfn_to_page(act->frame));
}
if ( (act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0 )
diff -r 76781ee87984 xen/include/asm-x86/mm.h
--- a/xen/include/asm-x86/mm.h Mon Jul 09 12:49:31 2007 +0100
+++ b/xen/include/asm-x86/mm.h Mon Jul 09 16:05:43 2007 +0100
@@ -197,6 +197,9 @@ static inline int get_page(struct page_i
return 1;
}
+/* Decide whether this page looks like iomem or real memory */
+int iomem_page_test(unsigned long mfn, struct page_info *page);
+
void put_page_type(struct page_info *page);
int get_page_type(struct page_info *page, unsigned long type);
int get_page_from_l1e(l1_pgentry_t l1e, struct domain *d);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Kieran Mansley
2007-Jul-13 11:05 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
[CC''ing xense-devel as this thread discussed some of the stuff I''m querying below: http://lists.xensource.com/archives/html/xen- devel/2007-05/msg00527.html] On Mon, 2007-07-09 at 16:23 +0100, Kieran Mansley wrote:> This patch addresses the problem of how to give iomem access to guests > from dom0. At the moment, these permissions can only be set up using > the xen tools, which pretty much rules out doing so programmatically > from a backend driver for example.[snip]> While this patch solves the problem above, there is a chicken-and-egg > situation that results. Currently, grant table operations are only > permitted if you have some iomem permissions. This is because of the > following in iocap.h: > > /* > * Until TLB flushing issues are sorted out we consider it unsafe for > * domains with no hardware-access privileges to perform grant > map/transfer > * operations. > */ > #define grant_operation_permitted(d) \ > (!rangeset_is_empty((d)->iomem_caps)) > > This means that you can''t do the grant table operation to provide the > iomemory capability because you don''t yet have the iomemory capability. > There are a couple of obvious solutions to this: > - make grant_operation_permitted more sophisticated so it can realise > that iomem grant operations should be allowed. > - replace grant_operation_permitted with something like the Xen > Security Policy stuff that is being proposed. > - solve the TLB issues alluded to in the above comment so we don''t have > to have this restriction.Any thoughts on the above? I''m happy to do some work to rectify this situation but don''t want to expend effort on something that won''t get accepted. Any background on the reasons for the restriction (i.e. what the TLB flushing issues are) would also be of help. Kieran _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-Jul-13 12:29 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On 13/7/07 12:05, "Kieran Mansley" <kmansley@solarflare.com> wrote:> Any thoughts on the above? > > I''m happy to do some work to rectify this situation but don''t want to > expend effort on something that won''t get accepted. Any background on > the reasons for the restriction (i.e. what the TLB flushing issues are) > would also be of help.The issue is that the granter is informed that the grant is released before stale grantee TLB entries are flushed. If the grantee is multi-vcpu then he could theoretically still access a granted page via a stale TLB entry after the granter has recycled the page. The window is extremely tiny though! The correct fix is to reorder the unmap operation to be unmap-list-of-grants then TLB-flush then update-grant-entries-to-indicate-release. Then the whole problem disappears. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kieran Mansley
2007-Jul-13 13:03 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On Fri, 2007-07-13 at 13:29 +0100, Keir Fraser wrote:> > > On 13/7/07 12:05, "Kieran Mansley" <kmansley@solarflare.com> wrote: > > > Any thoughts on the above? > > > > I''m happy to do some work to rectify this situation but don''t want to > > expend effort on something that won''t get accepted. Any background on > > the reasons for the restriction (i.e. what the TLB flushing issues are) > > would also be of help. > > The issue is that the granter is informed that the grant is released before > stale grantee TLB entries are flushed. If the grantee is multi-vcpu then he > could theoretically still access a granted page via a stale TLB entry after > the granter has recycled the page. The window is extremely tiny though! The > correct fix is to reorder the unmap operation to be unmap-list-of-grants > then TLB-flush then update-grant-entries-to-indicate-release. Then the whole > problem disappears.OK, that makes sense, and doesn''t at first impression look too hard to rectify. Am I right in thinking that it''s the shared grant table entry that is the critical one in this sense (as opposed to the "active" entry). Kieran _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-Jul-13 14:12 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On 13/7/07 14:03, "Kieran Mansley" <kmansley@solarflare.com> wrote:>> The issue is that the granter is informed that the grant is released before >> stale grantee TLB entries are flushed. If the grantee is multi-vcpu then he >> could theoretically still access a granted page via a stale TLB entry after >> the granter has recycled the page. The window is extremely tiny though! The >> correct fix is to reorder the unmap operation to be unmap-list-of-grants >> then TLB-flush then update-grant-entries-to-indicate-release. Then the whole >> problem disappears. > > OK, that makes sense, and doesn''t at first impression look too hard to > rectify. > > Am I right in thinking that it''s the shared grant table entry that is > the critical one in this sense (as opposed to the "active" entry).That''s correct. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Derek Murray
2007-Jul-13 14:13 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
Hi Kieran, On 13 Jul 2007, at 12:05, Kieran Mansley wrote: ...> Any thoughts on the above? > > I''m happy to do some work to rectify this situation but don''t want to > expend effort on something that won''t get accepted. Any background on > the reasons for the restriction (i.e. what the TLB flushing issues > are) > would also be of help.If it''s as straightforward as Keir has suggested elsewhere in this thread, then it would seem possible to get rid of this check altogether. The xsm_map_grantref hook would suffice to implement security policy, and could be used to prohibit certain domains from mapping any grants at all. Regards, Derek Murray. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kieran Mansley
2007-Jul-13 15:41 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On Fri, 2007-07-13 at 15:12 +0100, Keir Fraser wrote:> On 13/7/07 14:03, "Kieran Mansley" <kmansley@solarflare.com> wrote: > > >> The issue is that the granter is informed that the grant is released before > >> stale grantee TLB entries are flushed. If the grantee is multi-vcpu then he > >> could theoretically still access a granted page via a stale TLB entry after > >> the granter has recycled the page. The window is extremely tiny though! The > >> correct fix is to reorder the unmap operation to be unmap-list-of-grants > >> then TLB-flush then update-grant-entries-to-indicate-release. Then the whole > >> problem disappears. > > > > OK, that makes sense, and doesn''t at first impression look too hard to > > rectify. > > > > Am I right in thinking that it''s the shared grant table entry that is > > the critical one in this sense (as opposed to the "active" entry). > > That''s correct.OK, patch attached. It compiles and seems to work, but hasn''t been heavily tested yet. I''m sure it could be more efficient, but wanted to check I was heading in the right direction. Kieran Fix TLB flush on grant unmap diff -r 87cc3035108f xen/common/grant_table.c --- a/xen/common/grant_table.c Fri Jul 13 14:32:11 2007 +0100 +++ b/xen/common/grant_table.c Fri Jul 13 16:35:27 2007 +0100 @@ -505,16 +505,49 @@ __gnttab_unmap_common( } } - if ( (map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) - { - map->flags = 0; - put_maptrack_handle(ld->grant_table, op->handle); - } - /* If just unmapped a writable mapping, mark as dirtied */ if ( !(flags & GNTMAP_readonly) ) gnttab_mark_dirty(rd, frame); + unmap_out: + op->status = rc; + spin_unlock(&rd->grant_table->lock); + rcu_unlock_domain(rd); +} + +static void +__gnttab_unmap_common_complete(struct gnttab_unmap_common *op) +{ + struct domain *ld, *rd; + domid_t dom; + struct active_grant_entry *act; + grant_entry_t *sha; + struct grant_mapping *map; + u16 flags; + + ld = current->domain; + map = &maptrack_entry(ld->grant_table, op->handle); + dom = map->domid; + flags = map->flags; + + if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) ) { + /* This shouldn''t happen - __gnttab_unmap_common should have + already crashed the domain */ + gdprintk(XENLOG_INFO, "Could not find domain %d\n", dom); + return; + } + + spin_lock(&rd->grant_table->lock); + + act = &active_entry(rd->grant_table, map->ref); + sha = &shared_entry(rd->grant_table, map->ref); + + if ( (map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) + { + map->flags = 0; + put_maptrack_handle(ld->grant_table, op->handle); + } + if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) && !(flags & GNTMAP_readonly) ) gnttab_clear_flag(_GTF_writing, &sha->flags); @@ -522,8 +555,6 @@ __gnttab_unmap_common( if ( act->pin == 0 ) gnttab_clear_flag(_GTF_reading, &sha->flags); - unmap_out: - op->status = rc; spin_unlock(&rd->grant_table->lock); rcu_unlock_domain(rd); } @@ -542,28 +573,51 @@ __gnttab_unmap_grant_ref( op->status = common.status; } +static void +__gnttab_unmap_grant_ref_complete(struct gnttab_unmap_grant_ref *op) +{ + struct gnttab_unmap_common common = { + .host_addr = op->host_addr, + .dev_bus_addr = op->dev_bus_addr, + .handle = op->handle, + }; + + __gnttab_unmap_common_complete(&common); +} + static long gnttab_unmap_grant_ref( XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t) uop, unsigned int count) { - int i; + int i, done = 0; + long rc = -EFAULT; struct gnttab_unmap_grant_ref op; for ( i = 0; i < count; i++ ) { if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) - goto fault; + goto out; __gnttab_unmap_grant_ref(&op); + ++done; if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) ) - goto fault; - } - + goto out; + } + + rc = 0; + +out: flush_tlb_mask(current->domain->domain_dirty_cpumask); - return 0; - -fault: - flush_tlb_mask(current->domain->domain_dirty_cpumask); - return -EFAULT; + + for ( i = 0; i < done; i++ ) + { + if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) + /* This really shouldn''t happen as it worked earlier in + the function */ + continue; + __gnttab_unmap_grant_ref_complete(&op); + + } + return rc; } static void @@ -580,28 +634,51 @@ __gnttab_unmap_and_replace( op->status = common.status; } +static void +__gnttab_unmap_and_replace( + struct gnttab_unmap_and_replace *op) +{ + struct gnttab_unmap_common common = { + .host_addr = op->host_addr, + .new_addr = op->new_addr, + .handle = op->handle, + }; + + __gnttab_unmap_common_complete(&common); +} + static long gnttab_unmap_and_replace( XEN_GUEST_HANDLE(gnttab_unmap_and_replace_t) uop, unsigned int count) { - int i; + int i, done = 0; + long rc = -EFAULT; struct gnttab_unmap_and_replace op; for ( i = 0; i < count; i++ ) { if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) - goto fault; + goto out; __gnttab_unmap_and_replace(&op); + ++done; if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) ) - goto fault; - } - + goto out; + } + + rc = 0; + +out: flush_tlb_mask(current->domain->domain_dirty_cpumask); - return 0; - -fault: - flush_tlb_mask(current->domain->domain_dirty_cpumask); - return -EFAULT; + + for ( i = 0; i < done; i++ ) + { + if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) + /* This really shouldn''t happen as it worked earlier in + the function */ + continue; + __gnttab_unmap_and_replace_complete(&op); + } + return rc; } int _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-Jul-13 15:53 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On 13/7/07 16:41, "Kieran Mansley" <kmansley@solarflare.com> wrote:> OK, patch attached. It compiles and seems to work, but hasn''t been > heavily tested yet. I''m sure it could be more efficient, but wanted to > check I was heading in the right direction.No, it doesn''t compile. This isn''t baked enough to really comment on. Looks like kind of the right thing though. Bear in mind that after a certain point (in particular after the info is lost of from the grantee''s maptrack handle) then the unmap operation *must* complete in full. Otherwise the innocent granter can be left with leaked grant entries for all time! -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kieran Mansley
2007-Jul-13 15:54 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On Fri, 2007-07-13 at 16:41 +0100, Kieran Mansley wrote:> On Fri, 2007-07-13 at 15:12 +0100, Keir Fraser wrote: > > On 13/7/07 14:03, "Kieran Mansley" <kmansley@solarflare.com> wrote: > > > > >> The issue is that the granter is informed that the grant is released before > > >> stale grantee TLB entries are flushed. If the grantee is multi-vcpu then he > > >> could theoretically still access a granted page via a stale TLB entry after > > >> the granter has recycled the page. The window is extremely tiny though! The > > >> correct fix is to reorder the unmap operation to be unmap-list-of-grants > > >> then TLB-flush then update-grant-entries-to-indicate-release. Then the whole > > >> problem disappears. > > > > > > OK, that makes sense, and doesn''t at first impression look too hard to > > > rectify. > > > > > > Am I right in thinking that it''s the shared grant table entry that is > > > the critical one in this sense (as opposed to the "active" entry). > > > > That''s correct. > > OK, patch attached. It compilesOops, sorry, forgot to refresh the patch before sending. This one should compile better! Fix TLB flush on grant unmap diff -r 87cc3035108f xen/common/grant_table.c --- a/xen/common/grant_table.c Fri Jul 13 14:32:11 2007 +0100 +++ b/xen/common/grant_table.c Fri Jul 13 16:36:19 2007 +0100 @@ -505,16 +505,49 @@ __gnttab_unmap_common( } } - if ( (map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) - { - map->flags = 0; - put_maptrack_handle(ld->grant_table, op->handle); - } - /* If just unmapped a writable mapping, mark as dirtied */ if ( !(flags & GNTMAP_readonly) ) gnttab_mark_dirty(rd, frame); + unmap_out: + op->status = rc; + spin_unlock(&rd->grant_table->lock); + rcu_unlock_domain(rd); +} + +static void +__gnttab_unmap_common_complete(struct gnttab_unmap_common *op) +{ + struct domain *ld, *rd; + domid_t dom; + struct active_grant_entry *act; + grant_entry_t *sha; + struct grant_mapping *map; + u16 flags; + + ld = current->domain; + map = &maptrack_entry(ld->grant_table, op->handle); + dom = map->domid; + flags = map->flags; + + if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) ) { + /* This shouldn''t happen - __gnttab_unmap_common should have + already crashed the domain */ + gdprintk(XENLOG_INFO, "Could not find domain %d\n", dom); + return; + } + + spin_lock(&rd->grant_table->lock); + + act = &active_entry(rd->grant_table, map->ref); + sha = &shared_entry(rd->grant_table, map->ref); + + if ( (map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) + { + map->flags = 0; + put_maptrack_handle(ld->grant_table, op->handle); + } + if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) && !(flags & GNTMAP_readonly) ) gnttab_clear_flag(_GTF_writing, &sha->flags); @@ -522,8 +555,6 @@ __gnttab_unmap_common( if ( act->pin == 0 ) gnttab_clear_flag(_GTF_reading, &sha->flags); - unmap_out: - op->status = rc; spin_unlock(&rd->grant_table->lock); rcu_unlock_domain(rd); } @@ -542,28 +573,51 @@ __gnttab_unmap_grant_ref( op->status = common.status; } +static void +__gnttab_unmap_grant_ref_complete(struct gnttab_unmap_grant_ref *op) +{ + struct gnttab_unmap_common common = { + .host_addr = op->host_addr, + .dev_bus_addr = op->dev_bus_addr, + .handle = op->handle, + }; + + __gnttab_unmap_common_complete(&common); +} + static long gnttab_unmap_grant_ref( XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t) uop, unsigned int count) { - int i; + int i, done = 0; + long rc = -EFAULT; struct gnttab_unmap_grant_ref op; for ( i = 0; i < count; i++ ) { if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) - goto fault; + goto out; __gnttab_unmap_grant_ref(&op); + ++done; if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) ) - goto fault; - } - + goto out; + } + + rc = 0; + +out: flush_tlb_mask(current->domain->domain_dirty_cpumask); - return 0; - -fault: - flush_tlb_mask(current->domain->domain_dirty_cpumask); - return -EFAULT; + + for ( i = 0; i < done; i++ ) + { + if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) + /* This really shouldn''t happen as it worked earlier in + the function */ + continue; + __gnttab_unmap_grant_ref_complete(&op); + + } + return rc; } static void @@ -580,28 +634,51 @@ __gnttab_unmap_and_replace( op->status = common.status; } +static void +__gnttab_unmap_and_replace_complete( + struct gnttab_unmap_and_replace *op) +{ + struct gnttab_unmap_common common = { + .host_addr = op->host_addr, + .new_addr = op->new_addr, + .handle = op->handle, + }; + + __gnttab_unmap_common_complete(&common); +} + static long gnttab_unmap_and_replace( XEN_GUEST_HANDLE(gnttab_unmap_and_replace_t) uop, unsigned int count) { - int i; + int i, done = 0; + long rc = -EFAULT; struct gnttab_unmap_and_replace op; for ( i = 0; i < count; i++ ) { if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) - goto fault; + goto out; __gnttab_unmap_and_replace(&op); + ++done; if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) ) - goto fault; - } - + goto out; + } + + rc = 0; + +out: flush_tlb_mask(current->domain->domain_dirty_cpumask); - return 0; - -fault: - flush_tlb_mask(current->domain->domain_dirty_cpumask); - return -EFAULT; + + for ( i = 0; i < done; i++ ) + { + if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) + /* This really shouldn''t happen as it worked earlier in + the function */ + continue; + __gnttab_unmap_and_replace_complete(&op); + } + return rc; } int _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-Jul-13 16:20 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On 13/7/07 16:54, "Kieran Mansley" <kmansley@solarflare.com> wrote:> Oops, sorry, forgot to refresh the patch before sending. This one > should compile better!The put_page[_and_type] also needs to be on the far side of the TLB flush. This is because if a refcount falls to zero then the page can change type (e.g., to pagetable page) or be recycled to another domain! This is a safety issue for Xen rather than for the granter. :-) -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kieran Mansley
2007-Jul-18 11:29 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On Fri, 2007-07-13 at 17:20 +0100, Keir Fraser wrote:> On 13/7/07 16:54, "Kieran Mansley" <kmansley@solarflare.com> wrote: > > > Oops, sorry, forgot to refresh the patch before sending. This one > > should compile better! > > The put_page[_and_type] also needs to be on the far side of the TLB flush. > This is because if a refcount falls to zero then the page can change type > (e.g., to pagetable page) or be recycled to another domain! This is a safety > issue for Xen rather than for the granter. :-) >OK, here is another spin of the patch that should address all the concerns mentioned so far. I''m pretty sure that (apart from the change in when the tlb flush happens) the behaviour is the same as the old version. i.e. The same code will get executed in both cases, despite the function being split in two, even if something goes wrong. I would of course welcome more pairs of eyes to check this though. As this modifies a fairly heavily used and vital region of Xen I''m guessing that there''s a unit test somewhere that I could run (or get run by whoever controls the test). Can anyone point me at this? If there''s no such test, knowing that would help too. If this patch is acceptable (and once better tested) I''ll repost with it signed off, together with the change to redefine the grant_operation_permitted macro, and the iomem permission patch that prompted this in the first place. Thanks Kieran Fix TLB flush on grant unmap diff -r 1e208016e32e xen/common/grant_table.c --- a/xen/common/grant_table.c Wed Jul 18 10:05:04 2007 +0100 +++ b/xen/common/grant_table.c Wed Jul 18 12:01:41 2007 +0100 @@ -60,13 +60,25 @@ union grant_combo { /* Used to share code between unmap_grant_ref and unmap_and_replace. */ struct gnttab_unmap_common { + /* Input */ uint64_t host_addr; uint64_t dev_bus_addr; uint64_t new_addr; grant_handle_t handle; + /* Return */ int16_t status; + + /* Shared state beteen *_unmap and *_unmap_complete */ + u16 flags; + unsigned long frame; + struct grant_mapping *map; + struct domain *rd; }; + +/* Number of unmap operations that are done between each tlb flush */ +#define GNTTAB_UNMAP_BATCH_SIZE 32 + #define PIN_FAIL(_lbl, _rc, _f, _a...) \ do { \ @@ -411,18 +423,14 @@ __gnttab_unmap_common( struct gnttab_unmap_common *op) { domid_t dom; - grant_ref_t ref; struct domain *ld, *rd; struct active_grant_entry *act; grant_entry_t *sha; - struct grant_mapping *map; - u16 flags; s16 rc = 0; - unsigned long frame; ld = current->domain; - frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT); + op->frame = (unsigned long)(op->dev_bus_addr >> PAGE_SHIFT); if ( unlikely(op->handle >= ld->grant_table->maptrack_limit) ) { @@ -431,20 +439,19 @@ __gnttab_unmap_common( return; } - map = &maptrack_entry(ld->grant_table, op->handle); - - if ( unlikely(!map->flags) ) + op->map = &maptrack_entry(ld->grant_table, op->handle); + + if ( unlikely(!op->map->flags) ) { gdprintk(XENLOG_INFO, "Zero flags for handle (%d).\n", op->handle);op->status = GNTST_bad_handle; return; } - dom = map->domid; - ref = map->ref; - flags = map->flags; - - if ( unlikely((rd = rcu_lock_domain_by_id(dom)) == NULL) ) + dom = op->map->domid; + op->flags = op->map->flags; + + if ( unlikely((op->rd = rd = rcu_lock_domain_by_id(dom)) == NULL) ) { /* This can happen when a grant is implicitly unmapped. */ gdprintk(XENLOG_INFO, "Could not find domain %d\n", dom); @@ -456,71 +463,47 @@ __gnttab_unmap_common( spin_lock(&rd->grant_table->lock); - act = &active_entry(rd->grant_table, ref); - sha = &shared_entry(rd->grant_table, ref); - - if ( frame == 0 ) - { - frame = act->frame; + act = &active_entry(rd->grant_table, op->map->ref); + sha = &shared_entry(rd->grant_table, op->map->ref); + + if ( op->frame == 0 ) + { + op->frame = act->frame; } else { - if ( unlikely(frame != act->frame) ) + if ( unlikely(op->frame != act->frame) ) PIN_FAIL(unmap_out, GNTST_general_error, "Bad frame number doesn''t match gntref.\n"); - if ( flags & GNTMAP_device_map ) + if ( op->flags & GNTMAP_device_map ) { ASSERT(act->pin & (GNTPIN_devw_mask | GNTPIN_devr_mask)); - map->flags &= ~GNTMAP_device_map; - if ( flags & GNTMAP_readonly ) - { + op->map->flags &= ~GNTMAP_device_map; + if ( op->flags & GNTMAP_readonly ) act->pin -= GNTPIN_devr_inc; - put_page(mfn_to_page(frame)); - } else - { act->pin -= GNTPIN_devw_inc; - put_page_and_type(mfn_to_page(frame)); - } - } - } - - if ( (op->host_addr != 0) && (flags & GNTMAP_host_map) ) + } + } + + if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) { if ( (rc = replace_grant_host_mapping(op->host_addr, - frame, op->new_addr, flags)) < 0 ) + op->frame, op->new_addr, + op->flags)) < 0 ) goto unmap_out; ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask)); - map->flags &= ~GNTMAP_host_map; - if ( flags & GNTMAP_readonly ) - { + op->map->flags &= ~GNTMAP_host_map; + if ( op->flags & GNTMAP_readonly ) act->pin -= GNTPIN_hstr_inc; - put_page(mfn_to_page(frame)); - } else - { act->pin -= GNTPIN_hstw_inc; - put_page_and_type(mfn_to_page(frame)); - } - } - - if ( (map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) - { - map->flags = 0; - put_maptrack_handle(ld->grant_table, op->handle); } /* If just unmapped a writable mapping, mark as dirtied */ - if ( !(flags & GNTMAP_readonly) ) - gnttab_mark_dirty(rd, frame); - - if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) && - !(flags & GNTMAP_readonly) ) - gnttab_clear_flag(_GTF_writing, &sha->flags); - - if ( act->pin == 0 ) - gnttab_clear_flag(_GTF_reading, &sha->flags); + if ( !(op->flags & GNTMAP_readonly) ) + gnttab_mark_dirty(rd, op->frame); unmap_out: op->status = rc; @@ -529,78 +512,205 @@ __gnttab_unmap_common( } static void +__gnttab_unmap_common_complete(struct gnttab_unmap_common *op) +{ + struct domain *ld, *rd; + struct active_grant_entry *act; + grant_entry_t *sha; + + rd = op->rd; + + if ( rd == NULL ) { + /* + * Suggests that __gntab_unmap_common failed in + * rcu_lock_domain_by_id() or earlier, and so we have nothing + * to complete + */ + return; + } + + ld = current->domain; + + rcu_lock_domain(rd); + spin_lock(&rd->grant_table->lock); + + act = &active_entry(rd->grant_table, op->map->ref); + sha = &shared_entry(rd->grant_table, op->map->ref); + + if ( unlikely(op->frame != act->frame) ) + { + /* + * Suggests that __gntab_unmap_common failed early and so + * nothing further to do + */ + goto unmap_out; + } + + if ( op->flags & GNTMAP_device_map ) + { + if ( op->flags & GNTMAP_readonly ) + put_page(mfn_to_page(op->frame)); + else + put_page_and_type(mfn_to_page(op->frame)); + } + + if ( (op->host_addr != 0) && (op->flags & GNTMAP_host_map) ) + { + if ( op->status != 0 ) + { + /* + * Suggests that __gntab_unmap_common failed in + * replace_grant_host_mapping() so nothing further to do + */ + goto unmap_out; + } + + if ( op->flags & GNTMAP_readonly ) + put_page(mfn_to_page(op->frame)); + else + put_page_and_type(mfn_to_page(op->frame)); + } + + if ( (op->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) + { + op->map->flags = 0; + put_maptrack_handle(ld->grant_table, op->handle); + } + + if ( ((act->pin & (GNTPIN_devw_mask|GNTPIN_hstw_mask)) == 0) && + !(op->flags & GNTMAP_readonly) ) + gnttab_clear_flag(_GTF_writing, &sha->flags); + + if ( act->pin == 0 ) + gnttab_clear_flag(_GTF_reading, &sha->flags); + + unmap_out: + spin_unlock(&rd->grant_table->lock); + rcu_unlock_domain(rd); +} + +static void __gnttab_unmap_grant_ref( - struct gnttab_unmap_grant_ref *op) -{ - struct gnttab_unmap_common common = { - .host_addr = op->host_addr, - .dev_bus_addr = op->dev_bus_addr, - .handle = op->handle, - }; - - __gnttab_unmap_common(&common); - op->status = common.status; -} + struct gnttab_unmap_grant_ref *op, + struct gnttab_unmap_common *common) +{ + common->host_addr = op->host_addr; + common->dev_bus_addr = op->dev_bus_addr; + common->handle = op->handle; + + /* Intialise these in case common contains old state */ + common->new_addr = 0; + common->rd = NULL; + + __gnttab_unmap_common(common); + op->status = common->status; +} + static long gnttab_unmap_grant_ref( XEN_GUEST_HANDLE(gnttab_unmap_grant_ref_t) uop, unsigned int count) { - int i; + int i, c, partial_done, done = 0; struct gnttab_unmap_grant_ref op; - - for ( i = 0; i < count; i++ ) - { - if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) - goto fault; - __gnttab_unmap_grant_ref(&op); - if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) ) - goto fault; - } - - flush_tlb_mask(current->domain->domain_dirty_cpumask); + struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE]; + + while (count != 0) { + c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE); + partial_done = 0; + + for ( i = 0; i < c; i++ ) + { + if ( unlikely(__copy_from_guest_offset(&op, uop, done+i, 1)) ) + goto fault; + __gnttab_unmap_grant_ref(&op, &(common[i])); + ++partial_done; + if ( unlikely(__copy_to_guest_offset(uop, done+i, &op, 1)) ) + goto fault; + } + + flush_tlb_mask(current->domain->domain_dirty_cpumask); + + for ( i = 0; i < partial_done; i++ ) + { + __gnttab_unmap_common_complete(&(common[i])); + } + + count -= c; + done += c; + } + return 0; fault: flush_tlb_mask(current->domain->domain_dirty_cpumask); - return -EFAULT; + + for ( i = 0; i < partial_done; i++ ) + { + __gnttab_unmap_common_complete(&(common[i])); + } + return -EFAULT; } static void __gnttab_unmap_and_replace( - struct gnttab_unmap_and_replace *op) -{ - struct gnttab_unmap_common common = { - .host_addr = op->host_addr, - .new_addr = op->new_addr, - .handle = op->handle, - }; - - __gnttab_unmap_common(&common); - op->status = common.status; + struct gnttab_unmap_and_replace *op, + struct gnttab_unmap_common *common) +{ + common->host_addr = op->host_addr; + common->new_addr = op->new_addr; + common->handle = op->handle; + + /* Intialise these in case common contains old state */ + common->dev_bus_addr = 0; + common->rd = NULL; + + __gnttab_unmap_common(common); + op->status = common->status; } static long gnttab_unmap_and_replace( XEN_GUEST_HANDLE(gnttab_unmap_and_replace_t) uop, unsigned int count) { - int i; + int i, c, partial_done, done = 0; struct gnttab_unmap_and_replace op; - - for ( i = 0; i < count; i++ ) - { - if ( unlikely(__copy_from_guest_offset(&op, uop, i, 1)) ) - goto fault; - __gnttab_unmap_and_replace(&op); - if ( unlikely(__copy_to_guest_offset(uop, i, &op, 1)) ) - goto fault; - } - - flush_tlb_mask(current->domain->domain_dirty_cpumask); + struct gnttab_unmap_common common[GNTTAB_UNMAP_BATCH_SIZE]; + + while (count != 0) { + c = min(count, (unsigned int)GNTTAB_UNMAP_BATCH_SIZE); + partial_done = 0; + + for ( i = 0; i < c; i++ ) + { + if ( unlikely(__copy_from_guest_offset(&op, uop, done+i, 1)) ) + goto fault; + __gnttab_unmap_and_replace(&op, &(common[i])); + ++partial_done; + if ( unlikely(__copy_to_guest_offset(uop, done+i, &op, 1)) ) + goto fault; + } + + flush_tlb_mask(current->domain->domain_dirty_cpumask); + + for ( i = 0; i < partial_done; i++ ) + { + __gnttab_unmap_common_complete(&(common[i])); + } + + count -= c; + done += c; + } + return 0; fault: flush_tlb_mask(current->domain->domain_dirty_cpumask); + + for ( i = 0; i < partial_done; i++ ) + { + __gnttab_unmap_common_complete(&(common[i])); + } return -EFAULT; } _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Keir Fraser
2007-Jul-18 12:46 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On 18/7/07 12:29, "Kieran Mansley" <kmansley@solarflare.com> wrote:> As this modifies a fairly heavily used and vital region of Xen I''m > guessing that there''s a unit test somewhere that I could run (or get run > by whoever controls the test). Can anyone point me at this? If there''s > no such test, knowing that would help too.There''s no unit test. Running normal device drivers gives a pretty good workout though.> If this patch is acceptable (and once better tested) I''ll repost with it > signed off, together with the change to redefine the > grant_operation_permitted macro, and the iomem permission patch that > prompted this in the first place.I''ll add it to my to-look-at list. It might be a little while. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kieran Mansley
2007-Jul-25 16:14 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On Wed, 2007-07-18 at 13:46 +0100, Keir Fraser wrote:> On 18/7/07 12:29, "Kieran Mansley" <kmansley@solarflare.com> wrote: > > If this patch is acceptable (and once better tested) I''ll repost with it > > signed off, together with the change to redefine the > > grant_operation_permitted macro, and the iomem permission patch that > > prompted this in the first place. > > I''ll add it to my to-look-at list. It might be a little while.I''m glad you didn''t look at it straightaway as I''ve found a little bug during testing. A new patch is attached; the change was as follows: --- a/xen/common/grant_table.c Wed Jul 25 10:33:15 2007 +0100 +++ b/xen/common/grant_table.c Wed Jul 25 13:59:10 2007 +0100 @@ -571,7 +571,7 @@ __gnttab_unmap_common_complete(struct gn put_page_and_type(mfn_to_page(op->frame)); } - if ( (op->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) + if ( (op->map->flags & (GNTMAP_device_map|GNTMAP_host_map)) == 0 ) { op->map->flags = 0; put_maptrack_handle(ld->grant_table, op->handle); Kieran _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Kieran Mansley
2007-Aug-14 16:02 UTC
Re: [Xen-devel] [PATCH] Allow programatic iomem permissions
On Wed, 2007-07-18 at 13:46 +0100, Keir Fraser wrote:> > If this patch is acceptable (and once better tested) I''ll repost with it > > signed off, together with the change to redefine the > > grant_operation_permitted macro, and the iomem permission patch that > > prompted this in the first place. > > I''ll add it to my to-look-at list. It might be a little while.Attached is a new version of the patch that should apply cleanly to the latest xen-unstable.hg. Some whitespace changes to the files in question meant patch would get very confused when trying to apply the old version, but I think that''s the only change. Should make it a bit easier for you when you get the chance to look at it. Thanks Kieran _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel