From: Jonathan Gray <jsg@jsg.id.au>
Subject: Re: Probe xnf(4) and xbf(4) on Xen 4.17
To: Joel Knight <knight.joel@gmail.com>
Cc: tech <tech@openbsd.org>
Date: Tue, 26 Nov 2024 15:17:37 +1100

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

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 */
> 
>