pci: fail FLR when reset callback is missing by zp78256pxd-ux · Pull Request #866 · nutanix/libvfio-user

zp78256pxd-ux · 2026-06-19T22:46:52Z

What this PR does

Return EINVAL when a guest triggers PCI Function Level Reset (FLR) but no reset callback is registered.

Previously the library logged:
"FLR callback is not implemented"
but returned success to the client.

This caused FLR requests to appear successful even though no reset occurred.

Changes

Return ERROR_INT(EINVAL) in handle_px_pxdc_write()
when FLR is requested and reset callback is missing.
Add regression test covering FLR with FLRC enabled and
no registered reset callback.

Testing

pytest test/py/test_pci_caps.py -k "write_px" -vv

tmakatos

This will break virtual NVMe in SPDK, see #461 (comment)

jlevon · 2026-06-23T15:13:57Z

 	    return call_reset_cb(vfu_ctx, VFU_RESET_PCI_FLR);
        } else {
            vfu_log(vfu_ctx, LOG_ERR, "FLR callback is not implemented");
+            return ERROR_INT(EINVAL);


I think the idea was that the device might not have anything to do for FLR, but maybe that's unlikely.

I think we need to document this somewhere if we make this change?

@jlevon IIUC you're in favour of getting this merged? If we're not planning to work on proper support on SPDK then we should at least add a callback and make it a no-op?

we do have one, right?

we do have one, right?

we do, got confused that this callback is specific to FLR but it isn't, we pass the reason

I think the idea was that the device might not have anything to do for FLR, but maybe that's unlikely.

I think we need to document this somewhere if we make this change?

If the device advertises FLR then it should provide a reset callback, so I think the correct fix should be to fail adding the Device Control capability (pxdc) if a reset callback hasn't been provided. This means we'll have to document that the reset callback must be configured before adding capabilities.

I'm not sure myself, but perhaps this should be a failure at realize time - so setup can still be in any order.

That's better yes.

tmakatos · 2026-06-25T14:48:03Z

This will break virtual NVMe in SPDK, see #461 (comment)

Wrong, NVMe does provide a reset callback.

zp78256pxd-ux · 2026-06-27T12:22:56Z

Thanks for the feedback — that makes sense to me. Catching this when the capability is added rather than when FLR is actually triggered does seem like the more correct place to enforce it, since it surfaces the misconfiguration at device setup time instead of waiting for a guest to hit it at runtime.

I’m fairly new to this codebase, so I don’t have much visibility into how existing consumers structure their setup calls. A couple of things I wanted to check before attempting a rework:

Would you want this approach folded into this PR, or is it more involved than fits here? It looks like it would mean checking the flrc bit inside vfu_pci_add_capability() rather than just in the FLR write path, so I wasn’t sure whether that’s something you’d prefer to handle separately or whether you’d like me to take a pass at it.
From the SPDK discussion earlier in the thread, is there any known consumer that currently registers the reset callback after adding the capability? If so, enforcing the ordering requirement could break that setup, so I wanted to flag it in case that affects the preferred approach.

Happy to take a shot at whichever direction makes sense — I just didn’t want to make assumptions given I don’t have the full picture of downstream usage yet.

tmakatos · 2026-06-29T08:56:36Z

Thanks for the feedback — that makes sense to me. Catching this when the capability is added rather than when FLR is actually triggered does seem like the more correct place to enforce it, since it surfaces the misconfiguration at device setup time instead of waiting for a guest to hit it at runtime.

I’m fairly new to this codebase, so I don’t have much visibility into how existing consumers structure their setup calls. A couple of things I wanted to check before attempting a rework:

Would you want this approach folded into this PR, or is it more involved than fits here? It looks like it would mean checking the flrc bit inside vfu_pci_add_capability() rather than just in the FLR write path, so I wasn’t sure whether that’s something you’d prefer to handle separately or whether you’d like me to take a pass at it.

Yes it can be folded in this PR.

From the SPDK discussion earlier in the thread, is there any known consumer that currently registers the reset callback after adding the capability? If so, enforcing the ordering requirement could break that setup, so I wanted to flag it in case that affects the preferred approach.

Our API is technically unstable so it's acceptable to break it in this way.

Happy to take a shot at whichever direction makes sense — I just didn’t want to make assumptions given I don’t have the full picture of downstream usage yet.

Devices advertising PCIe Function Level Reset (FLR) support must provide a reset callback. Validate this requirement during device realization rather than when the guest first triggers an FLR request, ensuring invalid device configurations fail early. Add a regression test verifying that vfu_realize_ctx() fails with EINVAL when an FLR-capable device does not register a reset callback. Signed-off-by: Siddharth C <siddharthcibi@icloud.com>

zp78256pxd-ux · 2026-06-30T18:17:45Z

Updated per review feedback: moved the validation to vfu_realize_ctx() so the check happens during realization rather than when FLR is triggered, and added a regression test for the EINVAL case.

tmakatos

just one nit

tmakatos · 2026-07-02T14:43:51Z

+        struct pxcap *px;

+        px = (struct pxcap *)pci_config_space_ptr(vfu_ctx,
+		vfu_ctx->pci_cap_exp_off);


tmakatos · 2026-07-03T08:28:17Z

@zp78256pxd-ux checks are failing

zp78256pxd-ux · 2026-07-03T08:57:41Z

My current implementation validates this in vfu_realize_ctx(), which causes the samples/lspci example to fail because it advertises FLR support but does not register a reset callback.

Would you prefer:

keeping the validation in vfu_realize_ctx() and updating samples/lspci to provide a reset callback (or disable FLR), or
moving the validation to capability creation time as previously suggested?

tmakatos · 2026-07-03T09:14:09Z

My current implementation validates this in vfu_realize_ctx(), which causes the samples/lspci example to fail because it advertises FLR support but does not register a reset callback.

Would you prefer:

keeping the validation in vfu_realize_ctx() and updating samples/lspci to provide a reset callback (or disable FLR), or

this

zp78256pxd-ux · 2026-07-03T10:19:07Z

Updated samples/lspci to register a trivial reset callback when advertising FLR support. This keeps the sample compatible with validation in vfu_realize_ctx() requiring a reset callback when FLR is enabled.

Verified locally that samples/lspci builds and runs successfully with the change.

My local make pre-push currently fails due to cmocka 2.0.2 deprecation warnings in existing unit tests (expect_check() with -Werror), which appears unrelated to this change

tmakatos reviewed Jun 22, 2026

View reviewed changes

jlevon reviewed Jun 23, 2026

View reviewed changes

zp78256pxd-ux force-pushed the fix/flr-no-reset-callback-clean branch from 13f4f7c to dfbd202 Compare June 30, 2026 17:56

zp78256pxd-ux force-pushed the fix/flr-no-reset-callback-clean branch from dfbd202 to f2f038d Compare June 30, 2026 18:02

tmakatos reviewed Jul 2, 2026

View reviewed changes

billzypop and others added 2 commits July 3, 2026 06:49

lib: fix indentation in FLR reset callback check

1a7ffcb

Merge branch 'master' into fix/flr-no-reset-callback-clean

88a3a27

samples/lspci: register reset callback for FLR support

9e87dff

Merge branch 'master' into fix/flr-no-reset-callback-clean

2ab8f41

Uh oh!

Conversation

zp78256pxd-ux commented Jun 19, 2026

What this PR does

Changes

Testing

Uh oh!

tmakatos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmakatos commented Jun 25, 2026

Uh oh!

zp78256pxd-ux commented Jun 27, 2026

Uh oh!

tmakatos commented Jun 29, 2026

Uh oh!

zp78256pxd-ux commented Jun 30, 2026

Uh oh!

tmakatos left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmakatos commented Jul 3, 2026

Uh oh!

zp78256pxd-ux commented Jul 3, 2026

Uh oh!

tmakatos commented Jul 3, 2026

Uh oh!

zp78256pxd-ux commented Jul 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants