Skip to content

Conversation

@BengangY
Copy link
Contributor

@BengangY BengangY commented Jul 21, 2025

The FreeBSD driver used by NetScaler supports all power actions. However, older versions of the FreeBSD driver do not explicitly advertise these support. As a result, xapi does not attempt to signal these power actions.
This Free BSD driver issue has been fixed by this commit: 9dd5105
However, there may be many FreeBSD VMs running there which still have this issue. To address this as a workaround, all power actions should be permitted for FreeBSD guests.

Additionally, virtual machines with an explicit data/cant_suspend_reason set aren't allowed to suspend, which would crash Windows and other UEFI VMs.

@BengangY BengangY marked this pull request as ready for review July 21, 2025 10:16
@last-genius
Copy link
Contributor

last-genius commented Jul 21, 2025

Revert the commit "6118d47249aa9bd27a7ed5961b8c0bbdf2891437". This commit will cause issues in the NetScaler scenario due to the following reasons:

1. The VM is FreeBSD, not Windows.

2. The VM includes a PV driver.

3. The VM does not support these features. Therefore, I will revert this commit. We should then revisit the changes and determine how to implement it.

I do not understand which exact assumptions are broken by the commit.

Is it that the driver does not advertise these features in xenstore, but FreeBSD+driver actually supports them, therefore they should be allowed? Does xe vm-migrate force=true still work? That disregards the feature advertisement.

If that's the case, the driver is buggy. control/feature-* paths are documented upstream in xenstore-paths

@last-genius
Copy link
Contributor

last-genius commented Jul 21, 2025

For context, the current revert will mean that VMs with explicit data/cant_suspend_reason set will be allowed to suspend, which will keep crashing Windows (and other UEFI) VMs.

This is the reason for 6118d47 ("make cant_suspend_reason critical")

@psafont
Copy link
Member

psafont commented Jul 21, 2025

We should then revisit the changes and determine how to implement it.

I've spoken to Roger Pau, the maintainer of the FreeBSD driver. This is an issue with the guest driver used by netscaler, he fixed the issue last year for the upstream drivers: https://cgit.freebsd.org/src/commit/sys/dev/xen/control/control.c?id=9dd5105f22a2276cf099f0e10e318294fcc4f6a7

This means that xapi will need some sort of workaround until netscaler decides to update the driver :/ Don't you have some sort of stick to make them include the patch?

@robhoes
Copy link
Member

robhoes commented Jul 21, 2025

Is it that the driver does not advertise these features in xenstore, but FreeBSD+driver actually supports them, therefore they should be allowed?

Apparently so. We have a test failure because of this. Indeed this was a Netscaler VM. So we need a different plan to handle this, or the "can't suspend" case.

@last-genius
Copy link
Contributor

last-genius commented Jul 21, 2025

Here's an alternative fix that will keep old FreeBSD happy and will not allow suspend for VM's with data/cant_suspend_reason: master...last-genius:xen-api:asv/freebsd-feature

@robhoes
Copy link
Member

robhoes commented Jul 21, 2025

This means that xapi will need some sort of workaround until netscaler decides to update the driver

If a fix went upstream for FreeBSD just last year, then this will mean that there are probably many FreeBSD VMs out there in the world (Netscaler and others), which we still need to support for the time being.

@lindig
Copy link
Contributor

lindig commented Jul 21, 2025

The logic implementing this would benefit from a longer explanation. It probably makes sense when you wrapped your head around it but reading the code coming just in, it's more magic than logic. Maybe explain how Windows and FreeBSD differ to motivate the logic.

@BengangY BengangY force-pushed the private/bengangy/CA-413587 branch from 3e09dd3 to c7c4ecf Compare July 22, 2025 07:04
@BengangY
Copy link
Contributor Author

Update the PR as the following solutions:

  1. Add a workaround in xapi to support the old FreeBSD driver and data/cant_suspend_reason.
  2. Update the comment in the code to explain this situation.

I'm running the testcase for netscaler and BST+BVT for ring3.

Copy link
Contributor

@last-genius last-genius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me aside from a comment nitpick

@BengangY BengangY changed the title Revert "Stop assuming PV driver's presence implies all features" CA-413587: Checking feature for old FreeBSD driver Jul 22, 2025
The FreeBSD driver used by NetScaler supports all power actions. However,
older versions of the FreeBSD driver do not explicitly advertise these
support. As a result, xapi does not attempt to signal these power actions.
To address this as a workaround, all power actions should be permitted for
FreeBSD guests.

Additionally, virtual machines with an explicit `data/cant_suspend_reason`
set aren't allowed to suspend, which would crash Windows and other UEFI
VMs.

Signed-off-by: Bengang Yuan <[email protected]>
@BengangY BengangY force-pushed the private/bengangy/CA-413587 branch from c7c4ecf to 6732c89 Compare July 22, 2025 07:15
@BengangY BengangY requested review from lindig, psafont and robhoes July 22, 2025 07:23
@robhoes robhoes added this pull request to the merge queue Jul 22, 2025
Merged via the queue into xapi-project:master with commit 1b268dc Jul 22, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants