Re: [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics

From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Xen-devel <xen-devel@lists.xenproject.org>,
	"George Dunlap" <George.Dunlap@eu.citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Wei Liu" <wl@xen.org>, "Julien Grall" <julien@xen.org>,
	"Paul Durrant" <paul@xen.org>,
	"Michał Leszczyński" <michal.leszczynski@cert.pl>,
	"Hubert Jasudowicz" <hubert.jasudowicz@cert.pl>,
	"Tamas K Lengyel" <tamas@tklengyel.com>
Subject: Re: [PATCH v2 04/11] xen/memory: Fix acquire_resource size semantics
Date: Fri, 25 Sep 2020 17:56:36 +0200	[thread overview]
Message-ID: <1ba2a8dd-71b6-3c28-96d3-8bfa3a6816b4@suse.com> (raw)
In-Reply-To: <20200922182444.12350-5-andrew.cooper3@citrix.com>

On 22.09.2020 20:24, Andrew Cooper wrote:
> --- a/xen/common/memory.c
> +++ b/xen/common/memory.c
> @@ -1007,6 +1007,26 @@ static long xatp_permission_check(struct domain *d, unsigned int space)
>      return xsm_add_to_physmap(XSM_TARGET, current->domain, d);
>  }
>  
> +/*
> + * Return 0 on any kind of error.  Caller converts to -EINVAL.
> + *
> + * All nonzero values should be repeatable (i.e. derived from some fixed
> + * property of the domain), and describe the full resource (i.e. mapping the
> + * result of this call will be the entire resource).
> + */
> +static unsigned int resource_max_frames(struct domain *d,

With the lockless intentions I think this could be const from
here on through all the descendants. With this
Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit I have one more minor remark:

> @@ -1058,6 +1066,27 @@ static int acquire_resource(
>      if ( rc )
>          goto out;
>  
> +    max_frames = resource_max_frames(d, xmar.type, xmar.id);
> +
> +    rc = -EINVAL;
> +    if ( !max_frames )
> +        goto out;
> +
> +    if ( guest_handle_is_null(xmar.frame_list) )
> +    {
> +        if ( xmar.nr_frames )
> +            goto out;
> +
> +        xmar.nr_frames = max_frames;
> +
> +        rc = -EFAULT;
> +        if ( __copy_field_to_guest(arg, &xmar, nr_frames) )
> +            goto out;
> +
> +        rc = 0;
> +        goto out;
> +    }

That's a lot of "goto out" here. I don't suppose I could talk you
into reducing their amount some, since at least the last two look
to be easy to fold?

Jan