| Message ID | 1412347845-27755-3-git-send-email-stefano.stabellini@eu.citrix.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 3 Oct 2014, Stefano Stabellini wrote: > Introduce a new hypercall to perform cache maintenance operation on > behalf of the guest. The argument is a machine address and a size. The > implementation checks that the memory range is owned by the guest or the > guest has been granted access to it by another domain. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > Changes in v2: > - do not check for mfn_to_page errors; > - take a reference to the page; > - replace printk with gdprintk; > - split long line; > - remove out label; > - move rcu_lock_current_domain down before the loop. > - move the hypercall to GNTTABOP; > - take a spin_lock before calling grant_map_exists. > --- > xen/common/grant_table.c | 73 ++++++++++++++++++++++++++++++++++++++ > xen/include/public/grant_table.h | 19 ++++++++++ > 2 files changed, 92 insertions(+) > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > index 7a6399b..d5bb4f7 100644 > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -2641,6 +2641,79 @@ do_grant_table_op( > } > break; > } > + case GNTTABOP_cache_flush: > + { > + struct gnttab_cache_flush cflush; > + struct domain *d, *owner; > + struct page_info *page; > + uint64_t mfn; > + void *v; > + > + /* currently unimplemented */ > + if ( count != 1 ) > + return -ENOSYS; > + > + if ( copy_from_guest(&cflush, uop, 1) ) > + return -EFAULT; > + > + if ( cflush.offset + cflush.size > PAGE_SIZE ) > + return -EINVAL; > + > + if ( cflush.size == 0 || cflush.op == 0 ) > + return 0; > + > + if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) ) > + return -EINVAL; > + > + /* currently unimplemented */ > + if ( cflush.a.ref != 0 ) > + return -ENOSYS; > + > + d = rcu_lock_current_domain(); > + if ( d == NULL ) > + return -ESRCH; > + > + mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT; > + > + if ( !mfn_valid(mfn) ) > + { > + gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", mfn); > + rcu_unlock_domain(d); > + return -EINVAL; > + } > + > + page = mfn_to_page(mfn); > + owner = page_get_owner_and_reference(page); > + if ( !owner ) > + { > + rcu_unlock_domain(d); > + return -EFAULT; > + } > + > + spin_lock(&owner->grant_table->lock); > + > + if ( !grant_map_exists(d, owner->grant_table, mfn) ) > + { > + spin_unlock(&owner->grant_table->lock); > + put_page(page); > + rcu_unlock_domain(d); > + gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %d\n", > + mfn, owner->domain_id, d->domain_id); > + return -EINVAL; > + } > + > + v = map_domain_page(mfn); > + v += cflush.offset; > + > + if ( cflush.op & GNTTAB_CACHE_INVAL ) > + invalidate_xen_dcache_va_range(v, cflush.size); > + if ( cflush.op & GNTTAB_CACHE_CLEAN ) > + clean_xen_dcache_va_range(v, cflush.size); I realize now that I forgot to introduce empty cache cleaning stubs for x86. > + unmap_domain_page(v); > + spin_unlock(&owner->grant_table->lock); > + put_page(page); > + } > default: > rc = -ENOSYS; > break; > diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h > index b8a3d6c..1833bba 100644 > --- a/xen/include/public/grant_table.h > +++ b/xen/include/public/grant_table.h > @@ -309,6 +309,7 @@ typedef uint16_t grant_status_t; > #define GNTTABOP_get_status_frames 9 > #define GNTTABOP_get_version 10 > #define GNTTABOP_swap_grant_ref 11 > +#define GNTTABOP_cache_flush 12 > #endif /* __XEN_INTERFACE_VERSION__ */ > /* ` } */ > > @@ -574,6 +575,24 @@ struct gnttab_swap_grant_ref { > typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; > DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); > > +/* > + * Issue one or more cache maintenance operations on a portion of a > + * page granted to the calling domain by a foreign domain. > + */ > +struct gnttab_cache_flush { > + union { > + uint64_t dev_bus_addr; > + grant_ref_t ref; > + } a; > + uint32_t offset; /* offset from start of grant */ > + uint32_t size; /* size within the grant */ > +#define GNTTAB_CACHE_CLEAN (1<<0) > +#define GNTTAB_CACHE_INVAL (1<<1) > + uint32_t op; > +}; > +typedef struct gnttab_cache_flush gnttab_cache_flush_t; > +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t); > + > #endif /* __XEN_INTERFACE_VERSION__ */ > > /* > -- > 1.7.10.4 >
On 03/10/14 15:50, Stefano Stabellini wrote: > Introduce a new hypercall to perform cache maintenance operation on > behalf of the guest. The argument is a machine address and a size. The > implementation checks that the memory range is owned by the guest or the > guest has been granted access to it by another domain. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > --- > > Changes in v2: > - do not check for mfn_to_page errors; > - take a reference to the page; > - replace printk with gdprintk; > - split long line; > - remove out label; > - move rcu_lock_current_domain down before the loop. > - move the hypercall to GNTTABOP; > - take a spin_lock before calling grant_map_exists. > --- > xen/common/grant_table.c | 73 ++++++++++++++++++++++++++++++++++++++ > xen/include/public/grant_table.h | 19 ++++++++++ > 2 files changed, 92 insertions(+) > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > index 7a6399b..d5bb4f7 100644 > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -2641,6 +2641,79 @@ do_grant_table_op( > } > break; > } > + case GNTTABOP_cache_flush: > + { > + struct gnttab_cache_flush cflush; > + struct domain *d, *owner; > + struct page_info *page; > + uint64_t mfn; > + void *v; > + > + /* currently unimplemented */ > + if ( count != 1 ) > + return -ENOSYS; ENOTSUPP (and elsewhere). > + > + if ( copy_from_guest(&cflush, uop, 1) ) > + return -EFAULT; > + > + if ( cflush.offset + cflush.size > PAGE_SIZE ) > + return -EINVAL; This logic can still be fooled by cleverly crafted overflows, resulting in garbage being used later, which I am guessing will be a bad thing. You need to bound check them individually before bounds-checking the addition of the two. > + > + if ( cflush.size == 0 || cflush.op == 0 ) > + return 0; > + > + if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) ) > + return -EINVAL; > + > + /* currently unimplemented */ > + if ( cflush.a.ref != 0 ) > + return -ENOSYS; > + > + d = rcu_lock_current_domain(); > + if ( d == NULL ) > + return -ESRCH; rcu_lock_current_domain() cannot possibly fail. > + > + mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT; > + > + if ( !mfn_valid(mfn) ) > + { > + gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", mfn); PRIx64, as mfn is uint64_t (and elsewhere). > + rcu_unlock_domain(d); > + return -EINVAL; > + } > + > + page = mfn_to_page(mfn); > + owner = page_get_owner_and_reference(page); > + if ( !owner ) > + { > + rcu_unlock_domain(d); > + return -EFAULT; EPERM seems better, as it is an ownership check on an existing mfn. > + } > + > + spin_lock(&owner->grant_table->lock); Is this safe without taking the paging lock and doing a dying check on 'owner' ? > + > + if ( !grant_map_exists(d, owner->grant_table, mfn) ) > + { > + spin_unlock(&owner->grant_table->lock); > + put_page(page); > + rcu_unlock_domain(d); These two can be reordered to reduce the length of time the rcu lock is held. > + gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %d\n", > + mfn, owner->domain_id, d->domain_id); "hasn't been granted by d%d to d%d", to indicate that the numbers are domids. > + return -EINVAL; > + } > + > + v = map_domain_page(mfn); > + v += cflush.offset; > + > + if ( cflush.op & GNTTAB_CACHE_INVAL ) > + invalidate_xen_dcache_va_range(v, cflush.size); > + if ( cflush.op & GNTTAB_CACHE_CLEAN ) > + clean_xen_dcache_va_range(v, cflush.size); > + > + unmap_domain_page(v); > + spin_unlock(&owner->grant_table->lock); These two can be reordered to reduce the length of time the grant lock is held. (especially as it is a remote domains grant lock) ~Andrew > + put_page(page); > + } > default: > rc = -ENOSYS; > break; > diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h > index b8a3d6c..1833bba 100644 > --- a/xen/include/public/grant_table.h > +++ b/xen/include/public/grant_table.h > @@ -309,6 +309,7 @@ typedef uint16_t grant_status_t; > #define GNTTABOP_get_status_frames 9 > #define GNTTABOP_get_version 10 > #define GNTTABOP_swap_grant_ref 11 > +#define GNTTABOP_cache_flush 12 > #endif /* __XEN_INTERFACE_VERSION__ */ > /* ` } */ > > @@ -574,6 +575,24 @@ struct gnttab_swap_grant_ref { > typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; > DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); > > +/* > + * Issue one or more cache maintenance operations on a portion of a > + * page granted to the calling domain by a foreign domain. > + */ > +struct gnttab_cache_flush { > + union { > + uint64_t dev_bus_addr; > + grant_ref_t ref; > + } a; > + uint32_t offset; /* offset from start of grant */ > + uint32_t size; /* size within the grant */ > +#define GNTTAB_CACHE_CLEAN (1<<0) > +#define GNTTAB_CACHE_INVAL (1<<1) > + uint32_t op; > +}; > +typedef struct gnttab_cache_flush gnttab_cache_flush_t; > +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t); > + > #endif /* __XEN_INTERFACE_VERSION__ */ > > /*
On Fri, 3 Oct 2014, Andrew Cooper wrote: > On 03/10/14 15:50, Stefano Stabellini wrote: > > Introduce a new hypercall to perform cache maintenance operation on > > behalf of the guest. The argument is a machine address and a size. The > > implementation checks that the memory range is owned by the guest or the > > guest has been granted access to it by another domain. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > --- > > > > Changes in v2: > > - do not check for mfn_to_page errors; > > - take a reference to the page; > > - replace printk with gdprintk; > > - split long line; > > - remove out label; > > - move rcu_lock_current_domain down before the loop. > > - move the hypercall to GNTTABOP; > > - take a spin_lock before calling grant_map_exists. > > --- > > xen/common/grant_table.c | 73 ++++++++++++++++++++++++++++++++++++++ > > xen/include/public/grant_table.h | 19 ++++++++++ > > 2 files changed, 92 insertions(+) > > > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > > index 7a6399b..d5bb4f7 100644 > > --- a/xen/common/grant_table.c > > +++ b/xen/common/grant_table.c > > @@ -2641,6 +2641,79 @@ do_grant_table_op( > > } > > break; > > } > > + case GNTTABOP_cache_flush: > > + { > > + struct gnttab_cache_flush cflush; > > + struct domain *d, *owner; > > + struct page_info *page; > > + uint64_t mfn; > > + void *v; > > + > > + /* currently unimplemented */ > > + if ( count != 1 ) > > + return -ENOSYS; > > ENOTSUPP (and elsewhere). ENOTSUPP doesn't seem to be defined in Xen. Should I take the opportunity to introduce it as 252? > > + > > + if ( copy_from_guest(&cflush, uop, 1) ) > > + return -EFAULT; > > + > > + if ( cflush.offset + cflush.size > PAGE_SIZE ) > > + return -EINVAL; > > This logic can still be fooled by cleverly crafted overflows, resulting > in garbage being used later, which I am guessing will be a bad thing. > > You need to bound check them individually before bounds-checking the > addition of the two. > > > + > > + if ( cflush.size == 0 || cflush.op == 0 ) > > + return 0; > > + > > + if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) ) > > + return -EINVAL; > > + > > + /* currently unimplemented */ > > + if ( cflush.a.ref != 0 ) > > + return -ENOSYS; > > + > > + d = rcu_lock_current_domain(); > > + if ( d == NULL ) > > + return -ESRCH; > > rcu_lock_current_domain() cannot possibly fail. > > > + > > + mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT; > > + > > + if ( !mfn_valid(mfn) ) > > + { > > + gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", mfn); > > PRIx64, as mfn is uint64_t (and elsewhere). > > > + rcu_unlock_domain(d); > > + return -EINVAL; > > + } > > + > > + page = mfn_to_page(mfn); > > + owner = page_get_owner_and_reference(page); > > + if ( !owner ) > > + { > > + rcu_unlock_domain(d); > > + return -EFAULT; > > EPERM seems better, as it is an ownership check on an existing mfn. > > > + } > > + > > + spin_lock(&owner->grant_table->lock); > > Is this safe without taking the paging lock and doing a dying check on > 'owner' ? In gnttab_release_mappings we do take the grant_table lock before releasing any entries so it seems to me that it should be sufficient. > > + > > + if ( !grant_map_exists(d, owner->grant_table, mfn) ) > > + { > > + spin_unlock(&owner->grant_table->lock); > > + put_page(page); > > + rcu_unlock_domain(d); > > These two can be reordered to reduce the length of time the rcu lock is > held. > > > + gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %d\n", > > + mfn, owner->domain_id, d->domain_id); > > "hasn't been granted by d%d to d%d", to indicate that the numbers are > domids. > > > + return -EINVAL; > > + } > > + > > + v = map_domain_page(mfn); > > + v += cflush.offset; > > + > > + if ( cflush.op & GNTTAB_CACHE_INVAL ) > > + invalidate_xen_dcache_va_range(v, cflush.size); > > + if ( cflush.op & GNTTAB_CACHE_CLEAN ) > > + clean_xen_dcache_va_range(v, cflush.size); > > + > > + unmap_domain_page(v); > > + spin_unlock(&owner->grant_table->lock); > > These two can be reordered to reduce the length of time the grant lock > is held. (especially as it is a remote domains grant lock) > > ~Andrew > > > + put_page(page); > > + } > > default: > > rc = -ENOSYS; > > break; > > diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h > > index b8a3d6c..1833bba 100644 > > --- a/xen/include/public/grant_table.h > > +++ b/xen/include/public/grant_table.h > > @@ -309,6 +309,7 @@ typedef uint16_t grant_status_t; > > #define GNTTABOP_get_status_frames 9 > > #define GNTTABOP_get_version 10 > > #define GNTTABOP_swap_grant_ref 11 > > +#define GNTTABOP_cache_flush 12 > > #endif /* __XEN_INTERFACE_VERSION__ */ > > /* ` } */ > > > > @@ -574,6 +575,24 @@ struct gnttab_swap_grant_ref { > > typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; > > DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); > > > > +/* > > + * Issue one or more cache maintenance operations on a portion of a > > + * page granted to the calling domain by a foreign domain. > > + */ > > +struct gnttab_cache_flush { > > + union { > > + uint64_t dev_bus_addr; > > + grant_ref_t ref; > > + } a; > > + uint32_t offset; /* offset from start of grant */ > > + uint32_t size; /* size within the grant */ > > +#define GNTTAB_CACHE_CLEAN (1<<0) > > +#define GNTTAB_CACHE_INVAL (1<<1) > > + uint32_t op; > > +}; > > +typedef struct gnttab_cache_flush gnttab_cache_flush_t; > > +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t); > > + > > #endif /* __XEN_INTERFACE_VERSION__ */ > > > > /* > >
On 03/10/14 17:33, Stefano Stabellini wrote: > On Fri, 3 Oct 2014, Andrew Cooper wrote: >> On 03/10/14 15:50, Stefano Stabellini wrote: >>> Introduce a new hypercall to perform cache maintenance operation on >>> behalf of the guest. The argument is a machine address and a size. The >>> implementation checks that the memory range is owned by the guest or the >>> guest has been granted access to it by another domain. >>> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >>> >>> --- >>> >>> Changes in v2: >>> - do not check for mfn_to_page errors; >>> - take a reference to the page; >>> - replace printk with gdprintk; >>> - split long line; >>> - remove out label; >>> - move rcu_lock_current_domain down before the loop. >>> - move the hypercall to GNTTABOP; >>> - take a spin_lock before calling grant_map_exists. >>> --- >>> xen/common/grant_table.c | 73 ++++++++++++++++++++++++++++++++++++++ >>> xen/include/public/grant_table.h | 19 ++++++++++ >>> 2 files changed, 92 insertions(+) >>> >>> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c >>> index 7a6399b..d5bb4f7 100644 >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c >>> @@ -2641,6 +2641,79 @@ do_grant_table_op( >>> } >>> break; >>> } >>> + case GNTTABOP_cache_flush: >>> + { >>> + struct gnttab_cache_flush cflush; >>> + struct domain *d, *owner; >>> + struct page_info *page; >>> + uint64_t mfn; >>> + void *v; >>> + >>> + /* currently unimplemented */ >>> + if ( count != 1 ) >>> + return -ENOSYS; >> ENOTSUPP (and elsewhere). > ENOTSUPP doesn't seem to be defined in Xen. Should I take the > opportunity to introduce it as 252? Sorry. EOPNOTSUPP, 95. ~Andrew
On Fri, 2014-10-03 at 17:33 +0100, Stefano Stabellini wrote: > On Fri, 3 Oct 2014, Andrew Cooper wrote: > > On 03/10/14 15:50, Stefano Stabellini wrote: > > > Introduce a new hypercall to perform cache maintenance operation on > > > behalf of the guest. The argument is a machine address and a size. The > > > implementation checks that the memory range is owned by the guest or the > > > guest has been granted access to it by another domain. > > > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > > > > > --- > > > > > > Changes in v2: > > > - do not check for mfn_to_page errors; > > > - take a reference to the page; > > > - replace printk with gdprintk; > > > - split long line; > > > - remove out label; > > > - move rcu_lock_current_domain down before the loop. > > > - move the hypercall to GNTTABOP; > > > - take a spin_lock before calling grant_map_exists. > > > --- > > > xen/common/grant_table.c | 73 ++++++++++++++++++++++++++++++++++++++ > > > xen/include/public/grant_table.h | 19 ++++++++++ > > > 2 files changed, 92 insertions(+) > > > > > > diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c > > > index 7a6399b..d5bb4f7 100644 > > > --- a/xen/common/grant_table.c > > > +++ b/xen/common/grant_table.c > > > @@ -2641,6 +2641,79 @@ do_grant_table_op( > > > } > > > break; > > > } > > > + case GNTTABOP_cache_flush: > > > + { > > > + struct gnttab_cache_flush cflush; > > > + struct domain *d, *owner; > > > + struct page_info *page; > > > + uint64_t mfn; > > > + void *v; > > > + > > > + /* currently unimplemented */ > > > + if ( count != 1 ) > > > + return -ENOSYS; > > > > ENOTSUPP (and elsewhere). > > ENOTSUPP doesn't seem to be defined in Xen. Should I take the > opportunity to introduce it as 252? Use EOPNOTSUPP I think.
>>> On 03.10.14 at 18:33, <stefano.stabellini@eu.citrix.com> wrote: > On Fri, 3 Oct 2014, Andrew Cooper wrote: >> On 03/10/14 15:50, Stefano Stabellini wrote: >> > + } >> > + >> > + spin_lock(&owner->grant_table->lock); >> >> Is this safe without taking the paging lock and doing a dying check on >> 'owner' ? > > In gnttab_release_mappings we do take the grant_table lock before > releasing any entries so it seems to me that it should be sufficient. Now did you check at all when gnttab_release_mappings() gets called? Only ever on a dying domain (there's even a BUG_ON() to that effect at the start of the function). Yet here you want to avoid dying domains, i.e. your point of reference would rather be gnttab_transfer(). That said, since you take a reference to the page, I don't think you need to pay attention to whether the page owner is dying. Jan
On 03/10/14 15:50, Stefano Stabellini wrote: > Introduce a new hypercall to perform cache maintenance operation on > behalf of the guest. The argument is a machine address and a size. The > implementation checks that the memory range is owned by the guest or the > guest has been granted access to it by another domain. > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> [...] > --- a/xen/common/grant_table.c > +++ b/xen/common/grant_table.c > @@ -2641,6 +2641,79 @@ do_grant_table_op( [...] > + > + page = mfn_to_page(mfn); > + owner = page_get_owner_and_reference(page); > + if ( !owner ) > + { > + rcu_unlock_domain(d); > + return -EFAULT; > + } > + > + spin_lock(&owner->grant_table->lock); The grant table lock is already heavily contended, so you should skip the lock and the grant_map_exists() check if d == owner. > + > + if ( !grant_map_exists(d, owner->grant_table, mfn) ) Looping over all grant table entries or all maptrack entries looks expensive to me. Perhaps consider allowing suitably privileged domains to clean/invalidate any address without having to check if it's been granted. Instead of this hypercall, could the guest clean/invalidate by set/way? I guess this would need a suitable IPA which could be obtained by some (offset) 1:1 mapping in the stage 2 tables? David
On Mon, 6 Oct 2014, David Vrabel wrote: > On 03/10/14 15:50, Stefano Stabellini wrote: > > Introduce a new hypercall to perform cache maintenance operation on > > behalf of the guest. The argument is a machine address and a size. The > > implementation checks that the memory range is owned by the guest or the > > guest has been granted access to it by another domain. > > > > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > [...] > > --- a/xen/common/grant_table.c > > +++ b/xen/common/grant_table.c > > @@ -2641,6 +2641,79 @@ do_grant_table_op( > [...] > > + > > + page = mfn_to_page(mfn); > > + owner = page_get_owner_and_reference(page); > > + if ( !owner ) > > + { > > + rcu_unlock_domain(d); > > + return -EFAULT; > > + } > > + > > + spin_lock(&owner->grant_table->lock); > > The grant table lock is already heavily contended, so you should skip > the lock and the grant_map_exists() check if d == owner. OK, but it is going to make the code uglier. > > + > > + if ( !grant_map_exists(d, owner->grant_table, mfn) ) > > Looping over all grant table entries or all maptrack entries looks > expensive to me. > > Perhaps consider allowing suitably privileged domains to > clean/invalidate any address without having to check if it's been granted. I think that would weaken our security guarantees. > Instead of this hypercall, could the guest clean/invalidate by set/way? > I guess this would need a suitable IPA which could be obtained by some > (offset) 1:1 mapping in the stage 2 tables? I wish I could: as the clean/invalidate by set/way is implementation specific, there is now way to use it to clean a specific range of addresses.
On 08/10/14 12:54, Stefano Stabellini wrote: > On Mon, 6 Oct 2014, David Vrabel wrote: >> On 03/10/14 15:50, Stefano Stabellini wrote: >>> Introduce a new hypercall to perform cache maintenance operation on >>> behalf of the guest. The argument is a machine address and a size. The >>> implementation checks that the memory range is owned by the guest or the >>> guest has been granted access to it by another domain. >>> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> >> [...] >>> --- a/xen/common/grant_table.c >>> +++ b/xen/common/grant_table.c [...] >>> @@ -2641,6 +2641,79 @@ do_grant_table_op( >>> + >>> + if ( !grant_map_exists(d, owner->grant_table, mfn) ) >> >> Looping over all grant table entries or all maptrack entries looks >> expensive to me. >> >> Perhaps consider allowing suitably privileged domains to >> clean/invalidate any address without having to check if it's been granted. > > I think that would weaken our security guarantees. If the domain can map the foreign domain's frames by other means (e.g., dom0 or a qemu stubdom) then it already has the ability to to clean/invalidate the cache for arbitrary foreign frames. David
On Wed, 2014-10-08 at 12:54 +0100, Stefano Stabellini wrote: > > Instead of this hypercall, could the guest clean/invalidate by set/way? > > I guess this would need a suitable IPA which could be obtained by some > > (offset) 1:1 mapping in the stage 2 tables? > > I wish I could: as the clean/invalidate by set/way is implementation > specific, there is now way to use it to clean a specific range of > addresses. Clean by set/way is basically unusable except in very specific circumstances (e.g. tearing down prior to a suspend). It's certainly not usable on an active SMP system. http://lists.xen.org/archives/html/xen-devel/2014-10/msg00709.html Ian.
On 08/10/14 13:17, Ian Campbell wrote: > On Wed, 2014-10-08 at 12:54 +0100, Stefano Stabellini wrote: >>> Instead of this hypercall, could the guest clean/invalidate by set/way? >>> I guess this would need a suitable IPA which could be obtained by some >>> (offset) 1:1 mapping in the stage 2 tables? >> >> I wish I could: as the clean/invalidate by set/way is implementation >> specific, there is now way to use it to clean a specific range of >> addresses. > > Clean by set/way is basically unusable except in very specific > circumstances (e.g. tearing down prior to a suspend). It's certainly not > usable on an active SMP system. > > http://lists.xen.org/archives/html/xen-devel/2014-10/msg00709.html Yeah, I saw that email after I made my suggestion. David
On Wed, 8 Oct 2014, David Vrabel wrote: > On 08/10/14 12:54, Stefano Stabellini wrote: > > On Mon, 6 Oct 2014, David Vrabel wrote: > >> On 03/10/14 15:50, Stefano Stabellini wrote: > >>> Introduce a new hypercall to perform cache maintenance operation on > >>> behalf of the guest. The argument is a machine address and a size. The > >>> implementation checks that the memory range is owned by the guest or the > >>> guest has been granted access to it by another domain. > >>> > >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > >> [...] > >>> --- a/xen/common/grant_table.c > >>> +++ b/xen/common/grant_table.c > [...] > >>> @@ -2641,6 +2641,79 @@ do_grant_table_op( > >>> + > >>> + if ( !grant_map_exists(d, owner->grant_table, mfn) ) > >> > >> Looping over all grant table entries or all maptrack entries looks > >> expensive to me. > >> > >> Perhaps consider allowing suitably privileged domains to > >> clean/invalidate any address without having to check if it's been granted. > > > > I think that would weaken our security guarantees. > > If the domain can map the foreign domain's frames by other means (e.g., > dom0 or a qemu stubdom) then it already has the ability to to > clean/invalidate the cache for arbitrary foreign frames. True but the ability of mapping any other domain's memory is something we could/should get rid of in PV/PVH/ARM only deployments. Relying on it for page flushing is not going in the right direction.
On Wed, 2014-10-08 at 13:52 +0100, Stefano Stabellini wrote: > On Wed, 8 Oct 2014, David Vrabel wrote: > > On 08/10/14 12:54, Stefano Stabellini wrote: > > > On Mon, 6 Oct 2014, David Vrabel wrote: > > >> On 03/10/14 15:50, Stefano Stabellini wrote: > > >>> Introduce a new hypercall to perform cache maintenance operation on > > >>> behalf of the guest. The argument is a machine address and a size. The > > >>> implementation checks that the memory range is owned by the guest or the > > >>> guest has been granted access to it by another domain. > > >>> > > >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > >> [...] > > >>> --- a/xen/common/grant_table.c > > >>> +++ b/xen/common/grant_table.c > > [...] > > >>> @@ -2641,6 +2641,79 @@ do_grant_table_op( > > >>> + > > >>> + if ( !grant_map_exists(d, owner->grant_table, mfn) ) > > >> > > >> Looping over all grant table entries or all maptrack entries looks > > >> expensive to me. > > >> > > >> Perhaps consider allowing suitably privileged domains to > > >> clean/invalidate any address without having to check if it's been granted. > > > > > > I think that would weaken our security guarantees. > > > > If the domain can map the foreign domain's frames by other means (e.g., > > dom0 or a qemu stubdom) then it already has the ability to to > > clean/invalidate the cache for arbitrary foreign frames. > > True but the ability of mapping any other domain's memory is something > we could/should get rid of in PV/PVH/ARM only deployments. > Relying on it for page flushing is not going in the right direction. Agreed.
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c index 7a6399b..d5bb4f7 100644 --- a/xen/common/grant_table.c +++ b/xen/common/grant_table.c @@ -2641,6 +2641,79 @@ do_grant_table_op( } break; } + case GNTTABOP_cache_flush: + { + struct gnttab_cache_flush cflush; + struct domain *d, *owner; + struct page_info *page; + uint64_t mfn; + void *v; + + /* currently unimplemented */ + if ( count != 1 ) + return -ENOSYS; + + if ( copy_from_guest(&cflush, uop, 1) ) + return -EFAULT; + + if ( cflush.offset + cflush.size > PAGE_SIZE ) + return -EINVAL; + + if ( cflush.size == 0 || cflush.op == 0 ) + return 0; + + if ( cflush.op & ~(GNTTAB_CACHE_INVAL|GNTTAB_CACHE_CLEAN) ) + return -EINVAL; + + /* currently unimplemented */ + if ( cflush.a.ref != 0 ) + return -ENOSYS; + + d = rcu_lock_current_domain(); + if ( d == NULL ) + return -ESRCH; + + mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT; + + if ( !mfn_valid(mfn) ) + { + gdprintk(XENLOG_G_ERR, "mfn=%llx is not valid\n", mfn); + rcu_unlock_domain(d); + return -EINVAL; + } + + page = mfn_to_page(mfn); + owner = page_get_owner_and_reference(page); + if ( !owner ) + { + rcu_unlock_domain(d); + return -EFAULT; + } + + spin_lock(&owner->grant_table->lock); + + if ( !grant_map_exists(d, owner->grant_table, mfn) ) + { + spin_unlock(&owner->grant_table->lock); + put_page(page); + rcu_unlock_domain(d); + gdprintk(XENLOG_G_ERR, "mfn %llx hasn't been granted by %d to %d\n", + mfn, owner->domain_id, d->domain_id); + return -EINVAL; + } + + v = map_domain_page(mfn); + v += cflush.offset; + + if ( cflush.op & GNTTAB_CACHE_INVAL ) + invalidate_xen_dcache_va_range(v, cflush.size); + if ( cflush.op & GNTTAB_CACHE_CLEAN ) + clean_xen_dcache_va_range(v, cflush.size); + + unmap_domain_page(v); + spin_unlock(&owner->grant_table->lock); + put_page(page); + } default: rc = -ENOSYS; break; diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h index b8a3d6c..1833bba 100644 --- a/xen/include/public/grant_table.h +++ b/xen/include/public/grant_table.h @@ -309,6 +309,7 @@ typedef uint16_t grant_status_t; #define GNTTABOP_get_status_frames 9 #define GNTTABOP_get_version 10 #define GNTTABOP_swap_grant_ref 11 +#define GNTTABOP_cache_flush 12 #endif /* __XEN_INTERFACE_VERSION__ */ /* ` } */ @@ -574,6 +575,24 @@ struct gnttab_swap_grant_ref { typedef struct gnttab_swap_grant_ref gnttab_swap_grant_ref_t; DEFINE_XEN_GUEST_HANDLE(gnttab_swap_grant_ref_t); +/* + * Issue one or more cache maintenance operations on a portion of a + * page granted to the calling domain by a foreign domain. + */ +struct gnttab_cache_flush { + union { + uint64_t dev_bus_addr; + grant_ref_t ref; + } a; + uint32_t offset; /* offset from start of grant */ + uint32_t size; /* size within the grant */ +#define GNTTAB_CACHE_CLEAN (1<<0) +#define GNTTAB_CACHE_INVAL (1<<1) + uint32_t op; +}; +typedef struct gnttab_cache_flush gnttab_cache_flush_t; +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t); + #endif /* __XEN_INTERFACE_VERSION__ */ /*
Introduce a new hypercall to perform cache maintenance operation on behalf of the guest. The argument is a machine address and a size. The implementation checks that the memory range is owned by the guest or the guest has been granted access to it by another domain. Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> --- Changes in v2: - do not check for mfn_to_page errors; - take a reference to the page; - replace printk with gdprintk; - split long line; - remove out label; - move rcu_lock_current_domain down before the loop. - move the hypercall to GNTTABOP; - take a spin_lock before calling grant_map_exists. --- xen/common/grant_table.c | 73 ++++++++++++++++++++++++++++++++++++++ xen/include/public/grant_table.h | 19 ++++++++++ 2 files changed, 92 insertions(+)