Re: XEN3_DOM0 cpu_bootconf() and bootdev=xxx failure

[Christoph Badura <bad%bsd.de@localhost>]

On Sun, Jan 13, 2019 at 08:45:31AM -0000, Michael van Elst wrote:
> bad%bsd.de@localhost (Christoph Badura) writes:
> >> So, assuming 'raid0' has a component 'dk0' which is based on 'wd0', it
> >> should work to specify 'wd0' as bootdev.
> 
> >That does work.  However, that is besides the point  bootdev=raid0 is
> >supposed to work but it doesn't.
> 
> Which is of course is unrelated to the raid autoconfig I talked about
> and which is flawed too.
> 
> >The reason for that is in the code that overrides rootspec with bootspec.
> 
> Not really, read below.

> >rootspec gets initialized by config(8) like so:
> 
> >For "config <name> root on ?":
> >const char *rootspec = NULL;
> >dev_t   rootdev = NODEV;        /* wildcarded */
> 
> >And for, e.g., "config <name> root on sd0e":
> >const char *rootspec = "sd0e";
> >dev_t   rootdev = makedev(4, 4);        /* sd0e */
> 
> 
> rootspec is set by config(1) for three cases:
> 
> config <name> root on major 4 minor 4
> - sets rootspec to "sd0e" (or maybe "<4/4>") and rootdev to makedev(4,4)
> 
> config <name> root on sd0e
> - sets rootspec to "sd0e" and rootdev to makedev(4,4)
> 
> config <name> root on "some string constant"
> - sets rootspec to "some string constant" and rootdev to NODEV.

There is also:

config <name> root on ?
- sets rootspec to NULL and rootdev to NODEV

and the interesting case of a network device:

config <name> on wm0
- sets rootspec to "wm0" and rootdev to NODEV explicitly.

Also, one has to use a spec string to put root on dk(4) or a wedge name.
The former case is because dk(4)s don't exist at config(1) time.

> If rootspec is set the kernel checks for
> - rootspec specifying a network interface -> network boot
> - rootdev == NODEV -> resolve rootspec (as a wedge name)
> - rootdev != NODEV -> Use rootdev, ignore rootspec but which is same as rootdev

Can you give me the line numbers in kern_subr.c where you think those
decisions are made?  I can't find the code in question.  rootdev is never
used in a comparison in kern_subr.c.

> The intention is to resolve the 3 cases (network interface, disk device,
> wedge name), and rootspec basically overrides rootdev (in the second case
> they specify the same device, so nothing is "overridden" even when the
> code takes the rootdev value).

I'm unclear what the you mean by "in the second case".

> >I.e. rootspec and rootdev need to be set in pairs.
> 
> If rootspec isn't set by config(1), we talk about a wildcard boot and
> rootdev is already set to NODEV. So that's exactly the third case provided
> by config(1). And if I'm not mistaken then
> config <name> root on "raid0"
> doesn't work either.

Probably.  My proposed change makes that work, though.
The idea in that change is that if parsedisk() can resolve the device in
bootspec, that we simulate what "config <name> root on bootspec" would
have done.  That does handle the dk(4) and wedge name cases too, because
these devices now exist.

> So we could
> - explicitely clear rootdev to NODEV to not rely on config(1) defaults
>   when using bootspec.

That doesn't work because then rootdev would never get set and it has to
be set after you get to haveroot:.

> - make the code that resolves rootspec also handle regular disks and
>   not just dk (and flash) devices. This also keeps that logic in a
>   single place.

I have thought about why

		if (dv != NULL && device_class(dv) == DV_DISK &&
		    !DEV_USES_PARTITIONS(dv) &&
		    (majdev = devsw_name2blk(device_xname(dv), NULL, 0)) >= 0) {
			rootdv = dv;
			rootdev = makedev(majdev, device_unit(dv));
			goto haveroot;
		}

doesn't do the same thing as 

			if (DEV_USES_PARTITIONS(bootdv))
				rootdev = MAKEDISKDEV(majdev,
						      device_unit(bootdv),
						      bootpartition);
			else
				rootdev = makedev(majdev, device_unit(bootdv));
in the rootspec == NULL case.

That is another candidate for a likely fix.

But I can't easily test any longer as the machine I was testing on had to
leave the house 10 days ago.

Anyway, I see nothing wrong on a technical level with my change.
There's a lot to clean up and actually factor out related to setroot()
anyway.

--chris