From: David Gwynne <david@gwynne.id.au>
Subject: Re: Probe xnf(4) and xbf(4) on Xen 4.17
To: Jonathan Gray <jsg@jsg.id.au>
Cc: Joel Knight <knight.joel@gmail.com>, OpenBSD Tech <tech@openbsd.org>
Date: Tue, 26 Nov 2024 14:25:16 +1000



> On 26 Nov 2024, at 14:17, Jonathan Gray <jsg@jsg.id.au> wrote:
> 
> On Wed, Nov 20, 2024 at 07:11:11AM -0700, Joel Knight wrote:
>> On Tue, Nov 19, 2024 at 10:18 PM Jonathan Gray <jsg@jsg.id.au> wrote:
>>> 
>>> On Wed, Nov 13, 2024 at 05:14:00PM -0700, Joel Knight wrote:
>>>> Hi. As reported here[1], something changed between Xen 4.13 and 4.17
>>>> which causes xnf and xbf to no longer probe on boot. The culprit
>>>> appears to be XenStore advertising a "9pfs" device, which OpenBSD
>>>> doesn't support, so xen_probe_devices() aborts early. The 9pfs device
>>>> is first in the list of devices returned by XS.
>>>> 
>>> 
>>> Why is a device not attaching fatal?
>>> 
>>> Could the below diff work instead?  I can't find a list of xen devices,
>>> but that would avoid the same problem the next time another is added.
>>> 
>>> Index: sys/dev/pv/xen.c
>>> ===================================================================
>>> RCS file: /cvs/src/sys/dev/pv/xen.c,v
>>> diff -u -p -U8 -r1.98 xen.c
>>> --- sys/dev/pv/xen.c    24 May 2024 10:05:55 -0000      1.98
>>> +++ sys/dev/pv/xen.c    20 Nov 2024 05:10:47 -0000
>>> @@ -1421,17 +1421,17 @@ xen_probe_devices(struct xen_softc *sc)
>>>                for (j = 0; j < iov2_cnt; j++) {
>>>                        error = xen_attach_device(sc, xdl,
>>>                            (const char *)iovp1[i].iov_base,
>>>                            (const char *)iovp2[j].iov_base);
>>>                        if (error) {
>>>                                printf("%s: failed to attach \"%s/%s\"\n",
>>>                                    sc->sc_dev.dv_xname, path,
>>>                                    (const char *)iovp2[j].iov_base);
>>> -                               goto out;
>>> +                               continue;
>>>                        }
>>>                }
>>>                /* Setup a watch for every device subtree */
>>>                if (xs_watch(sc, "device", (char *)iovp1[i].iov_base,
>>>                    &xdl->dl_task, xen_hotplug, xdl))
>>>                        printf("%s: failed to setup hotplug watch for \"%s\"\n",
>>>                            sc->sc_dev.dv_xname, (char *)iovp1[i].iov_base);
>>>                SLIST_INSERT_HEAD(&sc->sc_devlists, xdl, dl_entry);
>>> 
>>> 
>> 
>> Fair. This works as well. Might as well remove the special case for
>> the 'suspend' device while we're at it.
> 
> Sorry for taking a while to reply.
> 
> The problem there is if the xs_cmd() fails there is a jump
> out of the loop.
> 
>> 
>> On this version of Xen, there are just the 3 devices:
>> 
>> % hostctl device
>> 9pfs
>> vbd
>> vif
>> 
>> dmesg with attached diff:
>> 
>> bios0: vendor Xen version "4.17" date 10/02/2024
>> bios0: Xen HVM domU
>> pvbus0 at mainbus0: Xen 4.17
>> xen0 at pvbus0: features 0x112705, 64 grant table frames, event channel 3
>> xen0: failed to attach "device/9pfs/"
>> xbf0 at xen0 backend 0 channel 8: cdrom
>> xbf1 at xen0 backend 0 channel 8: disk
>> sd0 at scsibus1 targ 0 lun 0: <Xen, phy xvda 768, 0000>
>> xbf2 at xen0 backend 0 channel 9: disk
>> sd1 at scsibus2 targ 0 lun 0: <Xen, phy xvdb 832, 0000>
>> xnf0 at xen0 backend 0 channel 10: address 4e:4b:b7:f4:f4:94
>> xspd0 at pci0 dev 3 function 0 "XenSource Platform Device" rev 0x02
>> xen0: failed to attach "device/9pfs/"
> 
> Ideally it would be something like
> "9pfs" at xen0 backend 0 channel 8 not configured

i agree.

> 
> config_found()/config_found_sm() should do that.  I don't follow why
> that doesn't happen.  Could you try and figure it out?  Is it because
> 9pfs is not a different device, but rather a property of /device/vbd/ ?
> Should xbf_match() not claim devices with the 9pfs property?
> 
>> 
>> 
>> Index: sys/dev/pv/xen.c
>> ===================================================================
>> RCS file: /cvs/src/sys/dev/pv/xen.c,v
>> diff -p -u -r1.98 xen.c
>> --- sys/dev/pv/xen.c    24 May 2024 10:05:55 -0000      1.98
>> +++ sys/dev/pv/xen.c    20 Nov 2024 14:05:52 -0000
>> @@ -1403,8 +1403,6 @@ xen_probe_devices(struct xen_softc *sc)
>>                return (error);
>> 
>>        for (i = 0; i < iov1_cnt; i++) {
>> -               if (strcmp("suspend", (char *)iovp1[i].iov_base) == 0)
>> -                       continue;
>>                snprintf(path, sizeof(path), "device/%s",
>>                    (char *)iovp1[i].iov_base);
>>                if ((error = xs_cmd(&xst, XS_LIST, path, &iovp2,
>> @@ -1426,7 +1424,7 @@ xen_probe_devices(struct xen_softc *sc)
>>                                printf("%s: failed to attach \"%s/%s\"\n",
>>                                    sc->sc_dev.dv_xname, path,
>>                                    (const char *)iovp2[j].iov_base);
>> -                               goto out;
>> +                               continue;
>>                        }
>>                }
>>                /* Setup a watch for every device subtree */