From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pve-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 2A6021FF176
for <inbox@lore.proxmox.com>; Fri, 7 Mar 2025 11:02:42 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id CA7B617C7E;
Fri, 7 Mar 2025 11:02:35 +0100 (CET)
Message-ID: <c2ccb252-32c3-4a15-af30-bb99e4dca2ac@proxmox.com>
Date: Fri, 7 Mar 2025 11:02:01 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird Beta
To: Fiona Ebner <f.ebner@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20250306104459.1272297-1-d.csapak@proxmox.com>
<20250306104459.1272297-7-d.csapak@proxmox.com>
<ad1703a0-ec57-4784-a87d-fddda4226cf2@proxmox.com>
Content-Language: en-US
From: Dominik Csapak <d.csapak@proxmox.com>
In-Reply-To: <ad1703a0-ec57-4784-a87d-fddda4226cf2@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.022 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pve-devel] [PATCH qemu-server 6/8] machine: add S3/S4 power
state properties
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
Reply-To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pve-devel-bounces@lists.proxmox.com
Sender: "pve-devel" <pve-devel-bounces@lists.proxmox.com>
On 3/6/25 15:52, Fiona Ebner wrote:
> Am 06.03.25 um 11:44 schrieb Dominik Csapak:
>> So users can disable them (they're enabled by default in QEMU)
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> This patch may make sense, regardless if we'll apply the reversal of the
>> default...
>>
>> PVE/QemuServer.pm | 2 ++
>> PVE/QemuServer/Machine.pm | 40 +++++++++++++++++++++++++++++++++++++++
>> 2 files changed, 42 insertions(+)
>>
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index 13af495d..b8ce4750 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -3975,6 +3975,8 @@ sub config_to_command {
>> push @$machineFlags, 'accel=tcg';
>> }
>>
>> + PVE::QemuServer::Machine::check_and_set_power_state_flags($globalFlags, $machine_conf);
>> +
>> push @$machineFlags, 'smm=off' if should_disable_smm($conf, $vga, $machine_type);
>>
>> my $machine_type_min = $machine_type;
>> diff --git a/PVE/QemuServer/Machine.pm b/PVE/QemuServer/Machine.pm
>> index ebaf2dcc..377abc8a 100644
>> --- a/PVE/QemuServer/Machine.pm
>> +++ b/PVE/QemuServer/Machine.pm
>> @@ -31,6 +31,16 @@ my $machine_fmt = {
>> enum => ['intel', 'virtio'],
>> optional => 1,
>> },
>> + 'enable-s3' => {
>> + type => 'boolean',
>> + description => "Enables S3 power state. Defaults to true.",
>> + optional => 1,
>> + },
>> + 'enable-s4' => {
>> + type => 'boolean',
>> + description => "Enables S4 power state. Defaults to true.",
>> + optional => 1,
>> + },
>> };
>>
>> PVE::JSONSchema::register_format('pve-qemu-machine-fmt', $machine_fmt);
>
> Note that an UI patch is needed, because editing the machine there will
> not preserve these values currently.
>
alternative suggestions: I'd prefer to return the +pveX machines (where X > 0)
also in the 'capabilities/qemu/machine' api call listing. This would
1. allow the users of the api to see that there are variants
2. allows the UI to work automagically (since it uses that api call)
- shows the correct version without losing info
- makes it possible to select e.g. a +pve1 variant
what do you say?
>> @@ -293,4 +303,34 @@ sub check_and_pin_machine_string {
>> return print_machine($machine_conf);
>> }
>>
>> +# disable s3/s4 by default for 9.2+pve1 machine types
>> +sub check_and_set_power_state_flags {
>> + my ($globalFlags, $machine_conf) = @_;
>> +
>> + my $get_flag = sub {
>> + my ($q35, $type, $value) = @_;
>
> Style nit: $value is always 1
>
>> +
>> + if ($q35) {
>> + return "ICH9-LPC.disable_${type}=${value}";
>> + } else {
>> + return "PIIX4_PM.disable_${type}=${value}";
>> + }
>> + };
>
> Rather than this closure, could just be $object = $q35 ? 'ICH9-LPC' :
> 'PIIX4_PM' and then use that for constructing the string when pusing.
ok makes sense, in an early version the code was structured differently
where this closure made more sense, and i didn't clean it up...
>
>> +
>> + my $q35 = $machine_conf->{type} && ($machine_conf->{type} =~ m/q35/) ? 1 : 0;
>> +
>> + my $s3 = $machine_conf->{'enable-s3'} // 1;
>> + my $s4 = $machine_conf->{'enable-s4'} // 1;
>> +
>> + # they're enabled by default in QEMU, so only add the flags to disable them
>> + if (!$s3) {
>> + push $globalFlags->@*, $get_flag->($q35, 's3', 1)
>
> Style nit: missing semicolon
>
>> + }
>> + if (!$s4) {
>> + push $globalFlags->@*, $get_flag->($q35, 's4', 1)
>
> Style nit: missing semicolon
>
>> + }
>> +
>> + return;
>> +}
>> +
>> 1;
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel