[PATCH v3 0/5] xen/arm: introduce GNTTABOP_cache_flush

[PATCH v3 0/5] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

Hi all,
this patch series introduces a new hypercall to perform cache
maintenance operations on behalf of the guest. It is useful for dom0 to
be able to cache flush pages involved in a dma operation with
non-coherent devices.

It also removes XENFEAT_grant_map_identity as the feature is no longer
necessary: it was used to achieve the same goal but the guest can now
use the hypercall instead. Keeping the flag would also have a
significant performance impact as a new p2m mapping gets created and
then destroyed for every grant that is mapped and unmapped in dom0.



Changes in v3:
- introduce two different  max_nr_dom0/domU_grant_frames parameters;
- x86: introduce more cache maintenance operations;
- reduce the time the grant_table lock is held;
- fix warning message;
- s/EFAULT/EPERM;
- s/llx/PRIx64;
- check offset and size independetly before checking their sum;
- rcu_lock_current_domain cannot fail;
- s/ENOSYS/EOPNOTSUPP;
- use clean_and_invalidate_xen_dcache_va_range to do both operations at
once;
- fold grant_map_exists in this patch;
- support "count" argument;
- make correspondent changes to compat/grant_table.c;
- introduce GNTTAB_CACHE_SOURCE_GREF to select the type of input in the
union;
- rename size field to length;
- make length and offset uint16_t;
- only take spin_lock if d != owner.

Changes in v2:
- make grant_map_exists static;
- remove the spin_lock in grant_map_exists;
- move the hypercall to GNTTABOP;
- do not check for mfn_to_page errors in GNTTABOP_cache_flush;
- take a reference to the page in GNTTABOP_cache_flush;
- replace printk with gdprintk in GNTTABOP_cache_flush;
- split long line in GNTTABOP_cache_flush;
- remove out label in GNTTABOP_cache_flush;
- move rcu_lock_current_domain down before the loop in
GNTTABOP_cache_flush;
- take a spin_lock before calling grant_map_exists in
GNTTABOP_cache_flush.



Stefano Stabellini (5):
      xen: introduce two different max_nr_dom0/domU_grant_frames parameters
      xen/arm: introduce invalidate_xen_dcache_va_range
      xen/x86: introduce more cache maintenance operations
      xen/arm: introduce GNTTABOP_cache_flush
      Revert "xen/arm: introduce XENFEAT_grant_map_identity"

 docs/misc/xen-command-line.markdown |    4 +-
 xen/arch/arm/domain.c               |    2 +-
 xen/arch/arm/mm.c                   |    2 +-
 xen/arch/x86/mm.c                   |    2 +-
 xen/common/compat/grant_table.c     |   16 ++-
 xen/common/grant_table.c            |  237 +++++++++++++++++++++++++++--------
 xen/common/kernel.c                 |    2 -
 xen/drivers/passthrough/arm/smmu.c  |   33 +++++
 xen/include/asm-arm/arm32/page.h    |    3 +
 xen/include/asm-arm/arm64/page.h    |    3 +
 xen/include/asm-arm/grant_table.h   |    5 +-
 xen/include/asm-arm/page.h          |   30 +++++
 xen/include/asm-x86/page.h          |    4 +
 xen/include/public/features.h       |    4 +-
 xen/include/public/grant_table.h    |   20 +++
 xen/include/xen/grant_table.h       |    6 +-
 xen/include/xlat.lst                |    1 +
 17 files changed, 305 insertions(+), 69 deletions(-)

[PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[Stefano Stabellini]

Remove max_nr_grant_frames, both variable and preprocessor symbol.
Introduce two global variables max_nr_dom0_grant_frames and
max_nr_domU_grant_frames, separately configurable via the Xen command line
option "gnttab_max_nr_frames".

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 docs/misc/xen-command-line.markdown |    4 +-
 xen/arch/arm/domain.c               |    2 +-
 xen/arch/arm/mm.c                   |    2 +-
 xen/arch/x86/mm.c                   |    2 +-
 xen/common/compat/grant_table.c     |    8 ++--
 xen/common/grant_table.c            |   72 +++++++++++++++++++++--------------
 xen/include/asm-arm/grant_table.h   |    2 +-
 xen/include/xen/grant_table.h       |    6 +--
 8 files changed, 58 insertions(+), 40 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 389701a..fa10f6c 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -609,7 +609,9 @@ does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
 Specify the serial parameters for the GDB stub.
 
 ### gnttab\_max\_nr\_frames
-> `= <integer>`
+> `= [<domU number>][,<dom0 number>]`
+
+> Default: `32,32`
 
 Specify the maximum number of frames per grant table operation.
 
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 2b53931..8d60472 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -409,7 +409,7 @@ struct domain *alloc_domain_struct(void)
         return NULL;
 
     clear_page(d);
-    d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_nr_grant_frames);
+    d->arch.grant_table_gpfn = xzalloc_array(xen_pfn_t, max_nr_domU_grant_frames);
     return d;
 }
 
diff --git a/xen/arch/arm/mm.c b/xen/arch/arm/mm.c
index c5b48ef..b65c725 100644
--- a/xen/arch/arm/mm.c
+++ b/xen/arch/arm/mm.c
@@ -1040,7 +1040,7 @@ int xenmem_add_to_physmap_one(
         else
         {
             if ( (idx >= nr_grant_frames(d->grant_table)) &&
-                    (idx < max_nr_grant_frames) )
+                    (idx < max_nr_grant_frames(d)) )
                 gnttab_grow_table(d, idx + 1);
 
             if ( idx < nr_grant_frames(d->grant_table) )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 5b3f06f..f72b23f 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -4539,7 +4539,7 @@ int xenmem_add_to_physmap_one(
             else
             {
                 if ( (idx >= nr_grant_frames(d->grant_table)) &&
-                     (idx < max_nr_grant_frames) )
+                     (idx < max_nr_grant_frames(d)) )
                     gnttab_grow_table(d, idx + 1);
 
                 if ( idx < nr_grant_frames(d->grant_table) )
diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 7ebbbc1..6c00b09 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -146,11 +146,11 @@ int compat_grant_table_op(unsigned int cmd,
                 unsigned int max_frame_list_size_in_page =
                     (COMPAT_ARG_XLAT_SIZE - sizeof(*nat.setup)) /
                     sizeof(*nat.setup->frame_list.p);
-                if ( max_frame_list_size_in_page < max_nr_grant_frames )
+                if ( max_frame_list_size_in_page < max_nr_grant_frames(current->domain) )
                 {
                     gdprintk(XENLOG_WARNING,
                              "max_nr_grant_frames is too large (%u,%u)\n",
-                             max_nr_grant_frames, max_frame_list_size_in_page);
+                             max_nr_grant_frames(current->domain), max_frame_list_size_in_page);
                     rc = -EINVAL;
                 }
                 else
@@ -284,11 +284,11 @@ int compat_grant_table_op(unsigned int cmd,
                 break;
             }
             if ( max_frame_list_size_in_pages <
-                 grant_to_status_frames(max_nr_grant_frames) )
+                 grant_to_status_frames(max_nr_grant_frames(current->domain)) )
             {
                 gdprintk(XENLOG_WARNING,
                          "grant_to_status_frames(max_nr_grant_frames) is too large (%u,%u)\n",
-                         grant_to_status_frames(max_nr_grant_frames),
+                         grant_to_status_frames(max_nr_grant_frames(current->domain)),
                          max_frame_list_size_in_pages);
                 rc = -EINVAL;
                 break;
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 23266c3..a56a1bb 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -24,6 +24,7 @@
  * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
  */
 
+#include <xen/ctype.h>
 #include <xen/err.h>
 #include <xen/iocap.h>
 #include <xen/lib.h>
@@ -40,10 +41,25 @@
 #include <xsm/xsm.h>
 #include <asm/flushtlb.h>
 
-#ifndef max_nr_grant_frames
-unsigned int max_nr_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
-integer_param("gnttab_max_nr_frames", max_nr_grant_frames);
-#endif
+unsigned int max_nr_dom0_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+unsigned int max_nr_domU_grant_frames = DEFAULT_MAX_NR_GRANT_FRAMES;
+
+unsigned int max_nr_grant_frames(struct domain *d)
+{
+    if ( is_hardware_domain(d) )
+        return max_nr_dom0_grant_frames;
+    else
+        return max_nr_domU_grant_frames;
+}
+
+static void __init parse_gnttab_max_nr_frames(const char *s)
+{
+    if ( isdigit(*s) )
+        max_nr_domU_grant_frames = simple_strtoul(s, &s, 0);
+    if ( *s == ',' && isdigit(*++s) )
+        max_nr_dom0_grant_frames = simple_strtoul(s, &s, 0);
+}
+custom_param("gnttab_max_nr_frames", parse_gnttab_max_nr_frames);
 
 /* The maximum number of grant mappings is defined as a multiplier of the
  * maximum number of grant table entries. This defines the multiplier used.
@@ -102,9 +118,9 @@ nr_maptrack_frames(struct grant_table *t)
     return t->maptrack_limit / MAPTRACK_PER_PAGE;
 }
 
-static unsigned inline int max_nr_maptrack_frames(void)
+static unsigned inline int max_nr_maptrack_frames(struct domain *d)
 {
-    return (max_nr_grant_frames * MAX_MAPTRACK_TO_GRANTS_RATIO);
+    return (max_nr_grant_frames(d) * MAX_MAPTRACK_TO_GRANTS_RATIO);
 }
 
 #define MAPTRACK_TAIL (~0u)
@@ -163,8 +179,8 @@ num_act_frames_from_sha_frames(const unsigned int num)
     return (num_sha_entries + (ACGNT_PER_PAGE - 1)) / ACGNT_PER_PAGE;
 }
 
-#define max_nr_active_grant_frames \
-    num_act_frames_from_sha_frames(max_nr_grant_frames)
+#define max_nr_active_grant_frames(d) \
+    num_act_frames_from_sha_frames(max_nr_grant_frames(d))
 
 static inline unsigned int
 nr_active_grant_frames(struct grant_table *gt)
@@ -259,19 +275,20 @@ put_maptrack_handle(
 
 static inline int
 get_maptrack_handle(
-    struct grant_table *lgt)
+    struct domain *ld)
 {
     int                   i;
     grant_handle_t        handle;
     struct grant_mapping *new_mt;
     unsigned int          new_mt_limit, nr_frames;
+    struct grant_table *lgt = ld->grant_table;
 
     spin_lock(&lgt->lock);
 
     while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
     {
         nr_frames = nr_maptrack_frames(lgt);
-        if ( nr_frames >= max_nr_maptrack_frames() )
+        if ( nr_frames >= max_nr_maptrack_frames(ld) )
             break;
 
         new_mt = alloc_xenheap_page();
@@ -567,7 +584,7 @@ __gnttab_map_grant_ref(
     }
 
     lgt = ld->grant_table;
-    if ( unlikely((handle = get_maptrack_handle(lgt)) == -1) )
+    if ( unlikely((handle = get_maptrack_handle(ld)) == -1) )
     {
         rcu_unlock_domain(rd);
         gdprintk(XENLOG_INFO, "Failed to obtain maptrack handle.\n");
@@ -1265,7 +1282,7 @@ gnttab_grow_table(struct domain *d, unsigned int req_nr_frames)
     struct grant_table *gt = d->grant_table;
     unsigned int i;
 
-    ASSERT(req_nr_frames <= max_nr_grant_frames);
+    ASSERT(req_nr_frames <= max_nr_grant_frames(d));
 
     gdprintk(XENLOG_INFO,
             "Expanding dom (%d) grant table from (%d) to (%d) frames.\n",
@@ -1338,15 +1355,6 @@ gnttab_setup_table(
         return -EFAULT;
     }
 
-    if ( unlikely(op.nr_frames > max_nr_grant_frames) )
-    {
-        gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table frames"
-                " per domain.\n",
-                max_nr_grant_frames);
-        op.status = GNTST_general_error;
-        goto out1;
-    }
-
     if ( !guest_handle_okay(op.frame_list, op.nr_frames) )
         return -EFAULT;
 
@@ -1358,6 +1366,15 @@ gnttab_setup_table(
         goto out2;
     }
 
+    if ( unlikely(op.nr_frames > max_nr_grant_frames(d)) )
+    {
+        gdprintk(XENLOG_INFO, "Xen only supports up to %d grant-table frames"
+                " per domain.\n",
+                max_nr_grant_frames(d));
+        op.status = GNTST_general_error;
+        goto out2;
+    }
+
     if ( xsm_grant_setup(XSM_TARGET, current->domain, d) )
     {
         op.status = GNTST_permission_denied;
@@ -1377,7 +1394,7 @@ gnttab_setup_table(
     {
         gdprintk(XENLOG_INFO,
                  "Expand grant table to %u failed. Current: %u Max: %u\n",
-                 op.nr_frames, nr_grant_frames(gt), max_nr_grant_frames);
+                 op.nr_frames, nr_grant_frames(gt), max_nr_grant_frames(d));
         op.status = GNTST_general_error;
         goto out3;
     }
@@ -1396,7 +1413,6 @@ gnttab_setup_table(
     spin_unlock(&gt->lock);
  out2:
     rcu_unlock_domain(d);
- out1:
     if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
         return -EFAULT;
 
@@ -1438,7 +1454,7 @@ gnttab_query_size(
     spin_lock(&d->grant_table->lock);
 
     op.nr_frames     = nr_grant_frames(d->grant_table);
-    op.max_nr_frames = max_nr_grant_frames;
+    op.max_nr_frames = max_nr_grant_frames(d);
     op.status        = GNTST_okay;
 
     spin_unlock(&d->grant_table->lock);
@@ -2647,7 +2663,7 @@ grant_table_create(
 
     /* Active grant table. */
     if ( (t->active = xzalloc_array(struct active_grant_entry *,
-                                    max_nr_active_grant_frames)) == NULL )
+                                    max_nr_active_grant_frames(d))) == NULL )
         goto no_mem_1;
     for ( i = 0;
           i < num_act_frames_from_sha_frames(INITIAL_NR_GRANT_FRAMES); i++ )
@@ -2659,7 +2675,7 @@ grant_table_create(
 
     /* Tracking of mapped foreign frames table */
     if ( (t->maptrack = xzalloc_array(struct grant_mapping *,
-                                      max_nr_maptrack_frames())) == NULL )
+                                      max_nr_maptrack_frames(d))) == NULL )
         goto no_mem_2;
     if ( (t->maptrack[0] = alloc_xenheap_page()) == NULL )
         goto no_mem_3;
@@ -2670,7 +2686,7 @@ grant_table_create(
     t->maptrack[0][i - 1].ref = MAPTRACK_TAIL;
 
     /* Shared grant table. */
-    if ( (t->shared_raw = xzalloc_array(void *, max_nr_grant_frames)) == NULL )
+    if ( (t->shared_raw = xzalloc_array(void *, max_nr_grant_frames(d))) == NULL )
         goto no_mem_3;
     for ( i = 0; i < INITIAL_NR_GRANT_FRAMES; i++ )
     {
@@ -2681,7 +2697,7 @@ grant_table_create(
     
     /* Status pages for grant table - for version 2 */
     t->status = xzalloc_array(grant_status_t *,
-                              grant_to_status_frames(max_nr_grant_frames));
+                              grant_to_status_frames(max_nr_grant_frames(d)));
     if ( t->status == NULL )
         goto no_mem_4;
 
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index 47147ce..e81c973 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -31,7 +31,7 @@ static inline int replace_grant_supported(void)
 
 #define gnttab_shared_gmfn(d, t, i)                                      \
     ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
-     (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
+     (i < max_nr_grant_frames(d))) ? 0 : (d->arch.grant_table_gpfn[i]))
 
 #define gnttab_need_iommu_mapping(d)           (is_domain_direct_mapped(d))
 
diff --git a/xen/include/xen/grant_table.h b/xen/include/xen/grant_table.h
index 5941191..7269d90 100644
--- a/xen/include/xen/grant_table.h
+++ b/xen/include/xen/grant_table.h
@@ -49,10 +49,10 @@
 /* Default maximum size of a grant table. [POLICY] */
 #define DEFAULT_MAX_NR_GRANT_FRAMES   32
 #endif
-#ifndef max_nr_grant_frames /* to allow arch to override */
 /* The maximum size of a grant table. */
-extern unsigned int max_nr_grant_frames;
-#endif
+extern unsigned int max_nr_dom0_grant_frames;
+extern unsigned int max_nr_domU_grant_frames;
+unsigned int max_nr_grant_frames(struct domain *d);
 
 /*
  * Tracks a mapping of another domain's grant reference. Each domain has a
-- 
1.7.10.4

Re: [PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[David Vrabel]

On 08/10/14 14:00, Stefano Stabellini wrote:
> Remove max_nr_grant_frames, both variable and preprocessor symbol.
> Introduce two global variables max_nr_dom0_grant_frames and
> max_nr_domU_grant_frames, separately configurable via the Xen command line
> option "gnttab_max_nr_frames".

Why?  I can't think of a sensible use case for this and you don't list
one.  dom0 doesn't really use grants so its current number grant frames
is usually 1 so it doesn't seem useful to restrict it differently.

David

Re: [PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[Stefano Stabellini]

On Wed, 8 Oct 2014, David Vrabel wrote:
> On 08/10/14 14:00, Stefano Stabellini wrote:
> > Remove max_nr_grant_frames, both variable and preprocessor symbol.
> > Introduce two global variables max_nr_dom0_grant_frames and
> > max_nr_domU_grant_frames, separately configurable via the Xen command line
> > option "gnttab_max_nr_frames".
> 
> Why?  I can't think of a sensible use case for this and you don't list
> one.  dom0 doesn't really use grants so its current number grant frames
> is usually 1 so it doesn't seem useful to restrict it differently.

See the recent discussion with Jan:

http://marc.info/?l=xen-devel&m=141260825209210

Re: [PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[David Vrabel]

On 08/10/14 17:01, Stefano Stabellini wrote:
> On Wed, 8 Oct 2014, David Vrabel wrote:
>> On 08/10/14 14:00, Stefano Stabellini wrote:
>>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
>>> Introduce two global variables max_nr_dom0_grant_frames and
>>> max_nr_domU_grant_frames, separately configurable via the Xen command line
>>> option "gnttab_max_nr_frames".
>>
>> Why?  I can't think of a sensible use case for this and you don't list
>> one.  dom0 doesn't really use grants so its current number grant frames
>> is usually 1 so it doesn't seem useful to restrict it differently.
> 
> See the recent discussion with Jan:
> 
> http://marc.info/?l=xen-devel&m=141260825209210

I still don't see a use case.  Either:

a) dom0 is the driver domain and doesn't need a large number of grants.

or

b) dom0 has a frontend driver, then the driver domain will have to scan
dom0's grant table thus dom0's grant table must be sized the same as domUs.

David

Re: [PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[Stefano Stabellini]

On Wed, 8 Oct 2014, David Vrabel wrote:
> On 08/10/14 17:01, Stefano Stabellini wrote:
> > On Wed, 8 Oct 2014, David Vrabel wrote:
> >> On 08/10/14 14:00, Stefano Stabellini wrote:
> >>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
> >>> Introduce two global variables max_nr_dom0_grant_frames and
> >>> max_nr_domU_grant_frames, separately configurable via the Xen command line
> >>> option "gnttab_max_nr_frames".
> >>
> >> Why?  I can't think of a sensible use case for this and you don't list
> >> one.  dom0 doesn't really use grants so its current number grant frames
> >> is usually 1 so it doesn't seem useful to restrict it differently.
> > 
> > See the recent discussion with Jan:
> > 
> > http://marc.info/?l=xen-devel&m=141260825209210
> 
> I still don't see a use case.  Either:
> 
> a) dom0 is the driver domain and doesn't need a large number of grants.
> 
> or
> 
> b) dom0 has a frontend driver, then the driver domain will have to scan
> dom0's grant table thus dom0's grant table must be sized the same as domUs.

I don't have an opinion on this, so I'll leave it up to you and Jan to
come to an agreement.

Re: [PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[David Vrabel]

On 08/10/14 17:59, Stefano Stabellini wrote:
> On Wed, 8 Oct 2014, David Vrabel wrote:
>> On 08/10/14 17:01, Stefano Stabellini wrote:
>>> On Wed, 8 Oct 2014, David Vrabel wrote:
>>>> On 08/10/14 14:00, Stefano Stabellini wrote:
>>>>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
>>>>> Introduce two global variables max_nr_dom0_grant_frames and
>>>>> max_nr_domU_grant_frames, separately configurable via the Xen command line
>>>>> option "gnttab_max_nr_frames".
>>>>
>>>> Why?  I can't think of a sensible use case for this and you don't list
>>>> one.  dom0 doesn't really use grants so its current number grant frames
>>>> is usually 1 so it doesn't seem useful to restrict it differently.
>>>
>>> See the recent discussion with Jan:
>>>
>>> http://marc.info/?l=xen-devel&m=141260825209210
>>
>> I still don't see a use case.  Either:
>>
>> a) dom0 is the driver domain and doesn't need a large number of grants.
>>
>> or
>>
>> b) dom0 has a frontend driver, then the driver domain will have to scan
>> dom0's grant table thus dom0's grant table must be sized the same as domUs.
> 
> I don't have an opinion on this, so I'll leave it up to you and Jan to
> come to an agreement.

I don't feel strongly on this so I'll defer to Jan.

David

Re: [PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[Jan Beulich]

>>> On 08.10.14 at 18:42, <david.vrabel@citrix.com> wrote:
> On 08/10/14 17:01, Stefano Stabellini wrote:
>> On Wed, 8 Oct 2014, David Vrabel wrote:
>>> On 08/10/14 14:00, Stefano Stabellini wrote:
>>>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
>>>> Introduce two global variables max_nr_dom0_grant_frames and
>>>> max_nr_domU_grant_frames, separately configurable via the Xen command line
>>>> option "gnttab_max_nr_frames".
>>>
>>> Why?  I can't think of a sensible use case for this and you don't list
>>> one.  dom0 doesn't really use grants so its current number grant frames
>>> is usually 1 so it doesn't seem useful to restrict it differently.
>> 
>> See the recent discussion with Jan:
>> 
>> http://marc.info/?l=xen-devel&m=141260825209210 
> 
> I still don't see a use case.  Either:
> 
> a) dom0 is the driver domain and doesn't need a large number of grants.
> 
> or
> 
> b) dom0 has a frontend driver, then the driver domain will have to scan
> dom0's grant table thus dom0's grant table must be sized the same as domUs.

Looks like you missed that max_nr_maptrack_frames(), which I
think you agree is needed in Dom0, also depends on
max_nr_grant_frames. Question then of course is whether the
distinction should be made at that: Allow controlling grant frames
and maptrack frames separately, rather than Dom0 and DomU.

Jan

Re: [PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[David Vrabel]

On 10/10/14 08:35, Jan Beulich wrote:
>>>> On 08.10.14 at 18:42, <david.vrabel@citrix.com> wrote:
>> On 08/10/14 17:01, Stefano Stabellini wrote:
>>> On Wed, 8 Oct 2014, David Vrabel wrote:
>>>> On 08/10/14 14:00, Stefano Stabellini wrote:
>>>>> Remove max_nr_grant_frames, both variable and preprocessor symbol.
>>>>> Introduce two global variables max_nr_dom0_grant_frames and
>>>>> max_nr_domU_grant_frames, separately configurable via the Xen command line
>>>>> option "gnttab_max_nr_frames".
>>>>
>>>> Why?  I can't think of a sensible use case for this and you don't list
>>>> one.  dom0 doesn't really use grants so its current number grant frames
>>>> is usually 1 so it doesn't seem useful to restrict it differently.
>>>
>>> See the recent discussion with Jan:
>>>
>>> http://marc.info/?l=xen-devel&m=141260825209210 
>>
>> I still don't see a use case.  Either:
>>
>> a) dom0 is the driver domain and doesn't need a large number of grants.
>>
>> or
>>
>> b) dom0 has a frontend driver, then the driver domain will have to scan
>> dom0's grant table thus dom0's grant table must be sized the same as domUs.
> 
> Looks like you missed that max_nr_maptrack_frames(), which I
> think you agree is needed in Dom0, also depends on
> max_nr_grant_frames.

Agreed.

> Question then of course is whether the
> distinction should be made at that: Allow controlling grant frames
> and maptrack frames separately, rather than Dom0 and DomU.

This would seem sensible.

David

Re: [PATCH v3 1/5] xen: introduce two different max_nr_dom0/domU_grant_frames parameters

[Jan Beulich]

>>> On 08.10.14 at 15:00, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -609,7 +609,9 @@ does not provide VM\_ENTRY\_LOAD\_GUEST\_PAT.
>  Specify the serial parameters for the GDB stub.
>  
>  ### gnttab\_max\_nr\_frames
> -> `= <integer>`
> +> `= [<domU number>][,<dom0 number>]`
> +
> +> Default: `32,32`

If we stay with this, I think the two parts need to be reversed:
Needing to override the Dom0 value is imo more likely, and hence
cases where the command line options is already getting used
should result in Dom0 behavior being unchanged.

Jan

[PATCH v3 2/5] xen/arm: introduce invalidate_xen_dcache_va_range

[Stefano Stabellini]

Take care of handling non-cacheline aligned addresses and sizes.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Reviewed-by: Julien Grall <julien.grall@linaro.org>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/include/asm-arm/arm32/page.h |    3 +++
 xen/include/asm-arm/arm64/page.h |    3 +++
 xen/include/asm-arm/page.h       |   30 ++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/xen/include/asm-arm/arm32/page.h b/xen/include/asm-arm/arm32/page.h
index 9740672..6fb2e68 100644
--- a/xen/include/asm-arm/arm32/page.h
+++ b/xen/include/asm-arm/arm32/page.h
@@ -19,6 +19,9 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
         : : "r" (pte.bits), "r" (p) : "memory");
 }
 
+/* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */
+#define __invalidate_xen_dcache_one(R) STORE_CP32(R, DCIMVAC)
+
 /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
 #define __clean_xen_dcache_one(R) STORE_CP32(R, DCCMVAC)
 
diff --git a/xen/include/asm-arm/arm64/page.h b/xen/include/asm-arm/arm64/page.h
index bb10164..f181b1b 100644
--- a/xen/include/asm-arm/arm64/page.h
+++ b/xen/include/asm-arm/arm64/page.h
@@ -14,6 +14,9 @@ static inline void write_pte(lpae_t *p, lpae_t pte)
         : : "r" (pte.bits), "r" (p) : "memory");
 }
 
+/* Inline ASM to invalidate dcache on register R (may be an inline asm operand) */
+#define __invalidate_xen_dcache_one(R) "dc ivac, %" #R ";"
+
 /* Inline ASM to flush dcache on register R (may be an inline asm operand) */
 #define __clean_xen_dcache_one(R) "dc cvac, %" #R ";"
 
diff --git a/xen/include/asm-arm/page.h b/xen/include/asm-arm/page.h
index d758b61..da02e97 100644
--- a/xen/include/asm-arm/page.h
+++ b/xen/include/asm-arm/page.h
@@ -268,6 +268,36 @@ extern size_t cacheline_bytes;
 /* Functions for flushing medium-sized areas.
  * if 'range' is large enough we might want to use model-specific
  * full-cache flushes. */
+
+static inline void invalidate_xen_dcache_va_range(const void *p, unsigned long size)
+{
+    size_t off;
+    const void *end = p + size;
+
+    dsb(sy);           /* So the CPU issues all writes to the range */
+
+    off = (unsigned long)p % cacheline_bytes;
+    if ( off )
+    {
+        p -= off;
+        asm volatile (__clean_and_invalidate_xen_dcache_one(0) : : "r" (p));
+        p += cacheline_bytes;
+        size -= cacheline_bytes - off;
+    }
+    off = (unsigned long)end % cacheline_bytes;
+    if ( off )
+    {
+        end -= off;
+        size -= off;
+        asm volatile (__clean_and_invalidate_xen_dcache_one(0) : : "r" (end));
+    }
+
+    for ( ; p < end; p += cacheline_bytes )
+        asm volatile (__invalidate_xen_dcache_one(0) : : "r" (p));
+
+    dsb(sy);           /* So we know the flushes happen before continuing */
+}
+
 static inline void clean_xen_dcache_va_range(const void *p, unsigned long size)
 {
     const void *end;
-- 
1.7.10.4

[PATCH v3 3/5] xen/x86: introduce more cache maintenance operations

[Stefano Stabellini]

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 xen/include/asm-x86/page.h |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h
index 9aa780e..9103d8e 100644
--- a/xen/include/asm-x86/page.h
+++ b/xen/include/asm-x86/page.h
@@ -346,6 +346,10 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
 
 /* No cache maintenance required on x86 architecture. */
 static inline void flush_page_to_ram(unsigned long mfn) {}
+static inline void clean_xen_dcache_va_range(const void *p, unsigned long size) {}
+static inline void invalidate_xen_dcache_va_range(const void *p, unsigned long size) {}
+static inline void clean_and_invalidate_xen_dcache_va_range
+    (const void *p, unsigned long size) {}
 
 /* return true if permission increased */
 static inline bool_t
-- 
1.7.10.4

Re: [PATCH v3 3/5] xen/x86: introduce more cache maintenance operations

[Jan Beulich]

>>> On 08.10.14 at 15:00, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/include/asm-x86/page.h
> +++ b/xen/include/asm-x86/page.h
> @@ -346,6 +346,10 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
>  
>  /* No cache maintenance required on x86 architecture. */
>  static inline void flush_page_to_ram(unsigned long mfn) {}
> +static inline void clean_xen_dcache_va_range(const void *p, unsigned long size) {}
> +static inline void invalidate_xen_dcache_va_range(const void *p, unsigned long size) {}
> +static inline void clean_and_invalidate_xen_dcache_va_range
> +    (const void *p, unsigned long size) {}

Mind explaining what purpose the "_xen" in these names serves?
Also, is simply stubbing these out correct? I.e. there are caches on
x86, and those can both be invalidated and cleaned, so why would
you not do so here?

Jan

Re: [PATCH v3 3/5] xen/x86: introduce more cache maintenance operations

[Stefano Stabellini]

On Fri, 10 Oct 2014, Jan Beulich wrote:
> >>> On 08.10.14 at 15:00, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/include/asm-x86/page.h
> > +++ b/xen/include/asm-x86/page.h
> > @@ -346,6 +346,10 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t cacheattr)
> >  
> >  /* No cache maintenance required on x86 architecture. */
> >  static inline void flush_page_to_ram(unsigned long mfn) {}
> > +static inline void clean_xen_dcache_va_range(const void *p, unsigned long size) {}
> > +static inline void invalidate_xen_dcache_va_range(const void *p, unsigned long size) {}
> > +static inline void clean_and_invalidate_xen_dcache_va_range
> > +    (const void *p, unsigned long size) {}
> 
> Mind explaining what purpose the "_xen" in these names serves?

It means a Xen virtual address (as opposed to a guest p2m address or a
guest virtual address), but I guess it is pretty obvious so I should
remove it?


> Also, is simply stubbing these out correct? I.e. there are caches on
> x86, and those can both be invalidated and cleaned, so why would
> you not do so here?

Good point. The problem is that to be sure that I am doing the right
thing I would need to know what kind of flushes are required with a
non-coherent device on x86, but I don't have any examples at hand.

>From the amd64 manual, I would implement invalidate with CLFLUSH, clean
and invalidate with WBINVD (even though it operates on the entire cache
so it is very heavyweight) and I would have no way to implement just
"clean". Do you agree?

Re: [PATCH v3 3/5] xen/x86: introduce more cache maintenance operations

[Jan Beulich]

>>> On 10.10.14 at 11:23, <stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 10 Oct 2014, Jan Beulich wrote:
>> >>> On 08.10.14 at 15:00, <stefano.stabellini@eu.citrix.com> wrote:
>> > --- a/xen/include/asm-x86/page.h
>> > +++ b/xen/include/asm-x86/page.h
>> > @@ -346,6 +346,10 @@ static inline uint32_t cacheattr_to_pte_flags(uint32_t 
> cacheattr)
>> >  
>> >  /* No cache maintenance required on x86 architecture. */
>> >  static inline void flush_page_to_ram(unsigned long mfn) {}
>> > +static inline void clean_xen_dcache_va_range(const void *p, unsigned long 
> size) {}
>> > +static inline void invalidate_xen_dcache_va_range(const void *p, unsigned 
> long size) {}
>> > +static inline void clean_and_invalidate_xen_dcache_va_range
>> > +    (const void *p, unsigned long size) {}
>> 
>> Mind explaining what purpose the "_xen" in these names serves?
> 
> It means a Xen virtual address (as opposed to a guest p2m address or a
> guest virtual address), but I guess it is pretty obvious so I should
> remove it?

Yes please.

>> Also, is simply stubbing these out correct? I.e. there are caches on
>> x86, and those can both be invalidated and cleaned, so why would
>> you not do so here?
> 
> Good point. The problem is that to be sure that I am doing the right
> thing I would need to know what kind of flushes are required with a
> non-coherent device on x86, but I don't have any examples at hand.
> 
> From the amd64 manual, I would implement invalidate with CLFLUSH, clean
> and invalidate with WBINVD (even though it operates on the entire cache
> so it is very heavyweight) and I would have no way to implement just
> "clean". Do you agree?

Except that "clean-without-invalidate" can fall back to "clean-and-
invalidate" just fine. The only thing you truly can't implement is
invalidate-without-clean, since INVD is global, yet you would want
range-restricted invalidation only. And no, CLFLUSH and WBINVD
do the same thing (address restricted vs globally).

Also x86 already has all of the logic in place, you just need to
call it (see xen/*/*x86/flushtlb.*). 

Jan

[PATCH v3 4/5] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

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.

Introduce grant_map_exists: an internal grant table function to check
whether an mfn has been granted to a given domain on a target grant
table.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v3:
- reduce the time the grant_table lock is held;
- fix warning message;
- s/EFAULT/EPERM;
- s/llx/PRIx64;
- check offset and size independetly before checking their sum;
- rcu_lock_current_domain cannot fail;
- s/ENOSYS/EOPNOTSUPP;
- use clean_and_invalidate_xen_dcache_va_range to do both operations at
once;
- fold grant_map_exists in this patch;
- support "count" argument;
- make correspondent changes to compat/grant_table.c;
- introduce GNTTAB_CACHE_SOURCE_GREF to select the type of input in the
union;
- rename size field to length;
- make length and offset uint16_t;
- only take spin_lock if d != owner.

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/compat/grant_table.c  |    8 +++
 xen/common/grant_table.c         |  135 ++++++++++++++++++++++++++++++++++++++
 xen/include/public/grant_table.h |   20 ++++++
 xen/include/xlat.lst             |    1 +
 4 files changed, 164 insertions(+)

diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
index 6c00b09..4a3a23d 100644
--- a/xen/common/compat/grant_table.c
+++ b/xen/common/compat/grant_table.c
@@ -51,6 +51,10 @@ CHECK_gnttab_get_version;
 CHECK_gnttab_swap_grant_ref;
 #undef xen_gnttab_swap_grant_ref
 
+#define xen_gnttab_cache_flush gnttab_cache_flush
+CHECK_gnttab_cache_flush;
+#undef xen_gnttab_cache_flush
+
 int compat_grant_table_op(unsigned int cmd,
                           XEN_GUEST_HANDLE_PARAM(void) cmp_uop,
                           unsigned int count)
@@ -106,6 +110,10 @@ int compat_grant_table_op(unsigned int cmd,
     CASE(swap_grant_ref);
 #endif
 
+#ifndef CHECK_gnttab_cache_flush
+    CASE(cache_flush);
+#endif
+
 #undef CASE
     default:
         return do_grant_table_op(cmd, cmp_uop, count);
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index a56a1bb..c16c11c 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -501,6 +501,36 @@ static int _set_status(unsigned gt_version,
         return _set_status_v2(domid, readonly, mapflag, shah, act, status);
 }
 
+static bool_t grant_map_exists(const struct domain *ld,
+                        struct grant_table *rgt,
+                        unsigned long mfn)
+{
+    const struct active_grant_entry *act;
+    grant_ref_t ref;
+    bool_t ret = 0;
+
+    spin_is_locked(&rgt->lock);
+
+    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
+    {
+        act = &active_entry(rgt, ref);
+
+        if ( !act->pin )
+            continue;
+
+        if ( act->domid != ld->domain_id )
+            continue;
+
+        if ( act->frame != mfn )
+            continue;
+
+        ret = 1;
+        break;
+    }
+
+    return ret;
+}
+
 static void mapcount(
     struct grant_table *lgt, struct domain *rd, unsigned long mfn,
     unsigned int *wrc, unsigned int *rdc)
@@ -2498,6 +2528,97 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
     return 0;
 }
 
+static int __gnttab_cache_flush(gnttab_cache_flush_t cflush)
+{
+    struct domain *d, *owner;
+    struct page_info *page;
+    uint64_t mfn;
+    void *v;
+
+    if ( cflush.offset > PAGE_SIZE ||
+            cflush.length > PAGE_SIZE ||
+            cflush.offset + cflush.length > PAGE_SIZE )
+        return -EINVAL;
+
+    if ( cflush.length == 0 || cflush.op == 0 )
+        return 0;
+
+    /* currently unimplemented */
+    if ( cflush.op & GNTTAB_CACHE_SOURCE_GREF )
+        return -EOPNOTSUPP;
+
+    d = rcu_lock_current_domain();
+    mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT;
+
+    if ( !mfn_valid(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 -EPERM;
+    }
+
+    if ( d != owner )
+    {
+        spin_lock(&owner->grant_table->lock);
+
+        if ( !grant_map_exists(d, owner->grant_table, mfn) )
+        {
+            spin_unlock(&owner->grant_table->lock);
+            rcu_unlock_domain(d);
+            put_page(page);
+            gdprintk(XENLOG_G_ERR, "mfn %"PRIx64" hasn't been granted by d%d to d%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) && (cflush.op & GNTTAB_CACHE_CLEAN) )
+        clean_and_invalidate_xen_dcache_va_range(v, cflush.length);
+    else if ( cflush.op & GNTTAB_CACHE_INVAL )
+        invalidate_xen_dcache_va_range(v, cflush.length);
+    else if ( cflush.op & GNTTAB_CACHE_CLEAN )
+        clean_xen_dcache_va_range(v, cflush.length);
+
+    if ( d != owner )
+        spin_unlock(&owner->grant_table->lock);
+    unmap_domain_page(v);
+    put_page(page);
+
+    return 0;
+}
+
+static long
+gnttab_cache_flush(XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) uop,
+                      unsigned int count)
+{
+    int i, ret;
+    gnttab_cache_flush_t op;
+
+    for ( i = 0; i < count; i++ )
+    {
+        if ( i && hypercall_preempt_check() )
+            return i;
+        if ( unlikely(__copy_from_guest(&op, uop, 1)) )
+            return -EFAULT;
+        ret = __gnttab_cache_flush(op);
+        if ( ret < 0 )
+            return -ret;
+        guest_handle_add_offset(uop, 1);
+    }
+    return 0;
+}
+
+
 long
 do_grant_table_op(
     unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
@@ -2627,6 +2748,20 @@ do_grant_table_op(
         }
         break;
     }
+    case GNTTABOP_cache_flush:
+    {
+        XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
+            guest_handle_cast(uop, gnttab_cache_flush_t);
+        if ( unlikely(!guest_handle_okay(cflush, count)) )
+            goto out;
+        rc = gnttab_cache_flush(cflush, count);
+        if ( rc > 0 )
+        {
+            guest_handle_add_offset(cflush, rc);
+            uop = guest_handle_cast(cflush, void);
+        }
+        break;
+    }
     default:
         rc = -ENOSYS;
         break;
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index b8a3d6c..20d4e77 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,25 @@ 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;
+    uint16_t offset; /* offset from start of grant */
+    uint16_t length; /* size within the grant */
+#define GNTTAB_CACHE_CLEAN          (1<<0)
+#define GNTTAB_CACHE_INVAL          (1<<1)
+#define GNTTAB_CACHE_SOURCE_GREF    (1<<31)
+    uint32_t op;
+};
+typedef struct gnttab_cache_flush gnttab_cache_flush_t;
+DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
+
 #endif /* __XEN_INTERFACE_VERSION__ */
 
 /*
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index 9a35dd7..3822b00 100644
--- a/xen/include/xlat.lst
+++ b/xen/include/xlat.lst
@@ -51,6 +51,7 @@
 ?       grant_entry_header              grant_table.h
 ?	grant_entry_v2			grant_table.h
 ?	gnttab_swap_grant_ref		grant_table.h
+?	gnttab_cache_flush		grant_table.h
 ?	kexec_exec			kexec.h
 !	kexec_image			kexec.h
 !	kexec_range			kexec.h
-- 
1.7.10.4

Re: [PATCH v3 4/5] xen/arm: introduce GNTTABOP_cache_flush

[Andrew Cooper]

On 08/10/14 14:00, 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.
>
> Introduce grant_map_exists: an internal grant table function to check
> whether an mfn has been granted to a given domain on a target grant
> table.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> ---
>
> Changes in v3:
> - reduce the time the grant_table lock is held;
> - fix warning message;
> - s/EFAULT/EPERM;
> - s/llx/PRIx64;
> - check offset and size independetly before checking their sum;
> - rcu_lock_current_domain cannot fail;
> - s/ENOSYS/EOPNOTSUPP;
> - use clean_and_invalidate_xen_dcache_va_range to do both operations at
> once;
> - fold grant_map_exists in this patch;
> - support "count" argument;
> - make correspondent changes to compat/grant_table.c;
> - introduce GNTTAB_CACHE_SOURCE_GREF to select the type of input in the
> union;
> - rename size field to length;
> - make length and offset uint16_t;
> - only take spin_lock if d != owner.
>
> 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/compat/grant_table.c  |    8 +++
>  xen/common/grant_table.c         |  135 ++++++++++++++++++++++++++++++++++++++
>  xen/include/public/grant_table.h |   20 ++++++
>  xen/include/xlat.lst             |    1 +
>  4 files changed, 164 insertions(+)
>
> diff --git a/xen/common/compat/grant_table.c b/xen/common/compat/grant_table.c
> index 6c00b09..4a3a23d 100644
> --- a/xen/common/compat/grant_table.c
> +++ b/xen/common/compat/grant_table.c
> @@ -51,6 +51,10 @@ CHECK_gnttab_get_version;
>  CHECK_gnttab_swap_grant_ref;
>  #undef xen_gnttab_swap_grant_ref
>  
> +#define xen_gnttab_cache_flush gnttab_cache_flush
> +CHECK_gnttab_cache_flush;
> +#undef xen_gnttab_cache_flush
> +
>  int compat_grant_table_op(unsigned int cmd,
>                            XEN_GUEST_HANDLE_PARAM(void) cmp_uop,
>                            unsigned int count)
> @@ -106,6 +110,10 @@ int compat_grant_table_op(unsigned int cmd,
>      CASE(swap_grant_ref);
>  #endif
>  
> +#ifndef CHECK_gnttab_cache_flush
> +    CASE(cache_flush);
> +#endif
> +
>  #undef CASE
>      default:
>          return do_grant_table_op(cmd, cmp_uop, count);
> diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
> index a56a1bb..c16c11c 100644
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -501,6 +501,36 @@ static int _set_status(unsigned gt_version,
>          return _set_status_v2(domid, readonly, mapflag, shah, act, status);
>  }
>  
> +static bool_t grant_map_exists(const struct domain *ld,
> +                        struct grant_table *rgt,
> +                        unsigned long mfn)
> +{
> +    const struct active_grant_entry *act;
> +    grant_ref_t ref;
> +    bool_t ret = 0;
> +
> +    spin_is_locked(&rgt->lock);

This is now dead code.  spin_is_locked() is just a predicate.

> +
> +    for ( ref = 0; ref != nr_grant_entries(rgt); ref++ )
> +    {
> +        act = &active_entry(rgt, ref);
> +
> +        if ( !act->pin )
> +            continue;
> +
> +        if ( act->domid != ld->domain_id )
> +            continue;
> +
> +        if ( act->frame != mfn )
> +            continue;
> +
> +        ret = 1;
> +        break;

how about return 1 here...

> +    }
> +
> +    return ret;

and return 0 here?

> +}
> +
>  static void mapcount(
>      struct grant_table *lgt, struct domain *rd, unsigned long mfn,
>      unsigned int *wrc, unsigned int *rdc)
> @@ -2498,6 +2528,97 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
>      return 0;
>  }
>  
> +static int __gnttab_cache_flush(gnttab_cache_flush_t cflush)

This should be passed via pointer.

> +{
> +    struct domain *d, *owner;
> +    struct page_info *page;
> +    uint64_t mfn;
> +    void *v;
> +
> +    if ( cflush.offset > PAGE_SIZE ||
> +            cflush.length > PAGE_SIZE ||
> +            cflush.offset + cflush.length > PAGE_SIZE )

Brackets, as per Xen style, please.

> +        return -EINVAL;
> +
> +    if ( cflush.length == 0 || cflush.op == 0 )
> +        return 0;
> +
> +    /* currently unimplemented */
> +    if ( cflush.op & GNTTAB_CACHE_SOURCE_GREF )
> +        return -EOPNOTSUPP;
> +
> +    d = rcu_lock_current_domain();
> +    mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT;
> +
> +    if ( !mfn_valid(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 -EPERM;
> +    }
> +
> +    if ( d != owner )
> +    {
> +        spin_lock(&owner->grant_table->lock);
> +
> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> +        {
> +            spin_unlock(&owner->grant_table->lock);
> +            rcu_unlock_domain(d);
> +            put_page(page);
> +            gdprintk(XENLOG_G_ERR, "mfn %"PRIx64" hasn't been granted by d%d to d%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) && (cflush.op & GNTTAB_CACHE_CLEAN) )
> +        clean_and_invalidate_xen_dcache_va_range(v, cflush.length);
> +    else if ( cflush.op & GNTTAB_CACHE_INVAL )
> +        invalidate_xen_dcache_va_range(v, cflush.length);
> +    else if ( cflush.op & GNTTAB_CACHE_CLEAN )
> +        clean_xen_dcache_va_range(v, cflush.length);
> +
> +    if ( d != owner )
> +        spin_unlock(&owner->grant_table->lock);
> +    unmap_domain_page(v);
> +    put_page(page);
> +
> +    return 0;
> +}
> +
> +static long
> +gnttab_cache_flush(XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) uop,
> +                      unsigned int count)
> +{
> +    int i, ret;
> +    gnttab_cache_flush_t op;
> +
> +    for ( i = 0; i < count; i++ )

i must be unsigned, or this can be an infinite loop, as count in an
unverified user parameter at this point.

~Andrew

> +    {
> +        if ( i && hypercall_preempt_check() )
> +            return i;
> +        if ( unlikely(__copy_from_guest(&op, uop, 1)) )
> +            return -EFAULT;
> +        ret = __gnttab_cache_flush(op);
> +        if ( ret < 0 )
> +            return -ret;
> +        guest_handle_add_offset(uop, 1);
> +    }
> +    return 0;
> +}
> +
> +
>  long
>  do_grant_table_op(
>      unsigned int cmd, XEN_GUEST_HANDLE_PARAM(void) uop, unsigned int count)
> @@ -2627,6 +2748,20 @@ do_grant_table_op(
>          }
>          break;
>      }
> +    case GNTTABOP_cache_flush:
> +    {
> +        XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
> +            guest_handle_cast(uop, gnttab_cache_flush_t);
> +        if ( unlikely(!guest_handle_okay(cflush, count)) )
> +            goto out;
> +        rc = gnttab_cache_flush(cflush, count);
> +        if ( rc > 0 )
> +        {
> +            guest_handle_add_offset(cflush, rc);
> +            uop = guest_handle_cast(cflush, void);
> +        }
> +        break;
> +    }
>      default:
>          rc = -ENOSYS;
>          break;
> diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
> index b8a3d6c..20d4e77 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,25 @@ 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;
> +    uint16_t offset; /* offset from start of grant */
> +    uint16_t length; /* size within the grant */
> +#define GNTTAB_CACHE_CLEAN          (1<<0)
> +#define GNTTAB_CACHE_INVAL          (1<<1)
> +#define GNTTAB_CACHE_SOURCE_GREF    (1<<31)
> +    uint32_t op;
> +};
> +typedef struct gnttab_cache_flush gnttab_cache_flush_t;
> +DEFINE_XEN_GUEST_HANDLE(gnttab_cache_flush_t);
> +
>  #endif /* __XEN_INTERFACE_VERSION__ */
>  
>  /*
> diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
> index 9a35dd7..3822b00 100644
> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -51,6 +51,7 @@
>  ?       grant_entry_header              grant_table.h
>  ?	grant_entry_v2			grant_table.h
>  ?	gnttab_swap_grant_ref		grant_table.h
> +?	gnttab_cache_flush		grant_table.h
>  ?	kexec_exec			kexec.h
>  !	kexec_image			kexec.h
>  !	kexec_range			kexec.h

Re: [PATCH v3 4/5] xen/arm: introduce GNTTABOP_cache_flush

[Jan Beulich]

>>> On 08.10.14 at 15:00, <stefano.stabellini@eu.citrix.com> wrote:
> @@ -2498,6 +2528,97 @@ gnttab_swap_grant_ref(XEN_GUEST_HANDLE_PARAM(gnttab_swap_grant_ref_t) uop,
>      return 0;
>  }
>  
> +static int __gnttab_cache_flush(gnttab_cache_flush_t cflush)
> +{
> +    struct domain *d, *owner;
> +    struct page_info *page;
> +    uint64_t mfn;
> +    void *v;
> +
> +    if ( cflush.offset > PAGE_SIZE ||

>= ?

> +            cflush.length > PAGE_SIZE ||
> +            cflush.offset + cflush.length > PAGE_SIZE )

Indentation.

> +        return -EINVAL;
> +
> +    if ( cflush.length == 0 || cflush.op == 0 )
> +        return 0;
> +
> +    /* currently unimplemented */
> +    if ( cflush.op & GNTTAB_CACHE_SOURCE_GREF )
> +        return -EOPNOTSUPP;
> +
> +    d = rcu_lock_current_domain();
> +    mfn = cflush.a.dev_bus_addr >> PAGE_SHIFT;
> +
> +    if ( !mfn_valid(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 -EPERM;
> +    }
> +
> +    if ( d != owner )
> +    {
> +        spin_lock(&owner->grant_table->lock);
> +
> +        if ( !grant_map_exists(d, owner->grant_table, mfn) )
> +        {
> +            spin_unlock(&owner->grant_table->lock);
> +            rcu_unlock_domain(d);
> +            put_page(page);
> +            gdprintk(XENLOG_G_ERR, "mfn %"PRIx64" hasn't been granted by d%d to d%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) && (cflush.op & GNTTAB_CACHE_CLEAN) )
> +        clean_and_invalidate_xen_dcache_va_range(v, cflush.length);
> +    else if ( cflush.op & GNTTAB_CACHE_INVAL )
> +        invalidate_xen_dcache_va_range(v, cflush.length);
> +    else if ( cflush.op & GNTTAB_CACHE_CLEAN )
> +        clean_xen_dcache_va_range(v, cflush.length);
> +
> +    if ( d != owner )
> +        spin_unlock(&owner->grant_table->lock);
> +    unmap_domain_page(v);
> +    put_page(page);
> +
> +    return 0;
> +}
> +
> +static long
> +gnttab_cache_flush(XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) uop,
> +                      unsigned int count)
> +{
> +    int i, ret;
> +    gnttab_cache_flush_t op;
> +
> +    for ( i = 0; i < count; i++ )
> +    {
> +        if ( i && hypercall_preempt_check() )
> +            return i;
> +        if ( unlikely(__copy_from_guest(&op, uop, 1)) )
> +            return -EFAULT;
> +        ret = __gnttab_cache_flush(op);
> +        if ( ret < 0 )
> +            return -ret;

Why "-ret"? Especially with ...

> @@ -2627,6 +2748,20 @@ do_grant_table_op(
>          }
>          break;
>      }
> +    case GNTTABOP_cache_flush:
> +    {
> +        XEN_GUEST_HANDLE_PARAM(gnttab_cache_flush_t) cflush =
> +            guest_handle_cast(uop, gnttab_cache_flush_t);
> +        if ( unlikely(!guest_handle_okay(cflush, count)) )
> +            goto out;
> +        rc = gnttab_cache_flush(cflush, count);
> +        if ( rc > 0 )
> +        {
> +            guest_handle_add_offset(cflush, rc);
> +            uop = guest_handle_cast(cflush, void);
> +        }
> +        break;
> +    }

... a positive value here meaning "continuation needed".

Jan

[PATCH v3 5/5] Revert "xen/arm: introduce XENFEAT_grant_map_identity"

[Stefano Stabellini]

Just keep the definition of XENFEAT_grant_map_identity.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---

Changes in v2:

- comment out the definition of XENFEAT_grant_map_identity.
---
 xen/common/grant_table.c           |   30 +++++-------------------------
 xen/common/kernel.c                |    2 --
 xen/drivers/passthrough/arm/smmu.c |   33 +++++++++++++++++++++++++++++++++
 xen/include/asm-arm/grant_table.h  |    3 ++-
 xen/include/public/features.h      |    4 +++-
 5 files changed, 43 insertions(+), 29 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c16c11c..dcedf87 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -785,23 +785,13 @@ __gnttab_map_grant_ref(
              !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
         {
             if ( wrc == 0 )
-            {
-                if ( is_domain_direct_mapped(ld) )
-                    err = arch_grant_map_page_identity(ld, frame, 1);
-                else
-                    err = iommu_map_page(ld, frame, frame,
-                            IOMMUF_readable|IOMMUF_writable);
-            }
+                err = iommu_map_page(ld, frame, frame,
+                                     IOMMUF_readable|IOMMUF_writable);
         }
         else if ( act_pin && !old_pin )
         {
             if ( (wrc + rdc) == 0 )
-            {
-                if ( is_domain_direct_mapped(ld) )
-                    err = arch_grant_map_page_identity(ld, frame, 0);
-                else
-                    err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
-            }
+                err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
         }
         if ( err )
         {
@@ -998,19 +988,9 @@ __gnttab_unmap_common(
         int err = 0;
         mapcount(lgt, rd, op->frame, &wrc, &rdc);
         if ( (wrc + rdc) == 0 )
-        {
-            if ( is_domain_direct_mapped(ld) )
-                err = arch_grant_unmap_page_identity(ld, op->frame);
-            else
-                err = iommu_unmap_page(ld, op->frame);
-        }
+            err = iommu_unmap_page(ld, op->frame);
         else if ( wrc == 0 )
-        {
-            if ( is_domain_direct_mapped(ld) )
-                err = arch_grant_map_page_identity(ld, op->frame, 0);
-            else
-                err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
-        }
+            err = iommu_map_page(ld, op->frame, op->frame, IOMMUF_readable);
         if ( err )
         {
             rc = GNTST_general_error;
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index ce65486..d23c422 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -332,8 +332,6 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) arg)
                 break;
             }
 #endif
-            if ( is_domain_direct_mapped(d) )
-                fi.submap |= 1U << XENFEAT_grant_map_identity;
             break;
         default:
             return -EINVAL;
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 3cbd206..9a95ac9 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -1536,6 +1536,37 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
     xfree(smmu_domain);
 }
 
+static int arm_smmu_map_page(struct domain *d, unsigned long gfn,
+                             unsigned long mfn, unsigned int flags)
+{
+    /* Grant mappings can be used for DMA requests. The dev_bus_addr returned by
+     * the hypercall is the MFN (not the IPA). For device protected by
+     * an IOMMU, Xen needs to add a 1:1 mapping in the domain p2m to
+     * allow DMA request to work.
+     * This is only valid when the domain is directed mapped. Hence this
+     * function should only be used by gnttab code with gfn == mfn.
+     */
+    BUG_ON(!is_domain_direct_mapped(d));
+    BUG_ON(mfn != gfn);
+
+    /* We only support readable and writable flags */
+    if ( !(flags & (IOMMUF_readable | IOMMUF_writable)) )
+        return -EINVAL;
+
+    return arch_grant_map_page_identity(d, mfn, flags & IOMMUF_writable);
+}
+
+static int arm_smmu_unmap_page(struct domain *d, unsigned long gfn)
+{
+    /* This function should only be used by gnttab code when the domain
+     * is direct mapped
+     */
+    if ( !is_domain_direct_mapped(d) )
+        return -EINVAL;
+
+    return arch_grant_unmap_page_identity(d, gfn);
+}
+
 static const struct iommu_ops arm_smmu_iommu_ops = {
     .init = arm_smmu_iommu_domain_init,
     .hwdom_init = arm_smmu_iommu_hwdom_init,
@@ -1544,6 +1575,8 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
     .assign_dt_device = arm_smmu_attach_dev,
     .reassign_dt_device = arm_smmu_reassign_dt_dev,
+    .map_page = arm_smmu_map_page,
+    .unmap_page = arm_smmu_unmap_page,
 };
 
 static int __init smmu_init(struct dt_device_node *dev,
diff --git a/xen/include/asm-arm/grant_table.h b/xen/include/asm-arm/grant_table.h
index e81c973..a9f0291 100644
--- a/xen/include/asm-arm/grant_table.h
+++ b/xen/include/asm-arm/grant_table.h
@@ -33,7 +33,8 @@ static inline int replace_grant_supported(void)
     ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
      (i < max_nr_grant_frames(d))) ? 0 : (d->arch.grant_table_gpfn[i]))
 
-#define gnttab_need_iommu_mapping(d)           (is_domain_direct_mapped(d))
+#define gnttab_need_iommu_mapping(d)                    \
+    (is_domain_direct_mapped(d) && need_iommu(d))
 
 #endif /* __ASM_GRANT_TABLE_H__ */
 /*
diff --git a/xen/include/public/features.h b/xen/include/public/features.h
index b7bf83f..16d92aa 100644
--- a/xen/include/public/features.h
+++ b/xen/include/public/features.h
@@ -94,8 +94,10 @@
 /* operation as Dom0 is supported */
 #define XENFEAT_dom0                      11
 
-/* Xen also maps grant references at pfn = mfn */
+/* Xen also maps grant references at pfn = mfn.
+ * This feature flag is deprecated and should not be used.
 #define XENFEAT_grant_map_identity        12
+ */
 
 #define XENFEAT_NR_SUBMAPS 1
 
-- 
1.7.10.4

Re: [PATCH v3 5/5] Revert "xen/arm: introduce XENFEAT_grant_map_identity"

[Julien Grall]

Hi Stefano,

On 10/08/2014 02:00 PM, Stefano Stabellini wrote:
> Just keep the definition of XENFEAT_grant_map_identity.

Can you provide the commit ID in the message?

BTW, I would also revert e25a5f4  "xen: introduce
arch_grant_(un)map_page_identity" which is not necessary anymore.

Regards,

-- 
Julien Grall

Re: [PATCH v3 5/5] Revert "xen/arm: introduce XENFEAT_grant_map_identity"

[Stefano Stabellini]

On Wed, 8 Oct 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 10/08/2014 02:00 PM, Stefano Stabellini wrote:
> > Just keep the definition of XENFEAT_grant_map_identity.
> 
> Can you provide the commit ID in the message?
> 
> BTW, I would also revert e25a5f4  "xen: introduce
> arch_grant_(un)map_page_identity" which is not necessary anymore.

Good idea

Re: [PATCH v3 0/5] xen/arm: introduce GNTTABOP_cache_flush

[David Vrabel]

On 08/10/14 13:58, Stefano Stabellini wrote:
> Hi all,
> this patch series introduces a new hypercall to perform cache
> maintenance operations on behalf of the guest. It is useful for dom0 to
> be able to cache flush pages involved in a dma operation with
> non-coherent devices.

I was going to suggest requiring the use of the SWIOTLB for all foreign
pages to non-coherent devices but Ian tells me you've already tried this
and it performs worse than this proposal.  Is this the case?

Using the SWIOTLB would require no Xen changes (except for the revert)
which might be sensible at this point in the Xen 4.5 release cycle.

David

Re: [PATCH v3 0/5] xen/arm: introduce GNTTABOP_cache_flush

[Stefano Stabellini]

On Wed, 8 Oct 2014, David Vrabel wrote:
> On 08/10/14 13:58, Stefano Stabellini wrote:
> > Hi all,
> > this patch series introduces a new hypercall to perform cache
> > maintenance operations on behalf of the guest. It is useful for dom0 to
> > be able to cache flush pages involved in a dma operation with
> > non-coherent devices.
> 
> I was going to suggest requiring the use of the SWIOTLB for all foreign
> pages to non-coherent devices but Ian tells me you've already tried this
> and it performs worse than this proposal.  Is this the case?
> 
> Using the SWIOTLB would require no Xen changes (except for the revert)
> which might be sensible at this point in the Xen 4.5 release cycle.

I benchmarked the swiotlb solution a long time ago and was the worst of
the lot. Back then I didn't try to narrow down its usage only to
non-coherent devices though because it is actually quite an hack and I
hadn't climbed down the ivory tower yet. At this point anything goes.

I have tried that approach now, and it is still the worst of the lot. It
is slower than the hypercall approach by 37% (485 timer ticks on average
vs 352).