Re: XEN3_DOM0 cpu_bootconf() and bootdev=xxx failure

[mlelstv%serpens.de@localhost (Michael van Elst)]

bad%bsd.de@localhost (Christoph Badura) writes:

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

Yes, I listed the cases where rootspec is set to non-NULL since
we talk about it being augmented by bootspec.

As for a network interface, there is no major number, so yes,
rootdev can only be set to NODEV and the interface must be
passed as rootspec, so in the end it is treated like the
third case.


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

Yes, thanks to how dk(4) is configured. However, you can specify it
by major/minor and get:

const char *rootspec = "dk0a";
dev_t   rootdev = makedev(168, 0);      /* dk0a */

Could even work.


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


Check the 'root on <dev>' path (netbsd-8 source for reference):

the network check is line 428.
the rootspec for disks is used in line 433.
the following code uses rootdev (which is equivalent to rootspec).

Obviously you don't see a direct comparison of rootdev.

Line 433 checks for rootspec being "dk" (originally, it now also
includes "flash"), as you've written above that catches the wedge
case with rootdev = NODEV.



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

second case in my list is 'config <name> root on sd0e'.


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

Indeed, but it also adds another place where rootspec is evaluated.


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

config <name> root on "bootspec" sets rootdev to NODEV.


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

if (rootspec == NULL) {
	rootspec = bootspec;
	rootdev = NODEV;
}

works fine. rootspec being NULL is the wildcard case and setting
rootdev to NODEV is a no-op (assuming config works as it works now).


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

It skips the case where the device is a regular disk with partitions.


>That is another candidate for a likely fix.

See my patch.

http://ftp.netbsd.org/pub/NetBSD/misc/mlelstv/kern_subr.diff


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

I don't like that it adds another place to parse rootspec, surely nothing
that simplifies the code. I also don't like that it uses parsedisk (which
is a more a problem with parsedisk than the change).

The patch (URL above) addresses both issues. And no, I'm still testing
as I may have missed something.



Greetings,
Michael van Elst
-- 
-- 
                                Michael van Elst
Internet: mlelstv%serpens.de@localhost
                                "A potential Snark may lurk in every tree."