Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable Hyper-V extension on ARM #2858

Merged
merged 2 commits into from
May 14, 2024
Merged

Disable Hyper-V extension on ARM #2858

merged 2 commits into from
May 14, 2024

Conversation

huzaifa-d
Copy link
Contributor

@huzaifa-d huzaifa-d commented May 10, 2024

Summary of the pull request

Disable config and creation flow for Hyper-V extension on ARM.

Validation steps performed

Tested on an ARM machine

PR checklist

@huzaifa-d huzaifa-d marked this pull request as ready for review May 10, 2024 17:32
@huzaifa-d huzaifa-d requested a review from dkbennett May 10, 2024 17:43
Copy link
Member

@dkbennett dkbennett left a comment

Choose a reason for hiding this comment

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

Approve, but have a design question that I don't think should block the PR.

src/Services/ActivationService.cs Outdated Show resolved Hide resolved
if (arch == Architecture.Arm64 || arch == Architecture.Arm || arch == Architecture.Armv6)
{
await _localSettingsService.SaveSettingAsync<bool>(CommonConstants.HyperVUniqueId + "-ExtensionDisabled", true);
Log.Information("Disabling Hyper-V extension on ARM {Architecture}", arch);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit

Suggested change
Log.Information("Disabling Hyper-V extension on ARM {Architecture}", arch);
Log.Information($"Disabling Hyper-V extension on ARM {arch}");

// Disable Hyper-V extension on ARM on first run
if (!await _localSettingsService.ReadSettingAsync<bool>(WellKnownSettingsKeys.IsNotFirstRun))
{
var arch = RuntimeInformation.ProcessArchitecture;
Copy link
Member

Choose a reason for hiding this comment

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

Note: it may be possible to run x86 or x64 DevHome on ARM64EC devices. We don't install those architectures, but users could force it or architectures we build can change in the future. It would be more correct to check device architecture instead of running process architecture.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

Comment on lines 101 to 150
var supportedOperations = ComputeSystemOperations.ApplyConfiguration;
var revertOperation = Guid.Empty.Equals(ParentCheckpointId) ? ComputeSystemOperations.None : ComputeSystemOperations.RevertSnapshot;

switch (GetState())
{
case ComputeSystemState.Running:
// Supported operations when running
return ComputeSystemOperations.ShutDown |
supportedOperations = ComputeSystemOperations.ShutDown |
ComputeSystemOperations.Terminate |
ComputeSystemOperations.Save |
ComputeSystemOperations.Pause |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Restart |
ComputeSystemOperations.ApplyConfiguration |
revertOperation;
break;
case ComputeSystemState.Stopped:
// Supported operations when stopped
return ComputeSystemOperations.Start |
supportedOperations = ComputeSystemOperations.Start |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Delete |
ComputeSystemOperations.ApplyConfiguration;
break;
case ComputeSystemState.Saved:
// Supported operations when saved
return ComputeSystemOperations.Start |
supportedOperations = ComputeSystemOperations.Start |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Delete |
ComputeSystemOperations.ApplyConfiguration |
revertOperation;
break;
case ComputeSystemState.Paused:
// Supported operations when paused
return ComputeSystemOperations.Terminate |
supportedOperations = ComputeSystemOperations.Terminate |
ComputeSystemOperations.Save |
ComputeSystemOperations.Resume |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.ApplyConfiguration |
revertOperation;
break;
}

// Before applying the configuration we start the VM. So we will allow the ApplyConfiguration to be the base supported operation
// for the VM.
return ComputeSystemOperations.ApplyConfiguration;
// Disable ApplyConfiguration for ARM
var arch = RuntimeInformation.ProcessArchitecture;
if (arch == Architecture.Arm64 || arch == Architecture.Arm || arch == Architecture.Armv6)
{
supportedOperations &= ~ComputeSystemOperations.ApplyConfiguration;
}

return supportedOperations;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be simpler to just remove the ComputeSystemOperations.ApplyConfiguration from the switch statements and then add it at the end in line 150? This way you can just do return supportedOperations so something like this:

Suggested change
var supportedOperations = ComputeSystemOperations.ApplyConfiguration;
var revertOperation = Guid.Empty.Equals(ParentCheckpointId) ? ComputeSystemOperations.None : ComputeSystemOperations.RevertSnapshot;
switch (GetState())
{
case ComputeSystemState.Running:
// Supported operations when running
return ComputeSystemOperations.ShutDown |
supportedOperations = ComputeSystemOperations.ShutDown |
ComputeSystemOperations.Terminate |
ComputeSystemOperations.Save |
ComputeSystemOperations.Pause |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Restart |
ComputeSystemOperations.ApplyConfiguration |
revertOperation;
break;
case ComputeSystemState.Stopped:
// Supported operations when stopped
return ComputeSystemOperations.Start |
supportedOperations = ComputeSystemOperations.Start |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Delete |
ComputeSystemOperations.ApplyConfiguration;
break;
case ComputeSystemState.Saved:
// Supported operations when saved
return ComputeSystemOperations.Start |
supportedOperations = ComputeSystemOperations.Start |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Delete |
ComputeSystemOperations.ApplyConfiguration |
revertOperation;
break;
case ComputeSystemState.Paused:
// Supported operations when paused
return ComputeSystemOperations.Terminate |
supportedOperations = ComputeSystemOperations.Terminate |
ComputeSystemOperations.Save |
ComputeSystemOperations.Resume |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.ApplyConfiguration |
revertOperation;
break;
}
// Before applying the configuration we start the VM. So we will allow the ApplyConfiguration to be the base supported operation
// for the VM.
return ComputeSystemOperations.ApplyConfiguration;
// Disable ApplyConfiguration for ARM
var arch = RuntimeInformation.ProcessArchitecture;
if (arch == Architecture.Arm64 || arch == Architecture.Arm || arch == Architecture.Armv6)
{
supportedOperations &= ~ComputeSystemOperations.ApplyConfiguration;
}
return supportedOperations;
var supportedOperations = ComputeSystemOperations.None;
var revertOperation = Guid.Empty.Equals(ParentCheckpointId) ? ComputeSystemOperations.None : ComputeSystemOperations.RevertSnapshot;
switch (GetState())
{
case ComputeSystemState.Running:
// Supported operations when running
supportedOperations = ComputeSystemOperations.ShutDown |
ComputeSystemOperations.Terminate |
ComputeSystemOperations.Save |
ComputeSystemOperations.Pause |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Restart |
revertOperation;
break;
case ComputeSystemState.Stopped:
// Supported operations when stopped
supportedOperations = ComputeSystemOperations.Start |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Delete;
break;
case ComputeSystemState.Saved:
// Supported operations when saved
supportedOperations = ComputeSystemOperations.Start |
ComputeSystemOperations.CreateSnapshot |
ComputeSystemOperations.Delete |
revertOperation;
break;
case ComputeSystemState.Paused:
// Supported operations when paused
supportedOperations = ComputeSystemOperations.Terminate |
ComputeSystemOperations.Save |
ComputeSystemOperations.Resume |
ComputeSystemOperations.CreateSnapshot |
revertOperation;
break;
}
// Disable ApplyConfiguration for ARM
var arch = RuntimeInformation.ProcessArchitecture;
if (arch == Architecture.Arm64 || arch == Architecture.Arm || arch == Architecture.Armv6)
{
return supportedOperations;
}
return supportedOperations | ComputeSystemOperations.ApplyConfiguration;

{
// Disable the create operation for ARM devices.
var arch = RuntimeInformation.OSArchitecture;
if (arch == Architecture.Arm64 || arch == Architecture.Arm || arch == Architecture.Armv6)
Copy link
Member

Choose a reason for hiding this comment

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

Coding guidelines: "Use parentheses to make clauses in an expression apparent."

@huzaifa-d huzaifa-d merged commit 0f0ec51 into main May 14, 2024
4 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.

Hyper-V features should be disabled in ARM devices
6 participants