Re: [Xen-devel] [PATCH v1] x86/mm: Make change_type_range return error

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 24, 2019 at 02:27:32PM +0000, Alexandru Stefan ISAILA wrote:
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 9e81a30cc4..27697d5a77 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -1028,7 +1028,7 @@ int p2m_change_type_one(struct domain *d, unsigned long 
> gfn_l,
>  }
>  
>  /* Modify the p2m type of [start, end_exclusive) from ot to nt. */
> -static void change_type_range(struct p2m_domain *p2m,
> +static int change_type_range(struct p2m_domain *p2m,
>                                unsigned long start, unsigned long 
> end_exclusive,
>                                p2m_type_t ot, p2m_type_t nt)
>  {
> @@ -1053,15 +1053,11 @@ static void change_type_range(struct p2m_domain *p2m,
>       * This should be revisited later, but for now post a warning.
>       */
>      if ( unlikely(end > host_max_pfn) )
> -    {
> -        printk(XENLOG_G_WARNING "Dom%d logdirty rangeset clipped to 
> max_mapped_pfn\n",
> -               d->domain_id);
> -        end = invalidate_end = host_max_pfn;
> -    }
> +        return -EINVAL;
>  
>      /* If the requested range is out of scope, return doing nothing. */
>      if ( start > end )
> -        return;
> +        return 0;

Since you are already changing the behavior of the function above this
should also return EINVAL IMO.

>  
>      if ( p2m_is_altp2m(p2m) )
>          invalidate_end = min(invalidate_end, max_pfn);
> @@ -1115,13 +1111,16 @@ static void change_type_range(struct p2m_domain *p2m,
>                 rc, d->domain_id);
>          domain_crash(d);
>      }
> +
> +    return 0;

Here you should use 'return rc;', or else you are dropping the error
code from the calls from the rangeset functions.

It would be worth to consider where the domain_crash paths in the
function could be followed by a return rc, so that you don't lose the
error code from the call to change_entry_type_range.

>  }
>  
> -void p2m_change_type_range(struct domain *d,
> -                           unsigned long start, unsigned long end,
> -                           p2m_type_t ot, p2m_type_t nt)
> +int p2m_change_type_range(struct domain *d,
> +                          unsigned long start, unsigned long end,
> +                          p2m_type_t ot, p2m_type_t nt)
>  {
>      struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
> +    int rc = 0;

I don't think you need to initialize rc, it will be unconditionally
initialized by the call to change_type_range below.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel