[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [XEN PATCH 2/7] xen: introduce _evtchn_alloc_unbound


  • To: Stefano Stabellini <sstabellini@xxxxxxxxxx>, Luca Miccio <lucmiccio@xxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 10 Jan 2022 11:25:14 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=dwYTt9qcNNqqBSANypvUW0ZJ33wnXzkJ+DTmv63V1KQ=; b=YOSDWmauvoPCNOJSSJ+zCwSG2Z0G5lLqc2eTeLQwxStF6glJkvoinuTXSpgoBJzBEwJXqCsoiW74DfPWB5+lLRjpUJ5Vj/onL4Hh1ej0MmCIO1mwUnl2OajIOY1lgYjx2n236vWY7U/DfmuzwwCf8uO2tX1hYo1m9kdGRo3VygQ2cIwvWntWsGVsGAnFvJOwJQtcfDpHLcRsIbze5WDrEwSrzTfYhBFNot2ReuYAzDJWVohzKlVYf19k+ECvAtaxtlS9L2vBLSrqIYaBjhqN28r4oLs+2s1mt9uMbkss6Y7+9muzF+tmK8aLnCgGjSDto7C1HmCQEVm8VXmV0D9xWA==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=EQxrnutYhJ1cVjLYB5T9IrCSfSxTu1pifLi0pd/rYjax8gnOsW3rR0f18dtAgqVBTdPGefCkIJA6PPP1+hMyh8YoOlX+3eUjse35scU3wvaOopRxBbrmyzs/9eboOQ0e4BwJDLiWcIxENgmKdLNF2nhOprY5gHY3Up/Knx/m7BmYU59iysIjuNrotabNmnzilmY3nYXDwPwT5gHoHyxYMFleqF5alp6dE7fcaCyRPIckHxb15IQzX7KsacCcG7NDkAyfptpMSV9mXx2JE8xMgd4sXU5VVB9IHaPvOFK6SIT8QgN25g4DlhqxOFl9iU1fvGmheaGUMqVFndcMhcoh0w==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: julien@xxxxxxx, Bertrand.Marquis@xxxxxxx, Stefano Stabellini <stefano.stabellini@xxxxxxxxxx>, Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, George Dunlap <george.dunlap@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Mon, 10 Jan 2022 10:25:40 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 08.01.2022 01:49, Stefano Stabellini wrote:
> @@ -284,11 +285,32 @@ void evtchn_free(struct domain *d, struct evtchn *chn)
>      xsm_evtchn_close_post(chn);
>  }
>  
> -static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> +struct evtchn *_evtchn_alloc_unbound(struct domain *d, domid_t remote_dom)

Function names want to be the other way around, to be in line with
naming rules of the C spec: The static function may be underscore-
prefixed, while the non-static one may not.

>  {
>      struct evtchn *chn;
> +    int port;
> +
> +    if ( (port = get_free_port(d)) < 0 )
> +        return ERR_PTR(port);
> +    chn = evtchn_from_port(d, port);
> +
> +    evtchn_write_lock(chn);
> +
> +    chn->state = ECS_UNBOUND;
> +    if ( (chn->u.unbound.remote_domid = remote_dom) == DOMID_SELF )
> +        chn->u.unbound.remote_domid = current->domain->domain_id;

I think the resolving of DOMID_SELF should remain in the caller, as I'm
pretty sure your planned new user(s) can't sensibly pass that value.

> +    evtchn_port_init(d, chn);
> +
> +    evtchn_write_unlock(chn);
> +
> +    return chn;
> +}
> +
> +static int evtchn_alloc_unbound(evtchn_alloc_unbound_t *alloc)
> +{
> +    struct evtchn *chn = NULL;

I don't think the initializer is needed.

> @@ -297,27 +319,22 @@ static int evtchn_alloc_unbound(evtchn_alloc_unbound_t 
> *alloc)
>  
>      spin_lock(&d->event_lock);
>  
> -    if ( (port = get_free_port(d)) < 0 )
> -        ERROR_EXIT_DOM(port, d);
> -    chn = evtchn_from_port(d, port);
> +    chn = _evtchn_alloc_unbound(d, alloc->remote_dom);
> +    if ( IS_ERR(chn) )
> +    {
> +        rc = PTR_ERR(chn);
> +        ERROR_EXIT_DOM(rc, d);
> +    }
>  
>      rc = xsm_evtchn_unbound(XSM_TARGET, d, chn, alloc->remote_dom);
>      if ( rc )
>          goto out;
>  
> -    evtchn_write_lock(chn);
> -
> -    chn->state = ECS_UNBOUND;

This cannot be pulled ahead of the XSM check (or in general anything
potentially resulting in an error), as check_free_port() relies on
->state remaining ECS_FREE until it is known that the calling function
can't fail anymore.

> -    if ( (chn->u.unbound.remote_domid = alloc->remote_dom) == DOMID_SELF )
> -        chn->u.unbound.remote_domid = current->domain->domain_id;
> -    evtchn_port_init(d, chn);
> -
> -    evtchn_write_unlock(chn);
> -
> -    alloc->port = port;
> +    alloc->port = chn->port;
>  
>   out:
> -    check_free_port(d, port);
> +    if ( chn != NULL )
> +        check_free_port(d, chn->port);

Without the initializer above it'll then be more obvious that the
condition here needs to be !IS_ERR(chn).

Also (nit) please prefer the shorter "if ( chn )".

Overall I wonder in how far it would be possible to instead re-use PV
shim's "backdoor" into port allocation: evtchn_allocate_port() was
specifically made available for it, iirc.

Jan




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.