Re: [PATCH v2] xen: Use evtchn_type_t as a type for event channels

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 3/7/20 8:43 AM, Yan Yankovskyi wrote:
> Make event channel functions pass event channel port using
> evtchn_port_t type. It eliminates signed <-> unsigned conversion.
>


>  static int find_virq(unsigned int virq, unsigned int cpu)
>  {
>  	struct evtchn_status status;
> -	int port, rc = -ENOENT;
> +	evtchn_port_t port;
> +	int rc = -ENOENT;
>  
>  	memset(&status, 0, sizeof(status));
>  	for (port = 0; port < xen_evtchn_max_channels(); port++) {
> @@ -962,7 +963,8 @@ EXPORT_SYMBOL_GPL(xen_evtchn_nr_channels);
>  int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
>  {
>  	struct evtchn_bind_virq bind_virq;
> -	int evtchn, irq, ret;
> +	evtchn_port_t evtchn = xen_evtchn_max_channels();
> +	int irq, ret;
>  
>  	mutex_lock(&irq_mapping_update_lock);
>  
> @@ -990,7 +992,6 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
>  			if (ret == -EEXIST)
>  				ret = find_virq(virq, cpu);
>  			BUG_ON(ret < 0);
> -			evtchn = ret;


This looks suspicious. What would you be passing to
xen_irq_info_virq_setup() below?

I also think that, given that this patch is trying to get types in
order, find_virq() will need more changes: it is supposed to return
evtchn_port_t. But then it also wants to return a (signed) error.

>  		}
>  
>  		ret = xen_irq_info_virq_setup(cpu, irq, evtchn, virq);
> @@ -1019,7 +1020,7 @@ static void unbind_from_irq(unsigned int irq)
>  	mutex_unlock(&irq_mapping_update_lock);
>  }
>  



> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -391,7 +391,7 @@ EXPORT_SYMBOL_GPL(xenbus_grant_ring);
>   * error, the device will switch to XenbusStateClosing, and the error will be
>   * saved in the store.
>   */
> -int xenbus_alloc_evtchn(struct xenbus_device *dev, int *port)
> +int xenbus_alloc_evtchn(struct xenbus_device *dev, evtchn_port_t *port)

Right. But then why is the declaration in include/xen/xenbus.h (at the
very end of the patch) different?

>  {
>  	struct evtchn_alloc_unbound alloc_unbound;
>  	int err;
> @@ -414,7 +414,7 @@ EXPORT_SYMBOL_GPL(xenbus_alloc_evtchn);
>  /**
>   * Free an existing event channel. Returns 0 on success or -errno on error.
>   */
> -int xenbus_free_evtchn(struct xenbus_device *dev, int port)
> +int xenbus_free_evtchn(struct xenbus_device *dev, evtchn_port_t port)

Here too.

>  {
>  	struct evtchn_close close;
>  	int err;
> @@ -423,7 +423,7 @@ int xenbus_free_evtchn(struct xenbus_device *dev, int port)

And why not here, especially since you updated format?

>  
>  	err = HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
>  	if (err)
> -		xenbus_dev_error(dev, err, "freeing event channel %d", port);
> +		xenbus_dev_error(dev, err, "freeing event channel %u", port);
>  
>  	return err;
>  }



>  
> diff --git a/include/xen/interface/event_channel.h b/include/xen/interface/event_channel.h
> index 45650c9a06d5..cf80e338fbb0 100644
> --- a/include/xen/interface/event_channel.h
> +++ b/include/xen/interface/event_channel.h
> @@ -220,7 +220,7 @@ struct evtchn_expand_array {
>  #define EVTCHNOP_set_priority    13
>  struct evtchn_set_priority {
>  	/* IN parameters. */
> -	uint32_t port;
> +	evtchn_port_t port;

This definition comes from Xen so I think it needs to be fixed there first.

>  	uint32_t priority;
>  };
>  
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index 89a889585ba0..4f35216064ba 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -218,8 +218,8 @@ int xenbus_unmap_ring(struct xenbus_device *dev,
>  		      grant_handle_t *handles, unsigned int nr_handles,
>  		      unsigned long *vaddrs);
>  
> -int xenbus_alloc_evtchn(struct xenbus_device *dev, int *port);
> -int xenbus_free_evtchn(struct xenbus_device *dev, int port);
> +int xenbus_alloc_evtchn(struct xenbus_device *dev, unsigned int *port);
> +int xenbus_free_evtchn(struct xenbus_device *dev, unsigned int port);

These.


-boris