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

[Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>]

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


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