[PATCH v2 0/3] tools/libs: decouple more from mini-os

[PATCH v2 0/3] tools/libs: decouple more from mini-os

[Juergen Gross]

This small series removes some hard coupling of the Xen build with some
Mini-OS internals, especially the struct file layout and the internal
organization of the file handling.

This series depends on the Mini-OS series posted recently:

https://lists.xen.org/archives/html/xen-devel/2022-01/threads.html#00347

The main idea is to no longer have Xen library specific structures
inside struct file, or to let struct file layout depend on the
configuration of Mini-OS.

All Xen libraries needing a hook in struct file should use the now
available generic dev pointer and allocate the needed data dynamically.

Additionally Xen libraries should get the pointer of struct file via
the new get_file_from_fd() function instead of accessing directly the
files[] array, which might go away in future (e.g. in order to support
dynamic allocation of struct file as needed).

Via using alloc_file_type() the libs provide a function vector in
order to enable Mini9-OS to remove the dedicated callbacks into the
libs via known function names and replacing them to use the new
vector.

Changes in V2:
- comments addressed
- add using alloc_file_type()
- new patch 3

Juergen Gross (3):
  tools/libs/evtchn: decouple more from mini-os
  tools/libs/gnttab: decouple more from mini-os
  tools/libs/ctrl: remove file related handling

 tools/libs/ctrl/xc_minios.c |   9 ---
 tools/libs/evtchn/minios.c  | 143 +++++++++++++++++++++++-------------
 tools/libs/gnttab/minios.c  |  68 ++++++++++++-----
 3 files changed, 139 insertions(+), 81 deletions(-)

-- 
2.26.2

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

[Juergen Gross]

Mini-OS and libevtchn are using implementation details of each other.
Change that by letting libevtchn use the new alloc_file_type() and
get_file_from_fd() functions and the generic dev pointer of struct
file from Mini-OS.

By using private struct declarations Mini-OS will be able to drop the
libevtchn specific definitions of struct evtchn_port_info and
evtchn_port_list in future. While at it use boll for "pending" and
"bound".

Switch to use xce as function parameter instead of fd where possible.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- use xce instead of fd as parameter internally (Andrew Cooper)
- add alloc_file_type() support
---
 tools/libs/evtchn/minios.c | 143 +++++++++++++++++++++++--------------
 1 file changed, 91 insertions(+), 52 deletions(-)

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;
 
+int minios_evtchn_close_fd(int fd);
+
 /* XXX Note: This is not threadsafe */
-static struct evtchn_port_info *port_alloc(int fd)
+static struct port_info *port_alloc(xenevtchn_handle *xce)
 {
-    struct evtchn_port_info *port_info;
+    struct port_info *port_info;
+    struct file *file = get_file_from_fd(xce->fd);
+    struct port_list *port_list = file->dev;
 
-    port_info = malloc(sizeof(struct evtchn_port_info));
+    port_info = malloc(sizeof(struct port_info));
     if ( port_info == NULL )
         return NULL;
 
-    port_info->pending = 0;
+    port_info->pending = false;
     port_info->port = -1;
-    port_info->bound = 0;
+    port_info->bound = false;
 
-    LIST_INSERT_HEAD(&files[fd].evtchn.ports, port_info, list);
+    LIST_INSERT_HEAD(port_list, port_info, list);
 
     return port_info;
 }
 
-static void port_dealloc(struct evtchn_port_info *port_info)
+static void port_dealloc(struct port_info *port_info)
 {
     if ( port_info->bound )
         unbind_evtchn(port_info->port);
@@ -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;
+
+    LIST_FOREACH_SAFE(port_info, port_list, list, tmp)
+        port_dealloc(port_info);
+    free(port_list);
+
+    return 0;
+}
+
+static struct file_ops evtchn_ops = {
+    .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);
+
+    list = malloc(sizeof(*list));
+    if ( !list )
         return -1;
 
-    LIST_INIT(&files[fd].evtchn.ports);
+    fd = alloc_fd(ftype_evtchn);
+    file = get_file_from_fd(fd);
+
+    if ( !file )
+    {
+        free(list);
+        return -1;
+    }
+
+    file->dev = list;
+    LIST_INIT(list);
     xce->fd = fd;
     printf("evtchn_open() -> %d\n", fd);
 
@@ -102,16 +149,6 @@ int osdep_evtchn_restrict(xenevtchn_handle *xce, domid_t domid)
     return -1;
 }
 
-void minios_evtchn_close_fd(int fd)
-{
-    struct evtchn_port_info *port_info, *tmp;
-
-    LIST_FOREACH_SAFE(port_info, &files[fd].evtchn.ports, list, tmp)
-        port_dealloc(port_info);
-
-    files[fd].type = FTYPE_NONE;
-}
-
 int xenevtchn_fd(xenevtchn_handle *xce)
 {
     return xce->fd;
@@ -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);
     return;
 
  found:
-    port_info->pending = 1;
-    files[fd].read = 1;
+    port_info->pending = true;
+    file->read = true;
     wake_up(&event_queue);
 }
 
 xenevtchn_port_or_error_t xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
                                                       uint32_t domid)
 {
-    int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct port_info *port_info;
     int ret;
     evtchn_port_t port;
 
     assert(get_current() == main_thread);
-    port_info = port_alloc(fd);
+    port_info = port_alloc(xce);
     if ( port_info == NULL )
         return -1;
 
     printf("xenevtchn_bind_unbound_port(%d)", domid);
-    ret = evtchn_alloc_unbound(domid, evtchn_handler,
-                               (void *)(intptr_t)fd, &port);
+    ret = evtchn_alloc_unbound(domid, evtchn_handler, xce, &port);
     printf(" = %d\n", ret);
 
     if ( ret < 0 )
@@ -179,7 +217,7 @@ xenevtchn_port_or_error_t xenevtchn_bind_unbound_port(xenevtchn_handle *xce,
         return -1;
     }
 
-    port_info->bound = 1;
+    port_info->bound = true;
     port_info->port = port;
     unmask_evtchn(port);
 
@@ -190,19 +228,18 @@ xenevtchn_port_or_error_t xenevtchn_bind_interdomain(xenevtchn_handle *xce,
                                                      uint32_t domid,
                                                      evtchn_port_t remote_port)
 {
-    int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct port_info *port_info;
     evtchn_port_t local_port;
     int ret;
 
     assert(get_current() == main_thread);
-    port_info = port_alloc(fd);
+    port_info = port_alloc(xce);
     if ( port_info == NULL )
         return -1;
 
     printf("xenevtchn_bind_interdomain(%d, %"PRId32")", domid, remote_port);
     ret = evtchn_bind_interdomain(domid, remote_port, evtchn_handler,
-                                  (void *)(intptr_t)fd, &local_port);
+                                  xce, &local_port);
     printf(" = %d\n", ret);
 
     if ( ret < 0 )
@@ -212,7 +249,7 @@ xenevtchn_port_or_error_t xenevtchn_bind_interdomain(xenevtchn_handle *xce,
         return -1;
     }
 
-    port_info->bound = 1;
+    port_info->bound = true;
     port_info->port = local_port;
     unmask_evtchn(local_port);
 
@@ -222,9 +259,11 @@ xenevtchn_port_or_error_t xenevtchn_bind_interdomain(xenevtchn_handle *xce,
 int xenevtchn_unbind(xenevtchn_handle *xce, evtchn_port_t port)
 {
     int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct file *file = get_file_from_fd(fd);
+    struct port_info *port_info;
+    struct port_list *port_list = file->dev;
 
-    LIST_FOREACH(port_info, &files[fd].evtchn.ports, list)
+    LIST_FOREACH(port_info, port_list, list)
     {
         if ( port_info->port == port )
         {
@@ -243,17 +282,16 @@ int xenevtchn_unbind(xenevtchn_handle *xce, evtchn_port_t port)
 xenevtchn_port_or_error_t xenevtchn_bind_virq(xenevtchn_handle *xce,
                                               unsigned int virq)
 {
-    int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct port_info *port_info;
     evtchn_port_t port;
 
     assert(get_current() == main_thread);
-    port_info = port_alloc(fd);
+    port_info = port_alloc(xce);
     if ( port_info == NULL )
         return -1;
 
     printf("xenevtchn_bind_virq(%d)", virq);
-    port = bind_virq(virq, evtchn_handler, (void *)(intptr_t)fd);
+    port = bind_virq(virq, evtchn_handler, xce);
     printf(" = %d\n", port);
 
     if ( port < 0 )
@@ -263,7 +301,7 @@ xenevtchn_port_or_error_t xenevtchn_bind_virq(xenevtchn_handle *xce,
         return -1;
     }
 
-    port_info->bound = 1;
+    port_info->bound = true;
     port_info->port = port;
     unmask_evtchn(port);
 
@@ -272,27 +310,28 @@ xenevtchn_port_or_error_t xenevtchn_bind_virq(xenevtchn_handle *xce,
 
 xenevtchn_port_or_error_t xenevtchn_pending(xenevtchn_handle *xce)
 {
-    int fd = xce->fd;
-    struct evtchn_port_info *port_info;
+    struct file *file = get_file_from_fd(xce->fd);
+    struct port_info *port_info;
+    struct port_list *port_list = file->dev;
     unsigned long flags;
     evtchn_port_t ret = -1;
 
     local_irq_save(flags);
 
-    files[fd].read = 0;
+    file->read = false;
 
-    LIST_FOREACH(port_info, &files[fd].evtchn.ports, list)
+    LIST_FOREACH(port_info, port_list, list)
     {
         if ( port_info->port != -1 && port_info->pending )
         {
             if ( ret == -1 )
             {
                 ret = port_info->port;
-                port_info->pending = 0;
+                port_info->pending = false;
             }
             else
             {
-                files[fd].read = 1;
+                file->read = true;
                 break;
             }
         }
-- 
2.26.2

[PATCH v2 2/3] tools/libs/gnttab: decouple more from mini-os

[Juergen Gross]

libgnttab is using implementation details of Mini-OS. Change that by
letting libgnttab use the new alloc_file_type() and get_file_from_fd()
functions and the generic dev pointer of struct file from Mini-OS.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- add alloc_file_type() support
---
 tools/libs/gnttab/minios.c | 68 +++++++++++++++++++++++++++-----------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
index f78caadd30..c19f339c8c 100644
--- a/tools/libs/gnttab/minios.c
+++ b/tools/libs/gnttab/minios.c
@@ -28,18 +28,53 @@
 #include <sys/mman.h>
 
 #include <errno.h>
+#include <malloc.h>
 #include <unistd.h>
 
 #include "private.h"
 
-void minios_gnttab_close_fd(int fd);
+int minios_gnttab_close_fd(int fd);
+
+int minios_gnttab_close_fd(int fd)
+{
+    struct file *file = get_file_from_fd(fd);
+
+    gntmap_fini(file->dev);
+    free(file->dev);
+
+    return 0;
+}
+
+static struct file_ops gnttab_ops = {
+    .name = "gnttab",
+    .close = minios_gnttab_close_fd,
+};
 
 int osdep_gnttab_open(xengnttab_handle *xgt)
 {
-    int fd = alloc_fd(FTYPE_GNTMAP);
-    if ( fd == -1 )
+    int fd;
+    struct file *file;
+    struct gntmap *gntmap;
+    static unsigned int ftype_gnttab;
+
+    if ( !ftype_gnttab )
+        ftype_gnttab = alloc_file_type(&gnttab_ops);
+
+    gntmap = malloc(sizeof(*gntmap));
+    if ( !gntmap )
+        return -1;
+
+    fd = alloc_fd(ftype_gnttab);
+    file = get_file_from_fd(fd);
+
+    if ( !file )
+    {
+        free(gntmap);
         return -1;
-    gntmap_init(&files[fd].gntmap);
+    }
+
+    file->dev = gntmap;
+    gntmap_init(gntmap);
     xgt->fd = fd;
     return 0;
 }
@@ -52,28 +87,22 @@ int osdep_gnttab_close(xengnttab_handle *xgt)
     return close(xgt->fd);
 }
 
-void minios_gnttab_close_fd(int fd)
-{
-    gntmap_fini(&files[fd].gntmap);
-    files[fd].type = FTYPE_NONE;
-}
-
 void *osdep_gnttab_grant_map(xengnttab_handle *xgt,
                              uint32_t count, int flags, int prot,
                              uint32_t *domids, uint32_t *refs,
                              uint32_t notify_offset,
                              evtchn_port_t notify_port)
 {
-    int fd = xgt->fd;
+    struct file *file = get_file_from_fd(xgt->fd);
     int stride = 1;
+
     if (flags & XENGNTTAB_GRANT_MAP_SINGLE_DOMAIN)
         stride = 0;
     if (notify_offset != -1 || notify_port != -1) {
         errno = ENOSYS;
         return NULL;
     }
-    return gntmap_map_grant_refs(&files[fd].gntmap,
-                                 count, domids, stride,
+    return gntmap_map_grant_refs(file->dev, count, domids, stride,
                                  refs, prot & PROT_WRITE);
 }
 
@@ -81,11 +110,10 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
                        void *start_address,
                        uint32_t count)
 {
-    int fd = xgt->fd;
+    struct file *file = get_file_from_fd(xgt->fd);
     int ret;
-    ret = gntmap_munmap(&files[fd].gntmap,
-                        (unsigned long) start_address,
-                        count);
+
+    ret = gntmap_munmap(file->dev, (unsigned long) start_address, count);
     if (ret < 0) {
         errno = -ret;
         return -1;
@@ -95,10 +123,10 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
 
 int osdep_gnttab_set_max_grants(xengnttab_handle *xgt, uint32_t count)
 {
-    int fd = xgt->fd;
+    struct file *file = get_file_from_fd(xgt->fd);
     int ret;
-    ret = gntmap_set_max_grants(&files[fd].gntmap,
-                                count);
+
+    ret = gntmap_set_max_grants(file->dev, count);
     if (ret < 0) {
         errno = -ret;
         return -1;
-- 
2.26.2

[PATCH v2 3/3] tools/libs/ctrl: remove file related handling

[Juergen Gross]

There is no special file handling related to libxenctrl in Mini-OS
any longer, so the close hook can be removed.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 tools/libs/ctrl/xc_minios.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/tools/libs/ctrl/xc_minios.c b/tools/libs/ctrl/xc_minios.c
index 1799daafdc..3dea7a78a5 100644
--- a/tools/libs/ctrl/xc_minios.c
+++ b/tools/libs/ctrl/xc_minios.c
@@ -35,15 +35,6 @@
 
 #include "xc_private.h"
 
-void minios_interface_close_fd(int fd);
-
-extern void minios_interface_close_fd(int fd);
-
-void minios_interface_close_fd(int fd)
-{
-    files[fd].type = FTYPE_NONE;
-}
-
 /* Optionally flush file to disk and discard page cache */
 void discard_file_cache(xc_interface *xch, int fd, int flush)
 {
-- 
2.26.2

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

[Andrew Cooper]

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

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

[Andrew Cooper]

On 11/01/2022 15:03, Juergen Gross wrote:
> libgnttab is using implementation details of Mini-OS. Change that by
> letting libgnttab use the new alloc_file_type() and get_file_from_fd()
> functions and the generic dev pointer of struct file from Mini-OS.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V2:
> - add alloc_file_type() support
> ---
>  tools/libs/gnttab/minios.c | 68 +++++++++++++++++++++++++++-----------
>  1 file changed, 48 insertions(+), 20 deletions(-)
>
> diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
> index f78caadd30..c19f339c8c 100644
> --- a/tools/libs/gnttab/minios.c
> +++ b/tools/libs/gnttab/minios.c
> @@ -28,18 +28,53 @@
>  #include <sys/mman.h>
>  
>  #include <errno.h>
> +#include <malloc.h>
>  #include <unistd.h>
>  
>  #include "private.h"
>  
> -void minios_gnttab_close_fd(int fd);
> +int minios_gnttab_close_fd(int fd);

Again, like evtchn, no need to forward declare.


However, I've only just realised...

> +
> +int minios_gnttab_close_fd(int fd)
> +{
> +    struct file *file = get_file_from_fd(fd);
> +
> +    gntmap_fini(file->dev);
> +    free(file->dev);
> +
> +    return 0;
> +}

The only reason this doesn't break the build is because the declaration
is not in a header.  After this change, you've got the function
returning int here, but declared as returning void as far as MiniOS is
concerned.

Furthermore, we cannot fix this mess atomically now that minios has
moved into a separate repo.  It's tolerable from an ABI point of view on
x86, but I don't know for certain on other architectures.

The least bad way I can think of doing this would be to leave void
minios_gnttab_close_fd(int fd) exactly as it was, and instead of
converting it's use here, use a separate static function straight away
for the file ops.  (Will be necessary anyway if you like my suggestion
of passing file too).  Then in the whole function in "tools/libs: final
cleanup making mini-os callbacks static", rather than just making it static.

~Andrew

Re: [PATCH v2 3/3] tools/libs/ctrl: remove file related handling

[Andrew Cooper]

On 11/01/2022 15:03, Juergen Gross wrote:
> There is no special file handling related to libxenctrl in Mini-OS
> any longer, so the close hook can be removed.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
> V2:
> - new patch
> ---
>  tools/libs/ctrl/xc_minios.c | 9 ---------
>  1 file changed, 9 deletions(-)
>
> diff --git a/tools/libs/ctrl/xc_minios.c b/tools/libs/ctrl/xc_minios.c
> index 1799daafdc..3dea7a78a5 100644
> --- a/tools/libs/ctrl/xc_minios.c
> +++ b/tools/libs/ctrl/xc_minios.c
> @@ -35,15 +35,6 @@
>  
>  #include "xc_private.h"
>  
> -void minios_interface_close_fd(int fd);
> -
> -extern void minios_interface_close_fd(int fd);

This is even more nonsense than the evtchn side.  Good riddance.

~Andrew

> -
> -void minios_interface_close_fd(int fd)
> -{
> -    files[fd].type = FTYPE_NONE;
> -}
> -
>  /* Optionally flush file to disk and discard page cache */
>  void discard_file_cache(xc_interface *xch, int fd, int flush)
>  {

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 4194 bytes --]

On 11.01.22 20:56, Andrew Cooper wrote:
> 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.

I think I should do that rather sooner than later.

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

Without it I get a build error due to no prototype defined.

> 
>> @@ -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?

If we do this for close(), we should do it for all callbacks. I think we
don't need fd at all in the callbacks, so switching to file seems to be
the way to go.

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

Yes.

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

I'll look into this.

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

Okay.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 2372 bytes --]

On 11.01.22 21:08, Andrew Cooper wrote:
> On 11/01/2022 15:03, Juergen Gross wrote:
>> libgnttab is using implementation details of Mini-OS. Change that by
>> letting libgnttab use the new alloc_file_type() and get_file_from_fd()
>> functions and the generic dev pointer of struct file from Mini-OS.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> V2:
>> - add alloc_file_type() support
>> ---
>>   tools/libs/gnttab/minios.c | 68 +++++++++++++++++++++++++++-----------
>>   1 file changed, 48 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/libs/gnttab/minios.c b/tools/libs/gnttab/minios.c
>> index f78caadd30..c19f339c8c 100644
>> --- a/tools/libs/gnttab/minios.c
>> +++ b/tools/libs/gnttab/minios.c
>> @@ -28,18 +28,53 @@
>>   #include <sys/mman.h>
>>   
>>   #include <errno.h>
>> +#include <malloc.h>
>>   #include <unistd.h>
>>   
>>   #include "private.h"
>>   
>> -void minios_gnttab_close_fd(int fd);
>> +int minios_gnttab_close_fd(int fd);
> 
> Again, like evtchn, no need to forward declare.
> 
> 
> However, I've only just realised...
> 
>> +
>> +int minios_gnttab_close_fd(int fd)
>> +{
>> +    struct file *file = get_file_from_fd(fd);
>> +
>> +    gntmap_fini(file->dev);
>> +    free(file->dev);
>> +
>> +    return 0;
>> +}
> 
> The only reason this doesn't break the build is because the declaration
> is not in a header.  After this change, you've got the function
> returning int here, but declared as returning void as far as MiniOS is
> concerned.
> 
> Furthermore, we cannot fix this mess atomically now that minios has
> moved into a separate repo.  It's tolerable from an ABI point of view on
> x86, but I don't know for certain on other architectures.

Mini-OS is x86 only right now (well, it has some Arm parts in it, but
it is not in a state to be usable on Arm).

> The least bad way I can think of doing this would be to leave void
> minios_gnttab_close_fd(int fd) exactly as it was, and instead of
> converting it's use here, use a separate static function straight away
> for the file ops.  (Will be necessary anyway if you like my suggestion
> of passing file too).  Then in the whole function in "tools/libs: final
> cleanup making mini-os callbacks static", rather than just making it static.

I can change it if you really want.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

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

[Andrew Cooper]

On 12/01/2022 07:27, Juergen Gross wrote:
> On 11.01.22 21:08, Andrew Cooper wrote:
>
>> The least bad way I can think of doing this would be to leave void
>> minios_gnttab_close_fd(int fd) exactly as it was, and instead of
>> converting it's use here, use a separate static function straight away
>> for the file ops.  (Will be necessary anyway if you like my suggestion
>> of passing file too).  Then in the whole function in "tools/libs: final
>> cleanup making mini-os callbacks static", rather than just making it
>> static.
>
> I can change it if you really want.

Well, it will happen as a natural side effect of passing file* rather
than fd, but I do think it is a safer course of action.

~Andrew

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

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1337 bytes --]

On 12.01.22 08:22, Juergen Gross wrote:
> On 11.01.22 20:56, Andrew Cooper wrote:
>> 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
>>>   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.
> 
> I'll look into this.

Turns out that I can't use __attribute__((constructor)), as this is
supported through newlib already (the linker script contains everything
needed, but the activation is outside of Mini-OS).

I'll use something like initcall() instead.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3135 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]