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

[Yan Yankovskyi <yyankovskyi@gmail.com>]

On Sat, Mar 07, 2020 at 02:41:44PM -0500, Boris Ostrovsky wrote:
> 
> 
> 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?

Right, this line should be preserved.

> 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.
 
As we don't care which error we got during find_virq call, we can just
return 0 in case of error, and port number otherwise. Port 0 is never
valid, so this approach can work for the other functions as well.
On the other hand, passing port using pointer and returning actual
error message, as it's done in xenbus_alloc_evtchn(), sounds like a
better approach overall. What do you think?

> >  		}
> >  
> >  		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);
> >  }
> >  
> 
> 
> 
> >  {
> >  	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?

I missed it.

> >  
> >  	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.

Will be done.

> > --- 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.
> 
> >  	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.

I was reluctant with inclusion of event channel header into xenbus.
Will be fixed.

> -boris
>