[PATCH 3/4] libmachdev: Introduce startup notification for clean rumpdisk shutdown
NB: Reboot still hangs on diskfs_S_startup_dosync() even though that
times out and then rumpdisk shuts down cleanly.
---
libmachdev/Makefile | 6 +--
libmachdev/ds_routines.c | 10
libmachdev/machdev-device_emul.h | 1 +
libmachdev/machdev.h | 1 +
libmachdev/startup.c | 86
libmachdev/startup.h | 28 +++
libmachdev/trivfs_server.c | 49 +-
libmachdev/trivfs_server.h | 30 +++
rumpdisk/block-rump.c| 16 +-
9 files changed, 221 insertions(+), 6 deletions(-)
create mode 100644 libmachdev/startup.c
create mode 100644 libmachdev/startup.h
create mode 100644 libmachdev/trivfs_server.h
diff --git a/libmachdev/Makefile b/libmachdev/Makefile
index 15b98cf1..bb4fcd8a 100644
--- a/libmachdev/Makefile
+++ b/libmachdev/Makefile
@@ -19,10 +19,10 @@ dir := libmachdev
makemode := library
libname = libmachdev
-SRCS = ds_routines.c trivfs_server.c \
- deviceServer.c notifyServer.c mach_i386Server.c
+SRCS = ds_routines.c trivfs_server.c startup_notifyServer.c \
+ deviceServer.c notifyServer.c mach_i386Server.c startup.c
-LCLHDRS = machdev.h machdev-device_emul.h machdev-dev_hdr.h mach_device.h
+LCLHDRS = machdev.h machdev-device_emul.h machdev-dev_hdr.h mach_device.h
startup.h
installhdrs = machdev.h machdev-device_emul.h machdev-dev_hdr.h
HURDLIBS = ports trivfs
LDLIBS += -lpthread -lmachuser
diff --git a/libmachdev/ds_routines.c b/libmachdev/ds_routines.c
index 53e0c080..64080176 100644
--- a/libmachdev/ds_routines.c
+++ b/libmachdev/ds_routines.c
@@ -317,6 +317,16 @@ void machdev_device_init()
}
}
+void machdev_device_shutdown()
+{
+ int i;
+ for (i = 0; i < num_emul; i++)
+{
+ if (emulation_list[i]->shutdown)
+emulation_list[i]->shutdown();
+}
+}
+
static int
demuxer (mach_msg_header_t *inp, mach_msg_header_t *outp)
{
diff --git a/libmachdev/machdev-device_emul.h b/libmachdev/machdev-device_emul.h
index ab1bd92b..edf79b96 100644
--- a/libmachdev/machdev-device_emul.h
+++ b/libmachdev/machdev-device_emul.h
@@ -64,6 +64,7 @@ struct machdev_device_emulation_ops
recnum_t, vm_offset_t, vm_size_t);
io_return_t (*writev_trap) (void *, dev_mode_t,
recnum_t, io_buf_vec_t *, vm_size_t);
+ void (*shutdown) (void);
};
#endif /* _MACHDEV_DEVICE_EMUL_H_ */
diff --git a/libmachdev/machdev.h b/libmachdev/machdev.h
index 55a56e0d..5f07d35f 100644
--- a/libmachdev/machdev.h
+++ b/libmachdev/machdev.h
@@ -30,6 +30,7 @@
void machdev_register (struct machdev_device_emulation_ops *ops);
void machdev_device_init(void);
+void machdev_device_shutdown(void);
void * machdev_server(void *);
error_t machdev_create_device_port (size_t size, void *result);
int machdev_trivfs_init(mach_port_t bootstrap_resume_task, const char *name,
mach_port_t *bootstrap);
diff --git a/libmachdev/startup.c b/libmachdev/startup.c
new file mode 100644
index ..49eec502
--- /dev/null
+++ b/libmachdev/startup.c
@@ -0,0 +1,86 @@
+/*
+ Copyright (C) 2020 Free Software Foundation, Inc.
+
+ The GNU Hurd is free software; you can redistribute it and/or
+ modify it under the terms of the GNU General Public License as
+ published by the Free Software Foundation; either version 2, or (at
+ your option) any later version.
+
+ The GNU Hurd is distributed in the hope that it will be useful, but
+ WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with the GNU Hurd. If not, see
[PATCH 1/4] diskfs: Add RPC for fsys_init to bootstrap if present
---
libdiskfs/boot-start.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/libdiskfs/boot-start.c b/libdiskfs/boot-start.c
index 29b8acc6..fa59e1b2 100644
--- a/libdiskfs/boot-start.c
+++ b/libdiskfs/boot-start.c
@@ -518,7 +518,9 @@ diskfs_S_fsys_init (struct diskfs_control *pt,
if (diskfs_exec_server_task != MACH_PORT_NULL)
{
+ mach_port_t bootstrap;
process_t execprocess;
+
err = proc_task2proc (procserver, diskfs_exec_server_task, );
assert_perror_backtrace (err);
@@ -533,6 +535,17 @@ diskfs_S_fsys_init (struct diskfs_control *pt,
execprocess, MACH_MSG_TYPE_COPY_SEND));
mach_port_deallocate (mach_task_self (), execprocess);
+ /* Give the real bootstrap filesystem an fsys_init RPC of its own */
+ err = task_get_bootstrap_port (mach_task_self (), );
+ assert_perror_backtrace (err);
+ if (bootstrap != MACH_PORT_NULL)
+{
+ err = fsys_init (bootstrap, procserver, MACH_MSG_TYPE_COPY_SEND,
+ authhandle);
+ mach_port_deallocate (mach_task_self (), bootstrap);
+ assert_perror_backtrace (err);
+}
+
/* We don't need this anymore. */
mach_port_deallocate (mach_task_self (), diskfs_exec_server_task);
diskfs_exec_server_task = MACH_PORT_NULL;
--
2.25.1
[PATCH 2/4] diskfs: Don't deallocate if dotdot is null in fsys-getroot.c
---
libdiskfs/fsys-getroot.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/libdiskfs/fsys-getroot.c b/libdiskfs/fsys-getroot.c
index 735f359a..2c0d15dd 100644
--- a/libdiskfs/fsys-getroot.c
+++ b/libdiskfs/fsys-getroot.c
@@ -194,7 +194,8 @@ diskfs_S_fsys_getroot (struct diskfs_control *pt,
if (! err)
{
- mach_port_deallocate (mach_task_self (), dotdot);
+ if (dotdot != MACH_PORT_NULL)
+mach_port_deallocate (mach_task_self (), dotdot);
*retry = FS_RETRY_NORMAL;
*retryname = '\0';
*returned_port = ports_get_right (newpi);
--
2.25.1
[PATCH 4/4] rumpdisk: Install as translator when bootstrapping
NB: Not sure why this seems to have no effect on /dev/rumpdisk
Am I setting the translator on the wrong port?
---
rumpdisk/main.c | 29 +
1 file changed, 29 insertions(+)
diff --git a/rumpdisk/main.c b/rumpdisk/main.c
index 27a8ea38..68fb8be2 100644
--- a/rumpdisk/main.c
+++ b/rumpdisk/main.c
@@ -31,6 +31,8 @@
#include
#include
+#define DEV_RUMPDISK "/dev/rumpdisk"
+
/* TODO: Add api to pciaccess to allow selecting backend.
* For now we pretend to be the arbiter and claim x86 method.
*/
@@ -97,6 +99,28 @@ static struct argp_child empty_argp_children[] = {{0}};
static struct argp rumpdisk_argp = {options, parse_opt, 0, 0,
empty_argp_children};
static const struct argp *rumpdisk_argp_bootup = _argp;
+static void
+install_as_translator (mach_port_t rumpdisk_port)
+{
+ error_t err;
+ file_t node;
+
+ node = file_name_lookup (DEV_RUMPDISK, O_NOTRANS, 0);
+ if (! MACH_PORT_VALID (node))
+{
+ if (errno == ENOENT)
+mach_print("Missing DEV_RUMPDISK\n");
+ return;
+}
+
+ err = file_set_translator (node,
+ 0, FS_TRANS_SET, 0,
+ NULL, 0,
+ rumpdisk_port, MACH_MSG_TYPE_COPY_SEND);
+ mach_port_deallocate (mach_task_self (), node);
+ assert_perror_backtrace (err);
+}
+
int
main (int argc, char **argv)
{
@@ -119,10 +143,15 @@ main (int argc, char **argv)
rump_register_block ();
machdev_device_init ();
machdev_trivfs_init (bootstrap_resume_task, "fs", );
+
err = pthread_create (, NULL, machdev_server, NULL);
if (err)
return err;
pthread_detach (t);
+
+ if (bootstrap != MACH_PORT_NULL)
+install_as_translator (bootstrap);
+
machdev_trivfs_server (bootstrap);
return 0;
}
--
2.25.1
Re: [PATCH] SMP initialization: detection and enumeration
> @@ -170,6 +172,14 @@ void machine_init(void)
> linux_init();
> #endif
>
> +#if NCPUS > 1
> + int smp_success = smp_init();
> +
> + if(smp_success != 0) {
> +printf("Error: no SMP found");
> +}
> +#endif /* NCPUS > 1 */
> +
> There's a bogus indent here. Tell your editor to show tab vs spaces to
> avoid introducing incoherent combinations.
Thanks. I'll fix It. I'm using CodeBlocks as editor, and this doesn't show
the tabs properly.
> Rather fold this into the commits that add these files. Otherwise the
> code is not even getting compiled when checking out the other commits.
Ok, I'll add It.
> Juan didn't assign copyright to the FSF. We usually do this, so we don't
> have long-term licensing concerns. Could you send his email address so
> we can send him the paper work? Alternatively the file could be put
> under a BSD license, which poses way less concerns.
Ok. I will tell him to solve this.
> +#ifdef __i386__
> +#include
> +#include
> We avoid putting arch-specific code in kern/, and rather put it in
> i386/i386. Note that you can have smp.h in i386/i386, and make other
> files include
Ok. Then I will fix It. I will add an i386/i386/smp.h to add these includes.
> > +static int acpi_apic_parse_table(struct acpi_apic *apic);
> This doesn't exist?
It's possible. Maybe I renamed this function, and I forget to rename its
declaration (or the inverse). I think I will rename the definition: It's
more coherent.
> @@ -46,9 +46,12 @@ AC_DEFINE([BOOTSTRAP_SYMBOLS], [0],
[BOOTSTRAP_SYMBOLS])
> # Multiprocessor support is still broken.
> AH_TEMPLATE([MULTIPROCESSOR], [set things up for a uniprocessor])
> mach_ncpus=1
> +AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])
> +
> AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])
> Err, this seems spurious?
Yes, It's a mistake. You can remove the duplicated line.
> Simply return rather than using a variable.
I prefer using a temporary variable to ease debugging in case of error.
Direct return can be difficult to debug in this case.
> You can just return it.
I prefer to avoid multiple return, to ease the reading. A single return can
be easy to read.
> That's too verbose for production?
I can remove the prints, If necessary.
> So you picked it up from somewhere?
> Since it's a BSD license it is fine, just making sure that it is the
> proper copyright notice.
This file existed in older versions of gnumach. Thomas removed It in 2007.
Simply, I recovered It and updated.
http://git.savannah.gnu.org/cgit/hurd/gnumach.git/tree/i386/imps/apic.h?id=f20666fc6b0471738829363e20c27f282f65dbf2
> Rather put the comment on the right of the corresponding field, that will
look much better.
Ok, I'll fix It.
> Ditto, invert the if to avoid indentation. Also notice the memory leak
> in that case, you'll want to make the kalloc only in the success case.
> +for (i = 0; i < apic_data.ncpus; i++)
> +new_list[i] = old_list[i];
> +
> +apic_data.cpu_lapic_list = new_list;
> +kfree(old_list);
Thanks, I'll solve this.
> I'm wondering: is it really *that* simple to get the current cpu number,
> just read a memory location? I'm surprised that this would provide
> different results on different cpus.
The APIC ID is stored in the Local APIC of each cpu. This address is common
for all Local APIC: accessing this from each cpu, it shows the Local APIC
of this cpu.
By example, if you access this address from cpu1, you can see the Local
APIC of cpu1.
About the rest of the comments, I will try to fix it soon.
Thanks
El mar., 28 jul. 2020 a las 1:45, Samuel Thibault ()
escribió:
> Hello,
>
> Thanks for your work, we're getting closer!
>
> Almudena Garcia, le lun. 27 juil. 2020 21:15:10 +0200, a ecrit:
> > @@ -170,6 +172,14 @@ void machine_init(void)
> > linux_init();
> > #endif
> >
> > +#if NCPUS > 1
> > + int smp_success = smp_init();
> > +
> > + if(smp_success != 0) {
> > +printf("Error: no SMP found");
> > +}
> > +#endif /* NCPUS > 1 */
> > +
>
> There's a bogus indent here. Tell your editor to show tab vs spaces to
> avoid introducing incoherent combinations.
>
> > Add smp.c, smp.h, apic.c, apic.h, acpi_parse_apic.c and
> acpi_parse_apic,h to compilation list
> > ---
> > Makefrag.am | 12
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/Makefrag.am b/Makefrag.am
> > index ea612275..69ac31a1 100644
> > --- a/Makefrag.am
> > +++ b/Makefrag.am
> > @@ -122,6 +122,18 @@ EXTRA_DIST += \
> > ipc/notify.defs
> >
> >
> > +#
> > +# SMP implementation (APIC, ACPI, etc)
> > +#
> > +
> > +libkernel_a_SOURCES += \
> > + i386/i386at/acpi_parse_apic.h \
> > + i386/i386at/acpi_parse_apic.c \
> > + i386/i386/apic.h \
> > + i386/i386/apic.c \
> > + kern/smp.h \
> > + kern/smp.c
>
> Rather fold this into the commits that add these files. Otherwise the
> code is not even getting compiled when checking out the other commits.
>
> >
Re: [PATCH] SMP initialization: detection and enumeration
> 256 is a common default
> (I know that's what FreeBSD uses for x86) as the APIC IDs are 8-bit,
> and I think this should be 256 rather than 255 too
I agree. It's a mistake.
> Though I do
> question what the point of mach_ncpus is if it isn't being used to
> determine NCPUS.
Currently, mach_ncpus is used to define NCPUS. I keep It, because It can be
useful to enable/disable the SMP support, and this patch is low-invasive,
avoiding to broke more things.
> AC_DEFINE_UNQUOTED([NCPUS], [$mach_ncpus], [number of CPUs])
This define can be removed, of course. I don't know because I keep It
there.
>> +return apic_id;
This line doesn't get the Kernel ID, but the APIC ID.
The APIC ID can be obtained from the common Local APIC address, which
points to the Local APIC of the current CPU (if you access this address
from cpu1, you get the APIC ID of cpu1).
The ACPI tables also stores the APIC ID of each CPU, so I enumerate the
processors in an array using this. The array is indexed by Kernel ID (the
logical ID), and stores the APIC ID for each cpu.
El mar., 28 jul. 2020 a las 2:19, Jessica Clarke ()
escribió:
> On 28 Jul 2020, at 00:44, Samuel Thibault wrote:
> > Almudena Garcia, le lun. 27 juil. 2020 21:15:10 +0200, a ecrit:
> >> [if [ $mach_ncpus -gt 1 ]; then]
> >> AC_DEFINE([MULTIPROCESSOR], [1], [set things up for a multiprocessor])
> >> + AC_DEFINE_UNQUOTED([NCPUS], [255], [number of CPUs])
> >
> > Perhaps useless to define to so many by default?
>
> That's fairly normal, having some static per-CPU data structures with
> an upper limit on the number of CPUs supported. 256 is a common default
> (I know that's what FreeBSD uses for x86) as the APIC IDs are 8-bit,
> and I think this should be 256 rather than 255 too. Though I do
> question what the point of mach_ncpus is if it isn't being used to
> determine NCPUS. Either always do `AC_DEFINE_UNQUOTED([NCPUS], [255],
> [number of CPUs])` (in which case you don't need the extra define) or
> make it mach_smp instead. Also, I personally don't like having two
> "calls" to AC_DEFINE_UNQUOTED; I know it's autoconf and m4 and all that
> so calls aren't always quite what you think, but it still just looks
> weird and confusing.
>
> >> +/*
> >> + * apic_get_current_cpu: returns the apic_id of current cpu.
> >> + */
> >> +uint16_t
> >> +apic_get_current_cpu(void)
> >> +{
> >> +uint16_t apic_id;
> >> +
> >> +if(lapic == NULL)
>
> You have a bunch of statements like this with missing whitespace.
>
> >> +return apic_id;
> >
> > I'm wondering: is it really *that* simple to get the current cpu number,
> > just read a memory location? I'm surprised that this would provide
> > different results on different cpus.
>
> No, it's not. They can be non-sequential, and there's no guarantee that
> the BSP's is even 0, but there'll be a bunch of simple chips where both
> are true (especially in emulators etc). Normally the current CPU's ID
> would be stored in per-CPU data, and each AP is told (or knows how to
> look up based on its hardware ID) its logical ID when being brought
> online.
>
> Jess
>
>
