Re: [PATCH v2 1/3] tools/libs/evtchn: decouple more from mini-os

[Andrew Cooper <amc96@srcf.net>]

On 11/01/2022 15:03, Juergen Gross wrote:
> diff --git a/tools/libs/evtchn/minios.c b/tools/libs/evtchn/minios.c
> index e5dfdc5ef5..c3a5ce3b98 100644
> --- a/tools/libs/evtchn/minios.c
> +++ b/tools/libs/evtchn/minios.c
> @@ -38,29 +38,40 @@
>  
>  #include "private.h"
>  
> -extern void minios_evtchn_close_fd(int fd);
> +LIST_HEAD(port_list, port_info);
> +
> +struct port_info {
> +    LIST_ENTRY(port_info) list;
> +    evtchn_port_t port;
> +    bool pending;
> +    bool bound;
> +};
>  
>  extern struct wait_queue_head event_queue;

Yuck.  This should come from minios's evtchn header, rather than being
extern'd like this, but lets consider that future cleanup work.

> +int minios_evtchn_close_fd(int fd);

You don't need this forward declaration, because nothing in this file
calls minios_evtchn_close_fd().  The extern should simply be deleted,
and it removes a hunk from your later xen.git series.

> @@ -69,18 +80,54 @@ static void port_dealloc(struct evtchn_port_info *port_info)
>      free(port_info);
>  }
>  
> +int minios_evtchn_close_fd(int fd)
> +{
> +    struct port_info *port_info, *tmp;
> +    struct file *file = get_file_from_fd(fd);
> +    struct port_list *port_list = file->dev;

Looking at this, the file_ops don't need to have the C ABI.

The single caller already needs access to the file structure, so could
pass both file and fd in to the ops->close() function.  Thoughts?

> +
> +    LIST_FOREACH_SAFE(port_info, port_list, list, tmp)
> +        port_dealloc(port_info);
> +    free(port_list);
> +
> +    return 0;
> +}
> +
> +static struct file_ops evtchn_ops = {

This wants to become const, when alloc_file_type() has been
appropriately const'd.

> +    .name = "evtchn",
> +    .close = minios_evtchn_close_fd,
> +    .select_rd = select_read_flag,
> +};
> +
>  /*
>   * XENEVTCHN_NO_CLOEXEC is being ignored, as there is no exec() call supported
>   * in Mini-OS.
>   */
>  int osdep_evtchn_open(xenevtchn_handle *xce, unsigned int flags)
>  {
> -    int fd = alloc_fd(FTYPE_EVTCHN);
> +    int fd;
> +    struct file *file;
> +    struct port_list *list;
> +    static unsigned int ftype_evtchn;
>  
> -    if ( fd == -1 )
> +    if ( !ftype_evtchn )
> +        ftype_evtchn = alloc_file_type(&evtchn_ops);

Hmm.  MiniOS doesn't appear to support __attribute__((constructor)) but
this would be an ideal candidate.

It would remove a non-threadsafe singleton from a (largely unrelated)
codepath.

Should be very simple to add to MiniOS.  See Xen's init_constructors(),
and add CONSTRUCTORS to the linker file.

> @@ -134,42 +171,43 @@ int xenevtchn_notify(xenevtchn_handle *xce, evtchn_port_t port)
>  
>  static void evtchn_handler(evtchn_port_t port, struct pt_regs *regs, void *data)
>  {
> -    int fd = (int)(intptr_t)data;
> -    struct evtchn_port_info *port_info;
> +    xenevtchn_handle *xce = data;
> +    struct file *file = get_file_from_fd(xce->fd);
> +    struct port_info *port_info;
> +    struct port_list *port_list;
>  
> -    assert(files[fd].type == FTYPE_EVTCHN);
> +    assert(file);
> +    port_list = file->dev;
>      mask_evtchn(port);
> -    LIST_FOREACH(port_info, &files[fd].evtchn.ports, list)
> +    LIST_FOREACH(port_info, port_list, list)
>      {
>          if ( port_info->port == port )
>              goto found;
>      }
>  
> -    printk("Unknown port for handle %d\n", fd);
> +    printk("Unknown port for handle %d\n", xce->fd);

As you're editing this line anyway, it really wants to become "Unknown
port %d for handle %d\n".

~Andrew