Re: [PATCH V12 0/8] Add architecture agnostic code to support vCPU Hotplug

[Gavin Shan]

Hi Salil and Igor,

On 6/26/24 9:51 AM, Salil Mehta wrote:

On Wed, Jun 5, 2024 at 3:03 PM Igor Mammedov mailto:imamm...@redhat.com>> wrote:
On Sun, 2 Jun 2024 18:03:05 -0400
"Michael S. Tsirkin" mailto:m...@redhat.com>> wrote:

 > On Thu, May 30, 2024 at 12:42:33AM +0100, Salil Mehta wrote:
 > > Virtual CPU hotplug support is being added across various 
architectures[1][3].
 > > This series adds various code bits common across all architectures:
 > >
 > > 1. vCPU creation and Parking code refactor [Patch 1]
 > > 2. Update ACPI GED framework to support vCPU Hotplug [Patch 2,3]
 > > 3. ACPI CPUs AML code change [Patch 4,5]
 > > 4. Helper functions to support unrealization of CPU objects [Patch 6,7]
 > > 5. Docs [Patch 8]
 > >
 > >
 > > Repository:
 > >
 > > [*] https://github.com/salil-mehta/qemu.git 
 virt-cpuhp-armv8/rfc-v3.arch.agnostic.v12
 > >
 > > NOTE: This series is meant to work in conjunction with Architecture 
specific patch-set.
 > > For ARM, this will work in combination of the architecture specific 
part based on
 > > RFC V2 [1]. This architecture specific patch-set RFC V3 shall be 
floated soon and is
 > > present at below location
 > >
 > > [*] https://github.com/salil-mehta/qemu/tree/virt-cpuhp-armv8/rfc-v3-rc1 

 > >
 >
 >
 > Igor plan to take a look?

Yep, I plan to review it


A gentle reminder on this.



Since the latest revision for this series is v13, so I guess Igor needs
to do the final screening on v13 instead?

v13: https://lists.nongnu.org/archive/html/qemu-arm/2024-06/msg00129.html

Thanks,
Gavin

Re: [PATCH v3] hw/arm/virt: Avoid unexpected warning from Linux guest on host with Fujitsu CPUs

[Gavin Shan]

On 6/12/24 12:05, Zhenyu Zhang wrote:

Multiple warning messages and corresponding backtraces are observed when Linux
guest is booted on the host with Fujitsu CPUs. One of them is shown as below.

[0.032443] [ cut here ]
[0.032446] uart-pl011 900.pl011: ARCH_DMA_MINALIGN smaller than
CTR_EL0.CWG (128 < 256)
[0.032454] WARNING: CPU: 0 PID: 1 at arch/arm64/mm/dma-mapping.c:54
arch_setup_dma_ops+0xbc/0xcc
[0.032470] Modules linked in:
[0.032475] CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.14.0-452.el9.aarch64
[0.032481] Hardware name: linux,dummy-virt (DT)
[0.032484] pstate: 6045 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[0.032490] pc : arch_setup_dma_ops+0xbc/0xcc
[0.032496] lr : arch_setup_dma_ops+0xbc/0xcc
[0.032501] sp : 80008003b860
[0.032503] x29: 80008003b860 x28:  x27: aae4b949049c
[0.032510] x26:  x25:  x24: 
[0.032517] x23: 0100 x22:  x21: 
[0.032523] x20: 0001 x19: 2f06c02ea400 x18: 
[0.032529] x17: 208a5f76 x16: 6589dbcb x15: aae4ba071c89
[0.032535] x14:  x13: aae4ba071c84 x12: 455f525443206e61
[0.032541] x11: 68742072656c6c61 x10: 0029 x9 : aae4b7d21da4
[0.032547] x8 : 0029 x7 : 4c414e494d5f414d x6 : 0029
[0.032553] x5 : 000f x4 : aae4b9617a00 x3 : 0001
[0.032558] x2 :  x1 :  x0 : 2f06c029be40
[0.032564] Call trace:
[0.032566]  arch_setup_dma_ops+0xbc/0xcc
[0.032572]  of_dma_configure_id+0x138/0x300
[0.032591]  amba_dma_configure+0x34/0xc0
[0.032600]  really_probe+0x78/0x3dc
[0.032614]  __driver_probe_device+0x108/0x160
[0.032619]  driver_probe_device+0x44/0x114
[0.032624]  __device_attach_driver+0xb8/0x14c
[0.032629]  bus_for_each_drv+0x88/0xe4
[0.032634]  __device_attach+0xb0/0x1e0
[0.032638]  device_initial_probe+0x18/0x20
[0.032643]  bus_probe_device+0xa8/0xb0
[0.032648]  device_add+0x4b4/0x6c0
[0.032652]  amba_device_try_add.part.0+0x48/0x360
[0.032657]  amba_device_add+0x104/0x144
[0.032662]  of_amba_device_create.isra.0+0x100/0x1c4
[0.032666]  of_platform_bus_create+0x294/0x35c
[0.032669]  of_platform_populate+0x5c/0x150
[0.032672]  of_platform_default_populate_init+0xd0/0xec
[0.032697]  do_one_initcall+0x4c/0x2e0
[0.032701]  do_initcalls+0x100/0x13c
[0.032707]  kernel_init_freeable+0x1c8/0x21c
[0.032712]  kernel_init+0x28/0x140
[0.032731]  ret_from_fork+0x10/0x20
[0.032735] ---[ end trace  ]---

In Linux, a check is applied to every device which is exposed through
device-tree node. The warning message is raised when the device isn't
DMA coherent and the cache line size is larger than ARCH_DMA_MINALIGN
(128 bytes). The cache line is sorted from CTR_EL0[CWG], which corresponds
to 256 bytes on the guest CPUs. The DMA coherent capability is claimed
through 'dma-coherent' in their device-tree nodes or parent nodes.

Fix the issue by adding 'dma-coherent' property to the device-tree root
node, meaning all devices are capable of DMA coherent by default.

Signed-off-by: Zhenyu Zhang 
---
v3: Add comments explaining why we add 'dma-coherent' property (Peter)
---
  hw/arm/virt.c | 11 +++
  1 file changed, 11 insertions(+)



Reviewed-by: Gavin Shan 

Re: Unexpected error in rme_configure_one() at ../target/arm/kvm-rme.c:159

[Gavin Shan]

On 6/6/24 15:05, Gavin Shan wrote:

Even the edk2 for the guest can be built successfully, but I'm not able to try 
it
because I'm unable to bring up the host now. I tried to rebuild the environment
from scratch, the host runs into crash inside EDK2 unfortunately...

   TF-RMM:   https://git.codelinaro.org/linaro/dcap/rmm.git 
  (branch: cca/v2)
   EDK2: g...@github.com:tianocore/edk2.git 
   (tag:    edk2-stable202402)
   TF-A: https://git.codelinaro.org/linaro/dcap/tf-a/trusted-firmware-a.git 
  (branch: cca/v2)
   QEMU: https://git.qemu.org/git/qemu.git  
  (branch: master)
   KERNEL:   https://git.gitlab.arm.com/linux-arm/linux-cca.git 
  (branch: cca-full/v2)
   BuildRoot: 

arm64-server# home/gavin/sandbox/qemu.main/build/qemu-system-aarch64  \
   -M virt,virtualization=on,secure=on,gic-version=3,acpi=off  \
   -cpu max,x-rme=on -m 8G -smp 8  \
   -monitor none -serial mon:stdio -nographic -nodefaults  \
   -bios /home/gavin/sandbox/CCA/tf-a/flash.bin    \
   -kernel /home/gavin/sandbox/CCA/linux/arch/arm64/boot/Image \
   -append console=ttyAMA0 root=/dev/vda   \
   -drive 
format=raw,if=none,file=/home/gavin/sandbox/CCA/buildroot/output/images/rootfs.ext4,id=hd0
 \
   -device virtio-blk-pci,drive=hd0    \
   -netdev 
tap,id=tap0,vhost=false,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown \
   -device virtio-net-pci,netdev=tap0,mac=52:54:00:f1:26:b0 
 \
   -fsdev 
local,security_model=none,path=/home/gavin/sandbox/CCA,id=shr0 \
   -device virtio-9p-device,fsdev=shr0,mount_tag=shr0
  :
NOTICE:  Booting Trusted Firmware
NOTICE:  BL1: v2.10.0(debug):99e0b97aa-dirty
NOTICE:  BL1: Built : 23:14:56, Jun  5 2024
INFO:    BL1: RAM 0xe0ee000 - 0xe0f7000
INFO:    BL1: Loading BL2
INFO:    Loading image id=1 at address 0xe06b000
INFO:    Image id=1 loaded: 0xe06b000 - 0xe0742d1
NOTICE:  BL1: Booting BL2
INFO:    Entry point address = 0xe06b000
INFO:    SPSR = 0x3cd
INFO:    [GPT] Boot Configuration
INFO:  PPS/T: 0x2/40
INFO:  PGS/P: 0x0/12
INFO:  L0GPTSZ/S: 0x0/30
INFO:  PAS count: 0x6
INFO:  L0 base:   0xedfe000
INFO:    [GPT] PAS[0]: base 0xe001000, size 0xff000, GPI 0xa, type 0x1
INFO:    [GPT] PAS[1]: base 0xe10, size 0xcfe000, GPI 0x8, type 0x1
INFO:    [GPT] PAS[2]: base 0xedfe000, size 0x202000, GPI 0xa, type 0x1
INFO:    [GPT] PAS[3]: base 0x4000, size 0x10, GPI 0x9, type 0x1
INFO:    [GPT] PAS[4]: base 0x4010, size 0x280, GPI 0xb, type 0x1
INFO:    [GPT] PAS[5]: base 0x4290, size 0x1fd70, GPI 0x9, type 0x1
INFO:    Enabling Granule Protection Checks
NOTICE:  BL2: v2.10.0(debug):99e0b97aa-dirty
NOTICE:  BL2: Built : 23:14:56, Jun  5 2024
INFO:    BL2: Doing platform setup
INFO:    Reserved RMM memory [0x4010, 0x428f] in Device tree
INFO:    BL2: Loading image id 3
INFO:    Loading image id=3 at address 0xe0a
INFO:    Image id=3 loaded: 0xe0a - 0xe0b10c4
INFO:    BL2: Loading image id 35
INFO:    Loading image id=35 at address 0x4010
INFO:    Image id=35 loaded: 0x4010 - 0x403033b0
INFO:    BL2: Loading image id 5
INFO:    Loading image id=5 at address 0x6000
INFO:    Image id=5 loaded: 0x6000 - 0x6020
NOTICE:  BL2: Booting BL31
INFO:    Entry point address = 0xe0a
INFO:    SPSR = 0x3cd
NOTICE:  BL31: v2.10.0(debug):99e0b97aa-dirty
NOTICE:  BL31: Built : 23:14:56, Jun  5 2024
INFO:    GICv3 without legacy support detected.
INFO:    ARM GICv3 driver initialized in EL3
INFO:    Maximum SPI INTID supported: 287
INFO:    BL31: Initializing runtime services
INFO:    RMM setup done.
INFO:    BL31: Initializing RMM
INFO:    RMM init start.
Booting RMM v.0.4.0(debug) 17924bc Built with GCC 11.4.1
RMM-EL3 Interface v.0.2
Boot Manifest Interface v.0.3
RMI/RSI ABI v.1.0/1.0 built: Jun  5 2024 23:03:00
INFO:    RMM init end.
INFO:    BL31: Preparing for EL3 exit to normal world
INFO:    Entry point address = 0x6000
INFO:    SPSR = 0x3c9
Loading driver at 0x00060009160 EntryPoint=0x000
ArmVirtGetMemoryMap: Dumping System DRAM Memory Map:
 PhysicalBase: 0x4000
 VirtualBase: 0x4000
 Length: 0x2
UEFI firmware (version  built at 23:28:51 on Jun  5 2024)
PlatformPeim: PL011 UART (console) @ 0x900
PlatformPeim: PL011 UART (debug) @ 0x900
   :
EFI stub: Booting Linux Kernel...
EFI stub: EFI_RNG_PROTOCOL unavailable
SetMemoryAttributes: BaseAddress == 0x22DC0, Length == 0x1CE, 
Attributes == 0x2
SetMemoryAttributes: BaseAddress == 0x22F8E, Length == 0xE5, Attributes 
== 0x4000
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services...
EFI stub

Re: Unexpected error in rme_configure_one() at ../target/arm/kvm-rme.c:159

[Gavin Shan]

On 6/6/24 01:56, Jean-Philippe Brucker wrote:

On Wed, Jun 05, 2024 at 11:28:47AM +1000, Gavin Shan wrote:

WriteSections64(): 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
 AARCH64 small code model requires identical ELF and PE/COFF section offsets 
modulo 4 KB.


Ah I've seen this once but it disappeared as I tried to investigate and
I've since changed the implementation, so I don't have many notes about
it.

Maybe you could try to bisect from "ArmVirtPkg: ArmCcaIoMmu: Provide an
implementation for SetAttribute", but it may give false positives if the
error depends on some random linker placement. Could be
"ArmVirtPkg/ArmPlatformLibQemu: Setup early UART mapping in a Realm" which
adds a 4k page to the data section for the ealy RSI config call, though
that has explicit 4kB alignment.

In my notes I also wrote that changing "-z common-page-size=0x20" to 4k in
the link flags may have made the error disappear, but I doubt it's the
right fix.

I'll try GCC 11 to see if I can reproduce.



Ok. I run a git-bisect and the first problematic commit is 1153ae939c
("ArmVirtPkg/ArmPlatformLibQemu: Add a third-level page table for the UART 
idmap")


Ah thanks, I'm able to reproduce the problem now, it was my local config
that masked it.



I'm not familiar with edk2. The error is raised by 
BaseTools/Source/C/GenFw/Elf64Convert.c::WriteSections64()
where the relocatable address isn't properly aligned to 4KB. So I modified the 
code
as below, but I have to run two consecutive builds. In the first attempt build, 
I
still hit the same error.


This seems to be because GenFw generates a file even on error, so it
doesn't retry the second time.

This commit moves the page tables from .rodata to .data. When linking
IdMap.obj into ArmPlatformPrePeiCore.dll, the alignment of the .text
section changes from 0x1000 to 0x800. This change comes from the linker
script putting .rodata into .text. I don't know why the included .rodata
alignment affects the .text alignment, but I don't think it matters here.

In GenFw, ScanSections64() calculates a mCoffAlignment as the max
.text/.data/.hii section alignement. Since with this commit, .data
alignement (0x1000) becomes larger than .text (0x800), it picks 0x1000 as
the output text offset, and then WriteSections64() complains that this
offset isn't equal to the input .text alignment modulo 0x1000.

The linker script says:

   /*
* The alignment of the .data section should be less than or equal to the
* alignment of the .text section. This ensures that the relative offset
* between these sections is the same in the ELF and the PE/COFF versions of
* this binary.
*/

but that's not what we're getting. I don't have a fix yet, other than
forcing the .text and .data alignment to 4k.



Jean, thanks for your explanation. Right, the issue is caused by mismatched
alignments for ELF and PE/COFF sections. I ever dumped the variables at the
failing point, showing the mismatched alignments (0x800 vs 0x1000). Apart from
that, the virtual address of 'text' section is aligned to 0x800 instead of
0x1000 after ArmPlatformPrePeiCore.dll is dumped by 'readelf'.

SecHdr->sh_addr:0x800  <<< Mismatched alignment 
between ELF and PE/COFF
SecOffset:  0x1000
SymShdr->sh_addr:   0x800
mCoffSectionsOffset[Sym->st_shndx]: 0x1000
GenFw: ERROR 3000: Invalid
  WriteSections64(): 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
 AARCH64 small code model requires identical ELF and PE/COFF section offsets 
modulo 4 KB.

# readelf -S 
Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
Section Headers:
  [Nr] Name  Type Address   Offset
   Size  EntSize  Flags  Link  Info  Align
  [ 0]   NULL   
        0 0 0
  [ 1] .text PROGBITS 0800  0800   <<< 
Aligned to 0x800
   51b8    AX   0 0 2048

With the following changes, I'm able to build the firmware successfully. I don't
see how COMMONPAGESIZE is sorted out because I don't find its definition in the
source code.

diff --git a/BaseTools/Scripts/GccBase.lds b/BaseTools/Scripts/GccBase.lds
index 9f27e83bb0..5463df47a9 100644
--- a/BaseTools/Scripts/GccBase.lds
+++ b/BaseTools/Scripts/GccBase.lds
@@ -20,7 +20,8 @@ SECTIONS {
*/
   . = PECOFF_HEADER_SIZE;
 
-  .text : ALIGN(CONSTANT(COMMONPAGESIZE)) {

+  /* .text : ALIGN(CONSTANT(COMMONPAGESIZE)) { */^M
+  .text : ALIGN(4096)

Re: Unexpected error in rme_configure_one() at ../target/arm/kvm-rme.c:159

[Gavin Shan]

Hi Jean,

On 6/4/24 21:15, Jean-Philippe Brucker wrote:

On Tue, Jun 04, 2024 at 01:02:08PM +1000, Gavin Shan wrote:

On 6/3/24 18:24, Jean-Philippe Brucker wrote:

On Sat, Jun 01, 2024 at 08:14:46PM +1000, Gavin Shan wrote:

---> guest edk2

# git clone https://git.codelinaro.org/linaro/dcap/edk2.git edk2-guest
# cd edk2-guest; git checkout origin/cca/v2 -b cca/v2
# git submodule update --init --recursive;  \
source edksetup.sh; make -j -C BaseTools; \
export GCC5_AARCH64_PREFIX=;  \


Doesn't this needs a cross-compiler, something like "aarch64-linux-gnu-" ?



No, I was building everything using a native compiler instead of a cross 
compiler.
All packages were compiled on a NVidia's grace-hopper machine.

[root@nvidia-grace-hopper-05 ~]# cat /etc/system-release
Red Hat Enterprise Linux release 9.5 Beta (Plow)
[root@nvidia-grace-hopper-05 ~]# uname -r
6.7.0-rc2-gavin+
[root@nvidia-grace-hopper-05 ~]# gcc --version
gcc (GCC) 11.4.1 20231218 (Red Hat 11.4.1-3)
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I tried the cross compiler and encountered the same build error.

[root@nvidia-grace-hopper-05 edk2-guest]# export | grep GCC5_AARCH64_PREFIX
declare -x GCC5_AARCH64_PREFIX="aarch64-linux-gnu-"
[root@nvidia-grace-hopper-05 edk2-guest]# build -b DEBUG -a AARCH64 -t GCC5 -p 
ArmVirtPkg/ArmVirtQemu.dsc
   :
--add-gnu-debuglink=/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.debug
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
cp -p -f 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.debug
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPrePeiCore.debug
"GenFw" -e SEC -o 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/OUTPUT/ArmPlatformPrePeiCore.efi
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
GenFw: ERROR 3000: Invalid
   WriteSections64(): 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
 AARCH64 small code model requires identical ELF and PE/COFF section offsets 
modulo 4 KB.


Ah I've seen this once but it disappeared as I tried to investigate and
I've since changed the implementation, so I don't have many notes about
it.

Maybe you could try to bisect from "ArmVirtPkg: ArmCcaIoMmu: Provide an
implementation for SetAttribute", but it may give false positives if the
error depends on some random linker placement. Could be
"ArmVirtPkg/ArmPlatformLibQemu: Setup early UART mapping in a Realm" which
adds a 4k page to the data section for the ealy RSI config call, though
that has explicit 4kB alignment.

In my notes I also wrote that changing "-z common-page-size=0x20" to 4k in
the link flags may have made the error disappear, but I doubt it's the
right fix.

I'll try GCC 11 to see if I can reproduce.



Ok. I run a git-bisect and the first problematic commit is 1153ae939c
("ArmVirtPkg/ArmPlatformLibQemu: Add a third-level page table for the UART 
idmap")

I'm not familiar with edk2. The error is raised by 
BaseTools/Source/C/GenFw/Elf64Convert.c::WriteSections64()
where the relocatable address isn't properly aligned to 4KB. So I modified the 
code
as below, but I have to run two consecutive builds. In the first attempt build, 
I
still hit the same error.

---> VirtPkg/Library/ArmPlatformLibQemu/IdMap.S

  .align12
  .globlidmap
  .globluart_pte
  .section  ".data.idmap", "aw", %progbits
  .align12

# source edksetup.sh; export GCC5_AARCH64_PREFIX=
# make -j -C BaseTools; \   <<< 
Failed on the first attempt
  build -b DEBUG -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc
   :
WriteSections64(): 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
 AARCH64 small code model requires identical ELF and PE/COFF section offsets 
modulo 4 KB.
make: *** [GNUmakefile:405: 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/OUTPUT/ArmPlatformPrePeiCore.efi]
 Error 2

# make -j -C BaseTools; \  <<< 
Succeed

Re: Unexpected error in rme_configure_one() at ../target/arm/kvm-rme.c:159

[Gavin Shan]

Hi Jean,

On 6/3/24 18:24, Jean-Philippe Brucker wrote:

On Sat, Jun 01, 2024 at 08:14:46PM +1000, Gavin Shan wrote:

---> guest edk2

# git clone https://git.codelinaro.org/linaro/dcap/edk2.git edk2-guest
# cd edk2-guest; git checkout origin/cca/v2 -b cca/v2
# git submodule update --init --recursive;  \
   source edksetup.sh; make -j -C BaseTools; \
   export GCC5_AARCH64_PREFIX=;  \


Doesn't this needs a cross-compiler, something like "aarch64-linux-gnu-" ?



No, I was building everything using a native compiler instead of a cross 
compiler.
All packages were compiled on a NVidia's grace-hopper machine.

[root@nvidia-grace-hopper-05 ~]# cat /etc/system-release
Red Hat Enterprise Linux release 9.5 Beta (Plow)
[root@nvidia-grace-hopper-05 ~]# uname -r
6.7.0-rc2-gavin+
[root@nvidia-grace-hopper-05 ~]# gcc --version
gcc (GCC) 11.4.1 20231218 (Red Hat 11.4.1-3)
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

I tried the cross compiler and encountered the same build error.

[root@nvidia-grace-hopper-05 edk2-guest]# export | grep GCC5_AARCH64_PREFIX
declare -x GCC5_AARCH64_PREFIX="aarch64-linux-gnu-"
[root@nvidia-grace-hopper-05 edk2-guest]# build -b DEBUG -a AARCH64 -t GCC5 -p 
ArmVirtPkg/ArmVirtQemu.dsc
  :
--add-gnu-debuglink=/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.debug
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
cp -p -f 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.debug
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPrePeiCore.debug
"GenFw" -e SEC -o 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/OUTPUT/ArmPlatformPrePeiCore.efi
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
GenFw: ERROR 3000: Invalid
  WriteSections64(): 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
 AARCH64 small code model requires identical ELF and PE/COFF section offsets 
modulo 4 KB.
GenFw: ERROR 3000: Invalid
  :


   build -b DEBUG -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemu.dsc
:
   WriteSections64(): 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/ArmPlatformPkg/PrePeiCore/PrePeiCoreUniCore/DEBUG/ArmPlatformPrePeiCore.dll
 AARCH64 small code model requires identical ELF and PE/COFF section offsets 
modulo 4 KB.
cp -p -f 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/OvmfPkg/VirtioFsDxe/VirtioFsDxe/DEBUG/VirtioFsDxe.dll
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/OvmfPkg/VirtioFsDxe/VirtioFsDxe/DEBUG/VirtioFsDxe.debug
cp -p -f 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/Disk/PartitionDxe/PartitionDxe/DEBUG/PartitionDxe.debug
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/PartitionDxe.debug
"gcc" -MMD -MF 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/CryptoPkg/Library/OpensslLib/OpensslLibCrypto/OUTPUT/openssl/crypto/asn1/x_sig.obj.deps
 
@/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/CryptoPkg/Library/OpensslLib/OpensslLibCrypto/OUTPUT/cc_resp.txt
  -c -o 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/CryptoPkg/Library/OpensslLib/OpensslLibCrypto/OUTPUT/openssl/crypto/asn1/x_sig.obj
  
/home/gavin/sandbox/CCA/edk2-guest/CryptoPkg/Library/OpensslLib/openssl/crypto/asn1/x_sig.c
"GenFw" -e DXE_CORE -o 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/OUTPUT/DxeCore.efi
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Core/Dxe/DxeMain/DEBUG/DxeCore.dll
GenSec -s EFI_SECTION_USER_INTERFACE -n ArmCpuDxe -o 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/FV/Ffs/B8D9777E-D72A-451F-9BDB-BAFB52A68415ArmCpuDxe/B8D9777E-D72A-451F-9BDB-BAFB52A68415SEC3.ui
cp -p -f 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/DisplayEngineDxe/DisplayEngineDxe/DEBUG/*.map
 
/home/gavin/sandbox/CCA/edk2-guest/Build/ArmVirtQemu-AARCH64/DEBUG_GCC5/AARCH64/MdeModulePkg/Universal/DisplayEn

Re: Unexpected error in rme_configure_one() at ../target/arm/kvm-rme.c:159

[Gavin Shan]

Hi Jean and Ard,

On 6/1/24 01:09, Jean-Philippe Brucker wrote:

On Fri, May 31, 2024 at 04:23:13PM +1000, Gavin Shan wrote:

I got a chance to try CCA software components, suggested by [1]. However, the 
edk2
is stuck somewhere. I didn't reach to stage of loading guest kernel yet. I'm 
replying
to see if anyone has a idea.

...

INFO:BL31: Preparing for EL3 exit to normal world
INFO:Entry point address = 0x6000
INFO:SPSR = 0x3c9
UEFI firmware (version  built at 01:31:23 on May 31 2024)

The boot is stuck and no more output after that. I tried adding more verbose 
output
from edk2 and found it's stuck at the following point.


ArmVirtPkg/PrePi/PrePi.c::PrePiMain
rmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c::PlatformPeim

  #ifdef MDE_CPU_AARCH64
   //
   // Set the SMCCC conduit to SMC if executing at EL2, which is typically the
   // exception level that services HVCs rather than the one that invokes them.
   //
   if (ArmReadCurrentEL () == AARCH64_EL2) {
 Status = PcdSetBoolS (PcdMonitorConduitHvc, FALSE);   // The function 
is never returned in my case
 ASSERT_EFI_ERROR (Status);
   }
  #endif


I'm able to reproduce this even without RME. This code was introduced
recently by c98f7f755089 ("ArmVirtPkg: Use dynamic PCD to set the SMCCC
conduit"). Maybe Ard (Cc'd) knows what could be going wrong here.

A slightly reduced reproducer:

$ cd edk2/
$ build -b DEBUG -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc
$ cd ..

$ git clone https://github.com/ARM-software/arm-trusted-firmware.git tf-a
$ cd tf-a/
$ make -j CROSS_COMPILE=aarch64-linux-gnu- PLAT=qemu DEBUG=1 LOG_LEVEL=40 
QEMU_USE_GIC_DRIVER=QEMU_GICV3 
BL33=../edk2/Build/ArmVirtQemuKernel-AARCH64/DEBUG_GCC5/FV/QEMU_EFI.fd all fip 
&& \
   dd if=build/qemu/debug/bl1.bin of=flash.bin && \
   dd if=build/qemu/debug/fip.bin of=flash.bin seek=64 bs=4096
$ qemu-system-aarch64 -M virt,virtualization=on,secure=on,gic-version=3 -cpu 
max -m 2G -smp 8 -monitor none -serial mon:stdio -nographic -bios flash.bin



Thanks for the hints. Eventually, I'm able to start the host with 
'edk2-stable202402'.
Note that 'edk2-stable202405' doesn't work. However, I failed to build the edk2 
for
guest and unable to start the guest successfully, more information is provided 
below.

--> host's edk2

# git clone g...@github.com:gwshan/edk2.git edk2
# cd edk2; git checkout edk2-stable202402 -b stable202402
# git submodule update --init --recursive;  \
  source edksetup.sh; make -j -C BaseTools; \
  export GCC5_AARCH64_PREFIX=;  \
  build -b DEBUG -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc

---> tf-a: rebuild using commands as you suggested.

---> Boot host

/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 
  \
-M virt,virtualization=on,secure=on,gic-version=3,acpi=off  
  \
-cpu max,x-rme=on -m 8G -smp 8  
  \
-nodefaults -monitor none -serial mon:stdio -nographic  
  \
-bios /home/gavin/sandbox/CCA/tf-a/flash.bin
  \
-kernel /home/gavin/sandbox/CCA/linux/arch/arm64/boot/Image 
  \
-drive 
format=raw,if=none,file=/home/gavin/sandbox/CCA/buildroot/output/images/rootfs.ext4,id=hd0
 \
-device virtio-blk-pci,drive=hd0
  \
-netdev 
tap,id=tap0,vhost=false,script=/etc/qemu-ifup,downscript=/etc/qemu-ifdown   
  \
-device virtio-net-pci,netdev=tap0,mac=52:54:00:f1:26:b0
  \
-append root=/dev/vda console=ttyAMA0   
  \
-device virtio-9p-device,fsdev=shr0,mount_tag=shr0  
  \
-fsdev local,security_model=none,path=/home/gavin/sandbox/CCA,id=shr0
  :
NOTICE:  Booting Trusted Firmware
NOTICE:  BL1: v2.10.0(debug):99e0b97aa-dirty
NOTICE:  BL1: Built : 00:31:35, May 31 2024
INFO:BL1: RAM 0xe0ee000 - 0xe0f7000
INFO:BL1: Loading BL2
  :
Booting RMM v.0.4.0(debug) 17924bc Built with GCC 11.4.1
RMM-EL3 Interface v.0.2
Boot Manifest Interface v.0.3
RMI/RSI ABI v.1.0/1.0 built: May 31 2024 00:21:59
INFO:RMM init end.
  :
UEFI firmware (version  built at 04:07:42 on Jun  1 2024)
PlatformPeim: PL011 UART (console) @ 0x900
PlatformPeim: PL011 UART (debug) @ 0x900
  :
EFI stub: Booting Linux Kernel...
EFI stub: EFI_RNG_PROTOCOL unavailable
  :
Welcome to Buildroot
buildroot login:
# ifconfig eth0 | grep 'inet addr'
  inet addr:10.26.1.212  Bcast:10.26.1.255  Mask:255.255.255.0

---> guest edk2

# git clone https://git.codelinaro.org/linaro/dcap/edk2.git edk2-guest
# cd edk2-guest; git checkout origin/cca/v2 -b cca/v2
# git submodule update --init --recursive;  \
  source edksetu

Re: Unexpected error in rme_configure_one() at ../target/arm/kvm-rme.c:159

[Gavin Shan]

On 5/31/24 14:19, Itaru Kitayama wrote:

On May 30, 2024, at 22:30, Philippe Mathieu-Daudé  wrote:
Cc'ing more developers

On 30/5/24 06:30, Itaru Kitayama wrote:

Hi,
When I see a Realm VM creation fails with:
Unexpected error in rme_configure_one() at ../target/arm/kvm-rme.c:159:
qemu-system-aarch64: RME: failed to configure SVE: Invalid argument
test.sh: line 8:  2502 Aborted qemu-system-aarch64 -M 
'virt,acpi=off,gic-version=3' -cpu host -enable-kvm -smp 2 -m 512M -overcommit 
'mem-lock=on' -M 'confidential-guest-support=rme0' -object 
'rme-guest,id=rme0,measurement-algo=sha512,num-pmu-counters=6,sve-vector-length=256'
 -kernel Image -initrd rootfs.cpio -append 'earycon console=ttyAMA0 
rdinit=/sbin/init' -nographic -net none
do I need to suspect first the VMM, QEMU, or the Image? The kernel is built 
with LLVM, does it matter?
Thanks,
Itaru.




I’m testing Jean’s repo at:

https://git.codelinaro.org/linaro/dcap/qemu/-/tree/cca/v2?ref_type=heads



I got a chance to try CCA software components, suggested by [1]. However, the 
edk2
is stuck somewhere. I didn't reach to stage of loading guest kernel yet. I'm 
replying
to see if anyone has a idea.

[1] 
https://linaro.atlassian.net/wiki/spaces/QEMU/pages/29051027459/Building+an+RME+stack+for+QEMU


---> tf-rmm

# git clone https://git.codelinaro.org/linaro/dcap/rmm.git tf-rmm
# cd tf-rmm
# git checkout origin/cca/v2 -b cca/v2
# git submodule update --init --recursive
# export CROSS_COMPILE=
# cmake -DCMAKE_BUILD_TYPE=Debug -DRMM_CONFIG=qemu_virt_defcfg -B build-qemu
# cmake --build build-qemu

---> edk2

# git clone g...@github.com:tianocore/edk2.git
# cd edk2
# git submodule update --init --recursive
# source edksetup.sh
# make -j -C BaseTools
# export GCC5_AARCH64_PREFIX=
# build -b RELEASE -a AARCH64 -t GCC5 -p ArmVirtPkg/ArmVirtQemuKernel.dsc

---> tf-a

# git clone https://git.codelinaro.org/linaro/dcap/tf-a/trusted-firmware-a.git 
tf-a
# cd tf-a
# git checkout origin/cca/v2 -b cca/v2
# make -j CROSS_COMPILE= PLAT=qemu ENABLE_RME=1 DEBUG=1 LOG_LEVEL=40 \
  QEMU_USE_GIC_DRIVER=QEMU_GICV3 RMM=../rmm/build-qemu/Debug/rmm.img \
  BL33=../edk2/Build/ArmVirtQemuKernel-AARCH64/RELEASE_GCC5/FV/QEMU_EFI.fd all 
fip
# dd if=build/qemu/debug/bl1.bin of=flash.bin
# dd if=build/qemu/debug/fip.bin of=flash.bin seek=64 bs=4096

---> QEMU

# git clone https://git.qemu.org/git/qemu.git qemu.main
# cd qemu.main
# ./configure --target-list=aarch64-softmmu
# make -j 60

---> boot the host on the emulated hardware

/home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 
   \
-M virt,virtualization=on,secure=on,gic-version=3,acpi=off  
   \
-cpu max,x-rme=on -m 8G -smp 8  
   \
-nodefaults -monitor none -serial mon:stdio -nographic  
   \
-bios /home/gavin/sandbox/CCA/tf-a/flash.bin
   \
-kernel /home/gavin/sandbox/CCA/linux/arch/arm64/boot/Image 
   \
-drive 
format=raw,if=none,file=${CCA}/buildroot/output/images/rootfs.ext4,id=hd0   \
-device virtio-blk-pci,drive=hd0
   \
-append "root=/dev/vda console=ttyAMA0"
  :
NOTICE:  Booting Trusted Firmware
NOTICE:  BL1: v2.10.0(debug):99e0b97aa-dirty
NOTICE:  BL1: Built : 00:31:35, May 31 2024
INFO:BL1: RAM 0xe0ee000 - 0xe0f7000
INFO:BL1: Loading BL2
INFO:Loading image id=1 at address 0xe06b000
INFO:Image id=1 loaded: 0xe06b000 - 0xe0742d1
NOTICE:  BL1: Booting BL2
INFO:Entry point address = 0xe06b000
INFO:SPSR = 0x3cd
INFO:[GPT] Boot Configuration
INFO:  PPS/T: 0x2/40
INFO:  PGS/P: 0x0/12
INFO:  L0GPTSZ/S: 0x0/30
INFO:  PAS count: 0x6
INFO:  L0 base:   0xedfe000
INFO:[GPT] PAS[0]: base 0xe001000, size 0xff000, GPI 0xa, type 0x1
INFO:[GPT] PAS[1]: base 0xe10, size 0xcfe000, GPI 0x8, type 0x1
INFO:[GPT] PAS[2]: base 0xedfe000, size 0x202000, GPI 0xa, type 0x1
INFO:[GPT] PAS[3]: base 0x4000, size 0x10, GPI 0x9, type 0x1
INFO:[GPT] PAS[4]: base 0x4010, size 0x280, GPI 0xb, type 0x1
INFO:[GPT] PAS[5]: base 0x4290, size 0x1fd70, GPI 0x9, type 0x1
INFO:Enabling Granule Protection Checks
NOTICE:  BL2: v2.10.0(debug):99e0b97aa-dirty
NOTICE:  BL2: Built : 00:31:35, May 31 2024
INFO:BL2: Doing platform setup
INFO:Reserved RMM memory [0x4010, 0x428f] in Device tree
INFO:BL2: Loading image id 3
INFO:Loading image id=3 at address 0xe0a
INFO:Image id=3 loaded: 0xe0a - 0xe0b10c4
INFO:BL2: Loading image id 35
INFO:Loading image id=35 at address 0x4010
INFO:Image id=35 loaded: 0x4010 - 0x403033b0
INFO:BL2: Loading image id 5
INFO:Loading image id=5 at address 0x6000
INFO:Image id=5 loaded: 0x6000 - 0x6020
NOTICE:  BL2: Booting BL31
INFO:Entry point address = 0xe0a
INFO:SPSR = 0x3cd
NOTICE:  BL31: 

Re: [PATCH v2 5/5] hw/arm/aspeed: Check for CPU types in machine_run_board_init()

[Gavin Shan]

On 1/24/24 08:48, Philippe Mathieu-Daudé wrote:

Aspeed SoCs use a single CPU type (set as AspeedSoCClass::cpu_type).
Convert it to a NULL-terminated array (of a single non-NULL element).

Set MachineClass::valid_cpu_types[] to use the common machine code
to provide hints when the requested CPU is invalid (see commit
e702cbc19e ("machine: Improve is_cpu_type_supported()").

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/hw/arm/aspeed_soc.h |  3 ++-
  hw/arm/aspeed.c |  1 +
  hw/arm/aspeed_ast10x0.c |  6 +-
  hw/arm/aspeed_ast2400.c | 12 ++--
  hw/arm/aspeed_ast2600.c |  6 +-
  hw/arm/aspeed_soc_common.c  |  5 -
  6 files changed, 27 insertions(+), 6 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 4/5] hw/arm/aspeed: Introduce aspeed_soc_cpu_type() helper

[Gavin Shan]

On 1/24/24 08:48, Philippe Mathieu-Daudé wrote:

In order to alter AspeedSoCClass::cpu_type in the next
commit, introduce the aspeed_soc_cpu_type() helper to
retrieve the per-SoC CPU type from AspeedSoCClass.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/hw/arm/aspeed_soc.h | 1 +
  hw/arm/aspeed_ast10x0.c | 2 +-
  hw/arm/aspeed_ast2400.c | 3 ++-
  hw/arm/aspeed_ast2600.c | 3 ++-
  hw/arm/aspeed_soc_common.c  | 5 +
  5 files changed, 11 insertions(+), 3 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 3/5] hw/arm/aspeed: Init CPU defaults in a common helper

[Gavin Shan]

Hi Phil,

On 1/24/24 08:48, Philippe Mathieu-Daudé wrote:

Rework aspeed_soc_num_cpus() as a new init_cpus_defaults()
helper to reduce code duplication.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/aspeed.c | 71 +++--
  1 file changed, 28 insertions(+), 43 deletions(-)



One nit needs to be addressed:

Reviewed-by: Gavin Shan 


diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 5b01a4dd28..636a6269aa 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1141,10 +1141,14 @@ static void aspeed_machine_class_props_init(ObjectClass 
*oc)
"Change the SPI Flash model");
  }
  
-static int aspeed_soc_num_cpus(const char *soc_name)

+static void aspeed_machine_class_init_cpus_defaults(MachineClass *mc)
  {
-   AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(soc_name));
-   return sc->num_cpus;
+AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(mc);
+AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
+
+mc->default_cpus = mc->min_cpus
+ = mc->max_cpus
+ = sc->num_cpus;
  }
  
  static void aspeed_machine_class_init(ObjectClass *oc, void *data)

@@ -1176,8 +1180,7 @@ static void 
aspeed_machine_palmetto_class_init(ObjectClass *oc, void *data)
  amc->num_cs= 1;
  amc->i2c_init  = palmetto_bmc_i2c_init;
  mc->default_ram_size   = 256 * MiB;
-mc->default_cpus = mc->min_cpus = mc->max_cpus =
-aspeed_soc_num_cpus(amc->soc_name);
+aspeed_machine_class_init_cpus_defaults(mc);
  };
  
  static void aspeed_machine_quanta_q71l_class_init(ObjectClass *oc, void *data)

@@ -1193,8 +1196,7 @@ static void 
aspeed_machine_quanta_q71l_class_init(ObjectClass *oc, void *data)
  amc->num_cs= 1;
  amc->i2c_init  = quanta_q71l_bmc_i2c_init;
  mc->default_ram_size   = 128 * MiB;
-mc->default_cpus = mc->min_cpus = mc->max_cpus =
-aspeed_soc_num_cpus(amc->soc_name);
+aspeed_machine_class_init_cpus_defaults(mc);
  }
  
  static void aspeed_machine_supermicrox11_bmc_class_init(ObjectClass *oc,

@@ -1212,8 +1214,7 @@ static void 
aspeed_machine_supermicrox11_bmc_class_init(ObjectClass *oc,
  amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON;
  amc->i2c_init  = palmetto_bmc_i2c_init;
  mc->default_ram_size = 256 * MiB;
-mc->default_cpus = mc->min_cpus = mc->max_cpus =
-aspeed_soc_num_cpus(amc->soc_name);
+aspeed_machine_class_init_cpus_defaults(mc);
  }
  
  static void aspeed_machine_supermicro_x11spi_bmc_class_init(ObjectClass *oc,

@@ -1231,8 +1232,7 @@ static void 
aspeed_machine_supermicro_x11spi_bmc_class_init(ObjectClass *oc,
  amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON;
  amc->i2c_init  = palmetto_bmc_i2c_init;
  mc->default_ram_size = 512 * MiB;
-mc->default_cpus = mc->min_cpus = mc->max_cpus =
-aspeed_soc_num_cpus(amc->soc_name);
+aspeed_machine_class_init_cpus_defaults(mc);
  }
  
  static void aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)

@@ -1248,8 +1248,7 @@ static void 
aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)
  amc->num_cs= 1;
  amc->i2c_init  = ast2500_evb_i2c_init;
  mc->default_ram_size   = 512 * MiB;
-mc->default_cpus = mc->min_cpus = mc->max_cpus =
-aspeed_soc_num_cpus(amc->soc_name);
+aspeed_machine_class_init_cpus_defaults(mc);
  };
  
  static void aspeed_machine_yosemitev2_class_init(ObjectClass *oc, void *data)

@@ -1266,8 +1265,7 @@ static void 
aspeed_machine_yosemitev2_class_init(ObjectClass *oc, void *data)
  amc->num_cs= 2;
  amc->i2c_init  = yosemitev2_bmc_i2c_init;
  mc->default_ram_size   = 512 * MiB;
-mc->default_cpus = mc->min_cpus = mc->max_cpus =
-aspeed_soc_num_cpus(amc->soc_name);
+aspeed_machine_class_init_cpus_defaults(mc);
  };
  
  static void aspeed_machine_romulus_class_init(ObjectClass *oc, void *data)

@@ -1283,8 +1281,7 @@ static void aspeed_machine_romulus_class_init(ObjectClass 
*oc, void *data)
  amc->num_cs= 2;
  amc->i2c_init  = romulus_bmc_i2c_init;
  mc->default_ram_size   = 512 * MiB;
-mc->default_cpus = mc->min_cpus = mc->max_cpus =
-aspeed_soc_num_cpus(amc->soc_name);
+aspeed_machine_class_init_cpus_defaults(mc);
  };
  
  static void aspeed_machine_tiogapass_class_init(ObjectClass *oc, void *data)

@@ -1301,8 +1298,7 @@ static void 
aspeed_machine_tiogapass_class_init(ObjectClass *oc, void *data)
  amc->num_cs= 2;
  amc->i2c_init  = tiogapass_bmc_i2c_init;
  mc->default_ram_size   = 1 * GiB;
-mc->default_cpus = mc->min_cpus = mc->max_cpus =
-aspeed_soc_num_cpus(amc->soc_name);
+aspeed_ma

Re: [PATCH v2 2/5] hw/arm/aspeed: Set default CPU count using aspeed_soc_num_cpus()

[Gavin Shan]

On 1/24/24 08:48, Philippe Mathieu-Daudé wrote:

Since commit b7f1a0cb76 ("arm/aspeed: Compute the number
of CPUs from the SoC definition") Aspeed machines use the
aspeed_soc_num_cpus() helper to set the number of CPUs.

Use it for the ast1030-evb (commit 356b230ed1 "aspeed/soc:
Add AST1030 support") and supermicrox11-bmc (commit 40a38df55e
"hw/arm/aspeed: Add board model for Supermicro X11 BMC") machines.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/aspeed.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 1/5] hw/arm/aspeed: Remove dead code

[Gavin Shan]

On 1/24/24 08:48, Philippe Mathieu-Daudé wrote:

Remove copy/paste typo from commit 6c323aba40 ("hw/arm/aspeed:
Adding new machine Tiogapass in QEMU").

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/aspeed.c | 1 -
  1 file changed, 1 deletion(-)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 6/6] hw/arm/zynq: Check for CPU types in machine_run_board_init()

[Gavin Shan]

On 1/24/24 08:25, Philippe Mathieu-Daudé wrote:

Restrict MachineClass::valid_cpu_types[] to the single
valid CPU type.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/xilinx_zynq.c | 5 +
  1 file changed, 5 insertions(+)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 5/6] hw/arm/vexpress: Check for CPU types in machine_run_board_init()

[Gavin Shan]

On 1/24/24 08:25, Philippe Mathieu-Daudé wrote:

Restrict MachineClass::valid_cpu_types[] to the single
valid CPU types.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/vexpress.c | 10 ++
  1 file changed, 10 insertions(+)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 4/6] hw/arm/highbank: Check for CPU types in machine_run_board_init()

[Gavin Shan]

On 1/24/24 08:25, Philippe Mathieu-Daudé wrote:

Restrict MachineClass::valid_cpu_types[] to the single
valid CPU types.

Instead of ignoring invalid CPU type requested by the user:

   $ qemu-system-arm -M midway -cpu cortex-a7 -S -monitor stdio
   QEMU 8.2.50 monitor - type 'help' for more information
   (qemu) info qom-tree
   /machine (midway-machine)
 /cpu[0] (cortex-a15-arm-cpu)
 ...

we now display an error:

   $ qemu-system-arm -M midway -cpu cortex-a7
   qemu-system-arm: Invalid CPU model: cortex-a7
   The only valid type is: cortex-a15

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/highbank.c | 10 ++
  1 file changed, 10 insertions(+)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 3/6] hw/arm/highbank: Add missing QOM parent for CPU cores

[Gavin Shan]

On 1/24/24 08:25, Philippe Mathieu-Daudé wrote:

QDev objects created with qdev_new() need to manually add
their parent relationship with object_property_add_child().

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/highbank.c | 1 +
  1 file changed, 1 insertion(+)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 2/6] hw/arm/exynos: Check for CPU types in machine_run_board_init()

[Gavin Shan]

On 1/24/24 08:25, Philippe Mathieu-Daudé wrote:

Restrict MachineClass::valid_cpu_types[] to the single
valid CPU type.

Instead of ignoring invalid CPU type requested by the user:

   $ qemu-system-arm -M nuri -cpu cortex-a7 -S -monitor stdio
   QEMU 8.2.50 monitor - type 'help' for more information
   (qemu) info qom-tree
   /machine (nuri-machine)
 /soc (exynos4210)
   /cpu[0] (cortex-a9-arm-cpu)
   ...

We now display an error:

   $ qemu-system-arm -M nuri -cpu cortex-a7
   qemu-system-arm: Invalid CPU model: cortex-a7
   The only valid type is: cortex-a9

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/exynos4_boards.c | 8 
  1 file changed, 8 insertions(+)



Reviewed-by: Gavin Shan 

Re: [PATCH v2 1/6] hw/arm/exynos: Add missing QOM parent for CPU cores

[Gavin Shan]

On 1/24/24 08:25, Philippe Mathieu-Daudé wrote:

QDev objects created with qdev_new() need to manually add
their parent relationship with object_property_add_child().

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/exynos4210.c | 1 +
  1 file changed, 1 insertion(+)



Reviewed-by: Gavin Shan 

Re: [PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[Gavin Shan]

Hi Phil,

On 1/11/24 18:21, Philippe Mathieu-Daudé wrote:

On 11/1/24 08:30, Gavin Shan wrote:

On 1/11/24 16:47, Philippe Mathieu-Daudé wrote:

Per cpu_model_from_type() docstring (added in commit 445946f4dd):

   * Returns: CPU model name or NULL if the CPU class doesn't exist

We must check the return value in order to avoid surprises, i.e.:

  $ qemu-system-arm -machine virt -cpu cortex-a9
   qemu-system-arm: Invalid CPU model: cortex-a9
   The valid models are: cortex-a7, cortex-a15, (null), (null), (null), (null), 
(null), (null), (null), (null), (null), (null), (null), max

Add assertions when the call can not fail (because the CPU type
must be registered).

Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
Reported-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
  cpu-target.c  | 1 +
  hw/core/machine.c | 5 +
  target/ppc/cpu_init.c | 1 +
  3 files changed, 7 insertions(+)

diff --git a/cpu-target.c b/cpu-target.c
index 5eecd7ea2d..b0f6deb13b 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -291,6 +291,7 @@ static void cpu_list_entry(gpointer data, gpointer 
user_data)
  const char *typename = object_class_get_name(OBJECT_CLASS(data));
  g_autofree char *model = cpu_model_from_type(typename);
+    assert(model);
  if (cc->deprecation_note) {
  qemu_printf("  %s (deprecated)\n", model);
  } else {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index fc239101f9..730ec10328 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1422,16 +1422,21 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
  /* The user specified CPU type isn't valid */
  if (!mc->valid_cpu_types[i]) {
  g_autofree char *requested = 
cpu_model_from_type(machine->cpu_type);
+    assert(requested);
  error_setg(errp, "Invalid CPU model: %s", requested);
  if (!mc->valid_cpu_types[1]) {
  g_autofree char *model = cpu_model_from_type(
mc->valid_cpu_types[0]);
+    assert(model);
  error_append_hint(errp, "The only valid type is: %s\n", 
model);
  } else {
  error_append_hint(errp, "The valid models are: ");
  for (i = 0; mc->valid_cpu_types[i]; i++) {
  g_autofree char *model = cpu_model_from_type(
mc->valid_cpu_types[i]);
+    if (!model) {
+    continue;
+    }


Shall we assert(model) for this case, to be consistent with other cases? :)


No, this is the "(null)" cases displayed in the example.

IOW, mc->valid_cpu_types[] contains a CPU type which isn't registered,
so we just skip it.



I thought this should be fixed by correcting mc->valid_cpu_types[] in
hw/arm/virt.c. It means the consistent mc->valid_cpu_types[] needs to
be  provided by the specific board. Otherwise, the logic is incorrect from
the code level at least. For example, "cortex-a9" isn't available to
qemu-system-arm but it has been wrongly declared as supported in hw/arm/virt.c

I've posted one patch against it:

https://lists.nongnu.org/archive/html/qemu-arm/2024-01/msg00531.html





  error_append_hint(errp, "%s%s",
    model,
    mc->valid_cpu_types[i + 1] ? ", " : "");


Otherwise, the separator here need to be adjusted because it's uncertain that
mc->valid_cpu_types[i+1] ... mc->valid_cpu_types[END] are valid.


Here we know mc->valid_cpu_types[i] is *not* NULL, but
mc->valid_cpu_types[i + 1] might be (signaling the end
of the array).

This seems correct to me, but I might be missing something.



When the class for mc->valid_cpu_types[i + 1] isn't registered, we will
skip the entry. it's possible that the class of mc->valid_cpu_types[i + 2]
isn't registered either. mc->valid_cpu_types[i + 3] to mc->valid_cpu_types[END 
- 1]
have the similar situations.

In order to correct the separator, we need to invalidate the return value
from cpu_model_from_type(mc->valid_cpu_types[i + 1]) ... 
cpu_model_from_type(mc->valid_cpu_types[END - 1]).
Too much complex for that and it's another reason why I suggested assert(model) 
as above




diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 344196a8ce..58f0c1e30e 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7037,6 +7037,7 @@ static void ppc_cpu_list_entry(gpointer data, gpointer 
user_data)
  }
  name = cpu_model_from_type(typename);
+    assert(name);
  qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
  for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
  PowerPCCPUAlias *alias = _cpu_aliases[i];




Thanks,
Gavin

Re: [PATCH] hw/core: Handle cpu_model_from_type() returning NULL value

[Gavin Shan]

Hi Phil,

On 1/11/24 16:47, Philippe Mathieu-Daudé wrote:

Per cpu_model_from_type() docstring (added in commit 445946f4dd):

   * Returns: CPU model name or NULL if the CPU class doesn't exist

We must check the return value in order to avoid surprises, i.e.:

  $ qemu-system-arm -machine virt -cpu cortex-a9
   qemu-system-arm: Invalid CPU model: cortex-a9
   The valid models are: cortex-a7, cortex-a15, (null), (null), (null), (null), 
(null), (null), (null), (null), (null), (null), (null), max

Add assertions when the call can not fail (because the CPU type
must be registered).

Fixes: 5422d2a8fa ("machine: Print CPU model name instead of CPU type")
Reported-by: Peter Maydell 
Signed-off-by: Philippe Mathieu-Daudé 
---
  cpu-target.c  | 1 +
  hw/core/machine.c | 5 +
  target/ppc/cpu_init.c | 1 +
  3 files changed, 7 insertions(+)

diff --git a/cpu-target.c b/cpu-target.c
index 5eecd7ea2d..b0f6deb13b 100644
--- a/cpu-target.c
+++ b/cpu-target.c
@@ -291,6 +291,7 @@ static void cpu_list_entry(gpointer data, gpointer 
user_data)
  const char *typename = object_class_get_name(OBJECT_CLASS(data));
  g_autofree char *model = cpu_model_from_type(typename);
  
+assert(model);

  if (cc->deprecation_note) {
  qemu_printf("  %s (deprecated)\n", model);
  } else {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index fc239101f9..730ec10328 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1422,16 +1422,21 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
  /* The user specified CPU type isn't valid */
  if (!mc->valid_cpu_types[i]) {
  g_autofree char *requested = 
cpu_model_from_type(machine->cpu_type);
+assert(requested);
  error_setg(errp, "Invalid CPU model: %s", requested);
  if (!mc->valid_cpu_types[1]) {
  g_autofree char *model = cpu_model_from_type(
   mc->valid_cpu_types[0]);
+assert(model);
  error_append_hint(errp, "The only valid type is: %s\n", 
model);
  } else {
  error_append_hint(errp, "The valid models are: ");
  for (i = 0; mc->valid_cpu_types[i]; i++) {
  g_autofree char *model = cpu_model_from_type(
   mc->valid_cpu_types[i]);
+if (!model) {
+continue;
+}


Shall we assert(model) for this case, to be consistent with other cases? :)


  error_append_hint(errp, "%s%s",
model,
mc->valid_cpu_types[i + 1] ? ", " : "");


Otherwise, the separator here need to be adjusted because it's uncertain that
mc->valid_cpu_types[i+1] ... mc->valid_cpu_types[END] are valid.



diff --git a/target/ppc/cpu_init.c b/target/ppc/cpu_init.c
index 344196a8ce..58f0c1e30e 100644
--- a/target/ppc/cpu_init.c
+++ b/target/ppc/cpu_init.c
@@ -7037,6 +7037,7 @@ static void ppc_cpu_list_entry(gpointer data, gpointer 
user_data)
  }
  
  name = cpu_model_from_type(typename);

+assert(name);
  qemu_printf("PowerPC %-16s PVR %08x\n", name, pcc->pvr);
  for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
  PowerPCCPUAlias *alias = _cpu_aliases[i];


Thanks,
Gavin

Re: [PULL 29/71] hw/arm/virt: Check CPU type in machine_run_board_init()

[Gavin Shan]

Hi Peter,

On 1/10/24 00:33, Peter Maydell wrote:

On Fri, 5 Jan 2024 at 15:46, Philippe Mathieu-Daudé  wrote:


From: Gavin Shan 

Set mc->valid_cpu_types so that the user specified CPU type can be
validated in machine_run_board_init(). We needn't to do the check
by ourselves.


Hi; after this change if you try to use the 'virt' board from
qemu-system-arm with an invalid CPU type you get an odd
error message full of "(null)"s:

$ ./build/x86/qemu-system-arm -machine virt -cpu cortex-a9
qemu-system-arm: Invalid CPU model: cortex-a9
The valid models are: cortex-a7, cortex-a15, (null), (null), (null),
(null), (null), (null), (null), (null), (null), (null), (null), max

This seems to be because we print a "(null)" for every 64-bit
only CPU in the list, instead of either ignoring them or not
compiling them into the list in the first place.

https://gitlab.com/qemu-project/qemu/-/issues/2084



Yes, it's because all 64-bits CPUs aren't available to 'qemu-system-arm'.
I've sent a fix for it. Please take a look when getting a chance.

https://lists.nongnu.org/archive/html/qemu-arm/2024-01/msg00531.html

Thanks,
Gavin

[PATCH] hw/arm/virt: Consolidate valid CPU types

[Gavin Shan]

It's found that some of the CPU type names in the array of valid
CPU types are invalid because their corresponding classes aren't
registered, as reported by Peter Maydell.

[gshan@gshan build]$ ./qemu-system-arm -machine virt -cpu cortex-a9
qemu-system-arm: Invalid CPU model: cortex-a9
The valid models are: cortex-a7, cortex-a15, (null), (null), (null),
(null), (null), (null), (null), (null), (null), (null), (null), max

Fix it by consolidating the array of valid CPU types. After it's
applied, we have the following output when TCG is enabled.

[gshan@gshan build]$ ./qemu-system-arm -machine virt -cpu cortex-a9
qemu-system-arm: Invalid CPU model: cortex-a9
The valid models are: cortex-a7, cortex-a15, max

[gshan@gshan build]$ ./qemu-system-aarch64 -machine virt -cpu cortex-a9
qemu-system-aarch64: Invalid CPU model: cortex-a9
The valid models are: cortex-a7, cortex-a15, cortex-a35, cortex-a55,
cortex-a72, cortex-a76, cortex-a710, a64fx, neoverse-n1, neoverse-v1,
neoverse-n2, cortex-a53, cortex-a57, max

Reported-by: Peter Maydell 
Fixes: fa8c617791 ("hw/arm/virt: Check CPU type in machine_run_board_init()")
Signed-off-by: Gavin Shan 
---
 hw/arm/virt.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2793121cb4..5cbc69dff8 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2905,6 +2905,7 @@ static void virt_machine_class_init(ObjectClass *oc, void 
*data)
 #ifdef CONFIG_TCG
 ARM_CPU_TYPE_NAME("cortex-a7"),
 ARM_CPU_TYPE_NAME("cortex-a15"),
+#ifdef TARGET_AARCH64
 ARM_CPU_TYPE_NAME("cortex-a35"),
 ARM_CPU_TYPE_NAME("cortex-a55"),
 ARM_CPU_TYPE_NAME("cortex-a72"),
@@ -2914,12 +2915,15 @@ static void virt_machine_class_init(ObjectClass *oc, 
void *data)
 ARM_CPU_TYPE_NAME("neoverse-n1"),
 ARM_CPU_TYPE_NAME("neoverse-v1"),
 ARM_CPU_TYPE_NAME("neoverse-n2"),
-#endif
+#endif /* TARGET_AARCH64 */
+#endif /* CONFIG_TCG */
+#ifdef TARGET_AARCH64
 ARM_CPU_TYPE_NAME("cortex-a53"),
 ARM_CPU_TYPE_NAME("cortex-a57"),
 #if defined(CONFIG_KVM) || defined(CONFIG_HVF)
 ARM_CPU_TYPE_NAME("host"),
-#endif
+#endif /* CONFIG_KVM || CONFIG_HVF */
+#endif /* TARGET_AARCH64 */
 ARM_CPU_TYPE_NAME("max"),
 NULL
 };
-- 
2.43.0

Re: [PATCH v9 0/9] Unified CPU type check

[Gavin Shan]

Hi Phil,

On 1/6/24 08:12, Philippe Mathieu-Daudé wrote:

On 13/12/23 11:54, Gavin Shan wrote:

On 12/13/23 20:08, Philippe Mathieu-Daudé wrote:

On 12/12/23 05:55, Gavin Shan wrote:

On 12/4/23 10:47, Gavin Shan wrote:

This series bases on Phil's repository because the prepatory commits
have been queued to the branch.

   https://gitlab.com/philmd/qemu.git (branch: cpus-next)

There are two places where the user specified CPU type is checked to see
if it's supported or allowed by the board: machine_run_board_init() and
mc->init(). We don't have to maintain two duplicate sets of logic. This
series intends to move the check to machine_run_board_init() so that we
have unified CPU type check.

This series can be checked out from:

   g...@github.com:gwshan/qemu.git (branch: kvm/cpu-type)

PATCH[1-4] refactors and improves the logic to validate CPU type in
    machine_run_board_init()
PATCH[5-9] validates the CPU type in machine_run_board_init() for the
    individual boards

v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html



Ping to see if there is a chance to queue it up before the Chrismas? :)


Series queued. "Before" Christmas will depend on the final release tag.

Thanks for the various iterations,



Phil, thank you for you continuous reviews and valuable comments.

Yes, the final merge to master branch depends on the release plan.
'queue' meant to merge the series to your 'cpus-next' branch ;-)


I had to fix 3 different issues caught by our CI. Next time please
run your series on GitLab CI, you just have to push your branch and
wait for the result :)

Now merged as 445946f4dd..cd75cc6337.

Happy new year!



Happy new year. I just came back from holiday.

Thanks for the reminder for GitLab CI, which I don't know how to run yet.
I will learn how to run it from gitlab :)

Thanks,
Gavin

Re: [PATCH v9 3/9] machine: Improve is_cpu_type_supported()

[Gavin Shan]

On 1/6/24 08:09, Philippe Mathieu-Daudé wrote:

On 4/12/23 01:47, Gavin Shan wrote:

It's no sense to check the CPU type when mc->valid_cpu_types[0] is
NULL, which is a program error. Raise an assert on this.

A precise hint for the error message is given when mc->valid_cpu_types[0]
is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0]
when we have mutiple valid entries there is avoided to increase the code
readability, as suggested by Philippe Mathieu-Daudé.

Besides, @cc comes from machine->cpu_type or mc->default_cpu_type. For
the later case, it can be NULL and it's also a program error. We should
use assert() in this case.

Signed-off-by: Gavin Shan 
v9: assert(mc->valid_cpu_types[0] != NULL)   (Phil)
 assert(cc != NULL);  (Phil)
---
  hw/core/machine.c | 20 ++--
  1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1797e002f9..4ae9aaee8e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1400,6 +1400,7 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
   * type is provided through '-cpu' option.
   */
  if (mc->valid_cpu_types && machine->cpu_type) {
+    assert(mc->valid_cpu_types[0] != NULL);
  for (i = 0; mc->valid_cpu_types[i]; i++) {
  if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
  break;
@@ -1409,20 +1410,27 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
  /* The user specified CPU type isn't valid */
  if (!mc->valid_cpu_types[i]) {
  error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
-    error_append_hint(errp, "The valid types are: %s",
-  mc->valid_cpu_types[0]);
-    for (i = 1; mc->valid_cpu_types[i]; i++) {
-    error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+    if (!mc->valid_cpu_types[1]) {
+    error_append_hint(errp, "The only valid type is: %s\n",
+  mc->valid_cpu_types[0]);
+    } else {
+    error_append_hint(errp, "The valid types are: ");
+    for (i = 0; mc->valid_cpu_types[i]; i++) {
+    error_append_hint(errp, "%s%s",
+  mc->valid_cpu_types[i],
+  mc->valid_cpu_types[i + 1] ? ", " : "");
+    }
+    error_append_hint(errp, "\n");
  }
-    error_append_hint(errp, "\n");
  return false;
  }
  }
  /* Check if CPU type is deprecated and warn if so */
  cc = CPU_CLASS(oc);
-    if (cc && cc->deprecation_note) {
+    assert(cc != NULL);
+    if (cc->deprecation_note) {
  warn_report("CPU model %s is deprecated -- %s",
  machine->cpu_type, cc->deprecation_note);
  }


Since we were getting:

$ qemu-system-s390x -M none
QEMU 8.2.50 monitor - type 'help' for more information
qemu-system-s390x: ../../hw/core/machine.c:1444: _Bool 
is_cpu_type_supported(const MachineState *, Error **): Assertion `cc != NULL' 
failed.
Aborted (core dumped)

I added a check on mc->valid_cpu_types before calling
is_cpu_type_supported() in the previous patch. See commit acbadc5a29.



Phil, thanks for fixing it up and it looks good to me.

Thanks,
Gavin

Re: [PATCH v9 1/9] machine: Use error handling when CPU type is checked

[Gavin Shan]

On 1/5/24 21:00, Philippe Mathieu-Daudé wrote:

On 4/12/23 01:47, Gavin Shan wrote:

Functions that use an Error **errp parameter to return errors should
not also report them to the user, because reporting is the caller's
job. The principle is violated by machine_run_board_init() because
it calls error_report(), error_printf(), and exit(1) when the machine
doesn't support the requested CPU type.

Clean this up by using error_setg() and error_append_hint() instead.
No functional change, as the only caller passes _fatal.

Suggested-by: Igor Mammedov 
Signed-off-by: Gavin Shan 
Reviewed-by: Markus Armbruster 
---
v9: Improved change log  (Markus)
---
  hw/core/machine.c | 13 +++--
  1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0c17398141..bde7f4af6d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1466,15 +1466,16 @@ void machine_run_board_init(MachineState *machine, 
const char *mem_path, Error *
  if (!machine_class->valid_cpu_types[i]) {
  /* The user specified CPU is not valid */
-    error_report("Invalid CPU type: %s", machine->cpu_type);
-    error_printf("The valid types are: %s",
- machine_class->valid_cpu_types[0]);
+    error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+    error_append_hint(errp, "The valid types are: %s",
+  machine_class->valid_cpu_types[0]);
  for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-    error_printf(", %s", machine_class->valid_cpu_types[i]);
+    error_append_hint(errp, ", %s",
+  machine_class->valid_cpu_types[i]);
  }
-    error_printf("\n");
-    exit(1);
+    error_append_hint(, "\n");


This doesn't build:

hw/core/machine.c:1488:31: error: incompatible pointer types passing 'Error ***' 
(aka 'struct Error ***') to parameter of type 'Error *const *' (aka 'struct Error 
*const *'); remove & [-Werror,-Wincompatible-pointer-types]
     error_append_hint(, "\n");
   ^



Yes,  should have been errp. The problematic code was carried from
previous revisions and has been corrected by PATCH[2/9]. It's how I missed
the building error. Thanks for fixing it up!

Thanks,
Gavin


+    return;
  }
  }


Squashing:
-- >8 --
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 021044aaaf..1898d1d1d7 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1487,3 +1487,3 @@ void machine_run_board_init(MachineState *machine, const 
char *mem_path, Error *

-    error_append_hint(, "\n");
+    error_append_hint(errp, "\n");
  return;
---

Re: [PATCH v9 0/9] Unified CPU type check

[Gavin Shan]

On 12/13/23 20:08, Philippe Mathieu-Daudé wrote:

On 12/12/23 05:55, Gavin Shan wrote:

On 12/4/23 10:47, Gavin Shan wrote:

This series bases on Phil's repository because the prepatory commits
have been queued to the branch.

   https://gitlab.com/philmd/qemu.git (branch: cpus-next)

There are two places where the user specified CPU type is checked to see
if it's supported or allowed by the board: machine_run_board_init() and
mc->init(). We don't have to maintain two duplicate sets of logic. This
series intends to move the check to machine_run_board_init() so that we
have unified CPU type check.

This series can be checked out from:

   g...@github.com:gwshan/qemu.git (branch: kvm/cpu-type)

PATCH[1-4] refactors and improves the logic to validate CPU type in
    machine_run_board_init()
PATCH[5-9] validates the CPU type in machine_run_board_init() for the
    individual boards

v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html



Ping to see if there is a chance to queue it up before the Chrismas? :)


Series queued. "Before" Christmas will depend on the final release tag.

Thanks for the various iterations,



Phil, thank you for you continuous reviews and valuable comments.

Yes, the final merge to master branch depends on the release plan.
'queue' meant to merge the series to your 'cpus-next' branch ;-)

Thanks,
Gavin

Re: [PATCH v9 0/9] Unified CPU type check

[Gavin Shan]

Hi Phil,

On 12/4/23 10:47, Gavin Shan wrote:

This series bases on Phil's repository because the prepatory commits
have been queued to the branch.

   https://gitlab.com/philmd/qemu.git (branch: cpus-next)

There are two places where the user specified CPU type is checked to see
if it's supported or allowed by the board: machine_run_board_init() and
mc->init(). We don't have to maintain two duplicate sets of logic. This
series intends to move the check to machine_run_board_init() so that we
have unified CPU type check.

This series can be checked out from:

   g...@github.com:gwshan/qemu.git (branch: kvm/cpu-type)

PATCH[1-4] refactors and improves the logic to validate CPU type in
machine_run_board_init()
PATCH[5-9] validates the CPU type in machine_run_board_init() for the
individual boards

v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html



Ping to see if there is a chance to queue it up before the Chrismas? :)

Thanks,
Gavin

Re: [PATCH v4] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

[Gavin Shan]

Hi Shaoqin,

On 12/7/23 20:36, Shaoqin Huang wrote:

The KVM_ARM_VCPU_PMU_V3_FILTER provide the ability to let the VMM decide

 ^^^
 provides

which PMU events are provided to the guest. Add a new option
`pmu-filter` as -accel sub-option to set the PMU Event Filtering.

  ^^
  '-accel kvm'


Without the filter, the KVM will expose all events from the host to
guest by default.


Without the filter, all PMU events are exposed from host to guest by
default.



The `pmu-filter` has such format:

   pmu-filter="{A,D}:start-end[;{A,D}:start-end...]"

The A means "allow" and D means "deny", start is the first event of the
range and the end is the last one. The first registered range defines
the global policy(global ALLOW if the first @action is DENY, global DENY
if the first @action is ALLOW). The start and end only support hex
format now. For example:

   pmu-filter="A:0x11-0x11;A:0x23-0x3a;D:0x30-0x30"

Since the first action is allow, we have a global deny policy. It
will allow event 0x11 (The cycle counter), events 0x23 to 0x3a is
also allowed except the event 0x30 is denied, and all the other events
are disallowed.



The section explaining how 'pmu-filter' is used has been well documented
in qemu-options.hx. So it can be dropped Instead, it can be mentioned in
the commit message, something like below.

The usage of the new sub-option can be found from the updated document
(qemu-options.hx).


Here is an real example shows how to use the PMU Event Filtering, when

  ^^^
  an example

we launch a guest by use kvm, add such command line:

   # qemu-system-aarch64 \
-accel kvm,pmu-filter="D:0x11-0x11"

Since the first action is deny, we have a global allow policy. This
disables the filtering of the cycle counter (event 0x11 being CPU_CYCLES).

And then in guest, use the perf to count the cycle:

   # perf stat sleep 1

Performance counter stats for 'sleep 1':

   1.22 msec task-clock   #0.001 CPUs 
utilized
  1  context-switches #  820.695 /sec
  0  cpu-migrations   #0.000 /sec
 55  page-faults  #   45.138 K/sec
  cycles
1128954  instructions
 227031  branches #  186.323 M/sec
   8686  branch-misses#3.83% of all 
branches

1.002492480 seconds time elapsed

0.001752000 seconds user
0.0 seconds sys

As we can see, the cycle counter has been disabled in the guest, but
other pmu events are still work.

Signed-off-by: Shaoqin Huang 
---
v3->v4:
   - Fix the wrong check for pmu_filter_init.[Sebastian]
   - Fix multiple alignment issue.   [Gavin]
   - Report error by warn_report() instead of error_report(), and don't use
   abort() since the PMU Event Filter is an add-on and best-effort feature.
 [Gavin]
   - Add several missing {  } for single line of code.   [Gavin]
   - Use the g_strsplit() to replace strtok().   [Gavin]

v2->v3:
   - Improve commits message, use kernel doc wording, add more explaination on
 filter example, fix some typo error.[Eric]
   - Add g_free() in kvm_arch_set_pmu_filter() to prevent memory leak. [Eric]
   - Add more precise error message report.  [Eric]
   - In options doc, add pmu-filter rely on KVM_ARM_VCPU_PMU_V3_FILTER support 
in
 KVM.[Eric]

v1->v2:
   - Add more description for allow and deny meaning in
 commit message. [Sebastian]
   - Small improvement.  [Sebastian]

v2: https://lore.kernel.org/all/20231117060838.39723-1-shahu...@redhat.com/
v1: https://lore.kernel.org/all/20231113081713.153615-1-shahu...@redhat.com/
---
  include/sysemu/kvm_int.h |  1 +
  qemu-options.hx  | 21 +++
  target/arm/kvm.c | 23 
  target/arm/kvm64.c   | 75 
  4 files changed, 120 insertions(+)


I would simplify the commit message like below, for your reference.

---

The KVM_ARM_VCPU_PMU_V3_FILTER provides the ability letting the VMM
to decide which PMU events are present to the guest. Add a new option
`pmu-filter` as '-accel kvm' sub-option to set the PMU Event Filtering.
The usage of the new sub-option can be found from the updated docuement
(qemu-options.hx).

Here is an example shows how to use the PMU Event Filtering.

  # qemu-system-aarch64 -accel kvm,pmu-filter="D:0x11-0x11"

Since the first action is deny, we have a global allow policy. This
disables the filtering of the cycle counter (event 0x11). In the
guest, we use the perf to count the 

Re: [PATCH-for-9.0 24/25] hw/nvram: Simplify memory_region_init_rom_device() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, arg5, arg6, errp;
@@
-   memory_region_init_rom_device(mr, owner, arg3, arg4, arg5, arg6, );
 if (
-   errp
+   !memory_region_init_rom_device(mr, owner, arg3, arg4, arg5, arg6, )
 ) {
 ...
 return;
 }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/nvram/nrf51_nvm.c | 7 ++-
  1 file changed, 2 insertions(+), 5 deletions(-)


Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 25/25] hw/pci-host/raven: Propagate error in raven_realize()

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

When an Error** reference is available, it is better to
propagate local errors, rather then using generic ones,
which might terminate the whole QEMU process.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/pci-host/raven.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 23/25] hw/misc: Simplify memory_region_init_ram_from_fd() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, arg5, arg6, arg7, errp;
@@
-   memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, 
);
 if (
-   errp
+   !memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, 
)
 ) {
 ...
 return;
 }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/misc/ivshmem.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 21/25] hw/arm: Simplify memory_region_init_rom() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, errp;
@@
-   memory_region_init_rom(mr, owner, arg3, arg4, );
 if (
-   errp
+   !memory_region_init_rom(mr, owner, arg3, arg4, )
 ) {
 ...
 return;
 }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/fsl-imx25.c | 13 -
  hw/arm/fsl-imx31.c | 13 -
  hw/arm/fsl-imx6.c  | 13 -
  3 files changed, 12 insertions(+), 27 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 22/25] hw/sparc: Simplify memory_region_init_ram_nomigrate() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, errp;
@@
-   memory_region_init_ram_nomigrate(mr, owner, arg3, arg4, );
 if (
-   errp
+   !memory_region_init_ram_nomigrate(mr, owner, arg3, arg4, )
 ) {
 ...
 return;
 }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/sparc/sun4m.c   | 20 ++--
  hw/sparc64/sun4u.c |  7 ++-
  2 files changed, 8 insertions(+), 19 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 20/25] hw: Simplify memory_region_init_ram() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, errp;
@@
-   memory_region_init_ram(mr, owner, arg3, arg4, );
 if (
-   errp
+   !memory_region_init_ram(mr, owner, arg3, arg4, )
 ) {
 ...
 return;
 }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé 
---
  hw/arm/aspeed_ast2400.c | 6 ++
  hw/arm/aspeed_ast2600.c | 6 ++
  hw/arm/fsl-imx25.c  | 6 ++
  hw/arm/fsl-imx31.c  | 6 ++
  hw/arm/fsl-imx6.c   | 6 ++
  hw/arm/integratorcp.c   | 7 ++-
  hw/arm/nrf51_soc.c  | 7 ++-
  hw/ppc/rs6000_mc.c  | 7 ++-
  8 files changed, 16 insertions(+), 35 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 19/25] misc: Simplify qemu_prealloc_mem() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Signed-off-by: Philippe Mathieu-Daudé 
---
  backends/hostmem.c | 22 +++---
  hw/virtio/virtio-mem.c |  6 ++
  2 files changed, 9 insertions(+), 19 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 18/25] util/oslib: Have qemu_prealloc_mem() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/qemu/osdep.h | 4 +++-
  util/oslib-posix.c   | 7 +--
  util/oslib-win32.c   | 4 +++-
  3 files changed, 11 insertions(+), 4 deletions(-)



s/cpu_exec_realizefn()/qemu_prealloc_mem()

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 17/25] backends: Reduce variable scope in host_memory_backend_memory_complete

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Reduce the _err variable use and remove the 'out:' label.

Signed-off-by: Philippe Mathieu-Daudé 
---
  backends/hostmem.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 16/25] backends: Have HostMemoryBackendClass::alloc() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/sysemu/hostmem.h | 10 +-
  backends/hostmem-epc.c   | 11 +--
  backends/hostmem-file.c  | 19 ++-
  backends/hostmem-memfd.c | 10 +-
  backends/hostmem-ram.c   |  9 +
  backends/hostmem.c   |  5 ++---
  6 files changed, 36 insertions(+), 28 deletions(-)



s/cpu_exec_realizefn()/HostMemoryBackendClass::alloc()

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 15/25] backends: Simplify host_memory_backend_memory_complete()

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Return early if bc->alloc is NULL. De-indent the if() ladder.

Note, this avoids a pointless call to error_propagate() with
errp=NULL at the 'out:' label.

Change trivial when reviewed with 'git-diff --ignore-all-space'.

Signed-off-by: Philippe Mathieu-Daudé 
---
  backends/hostmem.c | 133 +++--
  1 file changed, 67 insertions(+), 66 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 14/25] backends: Use g_autofree in HostMemoryBackendClass::alloc() handlers

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

In preparation of having HostMemoryBackendClass::alloc() handlers
return a boolean, have them use g_autofree.

Signed-off-by: Philippe Mathieu-Daudé 
---
  backends/hostmem-epc.c   | 3 +--
  backends/hostmem-file.c  | 3 +--
  backends/hostmem-memfd.c | 3 +--
  backends/hostmem-ram.c   | 3 +--
  4 files changed, 4 insertions(+), 8 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 13/25] memory: Have memory_region_init_ram_from_fd() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 4 +++-
  2 files changed, 6 insertions(+), 2 deletions(-)



s/cpu_exec_realizefn()/memory_region_init_ram_from_fd()

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 12/25] memory: Have memory_region_init_ram_from_file() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 4 +++-
  2 files changed, 6 insertions(+), 2 deletions(-)



s/cpu_exec_realizefn()/memory_region_init_ram_from_file()

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 11/25] memory: Have memory_region_init_resizeable_ram() return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 4 +++-
  2 files changed, 6 insertions(+), 2 deletions(-)



s/cpu_exec_realizefn()/memory_region_init_resizeable_ram()

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 10/25] memory: Have memory_region_init_rom_device() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 6 --
  2 files changed, 7 insertions(+), 3 deletions(-)



With Peter Xu's comments addressed, to 
s/cpu_exec_realizefn()/memory_region_init_rom_device()
in the commit messages.

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 09/25] memory: Simplify memory_region_init_rom_device_nomigrate() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, arg5, arg6, errp;
@@
-   memory_region_init_rom_device_nomigrate(mr, owner, arg3, arg4, arg5, arg6, 
);
 if (
-   errp
+   !memory_region_init_rom_device_nomigrate(mr, owner, arg3, arg4, arg5, arg6, 
)
 ) {
 ...
 return;
 }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé 
---
  system/memory.c | 7 ++-
  1 file changed, 2 insertions(+), 5 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 08/25] memory: Have memory_region_init_rom_device_nomigrate() return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 4 +++-
  2 files changed, 6 insertions(+), 2 deletions(-)



s/cpu_exec_realizefn()/memory_region_init_rom_device_nomigrate()

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 07/25] memory: Have memory_region_init_rom() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 6 --
  2 files changed, 7 insertions(+), 3 deletions(-)



s/cpu_exec_realizefn()/memory_region_init_rom()

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 06/25] memory: Have memory_region_init_ram() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 6 --
  2 files changed, 7 insertions(+), 3 deletions(-)



With Peter Xu's comments addressed:

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 05/25] memory: Simplify memory_region_init_ram_from_fd() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, arg5, arg6, arg7, errp;
@@
-   memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, 
);
 if (
-   errp
+   !memory_region_init_ram_from_fd(mr, owner, arg3, arg4, arg5, arg6, arg7, 
)
 ) {
 ...
 return;
 }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé 
---
  system/memory.c | 5 +
  1 file changed, 1 insertion(+), 4 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 04/25] memory: Simplify memory_region_init_rom_nomigrate() calls

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Mechanical change using the following coccinelle script:

@@
expression mr, owner, arg3, arg4, errp;
@@
-   memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, );
 if (
-   errp
+   !memory_region_init_rom_nomigrate(mr, owner, arg3, arg4, )
 ) {
 ...
 return;
 }

and removing the local Error variable.

Signed-off-by: Philippe Mathieu-Daudé 
---
  system/memory.c | 5 +
  1 file changed, 1 insertion(+), 4 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 03/25] memory: Have memory_region_init_rom_nomigrate() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 8 ++--
  2 files changed, 9 insertions(+), 3 deletions(-)



s/cpu_exec_realizefn()/memory_region_init_rom_nomigrate() in the
commit message, mentioned by Peter Xu. With this:

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 02/25] memory: Have memory_region_init_ram_nomigrate() handler return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 5 +++--
  2 files changed, 6 insertions(+), 3 deletions(-)



s/cpu_exec_realizefn()/memory_region_init_ram_nomigrate() for the commit
message, as mentioned by Peter Xu.

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 01/25] memory: Have memory_region_init_ram_flags_nomigrate() return a boolean

[Gavin Shan]

On 11/21/23 07:32, Philippe Mathieu-Daudé wrote:

Following the example documented since commit e3fe3988d7 ("error:
Document Error API usage rules"), have cpu_exec_realizefn()
return a boolean indicating whether an error is set or not.

Signed-off-by: Philippe Mathieu-Daudé 
---
  include/exec/memory.h | 4 +++-
  system/memory.c   | 4 +++-
  2 files changed, 6 insertions(+), 2 deletions(-)



With Peter Xu's comments addressed, to replace cpu_exec_realizefn()
with memory_region_init_ram_flags_nomigrate() in the commit log.

Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 2/2] target/arm/kvm: Use generic kvm_supports_guest_debug()

[Gavin Shan]

On 12/2/23 00:32, Philippe Mathieu-Daudé wrote:

Do not open-code the generic kvm_supports_guest_debug().

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm64.c | 9 ++---
  1 file changed, 2 insertions(+), 7 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 1/2] accel/kvm: Expose kvm_supports_guest_debug() prototype

[Gavin Shan]

On 12/2/23 00:32, Philippe Mathieu-Daudé wrote:

kvm_supports_guest_debug() should be accessible by KVM
implementations.

Signed-off-by: Philippe Mathieu-Daudé 
---
  accel/kvm/kvm-cpus.h | 1 -
  include/sysemu/kvm.h | 1 +
  2 files changed, 1 insertion(+), 1 deletion(-)



Reviewed-by: Gavin Shan 

[PATCH v9 6/9] hw/arm/virt: Check CPU type in machine_run_board_init()

[Gavin Shan]

Set mc->valid_cpu_types so that the user specified CPU type can be
validated in machine_run_board_init(). We needn't to do the check
by ourselves.

Signed-off-by: Gavin Shan 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
---
 hw/arm/virt.c | 62 +++
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 668c0d3194..04f9f5fa56 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -204,40 +204,6 @@ static const int a15irqmap[] = {
 [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
 };
 
-static const char *valid_cpus[] = {
-#ifdef CONFIG_TCG
-ARM_CPU_TYPE_NAME("cortex-a7"),
-ARM_CPU_TYPE_NAME("cortex-a15"),
-ARM_CPU_TYPE_NAME("cortex-a35"),
-ARM_CPU_TYPE_NAME("cortex-a55"),
-ARM_CPU_TYPE_NAME("cortex-a72"),
-ARM_CPU_TYPE_NAME("cortex-a76"),
-ARM_CPU_TYPE_NAME("cortex-a710"),
-ARM_CPU_TYPE_NAME("a64fx"),
-ARM_CPU_TYPE_NAME("neoverse-n1"),
-ARM_CPU_TYPE_NAME("neoverse-v1"),
-ARM_CPU_TYPE_NAME("neoverse-n2"),
-#endif
-ARM_CPU_TYPE_NAME("cortex-a53"),
-ARM_CPU_TYPE_NAME("cortex-a57"),
-#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
-ARM_CPU_TYPE_NAME("host"),
-#endif
-ARM_CPU_TYPE_NAME("max"),
-};
-
-static bool cpu_type_valid(const char *cpu)
-{
-int i;
-
-for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
-if (strcmp(cpu, valid_cpus[i]) == 0) {
-return true;
-}
-}
-return false;
-}
-
 static void create_randomness(MachineState *ms, const char *node)
 {
 struct {
@@ -2041,11 +2007,6 @@ static void machvirt_init(MachineState *machine)
 unsigned int smp_cpus = machine->smp.cpus;
 unsigned int max_cpus = machine->smp.max_cpus;
 
-if (!cpu_type_valid(machine->cpu_type)) {
-error_report("mach-virt: CPU type %s not supported", 
machine->cpu_type);
-exit(1);
-}
-
 possible_cpus = mc->possible_cpu_arch_ids(machine);
 
 /*
@@ -2939,6 +2900,28 @@ static void virt_machine_class_init(ObjectClass *oc, 
void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
 HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+static const char * const valid_cpu_types[] = {
+#ifdef CONFIG_TCG
+ARM_CPU_TYPE_NAME("cortex-a7"),
+ARM_CPU_TYPE_NAME("cortex-a15"),
+ARM_CPU_TYPE_NAME("cortex-a35"),
+ARM_CPU_TYPE_NAME("cortex-a55"),
+ARM_CPU_TYPE_NAME("cortex-a72"),
+ARM_CPU_TYPE_NAME("cortex-a76"),
+ARM_CPU_TYPE_NAME("cortex-a710"),
+ARM_CPU_TYPE_NAME("a64fx"),
+ARM_CPU_TYPE_NAME("neoverse-n1"),
+ARM_CPU_TYPE_NAME("neoverse-v1"),
+ARM_CPU_TYPE_NAME("neoverse-n2"),
+#endif
+ARM_CPU_TYPE_NAME("cortex-a53"),
+ARM_CPU_TYPE_NAME("cortex-a57"),
+#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
+ARM_CPU_TYPE_NAME("host"),
+#endif
+ARM_CPU_TYPE_NAME("max"),
+NULL
+};
 
 mc->init = machvirt_init;
 /* Start with max_cpus set to 512, which is the maximum supported by KVM.
@@ -2965,6 +2948,7 @@ static void virt_machine_class_init(ObjectClass *oc, void 
*data)
 #else
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("max");
 #endif
+mc->valid_cpu_types = valid_cpu_types;
 mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
 mc->kvm_type = virt_kvm_type;
 assert(!mc->get_hotplug_handler);
-- 
2.42.0

[PATCH v9 3/9] machine: Improve is_cpu_type_supported()

[Gavin Shan]

It's no sense to check the CPU type when mc->valid_cpu_types[0] is
NULL, which is a program error. Raise an assert on this.

A precise hint for the error message is given when mc->valid_cpu_types[0]
is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0]
when we have mutiple valid entries there is avoided to increase the code
readability, as suggested by Philippe Mathieu-Daudé.

Besides, @cc comes from machine->cpu_type or mc->default_cpu_type. For
the later case, it can be NULL and it's also a program error. We should
use assert() in this case.

Signed-off-by: Gavin Shan 
v9: assert(mc->valid_cpu_types[0] != NULL)   (Phil)
assert(cc != NULL);  (Phil)
---
 hw/core/machine.c | 20 ++--
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1797e002f9..4ae9aaee8e 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1400,6 +1400,7 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
  * type is provided through '-cpu' option.
  */
 if (mc->valid_cpu_types && machine->cpu_type) {
+assert(mc->valid_cpu_types[0] != NULL);
 for (i = 0; mc->valid_cpu_types[i]; i++) {
 if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
 break;
@@ -1409,20 +1410,27 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
 /* The user specified CPU type isn't valid */
 if (!mc->valid_cpu_types[i]) {
 error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
-error_append_hint(errp, "The valid types are: %s",
-  mc->valid_cpu_types[0]);
-for (i = 1; mc->valid_cpu_types[i]; i++) {
-error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+if (!mc->valid_cpu_types[1]) {
+error_append_hint(errp, "The only valid type is: %s\n",
+  mc->valid_cpu_types[0]);
+} else {
+error_append_hint(errp, "The valid types are: ");
+for (i = 0; mc->valid_cpu_types[i]; i++) {
+error_append_hint(errp, "%s%s",
+  mc->valid_cpu_types[i],
+  mc->valid_cpu_types[i + 1] ? ", " : "");
+}
+error_append_hint(errp, "\n");
 }
 
-error_append_hint(errp, "\n");
 return false;
 }
 }
 
 /* Check if CPU type is deprecated and warn if so */
 cc = CPU_CLASS(oc);
-if (cc && cc->deprecation_note) {
+assert(cc != NULL);
+if (cc->deprecation_note) {
 warn_report("CPU model %s is deprecated -- %s",
 machine->cpu_type, cc->deprecation_note);
 }
-- 
2.42.0

[PATCH v9 4/9] machine: Print CPU model name instead of CPU type

[Gavin Shan]

The names of supported CPU models instead of CPU types should be
printed when the user specified CPU type isn't supported, to be
consistent with the output from '-cpu ?'.

Correct the error messages to print CPU model names instead of CPU
type names.

Signed-off-by: Gavin Shan 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/core/machine.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 4ae9aaee8e..bc9c3e7dcd 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1409,15 +1409,19 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
 
 /* The user specified CPU type isn't valid */
 if (!mc->valid_cpu_types[i]) {
-error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+g_autofree char *requested = 
cpu_model_from_type(machine->cpu_type);
+error_setg(errp, "Invalid CPU model: %s", requested);
 if (!mc->valid_cpu_types[1]) {
-error_append_hint(errp, "The only valid type is: %s\n",
-  mc->valid_cpu_types[0]);
+g_autofree char *model = cpu_model_from_type(
+ mc->valid_cpu_types[0]);
+error_append_hint(errp, "The only valid type is: %s\n", model);
 } else {
-error_append_hint(errp, "The valid types are: ");
+error_append_hint(errp, "The valid models are: ");
 for (i = 0; mc->valid_cpu_types[i]; i++) {
+g_autofree char *model = cpu_model_from_type(
+ mc->valid_cpu_types[i]);
 error_append_hint(errp, "%s%s",
-  mc->valid_cpu_types[i],
+  model,
   mc->valid_cpu_types[i + 1] ? ", " : "");
 }
 error_append_hint(errp, "\n");
-- 
2.42.0

[PATCH v9 7/9] hw/arm/sbsa-ref: Check CPU type in machine_run_board_init()

[Gavin Shan]

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it
by ourselves.

Signed-off-by: Gavin Shan 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Leif Lindholm 
Reviewed-by: Richard Henderson 
---
 hw/arm/sbsa-ref.c | 36 ++--
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index f3c9704693..477dca0637 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -145,27 +145,6 @@ static const int sbsa_ref_irqmap[] = {
 [SBSA_GWDT_WS0] = 16,
 };
 
-static const char * const valid_cpus[] = {
-ARM_CPU_TYPE_NAME("cortex-a57"),
-ARM_CPU_TYPE_NAME("cortex-a72"),
-ARM_CPU_TYPE_NAME("neoverse-n1"),
-ARM_CPU_TYPE_NAME("neoverse-v1"),
-ARM_CPU_TYPE_NAME("neoverse-n2"),
-ARM_CPU_TYPE_NAME("max"),
-};
-
-static bool cpu_type_valid(const char *cpu)
-{
-int i;
-
-for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
-if (strcmp(cpu, valid_cpus[i]) == 0) {
-return true;
-}
-}
-return false;
-}
-
 static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
 {
 uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER;
@@ -733,11 +712,6 @@ static void sbsa_ref_init(MachineState *machine)
 const CPUArchIdList *possible_cpus;
 int n, sbsa_max_cpus;
 
-if (!cpu_type_valid(machine->cpu_type)) {
-error_report("sbsa-ref: CPU type %s not supported", machine->cpu_type);
-exit(1);
-}
-
 if (kvm_enabled()) {
 error_report("sbsa-ref: KVM is not supported for this machine");
 exit(1);
@@ -898,10 +872,20 @@ static void sbsa_ref_instance_init(Object *obj)
 static void sbsa_ref_class_init(ObjectClass *oc, void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-a57"),
+ARM_CPU_TYPE_NAME("cortex-a72"),
+ARM_CPU_TYPE_NAME("neoverse-n1"),
+ARM_CPU_TYPE_NAME("neoverse-v1"),
+ARM_CPU_TYPE_NAME("neoverse-n2"),
+ARM_CPU_TYPE_NAME("max"),
+NULL,
+};
 
 mc->init = sbsa_ref_init;
 mc->desc = "QEMU 'SBSA Reference' ARM Virtual Machine";
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("neoverse-n1");
+mc->valid_cpu_types = valid_cpu_types;
 mc->max_cpus = 512;
 mc->pci_allow_0_address = true;
 mc->minimum_page_bits = 12;
-- 
2.42.0

[PATCH v9 9/9] hw/riscv/shakti_c: Check CPU type in machine_run_board_init()

[Gavin Shan]

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it
by ourselves.

Signed-off-by: Gavin Shan 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/riscv/shakti_c.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
index 12ea74b032..3888034c2b 100644
--- a/hw/riscv/shakti_c.c
+++ b/hw/riscv/shakti_c.c
@@ -28,7 +28,6 @@
 #include "exec/address-spaces.h"
 #include "hw/riscv/boot.h"
 
-
 static const struct MemmapEntry {
 hwaddr base;
 hwaddr size;
@@ -47,12 +46,6 @@ static void shakti_c_machine_state_init(MachineState *mstate)
 ShaktiCMachineState *sms = RISCV_SHAKTI_MACHINE(mstate);
 MemoryRegion *system_memory = get_system_memory();
 
-/* Allow only Shakti C CPU for this platform */
-if (strcmp(mstate->cpu_type, TYPE_RISCV_CPU_SHAKTI_C) != 0) {
-error_report("This board can only be used with Shakti C CPU");
-exit(1);
-}
-
 /* Initialize SoC */
 object_initialize_child(OBJECT(mstate), "soc", >soc,
 TYPE_RISCV_SHAKTI_SOC);
@@ -82,9 +75,15 @@ static void shakti_c_machine_instance_init(Object *obj)
 static void shakti_c_machine_class_init(ObjectClass *klass, void *data)
 {
 MachineClass *mc = MACHINE_CLASS(klass);
+static const char * const valid_cpu_types[] = {
+RISCV_CPU_TYPE_NAME("shakti-c"),
+NULL
+};
+
 mc->desc = "RISC-V Board compatible with Shakti SDK";
 mc->init = shakti_c_machine_state_init;
 mc->default_cpu_type = TYPE_RISCV_CPU_SHAKTI_C;
+mc->valid_cpu_types = valid_cpu_types;
 mc->default_ram_id = "riscv.shakti.c.ram";
 }
 
-- 
2.42.0

[PATCH v9 2/9] machine: Introduce helper is_cpu_type_supported()

[Gavin Shan]

The logic, to check if the specified CPU type is supported in
machine_run_board_init(), is independent enough. Factor it out into
helper is_cpu_type_supported(). machine_run_board_init() looks a bit
clean with this. Since we're here, @machine_class is renamed to @mc to
avoid multiple line spanning of code. The comments are tweaked a bit
either.

No functional change intended.

Signed-off-by: Gavin Shan 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/core/machine.c | 83 +--
 1 file changed, 45 insertions(+), 38 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index bde7f4af6d..1797e002f9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1387,13 +1387,53 @@ out:
 return r;
 }
 
+static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
+{
+MachineClass *mc = MACHINE_GET_CLASS(machine);
+ObjectClass *oc = object_class_by_name(machine->cpu_type);
+CPUClass *cc;
+int i;
+
+/*
+ * Check if the user specified CPU type is supported when the valid
+ * CPU types have been determined. Note that the user specified CPU
+ * type is provided through '-cpu' option.
+ */
+if (mc->valid_cpu_types && machine->cpu_type) {
+for (i = 0; mc->valid_cpu_types[i]; i++) {
+if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
+break;
+}
+}
+
+/* The user specified CPU type isn't valid */
+if (!mc->valid_cpu_types[i]) {
+error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+error_append_hint(errp, "The valid types are: %s",
+  mc->valid_cpu_types[0]);
+for (i = 1; mc->valid_cpu_types[i]; i++) {
+error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+}
+
+error_append_hint(errp, "\n");
+return false;
+}
+}
+
+/* Check if CPU type is deprecated and warn if so */
+cc = CPU_CLASS(oc);
+if (cc && cc->deprecation_note) {
+warn_report("CPU model %s is deprecated -- %s",
+machine->cpu_type, cc->deprecation_note);
+}
+
+return true;
+}
 
 void machine_run_board_init(MachineState *machine, const char *mem_path, Error 
**errp)
 {
 ERRP_GUARD();
 MachineClass *machine_class = MACHINE_GET_CLASS(machine);
-ObjectClass *oc = object_class_by_name(machine->cpu_type);
-CPUClass *cc;
 
 /* This checkpoint is required by replay to separate prior clock
reading from the other reads, because timer polling functions query
@@ -1448,42 +1488,9 @@ void machine_run_board_init(MachineState *machine, const 
char *mem_path, Error *
 machine->ram = machine_consume_memdev(machine, machine->memdev);
 }
 
-/* If the machine supports the valid_cpu_types check and the user
- * specified a CPU with -cpu check here that the user CPU is supported.
- */
-if (machine_class->valid_cpu_types && machine->cpu_type) {
-int i;
-
-for (i = 0; machine_class->valid_cpu_types[i]; i++) {
-if (object_class_dynamic_cast(oc,
-  machine_class->valid_cpu_types[i])) {
-/* The user specified CPU is in the valid field, we are
- * good to go.
- */
-break;
-}
-}
-
-if (!machine_class->valid_cpu_types[i]) {
-/* The user specified CPU is not valid */
-error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
-error_append_hint(errp, "The valid types are: %s",
-  machine_class->valid_cpu_types[0]);
-for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-error_append_hint(errp, ", %s",
-  machine_class->valid_cpu_types[i]);
-}
-
-error_append_hint(, "\n");
-return;
-}
-}
-
-/* Check if CPU type is deprecated and warn if so */
-cc = CPU_CLASS(oc);
-if (cc && cc->deprecation_note) {
-warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
-cc->deprecation_note);
+/* Check if the CPU type is supported */
+if (!is_cpu_type_supported(machine, errp)) {
+return;
 }
 
 if (machine->cgs) {
-- 
2.42.0

[PATCH v9 8/9] hw/arm: Check CPU type in machine_run_board_init()

[Gavin Shan]

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it by
ourselves.

Signed-off-by: Gavin Shan 
Reviewed-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/arm/bananapi_m2u.c   | 12 ++--
 hw/arm/cubieboard.c | 12 ++--
 hw/arm/mps2-tz.c| 26 --
 hw/arm/mps2.c   | 26 --
 hw/arm/msf2-som.c   | 12 ++--
 hw/arm/musca.c  | 12 +---
 hw/arm/npcm7xx_boards.c | 12 +---
 hw/arm/orangepi.c   | 12 ++--
 8 files changed, 74 insertions(+), 50 deletions(-)

diff --git a/hw/arm/bananapi_m2u.c b/hw/arm/bananapi_m2u.c
index 8f24b18d8c..0a4b6f29b1 100644
--- a/hw/arm/bananapi_m2u.c
+++ b/hw/arm/bananapi_m2u.c
@@ -71,12 +71,6 @@ static void bpim2u_init(MachineState *machine)
 exit(1);
 }
 
-/* Only allow Cortex-A7 for this board */
-if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a7")) != 0) {
-error_report("This board can only be used with cortex-a7 CPU");
-exit(1);
-}
-
 r40 = AW_R40(object_new(TYPE_AW_R40));
 object_property_add_child(OBJECT(machine), "soc", OBJECT(r40));
 object_unref(OBJECT(r40));
@@ -133,12 +127,18 @@ static void bpim2u_init(MachineState *machine)
 
 static void bpim2u_machine_init(MachineClass *mc)
 {
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-a7"),
+NULL
+};
+
 mc->desc = "Bananapi M2U (Cortex-A7)";
 mc->init = bpim2u_init;
 mc->min_cpus = AW_R40_NUM_CPUS;
 mc->max_cpus = AW_R40_NUM_CPUS;
 mc->default_cpus = AW_R40_NUM_CPUS;
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
+mc->valid_cpu_types = valid_cpu_types;
 mc->default_ram_size = 1 * GiB;
 mc->default_ram_id = "bpim2u.ram";
 }
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index 29146f5018..b976727eef 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -52,12 +52,6 @@ static void cubieboard_init(MachineState *machine)
 exit(1);
 }
 
-/* Only allow Cortex-A8 for this board */
-if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a8")) != 0) {
-error_report("This board can only be used with cortex-a8 CPU");
-exit(1);
-}
-
 a10 = AW_A10(object_new(TYPE_AW_A10));
 object_property_add_child(OBJECT(machine), "soc", OBJECT(a10));
 object_unref(OBJECT(a10));
@@ -114,8 +108,14 @@ static void cubieboard_init(MachineState *machine)
 
 static void cubieboard_machine_init(MachineClass *mc)
 {
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-a8"),
+NULL
+};
+
 mc->desc = "cubietech cubieboard (Cortex-A8)";
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a8");
+mc->valid_cpu_types = valid_cpu_types;
 mc->default_ram_size = 1 * GiB;
 mc->init = cubieboard_init;
 mc->block_default_type = IF_IDE;
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 668db5ed61..5d8cdc1a4c 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -813,12 +813,6 @@ static void mps2tz_common_init(MachineState *machine)
 int num_ppcs;
 int i;
 
-if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
-error_report("This board can only be used with CPU %s",
- mc->default_cpu_type);
-exit(1);
-}
-
 if (machine->ram_size != mc->default_ram_size) {
 char *sz = size_to_str(mc->default_ram_size);
 error_report("Invalid RAM size, should be %s", sz);
@@ -1318,6 +1312,10 @@ static void mps2tz_an505_class_init(ObjectClass *oc, 
void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
 MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_CLASS(oc);
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-m33"),
+NULL
+};
 
 mc->desc = "ARM MPS2 with AN505 FPGA image for Cortex-M33";
 mc->default_cpus = 1;
@@ -1325,6 +1323,7 @@ static void mps2tz_an505_class_init(ObjectClass *oc, void 
*data)
 mc->max_cpus = mc->default_cpus;
 mmc->fpga_type = FPGA_AN505;
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m33");
+mc->valid_cpu_types = valid_cpu_types;
 mmc->scc_id = 0x41045050;
 mmc->sysclk_frq = 20 * 1000 * 1000; /* 20MHz */
 mmc->apb_periph_frq = mmc->sysclk_frq;
@@ -1347,6 +1346,10 @@ static void mps2tz_an521_class_init(ObjectClass *oc, 
void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
 MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_CLASS(oc);
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-m33&q

[PATCH v9 5/9] hw/arm/virt: Hide host CPU model for tcg

[Gavin Shan]

The 'host' CPU model isn't available until KVM or HVF is enabled.
For example, the following error messages are seen when the guest
is started with option '-cpu cortex-a8' on tcg after the next commit
is applied to check the CPU type in machine_run_board_init().

  ERROR:../hw/core/machine.c:1423:is_cpu_type_supported: \
  assertion failed: (model != NULL)
  Bail out! ERROR:../hw/core/machine.c:1423:is_cpu_type_supported: \
  assertion failed: (model != NULL)
  Aborted (core dumped)

Hide 'host' CPU model until KVM or HVF is enabled. With this applied,
the valid CPU models can be shown.

  qemu-system-aarch64: Invalid CPU type: cortex-a8
  The valid types are: cortex-a7, cortex-a15, cortex-a35, \
  cortex-a55, cortex-a72, cortex-a76, cortex-a710, a64fx, \
  neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53,  \
  cortex-a57, max

Signed-off-by: Gavin Shan 
Reviewed-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index be2856c018..668c0d3194 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -220,7 +220,9 @@ static const char *valid_cpus[] = {
 #endif
 ARM_CPU_TYPE_NAME("cortex-a53"),
 ARM_CPU_TYPE_NAME("cortex-a57"),
+#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
 ARM_CPU_TYPE_NAME("host"),
+#endif
 ARM_CPU_TYPE_NAME("max"),
 };
 
-- 
2.42.0

[PATCH v9 0/9] Unified CPU type check

[Gavin Shan]

This series bases on Phil's repository because the prepatory commits
have been queued to the branch.

  https://gitlab.com/philmd/qemu.git (branch: cpus-next)

There are two places where the user specified CPU type is checked to see
if it's supported or allowed by the board: machine_run_board_init() and
mc->init(). We don't have to maintain two duplicate sets of logic. This
series intends to move the check to machine_run_board_init() so that we
have unified CPU type check.

This series can be checked out from:

  g...@github.com:gwshan/qemu.git (branch: kvm/cpu-type)

PATCH[1-4] refactors and improves the logic to validate CPU type in
   machine_run_board_init()
PATCH[5-9] validates the CPU type in machine_run_board_init() for the
   individual boards

v1: https://lists.nongnu.org/archive/html/qemu-arm/2023-07/msg00302.html
v2: https://lists.nongnu.org/archive/html/qemu-arm/2023-07/msg00528.html
v3: https://lists.nongnu.org/archive/html/qemu-arm/2023-09/msg00157.html
v4: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg5.html
v5: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00611.html
v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html
v8: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01168.html

Testing
===

Before the series is applied:

  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-a53
  qemu-system-aarch64: sbsa-ref: CPU type cortex-a53-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
  qemu-system-aarch64: sbsa-ref: CPU type sa1100-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu host
  qemu-system-aarch64: unable to find CPU model 'host'

After the series is applied:

  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: Invalid CPU model: cortex-a8
  The valid models are: cortex-a7, cortex-a15, cortex-a35, cortex-a55, \
cortex-a72, cortex-a76, cortex-a710, a64fx,\
neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53, \
cortex-a57, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-a53
  qemu-system-aarch64: Invalid CPU model: cortex-a53
  The valid models are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1,  \
neoverse-n2, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
  qemu-system-aarch64: Invalid CPU model: sa1100
  The valid models are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1,  \
neoverse-n2, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu host
  qemu-system-aarch64: unable to find CPU model 'host'

Changelog
=
v9:
  * Pick r-bs from Markus and Phil   (Gavin)
  * Improved change log for PATCH[v9 1/9](Markus)
  * assert(mc->valid_cpu_types[0] != NULL) and assert(cc != NULL)
in is_cpu_type_supported() of PATCH[v9 3/9]  (Phil)
v8:
  * Pick r-bs from Phil  (Gavin)
  * Drop @local_err and use @errp in machine_run_board_init()(Phil)
  * Add PATCH[v8 3/9] to improve is_cpu_type_supported() (Phil)
  * Use g_autofree and replace 'type' with 'model' for the
error messages in is_cpu_type_supported()(Phil)
v7:
  * Add 'return' after error_propagate() in machine_run_board_init()
to avoid calling mc->init() in the failing case  (Marcin)
v6:
  * Drop PATCH[v5 01-23], queued by Phil (Phil)
  * Clearer hint if only one CPU type is supported and have
'const MachineState *' in is_cpu_type_supported()(Phil)
  * Move valid_cpu_types[] to board's class_init() function  (Phil)

Gavin Shan (9):
  machine: Use error handling when CPU type is checked
  machine: Introduce helper is_cpu_type_supported()
  machine: Improve is_cpu_type_supported()
  machine: Print CPU model name instead of CPU type
  hw/arm/virt: Hide host CPU model for tcg
  hw/arm/virt: Check CPU type in machine_run_board_init()
  hw/arm/sbsa-ref: Check CPU type in machine_run_board_init()
  hw/arm: Check CPU type in machine_run_board_init()
  hw/riscv/shakti_c: Check CPU type in machine_run_board_init()

 hw/arm/bananapi_m2u.c   | 12 +++---
 hw/arm/cubieboard.c | 12 +++---
 hw/arm/mps2-tz.c| 26 +---
 hw/arm/mps2.c   | 26 +---
 hw/arm/msf2-som.c   | 12 +++---
 hw/arm/musca.c  | 12 +++---
 hw/arm/npcm7xx_boards.c | 12 +++---
 hw/arm/orangepi.c   | 12 +++---
 hw/arm/sbsa-ref.c   | 36 +---
 hw/arm/virt.c  

[PATCH v9 1/9] machine: Use error handling when CPU type is checked

[Gavin Shan]

Functions that use an Error **errp parameter to return errors should
not also report them to the user, because reporting is the caller's
job. The principle is violated by machine_run_board_init() because
it calls error_report(), error_printf(), and exit(1) when the machine
doesn't support the requested CPU type.

Clean this up by using error_setg() and error_append_hint() instead.
No functional change, as the only caller passes _fatal.

Suggested-by: Igor Mammedov 
Signed-off-by: Gavin Shan 
Reviewed-by: Markus Armbruster 
---
v9: Improved change log  (Markus)
---
 hw/core/machine.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0c17398141..bde7f4af6d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1466,15 +1466,16 @@ void machine_run_board_init(MachineState *machine, 
const char *mem_path, Error *
 
 if (!machine_class->valid_cpu_types[i]) {
 /* The user specified CPU is not valid */
-error_report("Invalid CPU type: %s", machine->cpu_type);
-error_printf("The valid types are: %s",
- machine_class->valid_cpu_types[0]);
+error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+error_append_hint(errp, "The valid types are: %s",
+  machine_class->valid_cpu_types[0]);
 for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-error_printf(", %s", machine_class->valid_cpu_types[i]);
+error_append_hint(errp, ", %s",
+  machine_class->valid_cpu_types[i]);
 }
-error_printf("\n");
 
-exit(1);
+error_append_hint(, "\n");
+return;
 }
 }
 
-- 
2.42.0

Re: [PATCH v8 3/9] machine: Improve is_cpu_type_supported()

[Gavin Shan]

Hi Phil,

On 12/4/23 09:20, Gavin Shan wrote:

On 12/1/23 20:57, Philippe Mathieu-Daudé wrote:

On 29/11/23 05:20, Gavin Shan wrote:

It's no sense to check the CPU type when mc->valid_cpu_types[0] is
NULL.


This case is a programming error, right? We should simply:

  assert(!mc->valid_cpu_types || *mc->valid_cpu_types);



Yes, assert() should be used instead. I will do this like below:

     if (mc->valid_cpu_types && machine->cpu_type) {
     assert(mc->valid_cpu_types[1] != NULL);



I meant assert(mc->valid_cpu_types[0] != NULL) and sorry for the confusion.

  if (mc->valid_cpu_types && machine->cpu_type) {
  assert(mc->valid_cpu_types[0] != NULL);
  :
  }




So the check is skipped for this particular case. The constraint
has been taken when the error messags are appended.

A precise hint for the error message is given when mc->valid_cpu_types[0]
is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0]
when we have mutiple valid entries there is avoided to increase the code
readability, as suggested by Philippe Mathieu-Daudé.

Signed-off-by: Gavin Shan 
---
  hw/core/machine.c | 18 --
  1 file changed, 12 insertions(+), 6 deletions(-)




Thanks,
Gavin

Re: [PATCH v8 3/9] machine: Improve is_cpu_type_supported()

[Gavin Shan]

Hi Phil

On 12/1/23 20:57, Philippe Mathieu-Daudé wrote:

On 29/11/23 05:20, Gavin Shan wrote:

It's no sense to check the CPU type when mc->valid_cpu_types[0] is
NULL.


This case is a programming error, right? We should simply:

  assert(!mc->valid_cpu_types || *mc->valid_cpu_types);



Yes, assert() should be used instead. I will do this like below:

if (mc->valid_cpu_types && machine->cpu_type) {
assert(mc->valid_cpu_types[1] != NULL);



So the check is skipped for this particular case. The constraint
has been taken when the error messags are appended.

A precise hint for the error message is given when mc->valid_cpu_types[0]
is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0]
when we have mutiple valid entries there is avoided to increase the code
readability, as suggested by Philippe Mathieu-Daudé.

Signed-off-by: Gavin Shan 
---
  hw/core/machine.c | 18 --
  1 file changed, 12 insertions(+), 6 deletions(-)




Thanks,
Gavin

Re: [PATCH v8 2/9] machine: Introduce helper is_cpu_type_supported()

[Gavin Shan]

Hi Phil,

On 12/1/23 20:53, Philippe Mathieu-Daudé wrote:

On 29/11/23 05:20, Gavin Shan wrote:

The logic, to check if the specified CPU type is supported in
machine_run_board_init(), is independent enough. Factor it out into
helper is_cpu_type_supported(). machine_run_board_init() looks a bit
clean with this. Since we're here, @machine_class is renamed to @mc to
avoid multiple line spanning of code. The comments are tweaked a bit
either.

No functional change intended.

Signed-off-by: Gavin Shan 
---
v8: Move the precise message hint to PATCH[v8 3/9]    (Gavin)
---
  hw/core/machine.c | 83 +--
  1 file changed, 45 insertions(+), 38 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index bde7f4af6d..1797e002f9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1387,13 +1387,53 @@ out:
  return r;
  }
+static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
+{
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    ObjectClass *oc = object_class_by_name(machine->cpu_type);
+    CPUClass *cc;
+    int i;
+
+    /*
+ * Check if the user specified CPU type is supported when the valid
+ * CPU types have been determined. Note that the user specified CPU
+ * type is provided through '-cpu' option.
+ */
+    if (mc->valid_cpu_types && machine->cpu_type) {
+    for (i = 0; mc->valid_cpu_types[i]; i++) {
+    if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
+    break;
+    }
+    }
+
+    /* The user specified CPU type isn't valid */
+    if (!mc->valid_cpu_types[i]) {
+    error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+    error_append_hint(errp, "The valid types are: %s",
+  mc->valid_cpu_types[0]);
+    for (i = 1; mc->valid_cpu_types[i]; i++) {
+    error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+    }
+
+    error_append_hint(errp, "\n");
+    return false;
+    }
+    }
+
+    /* Check if CPU type is deprecated and warn if so */
+    cc = CPU_CLASS(oc);
+    if (cc && cc->deprecation_note) {


cc can't be NULL, right? Otherwise,

Reviewed-by: Philippe Mathieu-Daudé 



machine->cpu_type is either mc->default_cpu_type or returned from 
parse_cpu_option().
It can be NULL if mc->default_cpu_type is invalid, which is a program error. So
assert(cc != NULL) should be used instead. I will fold the change to PATCH[v9 
3/9]


+    warn_report("CPU model %s is deprecated -- %s",
+    machine->cpu_type, cc->deprecation_note);
+    }
+
+    return true;
+}




Thanks,
Gavin

Re: [PATCH v3] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER

[Gavin Shan]

Hi Shaoqin,

On 11/29/23 14:08, Shaoqin Huang wrote:

The KVM_ARM_VCPU_PMU_V3_FILTER provide the ability to let the VMM decide
which PMU events are provided to the guest. Add a new option
`pmu-filter` as -accel sub-option to set the PMU Event Filtering.
Without the filter, the KVM will expose all events from the host to
guest by default.

The `pmu-filter` has such format:

   pmu-filter="{A,D}:start-end[;{A,D}:start-end...]"

The A means "allow" and D means "deny", start is the first event of the
range and the end is the last one. The first registered range defines
the global policy(global ALLOW if the first @action is DENY, global DENY
if the first @action is ALLOW). The start and end only support hex
format now. For example:

   pmu-filter="A:0x11-0x11;A:0x23-0x3a;D:0x30-0x30"

Since the first action is allow, we have a global deny policy. It
will allow event 0x11 (The cycle counter), events 0x23 to 0x3a is
also allowed except the event 0x30 is denied, and all the other events
are disallowed.

Here is an real example shows how to use the PMU Event Filtering, when
we launch a guest by use kvm, add such command line:

   # qemu-system-aarch64 \
-accel kvm,pmu-filter="D:0x11-0x11"

Since the first action is deny, we have a global allow policy. This
disables the filtering of the cycle counter (event 0x11 being CPU_CYCLES).

And then in guest, use the perf to count the cycle:

   # perf stat sleep 1

Performance counter stats for 'sleep 1':

   1.22 msec task-clock   #0.001 CPUs 
utilized
  1  context-switches #  820.695 /sec
  0  cpu-migrations   #0.000 /sec
 55  page-faults  #   45.138 K/sec
  cycles
1128954  instructions
 227031  branches #  186.323 M/sec
   8686  branch-misses#3.83% of all 
branches

1.002492480 seconds time elapsed

0.001752000 seconds user
0.0 seconds sys

As we can see, the cycle counter has been disabled in the guest, but
other pmu events are still work.

Signed-off-by: Shaoqin Huang 
---
v2->v3:
   - Improve commits message, use kernel doc wording, add more explaination on
 filter example, fix some typo error.[Eric]
   - Add g_free() in kvm_arch_set_pmu_filter() to prevent memory leak. [Eric]
   - Add more precise error message report.  [Eric]
   - In options doc, add pmu-filter rely on KVM_ARM_VCPU_PMU_V3_FILTER support 
in
 KVM.[Eric]

v1->v2:
   - Add more description for allow and deny meaning in
 commit message. [Sebastian]
   - Small improvement.  [Sebastian]

v2: https://lore.kernel.org/all/20231117060838.39723-1-shahu...@redhat.com/
v1: https://lore.kernel.org/all/20231113081713.153615-1-shahu...@redhat.com/
---
  include/sysemu/kvm_int.h |  1 +
  qemu-options.hx  | 21 +
  target/arm/kvm.c | 23 ++
  target/arm/kvm64.c   | 68 
  4 files changed, 113 insertions(+)

diff --git a/include/sysemu/kvm_int.h b/include/sysemu/kvm_int.h
index fd846394be..8f4601474f 100644
--- a/include/sysemu/kvm_int.h
+++ b/include/sysemu/kvm_int.h
@@ -120,6 +120,7 @@ struct KVMState
  uint32_t xen_caps;
  uint16_t xen_gnttab_max_frames;
  uint16_t xen_evtchn_max_pirq;
+char *kvm_pmu_filter;
  };
  
  void kvm_memory_listener_register(KVMState *s, KVMMemoryListener *kml,

diff --git a/qemu-options.hx b/qemu-options.hx
index 42fd09e4de..8b721d6668 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -187,6 +187,7 @@ DEF("accel", HAS_ARG, QEMU_OPTION_accel,
  "tb-size=n (TCG translation block cache size)\n"
  "dirty-ring-size=n (KVM dirty ring GFN count, default 
0)\n"
  "eager-split-size=n (KVM Eager Page Split chunk size, default 
0, disabled. ARM only)\n"
+"pmu-filter={A,D}:start-end[;...] (KVM PMU Event Filter, 
default no filter. ARM only)\n"

  ^^^

Potential alignment issue, or the email isn't shown for me correctly.
Besides, why not follow the pattern in the commit log, which is nicer
than what's of being:

pmu-filter={A,D}:start-end[;...]

to

pmu-filter="{A,D}:start-end[;{A,D}:start-end...]


  "notify-vmexit=run|internal-error|disable,notify-window=n 
(enable notify VM exit and set notify window, x86 only)\n"
  "thread=single|multi (enable multi-threaded TCG)\n", 
QEMU_ARCH_ALL)
  SRST
@@ -259,6 +260,26 @@ SRST
  impact on the memory. By default, this feature is disabled
  (eager-split-size=0).
  
+``pmu-filter={A,D}:start-end[;...]``

+KVM implements pmu event filtering to prevent a guest from 

Re: [PATCH v8 1/9] machine: Use error handling when CPU type is checked

[Gavin Shan]

Hi Markus,

On 11/29/23 19:20, Markus Armbruster wrote:

Gavin Shan  writes:


QEMU will be terminated if the specified CPU type isn't supported
in machine_run_board_init(). The list of supported CPU type names
is tracked by mc->valid_cpu_types.


Suggest to drop the second sentence.



Indeed, it's not so helpful.


The error handling can be used to propagate error messages, to be
consistent how the errors are handled for other situations in the
same function.

No functional change intended.

Suggested-by: Igor Mammedov 
Signed-off-by: Gavin Shan 
---
v8: Drop @local_err and use @errp to be compatible with
 ERRP_GUARD()  (Phil)
---
  hw/core/machine.c | 13 +++--
  1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0c17398141..bde7f4af6d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1466,15 +1466,16 @@ void machine_run_board_init(MachineState *machine, 
const char *mem_path, Error *
  
  if (!machine_class->valid_cpu_types[i]) {

  /* The user specified CPU is not valid */
-error_report("Invalid CPU type: %s", machine->cpu_type);
-error_printf("The valid types are: %s",
- machine_class->valid_cpu_types[0]);
+error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+error_append_hint(errp, "The valid types are: %s",
+  machine_class->valid_cpu_types[0]);
  for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-error_printf(", %s", machine_class->valid_cpu_types[i]);
+error_append_hint(errp, ", %s",
+  machine_class->valid_cpu_types[i]);
  }
-error_printf("\n");
  
-exit(1);

+error_append_hint(, "\n");
+return;
  }
  }


This cleans up an anti-pattern: use of error_report() within a function that
returns errors through an Error **errp parameter.

Cleanup, not bug fix, because the only caller passes _abort.

Suggest to start the commit message with a mention of the anti-pattern.
Here's how I'd write it:

 Functions that use an Error **errp parameter to return errors should
 not also report them to the user, because reporting is the caller's
 job.

 machine_run_board_init() violates this principle: it calls
 error_report(), error_printf(), and exit(1) when the machine doesn't
 support the requested CPU type.

 Clean this up by using error_setg() and error_append_hint() instead.
 No functional change, as the only caller passes _fatal.



Thanks for the nice write-up. I will take it if v9 is needed to address
comments from other people.


Whether you use my suggestion or not:
Reviewed-by: Markus Armbruster 



Thanks for your review.

Thanks,
Gavin

[PATCH v8 7/9] hw/arm/sbsa-ref: Check CPU type in machine_run_board_init()

[Gavin Shan]

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it
by ourselves.

Signed-off-by: Gavin Shan 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Leif Lindholm 
Reviewed-by: Richard Henderson 
---
 hw/arm/sbsa-ref.c | 36 ++--
 1 file changed, 10 insertions(+), 26 deletions(-)

diff --git a/hw/arm/sbsa-ref.c b/hw/arm/sbsa-ref.c
index f3c9704693..477dca0637 100644
--- a/hw/arm/sbsa-ref.c
+++ b/hw/arm/sbsa-ref.c
@@ -145,27 +145,6 @@ static const int sbsa_ref_irqmap[] = {
 [SBSA_GWDT_WS0] = 16,
 };
 
-static const char * const valid_cpus[] = {
-ARM_CPU_TYPE_NAME("cortex-a57"),
-ARM_CPU_TYPE_NAME("cortex-a72"),
-ARM_CPU_TYPE_NAME("neoverse-n1"),
-ARM_CPU_TYPE_NAME("neoverse-v1"),
-ARM_CPU_TYPE_NAME("neoverse-n2"),
-ARM_CPU_TYPE_NAME("max"),
-};
-
-static bool cpu_type_valid(const char *cpu)
-{
-int i;
-
-for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
-if (strcmp(cpu, valid_cpus[i]) == 0) {
-return true;
-}
-}
-return false;
-}
-
 static uint64_t sbsa_ref_cpu_mp_affinity(SBSAMachineState *sms, int idx)
 {
 uint8_t clustersz = ARM_DEFAULT_CPUS_PER_CLUSTER;
@@ -733,11 +712,6 @@ static void sbsa_ref_init(MachineState *machine)
 const CPUArchIdList *possible_cpus;
 int n, sbsa_max_cpus;
 
-if (!cpu_type_valid(machine->cpu_type)) {
-error_report("sbsa-ref: CPU type %s not supported", machine->cpu_type);
-exit(1);
-}
-
 if (kvm_enabled()) {
 error_report("sbsa-ref: KVM is not supported for this machine");
 exit(1);
@@ -898,10 +872,20 @@ static void sbsa_ref_instance_init(Object *obj)
 static void sbsa_ref_class_init(ObjectClass *oc, void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-a57"),
+ARM_CPU_TYPE_NAME("cortex-a72"),
+ARM_CPU_TYPE_NAME("neoverse-n1"),
+ARM_CPU_TYPE_NAME("neoverse-v1"),
+ARM_CPU_TYPE_NAME("neoverse-n2"),
+ARM_CPU_TYPE_NAME("max"),
+NULL,
+};
 
 mc->init = sbsa_ref_init;
 mc->desc = "QEMU 'SBSA Reference' ARM Virtual Machine";
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("neoverse-n1");
+mc->valid_cpu_types = valid_cpu_types;
 mc->max_cpus = 512;
 mc->pci_allow_0_address = true;
 mc->minimum_page_bits = 12;
-- 
2.42.0

[PATCH v8 9/9] hw/riscv/shakti_c: Check CPU type in machine_run_board_init()

[Gavin Shan]

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it
by ourselves.

Signed-off-by: Gavin Shan 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/riscv/shakti_c.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/hw/riscv/shakti_c.c b/hw/riscv/shakti_c.c
index 12ea74b032..3888034c2b 100644
--- a/hw/riscv/shakti_c.c
+++ b/hw/riscv/shakti_c.c
@@ -28,7 +28,6 @@
 #include "exec/address-spaces.h"
 #include "hw/riscv/boot.h"
 
-
 static const struct MemmapEntry {
 hwaddr base;
 hwaddr size;
@@ -47,12 +46,6 @@ static void shakti_c_machine_state_init(MachineState *mstate)
 ShaktiCMachineState *sms = RISCV_SHAKTI_MACHINE(mstate);
 MemoryRegion *system_memory = get_system_memory();
 
-/* Allow only Shakti C CPU for this platform */
-if (strcmp(mstate->cpu_type, TYPE_RISCV_CPU_SHAKTI_C) != 0) {
-error_report("This board can only be used with Shakti C CPU");
-exit(1);
-}
-
 /* Initialize SoC */
 object_initialize_child(OBJECT(mstate), "soc", >soc,
 TYPE_RISCV_SHAKTI_SOC);
@@ -82,9 +75,15 @@ static void shakti_c_machine_instance_init(Object *obj)
 static void shakti_c_machine_class_init(ObjectClass *klass, void *data)
 {
 MachineClass *mc = MACHINE_CLASS(klass);
+static const char * const valid_cpu_types[] = {
+RISCV_CPU_TYPE_NAME("shakti-c"),
+NULL
+};
+
 mc->desc = "RISC-V Board compatible with Shakti SDK";
 mc->init = shakti_c_machine_state_init;
 mc->default_cpu_type = TYPE_RISCV_CPU_SHAKTI_C;
+mc->valid_cpu_types = valid_cpu_types;
 mc->default_ram_id = "riscv.shakti.c.ram";
 }
 
-- 
2.42.0

[PATCH v8 5/9] hw/arm/virt: Hide host CPU model for tcg

[Gavin Shan]

The 'host' CPU model isn't available until KVM or HVF is enabled.
For example, the following error messages are seen when the guest
is started with option '-cpu cortex-a8' on tcg after the next commit
is applied to check the CPU type in machine_run_board_init().

  ERROR:../hw/core/machine.c:1423:is_cpu_type_supported: \
  assertion failed: (model != NULL)
  Bail out! ERROR:../hw/core/machine.c:1423:is_cpu_type_supported: \
  assertion failed: (model != NULL)
  Aborted (core dumped)

Hide 'host' CPU model until KVM or HVF is enabled. With this applied,
the valid CPU models can be shown.

  qemu-system-aarch64: Invalid CPU type: cortex-a8
  The valid types are: cortex-a7, cortex-a15, cortex-a35, \
  cortex-a55, cortex-a72, cortex-a76, cortex-a710, a64fx, \
  neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53,  \
  cortex-a57, max

Signed-off-by: Gavin Shan 
Reviewed-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/arm/virt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index be2856c018..668c0d3194 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -220,7 +220,9 @@ static const char *valid_cpus[] = {
 #endif
 ARM_CPU_TYPE_NAME("cortex-a53"),
 ARM_CPU_TYPE_NAME("cortex-a57"),
+#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
 ARM_CPU_TYPE_NAME("host"),
+#endif
 ARM_CPU_TYPE_NAME("max"),
 };
 
-- 
2.42.0

[PATCH v8 2/9] machine: Introduce helper is_cpu_type_supported()

[Gavin Shan]

The logic, to check if the specified CPU type is supported in
machine_run_board_init(), is independent enough. Factor it out into
helper is_cpu_type_supported(). machine_run_board_init() looks a bit
clean with this. Since we're here, @machine_class is renamed to @mc to
avoid multiple line spanning of code. The comments are tweaked a bit
either.

No functional change intended.

Signed-off-by: Gavin Shan 
---
v8: Move the precise message hint to PATCH[v8 3/9](Gavin)
---
 hw/core/machine.c | 83 +--
 1 file changed, 45 insertions(+), 38 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index bde7f4af6d..1797e002f9 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1387,13 +1387,53 @@ out:
 return r;
 }
 
+static bool is_cpu_type_supported(const MachineState *machine, Error **errp)
+{
+MachineClass *mc = MACHINE_GET_CLASS(machine);
+ObjectClass *oc = object_class_by_name(machine->cpu_type);
+CPUClass *cc;
+int i;
+
+/*
+ * Check if the user specified CPU type is supported when the valid
+ * CPU types have been determined. Note that the user specified CPU
+ * type is provided through '-cpu' option.
+ */
+if (mc->valid_cpu_types && machine->cpu_type) {
+for (i = 0; mc->valid_cpu_types[i]; i++) {
+if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
+break;
+}
+}
+
+/* The user specified CPU type isn't valid */
+if (!mc->valid_cpu_types[i]) {
+error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+error_append_hint(errp, "The valid types are: %s",
+  mc->valid_cpu_types[0]);
+for (i = 1; mc->valid_cpu_types[i]; i++) {
+error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+}
+
+error_append_hint(errp, "\n");
+return false;
+}
+}
+
+/* Check if CPU type is deprecated and warn if so */
+cc = CPU_CLASS(oc);
+if (cc && cc->deprecation_note) {
+warn_report("CPU model %s is deprecated -- %s",
+machine->cpu_type, cc->deprecation_note);
+}
+
+return true;
+}
 
 void machine_run_board_init(MachineState *machine, const char *mem_path, Error 
**errp)
 {
 ERRP_GUARD();
 MachineClass *machine_class = MACHINE_GET_CLASS(machine);
-ObjectClass *oc = object_class_by_name(machine->cpu_type);
-CPUClass *cc;
 
 /* This checkpoint is required by replay to separate prior clock
reading from the other reads, because timer polling functions query
@@ -1448,42 +1488,9 @@ void machine_run_board_init(MachineState *machine, const 
char *mem_path, Error *
 machine->ram = machine_consume_memdev(machine, machine->memdev);
 }
 
-/* If the machine supports the valid_cpu_types check and the user
- * specified a CPU with -cpu check here that the user CPU is supported.
- */
-if (machine_class->valid_cpu_types && machine->cpu_type) {
-int i;
-
-for (i = 0; machine_class->valid_cpu_types[i]; i++) {
-if (object_class_dynamic_cast(oc,
-  machine_class->valid_cpu_types[i])) {
-/* The user specified CPU is in the valid field, we are
- * good to go.
- */
-break;
-}
-}
-
-if (!machine_class->valid_cpu_types[i]) {
-/* The user specified CPU is not valid */
-error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
-error_append_hint(errp, "The valid types are: %s",
-  machine_class->valid_cpu_types[0]);
-for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-error_append_hint(errp, ", %s",
-  machine_class->valid_cpu_types[i]);
-}
-
-error_append_hint(, "\n");
-return;
-}
-}
-
-/* Check if CPU type is deprecated and warn if so */
-cc = CPU_CLASS(oc);
-if (cc && cc->deprecation_note) {
-warn_report("CPU model %s is deprecated -- %s", machine->cpu_type,
-cc->deprecation_note);
+/* Check if the CPU type is supported */
+if (!is_cpu_type_supported(machine, errp)) {
+return;
 }
 
 if (machine->cgs) {
-- 
2.42.0

[PATCH v8 8/9] hw/arm: Check CPU type in machine_run_board_init()

[Gavin Shan]

Set mc->valid_cpu_types so that the user specified CPU type can
be validated in machine_run_board_init(). We needn't to do it by
ourselves.

Signed-off-by: Gavin Shan 
Reviewed-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
---
 hw/arm/bananapi_m2u.c   | 12 ++--
 hw/arm/cubieboard.c | 12 ++--
 hw/arm/mps2-tz.c| 26 --
 hw/arm/mps2.c   | 26 --
 hw/arm/msf2-som.c   | 12 ++--
 hw/arm/musca.c  | 12 +---
 hw/arm/npcm7xx_boards.c | 12 +---
 hw/arm/orangepi.c   | 12 ++--
 8 files changed, 74 insertions(+), 50 deletions(-)

diff --git a/hw/arm/bananapi_m2u.c b/hw/arm/bananapi_m2u.c
index 8f24b18d8c..0a4b6f29b1 100644
--- a/hw/arm/bananapi_m2u.c
+++ b/hw/arm/bananapi_m2u.c
@@ -71,12 +71,6 @@ static void bpim2u_init(MachineState *machine)
 exit(1);
 }
 
-/* Only allow Cortex-A7 for this board */
-if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a7")) != 0) {
-error_report("This board can only be used with cortex-a7 CPU");
-exit(1);
-}
-
 r40 = AW_R40(object_new(TYPE_AW_R40));
 object_property_add_child(OBJECT(machine), "soc", OBJECT(r40));
 object_unref(OBJECT(r40));
@@ -133,12 +127,18 @@ static void bpim2u_init(MachineState *machine)
 
 static void bpim2u_machine_init(MachineClass *mc)
 {
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-a7"),
+NULL
+};
+
 mc->desc = "Bananapi M2U (Cortex-A7)";
 mc->init = bpim2u_init;
 mc->min_cpus = AW_R40_NUM_CPUS;
 mc->max_cpus = AW_R40_NUM_CPUS;
 mc->default_cpus = AW_R40_NUM_CPUS;
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a7");
+mc->valid_cpu_types = valid_cpu_types;
 mc->default_ram_size = 1 * GiB;
 mc->default_ram_id = "bpim2u.ram";
 }
diff --git a/hw/arm/cubieboard.c b/hw/arm/cubieboard.c
index 29146f5018..b976727eef 100644
--- a/hw/arm/cubieboard.c
+++ b/hw/arm/cubieboard.c
@@ -52,12 +52,6 @@ static void cubieboard_init(MachineState *machine)
 exit(1);
 }
 
-/* Only allow Cortex-A8 for this board */
-if (strcmp(machine->cpu_type, ARM_CPU_TYPE_NAME("cortex-a8")) != 0) {
-error_report("This board can only be used with cortex-a8 CPU");
-exit(1);
-}
-
 a10 = AW_A10(object_new(TYPE_AW_A10));
 object_property_add_child(OBJECT(machine), "soc", OBJECT(a10));
 object_unref(OBJECT(a10));
@@ -114,8 +108,14 @@ static void cubieboard_init(MachineState *machine)
 
 static void cubieboard_machine_init(MachineClass *mc)
 {
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-a8"),
+NULL
+};
+
 mc->desc = "cubietech cubieboard (Cortex-A8)";
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a8");
+mc->valid_cpu_types = valid_cpu_types;
 mc->default_ram_size = 1 * GiB;
 mc->init = cubieboard_init;
 mc->block_default_type = IF_IDE;
diff --git a/hw/arm/mps2-tz.c b/hw/arm/mps2-tz.c
index 668db5ed61..5d8cdc1a4c 100644
--- a/hw/arm/mps2-tz.c
+++ b/hw/arm/mps2-tz.c
@@ -813,12 +813,6 @@ static void mps2tz_common_init(MachineState *machine)
 int num_ppcs;
 int i;
 
-if (strcmp(machine->cpu_type, mc->default_cpu_type) != 0) {
-error_report("This board can only be used with CPU %s",
- mc->default_cpu_type);
-exit(1);
-}
-
 if (machine->ram_size != mc->default_ram_size) {
 char *sz = size_to_str(mc->default_ram_size);
 error_report("Invalid RAM size, should be %s", sz);
@@ -1318,6 +1312,10 @@ static void mps2tz_an505_class_init(ObjectClass *oc, 
void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
 MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_CLASS(oc);
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-m33"),
+NULL
+};
 
 mc->desc = "ARM MPS2 with AN505 FPGA image for Cortex-M33";
 mc->default_cpus = 1;
@@ -1325,6 +1323,7 @@ static void mps2tz_an505_class_init(ObjectClass *oc, void 
*data)
 mc->max_cpus = mc->default_cpus;
 mmc->fpga_type = FPGA_AN505;
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-m33");
+mc->valid_cpu_types = valid_cpu_types;
 mmc->scc_id = 0x41045050;
 mmc->sysclk_frq = 20 * 1000 * 1000; /* 20MHz */
 mmc->apb_periph_frq = mmc->sysclk_frq;
@@ -1347,6 +1346,10 @@ static void mps2tz_an521_class_init(ObjectClass *oc, 
void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
 MPS2TZMachineClass *mmc = MPS2TZ_MACHINE_CLASS(oc);
+static const char * const valid_cpu_types[] = {
+ARM_CPU_TYPE_NAME("cortex-m33&q

[PATCH v8 4/9] machine: Print CPU model name instead of CPU type

[Gavin Shan]

The names of supported CPU models instead of CPU types should be
printed when the user specified CPU type isn't supported, to be
consistent with the output from '-cpu ?'.

Correct the error messages to print CPU model names instead of CPU
type names.

Signed-off-by: Gavin Shan 
---
v8: Use g_autofree(Phil)
Relace 'type' with 'model' in error messages  (Gavin)
---
 hw/core/machine.c | 14 +-
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c58c84abf5..946875930c 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1408,15 +1408,19 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
 
 /* The user specified CPU type isn't valid */
 if (!mc->valid_cpu_types[i]) {
-error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+g_autofree char *requested = 
cpu_model_from_type(machine->cpu_type);
+error_setg(errp, "Invalid CPU model: %s", requested);
 if (!mc->valid_cpu_types[1]) {
-error_append_hint(errp, "The only valid type is: %s\n",
-  mc->valid_cpu_types[0]);
+g_autofree char *model = cpu_model_from_type(
+ mc->valid_cpu_types[0]);
+error_append_hint(errp, "The only valid type is: %s\n", model);
 } else {
-error_append_hint(errp, "The valid types are: ");
+error_append_hint(errp, "The valid models are: ");
 for (i = 0; mc->valid_cpu_types[i]; i++) {
+g_autofree char *model = cpu_model_from_type(
+ mc->valid_cpu_types[i]);
 error_append_hint(errp, "%s%s",
-  mc->valid_cpu_types[i],
+  model,
   mc->valid_cpu_types[i + 1] ? ", " : "");
 }
 error_append_hint(errp, "\n");
-- 
2.42.0

[PATCH v8 3/9] machine: Improve is_cpu_type_supported()

[Gavin Shan]

It's no sense to check the CPU type when mc->valid_cpu_types[0] is
NULL. So the check is skipped for this particular case. The constraint
has been taken when the error messags are appended.

A precise hint for the error message is given when mc->valid_cpu_types[0]
is the only valid entry. Besides, enumeration on mc->valid_cpu_types[0]
when we have mutiple valid entries there is avoided to increase the code
readability, as suggested by Philippe Mathieu-Daudé.

Signed-off-by: Gavin Shan 
---
 hw/core/machine.c | 18 --
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1797e002f9..c58c84abf5 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1399,7 +1399,7 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
  * CPU types have been determined. Note that the user specified CPU
  * type is provided through '-cpu' option.
  */
-if (mc->valid_cpu_types && machine->cpu_type) {
+if (machine->cpu_type && mc->valid_cpu_types && mc->valid_cpu_types[0]) {
 for (i = 0; mc->valid_cpu_types[i]; i++) {
 if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
 break;
@@ -1409,13 +1409,19 @@ static bool is_cpu_type_supported(const MachineState 
*machine, Error **errp)
 /* The user specified CPU type isn't valid */
 if (!mc->valid_cpu_types[i]) {
 error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
-error_append_hint(errp, "The valid types are: %s",
-  mc->valid_cpu_types[0]);
-for (i = 1; mc->valid_cpu_types[i]; i++) {
-error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+if (!mc->valid_cpu_types[1]) {
+error_append_hint(errp, "The only valid type is: %s\n",
+  mc->valid_cpu_types[0]);
+} else {
+error_append_hint(errp, "The valid types are: ");
+for (i = 0; mc->valid_cpu_types[i]; i++) {
+error_append_hint(errp, "%s%s",
+  mc->valid_cpu_types[i],
+  mc->valid_cpu_types[i + 1] ? ", " : "");
+}
+error_append_hint(errp, "\n");
 }
 
-error_append_hint(errp, "\n");
 return false;
 }
 }
-- 
2.42.0

[PATCH v8 6/9] hw/arm/virt: Check CPU type in machine_run_board_init()

[Gavin Shan]

Set mc->valid_cpu_types so that the user specified CPU type can be
validated in machine_run_board_init(). We needn't to do the check
by ourselves.

Signed-off-by: Gavin Shan 
Reviewed-by: Philippe Mathieu-Daudé 
Reviewed-by: Richard Henderson 
---
 hw/arm/virt.c | 62 +++
 1 file changed, 23 insertions(+), 39 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 668c0d3194..04f9f5fa56 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -204,40 +204,6 @@ static const int a15irqmap[] = {
 [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
 };
 
-static const char *valid_cpus[] = {
-#ifdef CONFIG_TCG
-ARM_CPU_TYPE_NAME("cortex-a7"),
-ARM_CPU_TYPE_NAME("cortex-a15"),
-ARM_CPU_TYPE_NAME("cortex-a35"),
-ARM_CPU_TYPE_NAME("cortex-a55"),
-ARM_CPU_TYPE_NAME("cortex-a72"),
-ARM_CPU_TYPE_NAME("cortex-a76"),
-ARM_CPU_TYPE_NAME("cortex-a710"),
-ARM_CPU_TYPE_NAME("a64fx"),
-ARM_CPU_TYPE_NAME("neoverse-n1"),
-ARM_CPU_TYPE_NAME("neoverse-v1"),
-ARM_CPU_TYPE_NAME("neoverse-n2"),
-#endif
-ARM_CPU_TYPE_NAME("cortex-a53"),
-ARM_CPU_TYPE_NAME("cortex-a57"),
-#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
-ARM_CPU_TYPE_NAME("host"),
-#endif
-ARM_CPU_TYPE_NAME("max"),
-};
-
-static bool cpu_type_valid(const char *cpu)
-{
-int i;
-
-for (i = 0; i < ARRAY_SIZE(valid_cpus); i++) {
-if (strcmp(cpu, valid_cpus[i]) == 0) {
-return true;
-}
-}
-return false;
-}
-
 static void create_randomness(MachineState *ms, const char *node)
 {
 struct {
@@ -2041,11 +2007,6 @@ static void machvirt_init(MachineState *machine)
 unsigned int smp_cpus = machine->smp.cpus;
 unsigned int max_cpus = machine->smp.max_cpus;
 
-if (!cpu_type_valid(machine->cpu_type)) {
-error_report("mach-virt: CPU type %s not supported", 
machine->cpu_type);
-exit(1);
-}
-
 possible_cpus = mc->possible_cpu_arch_ids(machine);
 
 /*
@@ -2939,6 +2900,28 @@ static void virt_machine_class_init(ObjectClass *oc, 
void *data)
 {
 MachineClass *mc = MACHINE_CLASS(oc);
 HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+static const char * const valid_cpu_types[] = {
+#ifdef CONFIG_TCG
+ARM_CPU_TYPE_NAME("cortex-a7"),
+ARM_CPU_TYPE_NAME("cortex-a15"),
+ARM_CPU_TYPE_NAME("cortex-a35"),
+ARM_CPU_TYPE_NAME("cortex-a55"),
+ARM_CPU_TYPE_NAME("cortex-a72"),
+ARM_CPU_TYPE_NAME("cortex-a76"),
+ARM_CPU_TYPE_NAME("cortex-a710"),
+ARM_CPU_TYPE_NAME("a64fx"),
+ARM_CPU_TYPE_NAME("neoverse-n1"),
+ARM_CPU_TYPE_NAME("neoverse-v1"),
+ARM_CPU_TYPE_NAME("neoverse-n2"),
+#endif
+ARM_CPU_TYPE_NAME("cortex-a53"),
+ARM_CPU_TYPE_NAME("cortex-a57"),
+#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
+ARM_CPU_TYPE_NAME("host"),
+#endif
+ARM_CPU_TYPE_NAME("max"),
+NULL
+};
 
 mc->init = machvirt_init;
 /* Start with max_cpus set to 512, which is the maximum supported by KVM.
@@ -2965,6 +2948,7 @@ static void virt_machine_class_init(ObjectClass *oc, void 
*data)
 #else
 mc->default_cpu_type = ARM_CPU_TYPE_NAME("max");
 #endif
+mc->valid_cpu_types = valid_cpu_types;
 mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
 mc->kvm_type = virt_kvm_type;
 assert(!mc->get_hotplug_handler);
-- 
2.42.0

[PATCH v8 1/9] machine: Use error handling when CPU type is checked

[Gavin Shan]

QEMU will be terminated if the specified CPU type isn't supported
in machine_run_board_init(). The list of supported CPU type names
is tracked by mc->valid_cpu_types.

The error handling can be used to propagate error messages, to be
consistent how the errors are handled for other situations in the
same function.

No functional change intended.

Suggested-by: Igor Mammedov 
Signed-off-by: Gavin Shan 
---
v8: Drop @local_err and use @errp to be compatible with
ERRP_GUARD()  (Phil)
---
 hw/core/machine.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0c17398141..bde7f4af6d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1466,15 +1466,16 @@ void machine_run_board_init(MachineState *machine, 
const char *mem_path, Error *
 
 if (!machine_class->valid_cpu_types[i]) {
 /* The user specified CPU is not valid */
-error_report("Invalid CPU type: %s", machine->cpu_type);
-error_printf("The valid types are: %s",
- machine_class->valid_cpu_types[0]);
+error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+error_append_hint(errp, "The valid types are: %s",
+  machine_class->valid_cpu_types[0]);
 for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-error_printf(", %s", machine_class->valid_cpu_types[i]);
+error_append_hint(errp, ", %s",
+  machine_class->valid_cpu_types[i]);
 }
-error_printf("\n");
 
-exit(1);
+error_append_hint(, "\n");
+return;
 }
 }
 
-- 
2.42.0

[PATCH v8 0/9] Unified CPU type check

[Gavin Shan]

This series bases on Phil's repository because the prepatory commits
have been queued to the branch.

  https://gitlab.com/philmd/qemu.git (branch: cpus-next)

There are two places where the user specified CPU type is checked to see
if it's supported or allowed by the board: machine_run_board_init() and
mc->init(). We don't have to maintain two duplicate sets of logic. This
series intends to move the check to machine_run_board_init() so that we
have unified CPU type check.

This series can be checked out from:

  g...@github.com:gwshan/qemu.git (branch: kvm/cpu-type)

PATCH[1-4] refactors and improves the logic to validate CPU type in
   machine_run_board_init()
PATCH[5-9] validates the CPU type in machine_run_board_init() for the
   individual boards

v1: https://lists.nongnu.org/archive/html/qemu-arm/2023-07/msg00302.html
v2: https://lists.nongnu.org/archive/html/qemu-arm/2023-07/msg00528.html
v3: https://lists.nongnu.org/archive/html/qemu-arm/2023-09/msg00157.html
v4: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg5.html
v5: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00611.html
v6: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg00768.html
v7: https://lists.nongnu.org/archive/html/qemu-arm/2023-11/msg01045.html

Testing
===

Before the series is applied:

  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: mach-virt: CPU type cortex-a8-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-a53
  qemu-system-aarch64: sbsa-ref: CPU type cortex-a53-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
  qemu-system-aarch64: sbsa-ref: CPU type sa1100-arm-cpu not supported
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu host
  qemu-system-aarch64: unable to find CPU model 'host'

After the series is applied:

  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M virt -cpu cortex-a8
  qemu-system-aarch64: Invalid CPU model: cortex-a8
  The valid models are: cortex-a7, cortex-a15, cortex-a35, cortex-a55, \
cortex-a72, cortex-a76, cortex-a710, a64fx,\
neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53, \
cortex-a57, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu cortex-a53
  qemu-system-aarch64: Invalid CPU model: cortex-a53
  The valid models are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1,  \
neoverse-n2, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu sa1100
  qemu-system-aarch64: Invalid CPU model: sa1100
  The valid models are: cortex-a57, cortex-a72, neoverse-n1, neoverse-v1,  \
neoverse-n2, max
  [gshan@gshan q]$ ./build/qemu-system-aarch64 -M sbsa-ref -cpu host
  qemu-system-aarch64: unable to find CPU model 'host'

Changelog
=
v8:
  * Pick r-bs from Phil  (Gavin)
  * Drop @local_err and use @errp in machine_run_board_init()(Phil)
  * Add PATCH[v8 3/9] to improve is_cpu_type_supported() (Phil)
  * Use g_autofree and replace 'type' with 'model' for the
error messages in is_cpu_type_supported()(Phil)
v7:
  * Add 'return' after error_propagate() in machine_run_board_init()
to avoid calling mc->init() in the failing case  (Marcin)
v6:
  * Drop PATCH[v5 01-23], queued by Phil (Phil)
  * Clearer hint if only one CPU type is supported and have
'const MachineState *' in is_cpu_type_supported()(Phil)
  * Move valid_cpu_types[] to board's class_init() function  (Phil)

Gavin Shan (9):
  machine: Use error handling when CPU type is checked
  machine: Introduce helper is_cpu_type_supported()
  machine: Improve is_cpu_type_supported()
  machine: Print CPU model name instead of CPU type
  hw/arm/virt: Hide host CPU model for tcg
  hw/arm/virt: Check CPU type in machine_run_board_init()
  hw/arm/sbsa-ref: Check CPU type in machine_run_board_init()
  hw/arm: Check CPU type in machine_run_board_init()
  hw/riscv/shakti_c: Check CPU type in machine_run_board_init()

 hw/arm/bananapi_m2u.c   | 12 +++---
 hw/arm/cubieboard.c | 12 +++---
 hw/arm/mps2-tz.c| 26 +---
 hw/arm/mps2.c   | 26 +---
 hw/arm/msf2-som.c   | 12 +++---
 hw/arm/musca.c  | 12 +++---
 hw/arm/npcm7xx_boards.c | 12 +++---
 hw/arm/orangepi.c   | 12 +++---
 hw/arm/sbsa-ref.c   | 36 +---
 hw/arm/virt.c   | 60 +++
 hw/core/machine.c   | 92 -
 hw/riscv/shakti_c.c | 13 +++---
 12 files changed, 168 insertions(+), 157 deletions(-)

-- 
2.42.0

Re: [PATCH v7 3/8] machine: Print CPU model name instead of CPU type

[Gavin Shan]

Hi Phil,

On 11/28/23 20:55, Philippe Mathieu-Daudé wrote:

On 27/11/23 00:12, Gavin Shan wrote:

The names of supported CPU models instead of CPU types should be
printed when the user specified CPU type isn't supported, to be
consistent with the output from '-cpu ?'.

Correct the error messages to print CPU model names instead of CPU
type names.

Signed-off-by: Gavin Shan 
---
  hw/core/machine.c | 21 +++--
  1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 05e1922b89..898c25552a 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1392,6 +1392,7 @@ static void is_cpu_type_supported(const MachineState 
*machine, Error **errp)
  MachineClass *mc = MACHINE_GET_CLASS(machine);
  ObjectClass *oc = object_class_by_name(machine->cpu_type);
  CPUClass *cc;
+    char *model;
  int i;
  /*
@@ -1408,17 +1409,25 @@ static void is_cpu_type_supported(const MachineState 
*machine, Error **errp)
  /* The user specified CPU type isn't valid */
  if (!mc->valid_cpu_types[i]) {
-    error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+    model = cpu_model_from_type(machine->cpu_type);
+    g_assert(model != NULL);
+    error_setg(errp, "Invalid CPU type: %s", model);
+    g_free(model);


   g_autofree char *requested = cpu_model_from_type(machine->cpu_type);
   error_setg(errp, "Invalid CPU type: %s", requested);



Yes, g_autofree shall be used here. Besides, "Invalid CPU type" needs to
be "Invalid CPU model".


+
+    model = cpu_model_from_type(mc->valid_cpu_types[0]);
+    g_assert(model != NULL);
  if (!mc->valid_cpu_types[1]) {
-    error_append_hint(errp, "The only valid type is: %s",
-  mc->valid_cpu_types[0]);
+    error_append_hint(errp, "The only valid type is: %s", model);


   g_autofree char *model = cpu_model_from_type(mc->valid_cpu_types[0]);
   error_append_hint(errp, "The only valid type is: %s\n", model);



Yes, as above.


  } else {
-    error_append_hint(errp, "The valid types are: %s",
-  mc->valid_cpu_types[0]);
+    error_append_hint(errp, "The valid types are: %s", model);


Please move all the enumeration in this ladder, this makes the logic
simpler to follow:

   error_append_hint(errp, "The valid types are: ");
   for (i = 0; mc->valid_cpu_types[i]; i++) {
    g_autofree char *model =
    cpu_model_from_type(mc->valid_cpu_types[i]);
    error_append_hint(errp, ", %s", model);
   }
   error_append_hint(errp, "\n");



Yes, but we still need to ensure mc->valid_cpu_types[0] != NULL in advance.
"The valid types are: " needs to be "The valid models are: ". Besides,
your proposed code needs to be adjusted a bit like below. Otherwise, we
will get output "The valid types are: , aaa, bbb"

} else {
error_append_hint(errp, "The valid types are: ");
for (i = 0; mc->valid_cpu_types[i]; i++) {
error_append_hint(errp, "%s%s",
  mc->valid_cpu_types[i],
  mc->valid_cpu_types[i + 1] ? ", " : "");
}
error_append_hint(errp, "\n");
}

I will have separate PATCH[v8 3/9] to have the changes, together with the
precise hint when only mc->valid_cpu_types[0] is valid.


  }
+    g_free(model);
  for (i = 1; mc->valid_cpu_types[i]; i++) {
-    error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+    model = cpu_model_from_type(mc->valid_cpu_types[i]);
+    g_assert(model != NULL);
+    error_append_hint(errp, ", %s", model);
+    g_free(model);
  }
  error_append_hint(errp, "\n");


Thanks,
Gavin

Re: [PATCH v7 2/8] machine: Introduce helper is_cpu_type_supported()

[Gavin Shan]

Hi Phil,

On 11/28/23 20:38, Philippe Mathieu-Daudé wrote:

On 27/11/23 00:12, Gavin Shan wrote:

The logic, to check if the specified CPU type is supported in
machine_run_board_init(), is independent enough. Factor it out into
helper is_cpu_type_supported(). machine_run_board_init() looks a bit
clean with this. Since we're here, @machine_class is renamed to @mc
to avoid multiple line spanning of code. The error messages and comments
are tweaked a bit either.

No functional change intended.

Signed-off-by: Gavin Shan 
---
  hw/core/machine.c | 90 +++
  1 file changed, 51 insertions(+), 39 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index b3ef325936..05e1922b89 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1387,13 +1387,57 @@ out:
  return r;
  }
+static void is_cpu_type_supported(const MachineState *machine, Error **errp)


Functions taking an Error** last argument should return a boolean value.



Correct, especially @errp instead of @local_err will be passed from
machine_run_board_init() to is_cpu_type_supported(). We needs an
indicator for machine_run_board_init() to bail immediately to avoid
calling mc->init() there in the failing cases.


+{
+    MachineClass *mc = MACHINE_GET_CLASS(machine);
+    ObjectClass *oc = object_class_by_name(machine->cpu_type);
+    CPUClass *cc;
+    int i;
+
+    /*
+ * Check if the user specified CPU type is supported when the valid
+ * CPU types have been determined. Note that the user specified CPU
+ * type is provided through '-cpu' option.
+ */
+    if (mc->valid_cpu_types && machine->cpu_type) {
+    for (i = 0; mc->valid_cpu_types[i]; i++) {
+    if (object_class_dynamic_cast(oc, mc->valid_cpu_types[i])) {
+    break;
+    }
+    }
+
+    /* The user specified CPU type isn't valid */
+    if (!mc->valid_cpu_types[i]) {
+    error_setg(errp, "Invalid CPU type: %s", machine->cpu_type);
+    if (!mc->valid_cpu_types[1]) {
+    error_append_hint(errp, "The only valid type is: %s",
+  mc->valid_cpu_types[0]);
+    } else {
+    error_append_hint(errp, "The valid types are: %s",
+  mc->valid_cpu_types[0]);
+    }
+
+    for (i = 1; mc->valid_cpu_types[i]; i++) {
+    error_append_hint(errp, ", %s", mc->valid_cpu_types[i]);
+    }
+
+    error_append_hint(errp, "\n");
+    return;
+    }
+    }
+
+    /* Check if CPU type is deprecated and warn if so */
+    cc = CPU_CLASS(oc);
+    if (cc && cc->deprecation_note) {
+    warn_report("CPU model %s is deprecated -- %s",
+    machine->cpu_type, cc->deprecation_note);


Why did you move the deprecation warning within the is_supported check?



This check is more relevant to CPU type, to check if the CPU type has
been deprecated. Besides, @oc and @cc can be dropped from 
machine_run_board_init().


+    }
+}
  void machine_run_board_init(MachineState *machine, const char *mem_path, 
Error **errp)
  {
  ERRP_GUARD();
  MachineClass *machine_class = MACHINE_GET_CLASS(machine);
-    ObjectClass *oc = object_class_by_name(machine->cpu_type);
-    CPUClass *cc;
  Error *local_err = NULL;
  /* This checkpoint is required by replay to separate prior clock
@@ -1449,43 +1493,11 @@ void machine_run_board_init(MachineState *machine, 
const char *mem_path, Error *
  machine->ram = machine_consume_memdev(machine, machine->memdev);
  }
-    /* If the machine supports the valid_cpu_types check and the user
- * specified a CPU with -cpu check here that the user CPU is supported.
- */
-    if (machine_class->valid_cpu_types && machine->cpu_type) {
-    int i;
-
-    for (i = 0; machine_class->valid_cpu_types[i]; i++) {
-    if (object_class_dynamic_cast(oc,
-  machine_class->valid_cpu_types[i])) {
-    /* The user specified CPU is in the valid field, we are
- * good to go.
- */
-    break;
-    }
-    }
-
-    if (!machine_class->valid_cpu_types[i]) {
-    /* The user specified CPU is not valid */
-    error_setg(_err, "Invalid CPU type: %s", machine->cpu_type);
-    error_append_hint(_err, "The valid types are: %s",
-  machine_class->valid_cpu_types[0]);
-    for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-    error_append_hint(_err, ", %s",
-  machine_class->valid_cpu_types[i]);
-    }
-    error_append_hint(_err, "\n");

Re: [PATCH v7 1/8] machine: Use error handling when CPU type is checked

[Gavin Shan]

Hi Phil,

On 11/28/23 20:43, Philippe Mathieu-Daudé wrote:

On 27/11/23 00:12, Gavin Shan wrote:

QEMU will be terminated if the specified CPU type isn't supported
in machine_run_board_init(). The list of supported CPU type names
is tracked by mc->valid_cpu_types.

The error handling can be used to propagate error messages, to be
consistent how the errors are handled for other situations in the
same function.

No functional change intended.

Suggested-by: Igor Mammedov 
Signed-off-by: Gavin Shan 
---
v7: Add 'return' after error_propagate() to avoid calling into
 mc->init() in the failing case    (Marcin)
---
  hw/core/machine.c | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0c17398141..b3ef325936 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1394,6 +1394,7 @@ void machine_run_board_init(MachineState *machine, const 
char *mem_path, Error *
  MachineClass *machine_class = MACHINE_GET_CLASS(machine);
  ObjectClass *oc = object_class_by_name(machine->cpu_type);
  CPUClass *cc;
+    Error *local_err = NULL;


IIUC the big comment in "include/qapi/error.h", since commit ae7c80a7bd
("error: New macro ERRP_GUARD()") we want to use the new macro instead.



Yeah, there already has a ERRP_GUARD() in machine_run_board_init(). After
rechecking the implementation of ERRP_GUARD(), I found @local_err needs to
be dropped and use @errp below.



  /* This checkpoint is required by replay to separate prior clock
 reading from the other reads, because timer polling functions query
@@ -1466,15 +1467,17 @@ void machine_run_board_init(MachineState *machine, 
const char *mem_path, Error *
  if (!machine_class->valid_cpu_types[i]) {
  /* The user specified CPU is not valid */
-    error_report("Invalid CPU type: %s", machine->cpu_type);
-    error_printf("The valid types are: %s",
- machine_class->valid_cpu_types[0]);
+    error_setg(_err, "Invalid CPU type: %s", machine->cpu_type);
+    error_append_hint(_err, "The valid types are: %s",
+  machine_class->valid_cpu_types[0]);
  for (i = 1; machine_class->valid_cpu_types[i]; i++) {
-    error_printf(", %s", machine_class->valid_cpu_types[i]);
+    error_append_hint(_err, ", %s",
+  machine_class->valid_cpu_types[i]);
  }
-    error_printf("\n");
+    error_append_hint(_err, "\n");
-    exit(1);
+    error_propagate(errp, local_err);
+    return;
  }
  }


@local_err needs to be replaced with @errp in this chunk of code, as mentioned
above.

Thanks,
Gavin

Re: [PATCH v7 5/8] hw/arm/virt: Check CPU type in machine_run_board_init()

[Gavin Shan]

On 11/27/23 21:13, Marcin Juszkiewicz wrote:

W dniu 27.11.2023 o 00:12, Gavin Shan pisze:

@@ -2939,6 +2900,28 @@ static void virt_machine_class_init(ObjectClass *oc, 
void *data)
  {
  MachineClass *mc = MACHINE_CLASS(oc);
  HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(oc);
+    static const char * const valid_cpu_types[] = {
+#ifdef CONFIG_TCG
+    ARM_CPU_TYPE_NAME("cortex-a7"),
+    ARM_CPU_TYPE_NAME("cortex-a15"),
+    ARM_CPU_TYPE_NAME("cortex-a35"),
+    ARM_CPU_TYPE_NAME("cortex-a55"),
+    ARM_CPU_TYPE_NAME("cortex-a72"),
+    ARM_CPU_TYPE_NAME("cortex-a76"),
+    ARM_CPU_TYPE_NAME("cortex-a710"),
+    ARM_CPU_TYPE_NAME("a64fx"),
+    ARM_CPU_TYPE_NAME("neoverse-n1"),
+    ARM_CPU_TYPE_NAME("neoverse-v1"),
+    ARM_CPU_TYPE_NAME("neoverse-n2"),
+#endif
+    ARM_CPU_TYPE_NAME("cortex-a53"),
+    ARM_CPU_TYPE_NAME("cortex-a57"),
+#if defined(CONFIG_KVM) || defined(CONFIG_HVF)
+    ARM_CPU_TYPE_NAME("host"),
+#endif
+    ARM_CPU_TYPE_NAME("max"),
+    NULL
+    };


I understand that you just move list from one place to the other but also 
wonder why a53/a57 were/are outside of 'ifdef CONFIG_TCG' check.



I'm not sure about HVF, but a53/a57 can be supported by KVM. The supported list 
of
CPUs by KVM is defined in linux/arch/arm64/include/uapi/asm/kvm.h as below

/*
 * Supported CPU Targets - Adding a new target type is not recommended,
 * unless there are some special registers not supported by the
 * genericv8 syreg table.
 */
#define KVM_ARM_TARGET_AEM_V8   0
#define KVM_ARM_TARGET_FOUNDATION_V81
#define KVM_ARM_TARGET_CORTEX_A57   2
#define KVM_ARM_TARGET_XGENE_POTENZA3
#define KVM_ARM_TARGET_CORTEX_A53   4
/* Generic ARM v8 target */
#define KVM_ARM_TARGET_GENERIC_V8   5

#define KVM_ARM_NUM_TARGETS 6

And the following QEMU commit gives more hints about it.

[gshan@gshan q]$ git show 39920a04952
commit 39920a04952b67fb1fce8fc3519ac18b7a95f3f3
Author: Fabiano Rosas 
Date:   Wed Apr 26 15:00:05 2023 -0300

target/arm: Move 64-bit TCG CPUs into tcg/

Move the 64-bit CPUs that are TCG-only:

- cortex-a35
- cortex-a55
- cortex-a72
- cortex-a76
- a64fx
- neoverse-n1

Keep the CPUs that can be used with KVM:

- cortex-a57
- cortex-a53
- max
- host

Signed-off-by: Fabiano Rosas 

Reviewed-by: Richard Henderson 
Reviewed-by: Philippe Mathieu-Daudé 
Message-id: 20230426180013.14814-6-faro...@suse.de
Signed-off-by: Peter Maydell 

Thanks,
Gavin

Re: [PATCH v7 0/8] Unified CPU type check

[Gavin Shan]

On 11/27/23 21:10, Marcin Juszkiewicz wrote:

W dniu 27.11.2023 o 00:12, Gavin Shan pisze:

After the series is applied:

   [gshan@gshan q]$ ./build/qemu-system-aarch64 -M virt -cpu cortex-a8
   qemu-system-aarch64: Invalid CPU type: cortex-a8
   The valid types are: cortex-a7, cortex-a15, cortex-a35, cortex-a55, \
    cortex-a72, cortex-a76, cortex-a710, a64fx,    \
    neoverse-n1, neoverse-v1, neoverse-n2, cortex-a53, \
    cortex-a57, max


Can this list have some better order? A35, A53, A55, A57, A72 feels better than 
current one.



Definitely a good idea. I think this series is about to be queued,
let me send one additional patch to do so after this series is
merged.

Thanks,
Gavin

Re: [PATCH-for-9.0 16/16] target/arm/kvm: Have kvm_arm_hw_debug_active take a ARMCPU argument

[Gavin Shan]

Hi Phil,

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)



With the following comments addressed:

Reviewed-by: Gavin Shan 


diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 1f6da5529f..cbfea689cc 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -1455,11 +1455,11 @@ int kvm_arch_process_async_events(CPUState *cs)
  
  /**

   * kvm_arm_hw_debug_active:
- * @cs: CPU State
+ * @cpu: ARMCPU
   *
   * Return: TRUE if any hardware breakpoints in use.
   */
-static bool kvm_arm_hw_debug_active(CPUState *cs)
+static bool kvm_arm_hw_debug_active(ARMCPU *cpu)
  {
  return ((cur_hw_wps > 0) || (cur_hw_bps > 0));
  }


Either @cs or @cpu isn't dereferenced in kvm_arm_hw_debug_active(). So I guess
the argument can be simply droped?


@@ -1493,7 +1493,7 @@ void kvm_arch_update_guest_debug(CPUState *cs, struct 
kvm_guest_debug *dbg)
  if (kvm_sw_breakpoints_active(cs)) {
  dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_SW_BP;
  }
-if (kvm_arm_hw_debug_active(cs)) {
+if (kvm_arm_hw_debug_active(ARM_CPU(cs))) {
  dbg->control |= KVM_GUESTDBG_ENABLE | KVM_GUESTDBG_USE_HW;
  kvm_arm_copy_hw_debug_data(>arch);
  }


Thanks,
Gavin

Re: [PATCH-for-9.0 15/16] target/arm/kvm: Have kvm_arm_handle_debug take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 14/16] target/arm/kvm: Have kvm_arm_handle_dabt_nisv take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 13/16] target/arm/kvm: Have kvm_arm_verify_ext_dabt_pending take a ARMCPU arg

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 12/16] target/arm/kvm: Have kvm_arm_[get|put]_virtual_time take ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 23 ++-
  1 file changed, 10 insertions(+), 13 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 10/16] target/arm/kvm: Have kvm_arm_vcpu_init take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 11 +--
  1 file changed, 5 insertions(+), 6 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 11/16] target/arm/kvm: Have kvm_arm_vcpu_finalize take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 10/16] target/arm/kvm: Have kvm_arm_vcpu_init take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 11 +--
  1 file changed, 5 insertions(+), 6 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 09/16] target/arm/kvm: Have kvm_arm_pmu_set_irq take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm_arm.h | 4 ++--
  hw/arm/virt.c| 2 +-
  target/arm/kvm.c | 6 +++---
  3 files changed, 6 insertions(+), 6 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 08/16] target/arm/kvm: Have kvm_arm_pmu_init take a ARMCPU argument

[Gavin Shan]

Hi Phil,

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm_arm.h | 4 ++--
  hw/arm/virt.c| 2 +-
  target/arm/kvm.c | 6 +++---
  3 files changed, 6 insertions(+), 6 deletions(-)



One nit below, but I guess it doesn't matter.

Reviewed-by: Gavin Shan 


diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 0e12a008ab..fde1c45609 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -200,8 +200,8 @@ int kvm_arm_get_max_vm_ipa_size(MachineState *ms, bool 
*fixed_ipa);
  
  int kvm_arm_vgic_probe(void);
  
+void kvm_arm_pmu_init(ARMCPU *cpu);

  void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
-void kvm_arm_pmu_init(CPUState *cs);
  


Why the order of the declaration is changed? I guess the reason would be
kvm_arm_pmu_init() is called prior to kvm_arm_pmu_set_irq().


  /**
   * kvm_arm_pvtime_init:
@@ -263,7 +263,7 @@ static inline void kvm_arm_pmu_set_irq(CPUState *cs, int 
irq)
  g_assert_not_reached();
  }
  
-static inline void kvm_arm_pmu_init(CPUState *cs)

+static inline void kvm_arm_pmu_init(ARMCPU *cpu)
  {
  g_assert_not_reached();
  }
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index b6efe9da4d..63f3c0b750 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2000,7 +2000,7 @@ static void virt_cpu_post_init(VirtMachineState *vms, 
MemoryRegion *sysmem)
  if (kvm_irqchip_in_kernel()) {
  kvm_arm_pmu_set_irq(cpu, VIRTUAL_PMU_IRQ);
  }
-kvm_arm_pmu_init(cpu);
+kvm_arm_pmu_init(ARM_CPU(cpu));
  }
  if (steal_time) {
  kvm_arm_pvtime_init(ARM_CPU(cpu), pvtime_reg_base
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 82c5924ab5..e7cbe1ff05 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -1711,17 +1711,17 @@ static bool kvm_arm_set_device_attr(ARMCPU *cpu, struct 
kvm_device_attr *attr,
  return true;
  }
  
-void kvm_arm_pmu_init(CPUState *cs)

+void kvm_arm_pmu_init(ARMCPU *cpu)
  {
  struct kvm_device_attr attr = {
  .group = KVM_ARM_VCPU_PMU_V3_CTRL,
  .attr = KVM_ARM_VCPU_PMU_V3_INIT,
  };
  
-if (!ARM_CPU(cs)->has_pmu) {

+if (!cpu->has_pmu) {
  return;
  }
-if (!kvm_arm_set_device_attr(ARM_CPU(cs), , "PMU")) {
+if (!kvm_arm_set_device_attr(cpu, , "PMU")) {
  error_report("failed to init PMU");
  abort();
  }


Thanks,
Gavin

Re: [PATCH-for-9.0 07/16] target/arm/kvm: Have kvm_arm_pvtime_init take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm_arm.h | 6 +++---
  hw/arm/virt.c| 5 +++--
  target/arm/kvm.c | 6 +++---
  3 files changed, 9 insertions(+), 8 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 06/16] target/arm/kvm: Have kvm_arm_set_device_attr take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)



Reviewed-by: Gavin Shan 

Re: [PATCH-for-9.0 05/16] target/arm/kvm: Have kvm_arm_sve_get_vls take a ARMCPU argument

[Gavin Shan]

Hi Phil,

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm_arm.h | 6 +++---
  target/arm/cpu64.c   | 2 +-
  target/arm/kvm.c | 2 +-
  3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 6fb8a5f67e..84f87f5ed7 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -129,13 +129,13 @@ void kvm_arm_destroy_scratch_host_vcpu(int *fdarray);
  
  /**

   * kvm_arm_sve_get_vls:
- * @cs: CPUState
+ * @cpu: ARMCPU
   *
   * Get all the SVE vector lengths supported by the KVM host, setting
   * the bits corresponding to their length in quadwords minus one
   * (vq - 1) up to ARM_MAX_VQ.  Return the resulting map.
   */
-uint32_t kvm_arm_sve_get_vls(CPUState *cs);
+uint32_t kvm_arm_sve_get_vls(ARMCPU *cpu);
  


Either @cs or @cpu isn't dereferenced in kvm_arm_sve_get_vls(). So I guess
the argument can be simply droped?


  /**
   * kvm_arm_set_cpu_features_from_host:
@@ -278,7 +278,7 @@ static inline void kvm_arm_steal_time_finalize(ARMCPU *cpu, 
Error **errp)
  g_assert_not_reached();
  }
  
-static inline uint32_t kvm_arm_sve_get_vls(CPUState *cs)

+static inline uint32_t kvm_arm_sve_get_vls(ARMCPU *cpu)
  {
  g_assert_not_reached();
  }
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 1e9c6c85ae..8e30a7993e 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -66,7 +66,7 @@ void arm_cpu_sve_finalize(ARMCPU *cpu, Error **errp)
   */
  if (kvm_enabled()) {
  if (kvm_arm_sve_supported()) {
-cpu->sve_vq.supported = kvm_arm_sve_get_vls(CPU(cpu));
+cpu->sve_vq.supported = kvm_arm_sve_get_vls(cpu);
  vq_supported = cpu->sve_vq.supported;
  } else {
  assert(!cpu_isar_feature(aa64_sve, cpu));
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 71833a845a..766a077bcf 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -1803,7 +1803,7 @@ bool kvm_arm_sve_supported(void)
  
  QEMU_BUILD_BUG_ON(KVM_ARM64_SVE_VQ_MIN != 1);
  
-uint32_t kvm_arm_sve_get_vls(CPUState *cs)

+uint32_t kvm_arm_sve_get_vls(ARMCPU *cpu)
  {
  /* Only call this function if kvm_arm_sve_supported() returns true. */
  static uint64_t vls[KVM_ARM64_SVE_VLS_WORDS];


Thanks,
Gavin

Re: [PATCH-for-9.0 04/16] target/arm/kvm: Have kvm_arm_sve_set_vls take a ARMCPU argument

[Gavin Shan]

On 11/24/23 05:35, Philippe Mathieu-Daudé wrote:

Unify the "kvm_arm.h" API: All functions related to ARM vCPUs
take a ARMCPU* argument. Use the CPU() QOM cast macro When
calling the generic vCPU API from "sysemu/kvm.h".

Signed-off-by: Philippe Mathieu-Daudé 
---
  target/arm/kvm.c | 7 +++
  1 file changed, 3 insertions(+), 4 deletions(-)



Reviewed-by: Gavin Shan