diff mbox series

[4/7] Mini-OS: add 9pfs frontend

Message ID 20230203091809.14478-5-jgross@suse.com (mailing list archive)
State New, archived
Headers show
Series Mini-OS: ad minimal 9pfs support | expand

Commit Message

Jürgen Groß Feb. 3, 2023, 9:18 a.m. UTC
Add a frontend for the 9pfs PV device. For now add only the code needed
to connect to the backend and the related disconnect functionality. The
9pfs protocol support will be added later.

Due to its nature (ability to access files) the whole code is guarded
by "#ifdef HAVE_LIBC".

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 9pfront.c                     | 280 ++++++++++++++++++++++++++++++++++
 Config.mk                     |   1 +
 Makefile                      |   1 +
 arch/x86/testbuild/all-no     |   1 +
 arch/x86/testbuild/all-yes    |   1 +
 arch/x86/testbuild/newxen-yes |   1 +
 include/9pfront.h             |   7 +
 7 files changed, 292 insertions(+)
 create mode 100644 9pfront.c
 create mode 100644 include/9pfront.h

Comments

Samuel Thibault Feb. 6, 2023, 9:01 a.m. UTC | #1
Juergen Gross, le ven. 03 févr. 2023 10:18:06 +0100, a ecrit:
> +void *init_9pfront(unsigned int id, const char *mnt)
> +{
[...]
> +    free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));

Better check for errors, otherwise the rest will hang without useful
feedback.

> +    for ( v = version; *v; v++ )
> +    {
> +        if ( strtoul(v, &v, 10) == 1 )
> +        {
> +            v = NULL;
> +            break;

This looks fragile? if version is "2.1" it will accept it apparently? I
guess better check whether strtoul did read a number, and in that case
break the loop anyway, successfully if the number is 1 and with failure
otherwise.

> +        }
> +    }
> +    free(version);
> +    if ( v )
> +    {
> +        reason = "version 1 not supported";
> +        goto err;
> +    }

This looks odd: when number 1 is detected this breaks out successfully,
while the error message otherwise says that it's version 1 which is not
supported? Is the message supposed to be "version greater than 1 not
supported"?

> + err:
> +    if ( bepath[0] )
> +        free(xenbus_unwatch_path_token(XBT_NIL, bepath, bepath));
> +    if ( msg )
> +        printk("9pfsfront add %u failed, error %s accessing Xenstore\n",
> +               id, msg);
> +    else
> +        printk("9pfsfront add %u failed, %s\n", id, reason);
> +    free_9pfront(dev);

In case of early errors, this will try to free uninitialized evtchn,
ring_ref, etc.

> +    free(msg);
> +    return NULL;
> +}

Samuel
Jürgen Groß Feb. 6, 2023, 9:22 a.m. UTC | #2
On 06.02.23 10:01, Samuel Thibault wrote:
> Juergen Gross, le ven. 03 févr. 2023 10:18:06 +0100, a ecrit:
>> +void *init_9pfront(unsigned int id, const char *mnt)
>> +{
> [...]
>> +    free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));
> 
> Better check for errors, otherwise the rest will hang without useful
> feedback.

This is a common pattern in Mini-OS frontends.

I can add an error check, of course.

> 
>> +    for ( v = version; *v; v++ )
>> +    {
>> +        if ( strtoul(v, &v, 10) == 1 )
>> +        {
>> +            v = NULL;
>> +            break;
> 
> This looks fragile? if version is "2.1" it will accept it apparently? I
> guess better check whether strtoul did read a number, and in that case
> break the loop anyway, successfully if the number is 1 and with failure
> otherwise.

Versions are defined to be integers.

I can add checks for sanitizing backend written data, but I'm not sure we
need that. In case the backend wants to fool us, it can easily tell us to
support version 1 even if it doesn't.

> 
>> +        }
>> +    }
>> +    free(version);
>> +    if ( v )
>> +    {
>> +        reason = "version 1 not supported";
>> +        goto err;
>> +    }
> 
> This looks odd: when number 1 is detected this breaks out successfully,
> while the error message otherwise says that it's version 1 which is not
> supported? Is the message supposed to be "version greater than 1 not
> supported"?

I can change the message to "Backend doesn't support version 1".

> 
>> + err:
>> +    if ( bepath[0] )
>> +        free(xenbus_unwatch_path_token(XBT_NIL, bepath, bepath));
>> +    if ( msg )
>> +        printk("9pfsfront add %u failed, error %s accessing Xenstore\n",
>> +               id, msg);
>> +    else
>> +        printk("9pfsfront add %u failed, %s\n", id, reason);
>> +    free_9pfront(dev);
> 
> In case of early errors, this will try to free uninitialized evtchn,
> ring_ref, etc.

Oh right, I need to check those for being not 0.


Juergen
Samuel Thibault Feb. 6, 2023, 9:48 a.m. UTC | #3
Juergen Gross, le lun. 06 févr. 2023 10:22:10 +0100, a ecrit:
> On 06.02.23 10:01, Samuel Thibault wrote:
> > Juergen Gross, le ven. 03 févr. 2023 10:18:06 +0100, a ecrit:
> > > +void *init_9pfront(unsigned int id, const char *mnt)
> > > +{
> > [...]
> > > +    free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));
> > 
> > Better check for errors, otherwise the rest will hang without useful
> > feedback.
> 
> This is a common pattern in Mini-OS frontends.

Ah, indeed. Ok, then.

> > > +    for ( v = version; *v; v++ )
> > > +    {
> > > +        if ( strtoul(v, &v, 10) == 1 )
> > > +        {
> > > +            v = NULL;
> > > +            break;
> > 
> > This looks fragile? if version is "2.1" it will accept it apparently? I
> > guess better check whether strtoul did read a number, and in that case
> > break the loop anyway, successfully if the number is 1 and with failure
> > otherwise.
> 
> Versions are defined to be integers.

Ah, it's a list of versions?
(I don't know the protocol at all).

> I can add checks for sanitizing backend written data, but I'm not sure we
> need that. In case the backend wants to fool us, it can easily tell us to
> support version 1 even if it doesn't.

Not necessarily fooling, but misprogramming :)

> > > +        }
> > > +    }
> > > +    free(version);
> > > +    if ( v )
> > > +    {
> > > +        reason = "version 1 not supported";
> > > +        goto err;
> > > +    }
> > 
> > This looks odd: when number 1 is detected this breaks out successfully,
> > while the error message otherwise says that it's version 1 which is not
> > supported? Is the message supposed to be "version greater than 1 not
> > supported"?
> 
> I can change the message to "Backend doesn't support version 1".

Aah, yes indeed, that was the part I missed for understanding it all :)

Samuel
diff mbox series

Patch

diff --git a/9pfront.c b/9pfront.c
new file mode 100644
index 00000000..cf4b5cb3
--- /dev/null
+++ b/9pfront.c
@@ -0,0 +1,280 @@ 
+/*
+ * Minimal 9pfs PV frontend for Mini-OS.
+ * Copyright (c) 2023 Juergen Gross, SUSE Software Solution GmbH
+ */
+
+#include <mini-os/os.h>
+#include <mini-os/lib.h>
+#include <mini-os/events.h>
+#include <mini-os/gnttab.h>
+#include <mini-os/xenbus.h>
+#include <mini-os/xmalloc.h>
+#include <errno.h>
+#include <xen/io/9pfs.h>
+#include <mini-os/9pfront.h>
+
+#ifdef HAVE_LIBC
+struct dev_9pfs {
+    int id;
+    char nodename[20];
+    unsigned int dom;
+    char *backend;
+
+    char *tag;
+    const char *mnt;
+
+    struct xen_9pfs_data_intf *intf;
+    struct xen_9pfs_data data;
+    RING_IDX prod_pvt_out;
+    RING_IDX cons_pvt_in;
+
+    grant_ref_t ring_ref;
+    evtchn_port_t evtchn;
+    unsigned int ring_order;
+    xenbus_event_queue events;
+};
+
+#define DEFAULT_9PFS_RING_ORDER  4
+
+static unsigned int ftype_9pfs;
+
+static void intr_9pfs(evtchn_port_t port, struct pt_regs *regs, void *data)
+{
+}
+
+static int open_9pfs(struct mount_point *mnt, const char *pathname, int flags,
+                     mode_t mode)
+{
+    errno = ENOSYS;
+
+    return -1;
+}
+
+static void free_9pfront(struct dev_9pfs *dev)
+{
+    unsigned int i;
+
+    if ( dev->data.in && dev->intf )
+    {
+        for ( i = 0; i < (1 << dev->ring_order); i++ )
+            gnttab_end_access(dev->intf->ref[i]);
+        free_pages(dev->data.in, dev->ring_order);
+    }
+    unbind_evtchn(dev->evtchn);
+    gnttab_end_access(dev->ring_ref);
+    free_page(dev->intf);
+    free(dev->backend);
+    free(dev->tag);
+    free(dev);
+}
+
+void *init_9pfront(unsigned int id, const char *mnt)
+{
+    struct dev_9pfs *dev;
+    char *msg;
+    char *reason = "";
+    xenbus_transaction_t xbt;
+    int retry = 1;
+    char bepath[64] = { 0 };
+    XenbusState state;
+    unsigned int i;
+    void *addr;
+    char *version;
+    char *v;
+
+    printk("9pfsfront add %u, for mount at %s\n", id, mnt);
+    dev = malloc(sizeof(*dev));
+    memset(dev, 0, sizeof(*dev));
+    snprintf(dev->nodename, sizeof(dev->nodename), "device/9pfs/%u", id);
+    dev->id = id;
+
+    msg = xenbus_read_unsigned(XBT_NIL, dev->nodename, "backend-id", &dev->dom);
+    if ( msg )
+        goto err;
+    msg = xenbus_read_string(XBT_NIL, dev->nodename, "backend", &dev->backend);
+    if ( msg )
+        goto err;
+    msg = xenbus_read_string(XBT_NIL, dev->nodename, "tag", &dev->tag);
+    if ( msg )
+        goto err;
+
+    snprintf(bepath, sizeof(bepath), "%s/state", dev->backend);
+    free(xenbus_watch_path_token(XBT_NIL, bepath, bepath, &dev->events));
+    state = xenbus_read_integer(bepath);
+    while ( msg == NULL && state < XenbusStateInitWait )
+        msg = xenbus_wait_for_state_change(bepath, &state, &dev->events);
+    if ( msg || state != XenbusStateInitWait )
+    {
+        reason = "illegal backend state";
+        goto err;
+    }
+
+    msg = xenbus_read_unsigned(XBT_NIL, dev->backend, "max-ring-page-order",
+                               &dev->ring_order);
+    if ( msg )
+        goto err;
+    if ( dev->ring_order > DEFAULT_9PFS_RING_ORDER )
+        dev->ring_order = DEFAULT_9PFS_RING_ORDER;
+
+    msg = xenbus_read_string(XBT_NIL, dev->backend, "versions", &version);
+    if ( msg )
+        goto err;
+    for ( v = version; *v; v++ )
+    {
+        if ( strtoul(v, &v, 10) == 1 )
+        {
+            v = NULL;
+            break;
+        }
+    }
+    free(version);
+    if ( v )
+    {
+        reason = "version 1 not supported";
+        goto err;
+    }
+
+    dev->ring_ref = gnttab_alloc_and_grant((void **)&dev->intf);
+    memset(dev->intf, 0, PAGE_SIZE);
+    if ( evtchn_alloc_unbound(dev->dom, intr_9pfs, dev, &dev->evtchn) )
+    {
+        reason = "no event channel";
+        goto err;
+    }
+    dev->intf->ring_order = dev->ring_order;
+    dev->data.in = (void *)alloc_pages(dev->ring_order);
+    dev->data.out = dev->data.in + XEN_FLEX_RING_SIZE(dev->ring_order);
+    for ( i = 0; i < (1 << dev->ring_order); i++ )
+    {
+        addr = dev->data.in + i * PAGE_SIZE;
+        dev->intf->ref[i] = gnttab_grant_access(dev->dom, virt_to_mfn(addr), 0);
+    }
+
+    while ( retry )
+    {
+        msg = xenbus_transaction_start(&xbt);
+        if ( msg )
+        {
+            free(msg);
+            msg = NULL;
+            reason = "starting transaction";
+            goto err;
+        }
+
+        msg = xenbus_printf(xbt, dev->nodename, "version", "%u", 1);
+        if ( msg )
+            goto err_tr;
+        msg = xenbus_printf(xbt, dev->nodename, "num-rings", "%u", 1);
+        if ( msg )
+            goto err_tr;
+        msg = xenbus_printf(xbt, dev->nodename, "ring-ref0", "%u",
+                            dev->ring_ref);
+        if ( msg )
+            goto err_tr;
+        msg = xenbus_printf(xbt, dev->nodename, "event-channel-0", "%u",
+                            dev->evtchn);
+        if ( msg )
+            goto err_tr;
+        msg = xenbus_printf(xbt, dev->nodename, "state", "%u",
+                            XenbusStateInitialised);
+        if ( msg )
+            goto err_tr;
+
+        free(xenbus_transaction_end(xbt, 0, &retry));
+    }
+
+    state = xenbus_read_integer(bepath);
+    while ( msg == NULL && state < XenbusStateConnected )
+        msg = xenbus_wait_for_state_change(bepath, &state, &dev->events);
+    if ( msg || state != XenbusStateConnected )
+    {
+        reason = "illegal backend state";
+        goto err;
+    }
+
+    msg = xenbus_printf(XBT_NIL, dev->nodename, "state", "%u",
+                        XenbusStateConnected);
+    if ( msg )
+        goto err;
+
+    unmask_evtchn(dev->evtchn);
+
+    dev->mnt = mnt;
+    if ( mount(dev->mnt, dev, open_9pfs) )
+    {
+        reason = "mount failed";
+        goto err;
+    }
+
+    return dev;
+
+ err_tr:
+    free(xenbus_transaction_end(xbt, 1, &retry));
+
+ err:
+    if ( bepath[0] )
+        free(xenbus_unwatch_path_token(XBT_NIL, bepath, bepath));
+    if ( msg )
+        printk("9pfsfront add %u failed, error %s accessing Xenstore\n",
+               id, msg);
+    else
+        printk("9pfsfront add %u failed, %s\n", id, reason);
+    free_9pfront(dev);
+    free(msg);
+    return NULL;
+}
+
+void shutdown_9pfront(void *dev)
+{
+    struct dev_9pfs *dev9p = dev;
+    char bepath[64];
+    XenbusState state;
+    char *msg;
+    char *reason = "";
+
+    umount(dev9p->mnt);
+    snprintf(bepath, sizeof(bepath), "%s/state", dev9p->backend);
+
+    msg = xenbus_printf(XBT_NIL, dev9p->nodename, "state", "%u",
+                        XenbusStateClosing);
+    if ( msg )
+        goto err;
+
+    state = xenbus_read_integer(bepath);
+    while ( msg == NULL && state < XenbusStateClosing)
+        msg = xenbus_wait_for_state_change(bepath, &state, &dev9p->events);
+    if ( msg || state != XenbusStateClosing )
+    {
+        reason = "illegal backend state";
+        goto err;
+    }
+
+    msg = xenbus_printf(XBT_NIL, dev9p->nodename, "state", "%u",
+                        XenbusStateClosed);
+    if ( msg )
+        goto err;
+
+    free_9pfront(dev9p);
+
+    return;
+
+ err:
+    if ( msg )
+        printk("9pfsfront shutdown %u failed, error %s accessing Xenstore\n",
+               dev9p->id, msg);
+    else
+        printk("9pfsfront shutdown %u failed, %s\n", dev9p->id, reason);
+    free(msg);
+}
+
+static const struct file_ops ops_9pfs = {
+    .name = "9pfs",
+};
+
+__attribute__((constructor))
+static void initialize_9pfs(void)
+{
+    ftype_9pfs = alloc_file_type(&ops_9pfs);
+}
+
+#endif /* HAVE_LIBC */
diff --git a/Config.mk b/Config.mk
index 1a24b30e..677e93df 100644
--- a/Config.mk
+++ b/Config.mk
@@ -187,6 +187,7 @@  CONFIG-n += CONFIG_QEMU_XS_ARGS
 CONFIG-n += CONFIG_TEST
 CONFIG-n += CONFIG_PCIFRONT
 CONFIG-n += CONFIG_TPMFRONT
+CONFIG-n += CONFIG_9PFRONT
 CONFIG-n += CONFIG_TPM_TIS
 CONFIG-n += CONFIG_TPMBACK
 CONFIG-n += CONFIG_BALLOON
diff --git a/Makefile b/Makefile
index 747c7c01..7ee181a2 100644
--- a/Makefile
+++ b/Makefile
@@ -39,6 +39,7 @@  SUBDIRS := lib
 src-$(CONFIG_BLKFRONT) += blkfront.c
 src-$(CONFIG_CONSFRONT) += consfront.c
 src-$(CONFIG_TPMFRONT) += tpmfront.c
+src-$(CONFIG_9PFRONT) += 9pfront.c
 src-$(CONFIG_TPM_TIS) += tpm_tis.c
 src-$(CONFIG_TPMBACK) += tpmback.c
 src-y += console.c
diff --git a/arch/x86/testbuild/all-no b/arch/x86/testbuild/all-no
index f79a1012..5b3e99ed 100644
--- a/arch/x86/testbuild/all-no
+++ b/arch/x86/testbuild/all-no
@@ -12,6 +12,7 @@  CONFIG_NETFRONT = n
 CONFIG_FBFRONT = n
 CONFIG_KBDFRONT = n
 CONFIG_CONSFRONT = n
+CONFIG_9PFRONT = n
 CONFIG_XENBUS = n
 CONFIG_LIBXS = n
 CONFIG_LWIP = n
diff --git a/arch/x86/testbuild/all-yes b/arch/x86/testbuild/all-yes
index faa3af32..8ae489a4 100644
--- a/arch/x86/testbuild/all-yes
+++ b/arch/x86/testbuild/all-yes
@@ -12,6 +12,7 @@  CONFIG_NETFRONT = y
 CONFIG_FBFRONT = y
 CONFIG_KBDFRONT = y
 CONFIG_CONSFRONT = y
+CONFIG_9PFRONT = y
 CONFIG_XENBUS = y
 CONFIG_LIBXS = y
 CONFIG_BALLOON = y
diff --git a/arch/x86/testbuild/newxen-yes b/arch/x86/testbuild/newxen-yes
index dc83e670..ee27a328 100644
--- a/arch/x86/testbuild/newxen-yes
+++ b/arch/x86/testbuild/newxen-yes
@@ -12,6 +12,7 @@  CONFIG_NETFRONT = y
 CONFIG_FBFRONT = y
 CONFIG_KBDFRONT = y
 CONFIG_CONSFRONT = y
+CONFIG_9PFRONT = y
 CONFIG_XENBUS = y
 CONFIG_LIBXS = y
 CONFIG_BALLOON = y
diff --git a/include/9pfront.h b/include/9pfront.h
new file mode 100644
index 00000000..722ec564
--- /dev/null
+++ b/include/9pfront.h
@@ -0,0 +1,7 @@ 
+#ifndef __9PFRONT_H__
+#define __9PFRONT_H__
+
+void *init_9pfront(unsigned int id, const char *mnt);
+void shutdown_9pfront(void *dev);
+
+#endif /* __9PFRONT_H__ */