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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 23.07.14 at 15:31, <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> On Wed, 23 Jul 2014, Jan Beulich wrote:
>> >>> On 08.07.14 at 17:53, <stefano.stabellini@xxxxxxxxxxxxx> 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


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