From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 23AA01FF144 for ; Tue, 10 Mar 2026 09:54:16 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 175351420C; Tue, 10 Mar 2026 09:54:10 +0100 (CET) Date: Tue, 10 Mar 2026 09:53:31 +0100 From: Arthur Bied-Charreton To: Fiona Ebner Subject: Re: [PATCH qemu-server v3 1/2] cpu config: Add 'arch' property to cpu_fmt Message-ID: References: <20260217141036.338747-1-a.bied-charreton@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1773132779144 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.799 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 Message-ID-Hash: P3FDRFQ357SAC7RDD2C7L2BG6Q7CJIZK X-Message-ID-Hash: P3FDRFQ357SAC7RDD2C7L2BG6Q7CJIZK X-MailFrom: a.bied-charreton@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, Feb 26, 2026 at 03:01:47PM +0100, Fiona Ebner wrote: > Please always send a cover-letter if there is more than one patch, see > [0]. This helps with tooling like b4 (e.g. a R-b for the cover-letter > will be picked up for the whole series). I was not aware of this, thank you! > Am 17.02.26 um 3:11 PM schrieb Arthur Bied-Charreton: > > Preparatory step for adding support for configuring custom CPU types in > > the PVE UI. > > > > Add optional property 'arch' (x86_64|aarch64) to cpu_fmt to allow custom > > models to indicate which architecture they belong to, default to x86_64 > > for backwards compatibility. > > > > Add checks to get_cpu_options and validate_cpu_conf to deny illegal > > configs early. > > > > Signed-off-by: Arthur Bied-Charreton > > I still gave a review below, since the comments are worthwhile for the > future too, but I'm really not sure anymore if we actually want this. > Since this was my idea, sorry about that! We'd at least need to also > check in validate_vm_cpu_conf(), since $cpu_fmt is used there too. But > again, I don't think it's worthwhile: > > The only use case for an explicit arch for custom models is when the > reported-model cannot uniquely be associated to an arch. In all other > cases, the option is just additional friction. Right now, the only > affected models are 'host' and 'max' models. Supporting mixed arch > clusters is not planned right now AFAIK, so actually just 'max'. And in > practice, we can just associate the model to the arch of the VM its > assigned to. If really needed, users can just define two custom models, > one with flags for x86_64 and one with flags for aarch64. Even if at > some point there is a model name conflict between archs for other > models, it's gonna be very rare that users need both and if they do, > they can just define two custom models. > > What do you think? > > If we really do see the need to add it (in the future), I think the > default should rather be "arch of the reported-model and if that is not > unique, arch of the VM it's assigned to". > I agree, let's just add it if/when we need it. Thanks for the other feedback as well, will keep it in mind for the future. [...] > > + arch => { > > + %{ PVE::JSONSchema::get_standard_option('pve-qm-cpu-arch') }, > > + default => 'x86_64', > > + description => 'The architecture the CPU model belongs to.', > > + optional => 1, > > + }, > > Please note that get_standard_option() has an explicit way to specify a > base via its second argument, which is a bit more explicit. > > > }; > > > > my $sev_fmt = { > > @@ -475,6 +481,13 @@ sub validate_cpu_conf { > > my ($cpu) = @_; > > # required, but can't be forced in schema since it's encoded in section header for custom models > > die "CPU is missing cputype\n" if !$cpu->{cputype}; > > + > > + if (my $reported_model = $cpu->{'reported-model'}) { > > + my $arch = $cpu->{arch} // $cpu_fmt->{arch}->{default}; > > We use this logic three times and the $cpu_fmt name is a bit generic, > which both suggest that having a helper function get_custom_cpu_arch() > would be nice. > [...] > > [0]: > https://lore.proxmox.com/all/176174696524.1776006.10760334798456181474.b4-ty@proxmox.com/