[BUG] hw/xen: features published after InitWait since 240cc11369fc

[Mark Syms <mark.syms@xxxxxxxxxx>]

Hi,

We think there is an ordering regression in the Xen PV backend setup
path, introduced by a commit that was itself fixing a genuine crash. We
are reporting rather than patching, for reasons at the end.

## Symptom

A blkif VBD hot plugged into a running Linux guest silently negotiates
a single page (order 0) ring instead of the eight pages the backend is
willing to offer. That is 32 request slots instead of 256.

Nothing reports this. The disk attaches, works, and is simply eight
times shallower. We found it on XenServer 9 (qemu 10.2.2, Linux 6.1
guest) while setting up an unrelated performance measurement, and lost
some measurement time before noticing the ring depth was not what we
had configured.

The same VBD attached at guest boot gets the full order 3 ring every
time. Hot plugged, in 20 unplug/plug cycles, it took the 32 slot ring
20 times out of 20. It is not an occasional race; hot plug loses it
essentially always.

## Cause, as far as we can tell

`xen_device_realize()` in hw/xen/xen-bus.c currently advertises the
backend state before the device's `realize` has written its feature
nodes:

    xen_device_backend_set_state(xendev, XenbusStateInitWait);
    ...
    xendev_class->realize(xendev, errp);

and for xen-block it is that `realize` which writes the feature nodes,
in `xen_block_realize()`:

    if (qemu_xen_gnttab_can_map_multi()) {
        xen_device_backend_printf(xendev, "max-ring-page-order", "%u",
                                  blockdev->props.max_ring_page_order);
    }

Our understanding of the xenbus handshake is that `InitWait` is the
signal that the backend's parameters are readable. A frontend which
acts on it promptly can therefore look before those nodes exist.

Linux blkfront reads the key with no wait and no retry:

    max_page_order = xenbus_read_unsigned(info->xbdev->otherend,
                                          "max-ring-page-order", 0);
    ring_page_order = min(xen_blkif_max_ring_order, max_page_order);

`xenbus_read_unsigned(..., 0)` returns 0 when the key is absent, so
losing the race is indistinguishable from talking to a backend that
does not support multipage rings.

That also explains why only hot plug is affected. A booting guest takes
seconds to reach blkfront probe, by which time the backend has long
since finished writing. A VBD hot plugged into a running guest is
answered in microseconds.

`max-ring-page-order` is simply the one we noticed. Everything
`xen_block_realize()` writes after the state transition looks equally
exposed, including `feature-discard`, `discard-granularity`,
`feature-flush-cache`, `info` and `mode`.

## Where it came from

This was not always the case. The order was inverted by:

    240cc11369fc ("hw/xen: Avoid crash when backend watch fires too
                   early", Jan 2023)

Before that commit `xendev_class->realize()` ran before
`set_state(XenbusStateInitWait)`, so the feature nodes were published
first and the handshake was correct.

We want to be clear that commit was fixing a real bug, and a nastier
one than this. Quoting it:

    The xen-block code ends up calling aio_poll() through
    blkconf_geometry(), which means we see watch events during the
    indirect call to xendev_class->realize() in xen_device_realize().
    Unfortunately this call is made before populating the initial
    frontend and backend device nodes in xenstore and hence
    xen_block_frontend_changed() (which is called from a watch event)
    fails to read the frontend's 'state' node, and hence believes the
    device is being torn down.

Moving `realize` after node population fixed that crash. It also moved
it after `set_state(InitWait)`, because that call happens to sit in the
same block. We think that side effect is the regression, rather than
anything wrong with the intent.

## Why we are not sending a patch

The obvious change, moving `realize` back above the state transition,
would reintroduce the 2023 crash, so please do not take that as our
suggestion. The two constraints look reconcilable: `realize` needs the
frontend and backend path nodes to exist before it runs, and the
feature nodes need to be published before `InitWait`, and those only
conflict because a single `set_state()` call sits inside the block that
`realize` was moved after.

Whether it is safe to defer only that call, and whether any backend
depends on the state already being `InitWait` during its `realize`, is
a judgement for people who know this code better than we do. We have
not tested any such change.

Separately, and the reason there is no patch attached to this mail at
all: this work made extensive use of Claude Opus 5, and we understand
qemu will not accept code produced that way. That constraint applies to
the fix regardless of how small it is. We are sending the report
because the defect seemed worth your knowing about even if we cannot
contribute the change ourselves.

## Confirmation

We tested the diagnosis rather than leaving it as a reading of the
source. Deferring only the `set_state(XenbusStateInitWait)` call until
after `xendev_class->realize()`, and leaving the node writes and
`set_online()` where they are so that 240cc11369fc's crash fix is
undisturbed, gives:

    stock ordering            order 0 in 20 of 20 hot plugs
    InitWait deferred         order 3 in 20 of 20 hot plugs

Same guest, same VBD, same script, back to back. We are describing that
result rather than offering the change, for the reason above.

We did not check whether any other `XenDeviceClass` backend depends on
the backend state already being `InitWait` during its `realize`; we only
exercised xen-block. That is the part we would expect you to want to
verify.

## What we have not established

We have not instrumented the frontend to catch the read of the missing
key directly, so the mechanism is inferred from the ordering plus the
A/B above rather than observed in blkfront. We also have not measured
how wide the window is, or whether other frontends behave differently.

Happy to run further experiments on our side if that would help.

Regards,

Mark
XenServer Storage Engineering