Re: [PATCH] machdev,pci,rump: Reuse shutdown notify from machdev

[Samuel Thibault]

Hello,

Just to make sure, could you post the exact grub.cfg module setup you
are using?

Damien Zammit, le ven. 12 mars 2021 23:04:47 +1100, a ecrit:
> diff --git a/libmachdev/trivfs_server.c b/libmachdev/trivfs_server.c
> index e3e4045d..8ee6be65 100644
> --- a/libmachdev/trivfs_server.c
> +++ b/libmachdev/trivfs_server.c
> @@ -245,6 +245,39 @@ trivfs_S_fsys_startup (mach_port_t bootport,
>    return 0;
>  }
>  
> +error_t
> +machdev_fsys_init(mach_port_t proc, mach_port_t auth)
> +{
> +  /* No more nesting of servers */
> +  if (!machdev_bootport)
> +    return 0;
> +
> +  return fsys_init (machdev_bootport, proc, MACH_MSG_TYPE_COPY_SEND, auth);
> +}

Rather inline it inside trivfs_S_fsys_init, and rather use
task_get_bootstrap_port() to get the bootstrap port, rather than relying
on the machdev_bootport variable which I believe we don't need.

> +void
> +machdev_use_this_port(mach_port_t bootstrap)
> +{
> +  machdev_bootport = bootstrap;
> +}

This does not seem actually needed? (since it's always called with
MACH_PORT_NULL and the machdev_bootport is already originally null
anyway.

> +void
> +machdev_get_ports(void)
> +{
> +  int i;
> +  task_t parent_task;
> +
> +  /* If we are not the bootstrap filesystem, get this port from parent */
> +  if (!machdev_bootport)
> +    {
> +      /* Need to synchronise with parent to wait for fsys_startup */
> +      for (i = 0; i < 5 && machdev_bootport == MACH_PORT_NULL; i++, sleep(1))
> +        task_get_bootstrap_port (mach_task_self (), &machdev_bootport);

We really don't want this long-term :)

Actually I don't understand: the rumpdisk's bootstrap port is supposed
to be set by pci-arbiter even before it resumes the rumpdisk task (in
machdev_trivfs_init), and thus before rumpdisk executes at all. Just
like ext2fs' bootstrap port is set by rumpdisk even before it resumes
the ext2fs task. So I don't see why we'd have to wait at all here.

> +  fsys_getpriv (machdev_bootport, &_hurd_host_priv, &_hurd_device_master, 
> &parent_task);
> +}

This should really be already working as soon as the task is running,
since the parent task is supposed to set the bootstrap port of its child
task before resuming it. In the pci-arbiter case that should be getting
done in machdev_trivfs_init: after creating its own control port, it'd
resume the rumpdisk translator.

> diff --git a/rumpdisk/main.c b/rumpdisk/main.c
> index a26e17cb..8bed7a50 100644
> --- a/rumpdisk/main.c
> +++ b/rumpdisk/main.c
> @@ -117,6 +112,7 @@ main (int argc, char **argv)
>      }
>  
>    rump_register_block ();
> +  machdev_get_ports();
>    machdev_device_init ();
>    machdev_trivfs_init (bootstrap_resume_task, "rumpdisk", "/dev/rumpdisk", 
> &bootstrap);
>    err = pthread_create (&t, NULL, machdev_server, NULL);

Rather inline the content of machdev_get_ports() in machdev_trivfs_init,
since that's where bootstrap stuff is managed.

Looking at it, machdev_trivfs_init seems bogus, and that brings
bootstrap port confusion: currently it does

- if it has a child bootstrap_resume_task, resume it and create a
  control port on oneself and return that as bootstrap port

- otherwise, just get the bootstrap port with task_get_bootstrap_port.

Here, rumpdisk needs *both*!

- it needs to resume its child (ext2fs).
- *and* it needs to get its bootstrap port with task_get_bootstrap_port.

While pci-arbiter needs only the former, and thus not have a bootstrap
port, which is fine since it's at the root of bootstrap ports, it does
not need to forward fsys_startup and fsys_init.

I.e. in machdev_trivfs_init keep bootstrapped = FALSE in the else part,
and calls task_get_bootstrap_port in all cases to get the bootstrap
port.


There are really two completely independent things:

- being a bootstrap translator, i.e. having a child to resume, and thus
having to install_as_translator when fsys_init gets called from the
child.

- being the child of another bootstrap translator, and thus having to
forward the fsys_startup and fsys_init calls, both on the bootstrap
port.


I'd say rename bootstrapped to bootstrapping, otherwise it's confusing
(it did confuse me).

Samuel