diff mbox

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

Message ID alpine.DEB.2.02.1407231419240.2293@kaball.uk.xensource.com
State New
Headers show

Commit Message

Stefano Stabellini July 23, 2014, 1:31 p.m. UTC 
On Wed, 23 Jul 2014, Jan Beulich wrote:
> >>> On 08.07.14 at 17:53, <stefano.stabellini@eu.citrix.com> 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:


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

Comments

Jan Beulich July 23, 2014, 2:15 p.m. UTC | #1 
>>> On 23.07.14 at 15:31, <stefano.stabellini@eu.citrix.com> wrote:
> On Wed, 23 Jul 2014, Jan Beulich wrote:
>> >>> On 08.07.14 at 17:53, <stefano.stabellini@eu.citrix.com> wrote:
>> > --- 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?

Hmm, overall readability suffers, but at least it makes more obvious
what arch_grant_map_page() is about.

Jan
diff mbox

Patch

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 )
         {