Re: [Xen-devel] [RFC PATCH 2/2] xen/arm: introduce XENFEAT_grant_map_11

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 23 Jul 2014, Jan Beulich wrote:
> >>> On 08.07.14 at 17:53, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > --- a/xen/common/kernel.c
> > +++ b/xen/common/kernel.c
> > @@ -325,6 +325,10 @@ DO(xen_version)(int cmd, XEN_GUEST_HANDLE_PARAM(void) 
> > arg)
> >                  break;
> >              }
> >  #endif
> > +#ifdef CONFIG_ARM
> > +            if ( is_domain_direct_mapped(d) )
> 
> The #ifdef and if() seem kind of redundant - can't x86 simply get a
> stub always returning false?

Yes, you are right


> > --- a/xen/include/asm-arm/grant_table.h
> > +++ b/xen/include/asm-arm/grant_table.h
> > @@ -33,8 +33,7 @@ static inline int replace_grant_supported(void)
> >      ( ((i >= nr_grant_frames(d->grant_table)) &&                         \
> >       (i < max_nr_grant_frames)) ? 0 : (d->arch.grant_table_gpfn[i]))
> >  
> > -#define gnttab_need_iommu_mapping(d)                    \
> > -    (is_domain_direct_mapped(d) && need_iommu(d))
> > +#define gnttab_need_iommu_mapping(d)            
> > (is_domain_direct_mapped(d))
> 
> How is this change related to the subject of the patch?

This change is the one that actually enables the second mapping on ARM.
gnttab_need_iommu_mapping needs to return true for dom0, so that
arch_iommu_grant_map_page gets called by __gnttab_map_grant_ref.

I understand that actually XENFEAT_grant_map_11 doesn't have much to do
with IOMMUs or SMMUs, however it would still be nice to share the code
under:

if ( gnttab_need_iommu_mapping(ld) )
{
         
in __gnttab_map_grant_ref.
An alternative to this change that would also get rid of patch #1 of
this series could be something like:

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 464007e..e4e945a 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -727,7 +727,7 @@ __gnttab_map_grant_ref(
 
     double_gt_lock(lgt, rgt);
 
-    if ( gnttab_need_iommu_mapping(ld) )
+    if ( gnttab_need_iommu_mapping(ld) || is_domain_direct_mapped(ld) )
     {
         unsigned int wrc, rdc;
         int err = 0;
@@ -738,13 +738,23 @@ __gnttab_map_grant_ref(
              !(old_pin & (GNTPIN_hstw_mask|GNTPIN_devw_mask)) )
         {
             if ( wrc == 0 )
-                err = iommu_map_page(ld, frame, frame,
-                                     IOMMUF_readable|IOMMUF_writable);
+            {
+                if ( gnttab_need_iommu_mapping(ld) )
+                    err = iommu_map_page(ld, frame, frame,
+                            IOMMUF_readable|IOMMUF_writable);
+                else
+                    err = arch_grant_map_page(ld, frame, 
IOMMUF_readable|IOMMUF_writable);
+            }
         }
         else if ( act_pin && !old_pin )
         {
             if ( (wrc + rdc) == 0 )
-                err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
+            {
+                if ( gnttab_need_iommu_mapping(ld) )
+                    err = iommu_map_page(ld, frame, frame, IOMMUF_readable);
+                else
+                    err = arch_grant_map_page(ld, frame, IOMMUF_readable);
+            }
         }
         if ( err )
         {

do you think this would be better?


> > --- a/xen/include/public/features.h
> > +++ b/xen/include/public/features.h
> > @@ -94,6 +94,9 @@
> >  /* operation as Dom0 is supported */
> >  #define XENFEAT_dom0                      11
> >  
> > +/* Xen also maps grant references at pfn = mfn */
> > +#define XENFEAT_grant_map_11              12
> 
> As already said by others, this needs a better name.

I'll rename it to XENFEAT_grant_map_identity

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel