Re: [PATCH 3/9] gnttab: fold recurring is_iomem_page()

[Julien Grall <julien@xxxxxxx>]

Hi Jan,

On 26/08/2021 11:12, Jan Beulich wrote:
> In all cases call the function just once instead of up to four times, at
extra NIT: It is more like two because there is a else in 
gnttab_release_mappings() :)
> the same time avoiding to store a dangling pointer in a local variable.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
[...]

Everything below looks a duplicate. Might be an issue in your tools?

> In all cases call the function just once instead of up to four times, at
> the same time avoiding to store a dangling pointer in a local variable.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -1587,11 +1587,11 @@ unmap_common_complete(struct gnttab_unma
>       else
>           status = &status_entry(rgt, op->ref);
>   
> -    pg = mfn_to_page(op->mfn);
> +    pg = !is_iomem_page(act->mfn) ? mfn_to_page(op->mfn) : NULL;
>   
>       if ( op->done & GNTMAP_device_map )
>       {
> -        if ( !is_iomem_page(act->mfn) )
> +        if ( pg )
>           {
>               if ( op->done & GNTMAP_readonly )
>                   put_page(pg);
> @@ -1608,7 +1608,7 @@ unmap_common_complete(struct gnttab_unma
>   
>       if ( op->done & GNTMAP_host_map )
>       {
> -        if ( !is_iomem_page(op->mfn) )
> +        if ( pg )
>           {
>               if ( gnttab_host_mapping_get_page_type(op->done & GNTMAP_readonly,
>                                                      ld, rd) )
> @@ -3778,7 +3778,7 @@ int gnttab_release_mappings(struct domai
>           else
>               status = &status_entry(rgt, ref);
>   
> -        pg = mfn_to_page(act->mfn);
> +        pg = !is_iomem_page(act->mfn) ? mfn_to_page(act->mfn) : NULL;
>   
>           if ( map->flags & GNTMAP_readonly )
>           {
> @@ -3786,7 +3786,7 @@ int gnttab_release_mappings(struct domai
>               {
>                   BUG_ON(!(act->pin & GNTPIN_devr_mask));
>                   act->pin -= GNTPIN_devr_inc;
> -                if ( !is_iomem_page(act->mfn) )
> +                if ( pg )
>                       put_page(pg);
>               }
>   
> @@ -3794,8 +3794,7 @@ int gnttab_release_mappings(struct domai
>               {
>                   BUG_ON(!(act->pin & GNTPIN_hstr_mask));
>                   act->pin -= GNTPIN_hstr_inc;
> -                if ( gnttab_release_host_mappings(d) &&
> -                     !is_iomem_page(act->mfn) )
> +                if ( pg && gnttab_release_host_mappings(d) )
>                       put_page(pg);
>               }
>           }
> @@ -3805,7 +3804,7 @@ int gnttab_release_mappings(struct domai
>               {
>                   BUG_ON(!(act->pin & GNTPIN_devw_mask));
>                   act->pin -= GNTPIN_devw_inc;
> -                if ( !is_iomem_page(act->mfn) )
> +                if ( pg )
>                       put_page_and_type(pg);
>               }
>   
> @@ -3813,8 +3812,7 @@ int gnttab_release_mappings(struct domai
>               {
>                   BUG_ON(!(act->pin & GNTPIN_hstw_mask));
>                   act->pin -= GNTPIN_hstw_inc;
> -                if ( gnttab_release_host_mappings(d) &&
> -                     !is_iomem_page(act->mfn) )
> +                if ( pg && gnttab_release_host_mappings(d) )
>                   {
>                       if ( gnttab_host_mapping_get_page_type(false, d, rd) )
>                           put_page_type(pg);
--
Julien Grall