Re: [PATCH v3] i386/cpu: fixup number of addressable IDs for processor cores in the physical package
On Mon, Jul 01, 2024 at 09:48:02AM +0300, Michael Tokarev wrote:
> Date: Mon, 1 Jul 2024 09:48:02 +0300
> From: Michael Tokarev
> Subject: Re: [PATCH v3] i386/cpu: fixup number of addressable IDs for
> processor cores in the physical package
>
> 11.06.2024 06:23, Chuang Xu wrote:
> > When QEMU is started with:
> > -cpu host,host-cache-info=on,l3-cache=off \
> > -smp 2,sockets=1,dies=1,cores=1,threads=2
> > Guest can't acquire maximum number of addressable IDs for processor cores in
> > the physical package from CPUID[04H].
> >
> > When creating a CPU topology of 1 core per package, host-cache-info only
> > uses the Host's addressable core IDs field (CPUID.04H.EAX[bits 31-26]),
> > resulting in a conflict (on the multicore Host) between the Guest core
> > topology information in this field and the Guest's actual cores number.
> >
> > Fix it by removing the unnecessary condition to cover 1 core per package
> > case. This is safe because cores_per_pkg will not be 0 and will be at
> > least 1.
> >
> > Fixes: d7caf13b5fcf ("x86: cpu: fixup number of addressable IDs for logical
> > processors sharing cache")
> > Signed-off-by: Guixiong Wei
> > Signed-off-by: Yipeng Yin
> > Signed-off-by: Chuang Xu
> > ---
> > target/i386/cpu.c | 6 ++
> > 1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index bc2dceb647..b68f7460db 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -6426,10 +6426,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> > uint32_t count,
> > if (*eax & 31) {
> > int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
> > -if (cores_per_pkg > 1) {
> > -*eax &= ~0xFC00;
> > -*eax |= max_core_ids_in_package(_info) << 26;
> > -}
> > +*eax &= ~0xFC00;
> > +*eax |= max_core_ids_in_package(_info) << 26;
> > if (host_vcpus_per_cache > threads_per_pkg) {
> > *eax &= ~0x3FFC000;
>
> In qemu 9.0, the context is a bit different here:
>
>
> if (*eax & 31) {
> int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
> int vcpus_per_socket = cs->nr_cores * cs->nr_threads;
> if (cs->nr_cores > 1) {
> *eax &= ~0xFC00;
> *eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
> }
> if (host_vcpus_per_cache > vcpus_per_socket) {
>
> Ie, no max_core_ids_in_package(), cores_per_pkg etc, introduced in
> v9.0.0-790-gf602eb925a "i386/cpu: Use CPUCacheInfo.share_level to encode
> CPUID[4]" and nearby.
>
> Am I right the above change becomes
>
> if (*eax & 31) {
> int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
> int vcpus_per_socket = cs->nr_cores * cs->nr_threads;
> -if (cs->nr_cores > 1) {
> -*eax &= ~0xFC00;
> -*eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
> -}
> +*eax &= ~0xFC00;
> +*eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
> if (host_vcpus_per_cache > vcpus_per_socket) {
> *eax &= ~0x3FFC000;
> *eax |= (pow2ceil(vcpus_per_socket) - 1) << 14;
>
> in 9.0 -- in other words, just remove the nr_cores condition check
> and do the *eax assignment unconditionally ?
>
> From the patch description it seems like it is, but I thought I'd
> ask anyway :)
Hi Michael,
I can help confirm your changes are correct.
-Zhao
Re: [PATCH 2/2] target/i386: drop AMD machine check bits from Intel CPUID
On Fri, Jun 28, 2024 at 03:23:11PM +0200, Paolo Bonzini wrote: > Date: Fri, 28 Jun 2024 15:23:11 +0200 > From: Paolo Bonzini > Subject: Re: [PATCH 2/2] target/i386: drop AMD machine check bits from > Intel CPUID > > Il ven 28 giu 2024, 10:32 Xiaoyao Li ha scritto: > > > On 6/27/2024 10:06 PM, Paolo Bonzini wrote: > > > The recent addition of the SUCCOR bit to kvm_arch_get_supported_cpuid() > > > causes the bit to be visible when "-cpu host" VMs are started on Intel > > > processors. > > > > > > While this should in principle be harmless, it's not tidy and we don't > > > even know for sure that it doesn't cause any guest OS to take unexpected > > > paths. Since x86_cpu_get_supported_feature_word() can return different > > > different values depending on the guest, adjust it to hide the SUCCOR > > > > superfluous different > > > > > bit if the guest has non-AMD vendor. > > > > It seems to adjust it based on vendor in kvm_arch_get_supported_cpuid() > > is better than in x86_cpu_get_supported_feature_word(). Otherwise > > kvm_arch_get_supported_cpuid() still returns "risky" value for Intel VMs. > > > > But the cpuid bit is only invalid for Intel *guest* vendor, not host. It is > not a problem to have it if you run on Intel host but have a guest model > with AMD vendor. > > I will check if there are other callers of kvm_arch_get_supported_cpuid(), > or callers of x86_cpu_get_supported_feature_word() with NULL cpu, that > might care about the difference. Another example is CPUID_EXT3_TOPOEXT, though it's a no_autoenable_flags, it can be set by "-cpu host,+topoext" on Intel platforms. For this case, we have recognized that that the host/max CPU should only contain vender specific features, and I think it would be hard to expand such a rule afterwards, especially since there's other x86 vender like zhaoxin who implement a subset of Intel/AMD: https://lore.kernel.org/qemu-devel/d4c0dae5-b9d5-4deb-b300-78492ab11...@zhaoxin.com/#t What about a new flag "host_bare_metal_check" in FeatureWordInfo? Then if a feature is marked as "host_bare_metal_check", in addition to the current checks in x86_cpu_get_supported_feature_word(), bare-metal CPUID check is also needed (by host_cpuid()) for "host" CPU. -Zhao
Re: [PATCH v2 0/6] target/i386: Misc cleanup on KVM PV defs and outdated comments
Hi Paolo, A gentle poke for this series. Thanks, Zhao On Thu, Jun 06, 2024 at 05:25:31PM +0800, Zhao Liu wrote: > Date: Thu, 6 Jun 2024 17:25:31 +0800 > From: Zhao Liu > Subject: Re: [PATCH v2 0/6] target/i386: Misc cleanup on KVM PV defs and > outdated comments > > Hi Paolo, > > Just a ping for this cleanup series. > > Thanks, > Zhao > > On Mon, May 06, 2024 at 04:51:47PM +0800, Zhao Liu wrote: > > Date: Mon, 6 May 2024 16:51:47 +0800 > > From: Zhao Liu > > Subject: [PATCH v2 0/6] target/i386: Misc cleanup on KVM PV defs and > > outdated comments > > X-Mailer: git-send-email 2.34.1 > > > > Hi, > > > > This is my v2 cleanup series. Compared with v1 [1], only tags (R/b, S/b) > > updates, and a typo fix, no code change. > > > > This series picks cleanup from my previous kvmclock [2] (as other > > renaming attempts were temporarily put on hold). > > > > In addition, this series also include the cleanup on a historically > > workaround and recent comment of coco interface [3]. > > > > Avoiding the fragmentation of these misc cleanups, I consolidated them > > all in one series and was able to tackle them in one go! > > > > [1]: > > https://lore.kernel.org/qemu-devel/20240426100716.2111688-1-zhao1@intel.com/ > > [2]: > > https://lore.kernel.org/qemu-devel/20240329101954.3954987-1-zhao1@linux.intel.com/ > > [3]: > > https://lore.kernel.org/qemu-devel/2815f0f1-9e20-4985-849c-d74c6cdc9...@intel.com/ > > > > Thanks and Best Regards, > > Zhao > > --- > > Zhao Liu (6): > > target/i386/kvm: Add feature bit definitions for KVM CPUID > > target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and > > MSR_KVM_SYSTEM_TIME definitions > > target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled > > target/i386/kvm: Save/load MSRs of kvmclock2 > > (KVM_FEATURE_CLOCKSOURCE2) > > target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo > > target/i386/confidential-guest: Fix comment of > > x86_confidential_guest_kvm_type() > > > > hw/i386/kvm/clock.c | 5 +-- > > target/i386/confidential-guest.h | 2 +- > > target/i386/cpu.h| 25 + > > target/i386/kvm/kvm.c| 63 +++- > > 4 files changed, 66 insertions(+), 29 deletions(-) > > > > -- > > 2.34.1 > > >
Re: [PATCH] target/i386/tcg: remove unused enum
On Thu, Jun 27, 2024 at 12:59:19PM +0200, Paolo Bonzini wrote: > Date: Thu, 27 Jun 2024 12:59:19 +0200 > From: Paolo Bonzini > Subject: [PATCH] target/i386/tcg: remove unused enum > X-Mailer: git-send-email 2.45.2 > > Signed-off-by: Paolo Bonzini > --- > target/i386/tcg/translate.c | 16 > 1 file changed, 16 deletions(-) > Reviewed-by: Zhao Liu
Re: [RFC PATCH v3 5/5] DO NOT MERGE: replace TYPE_PL011 with x-pl011-rust in arm virt machine
Hi Manos,
On Wed, Jun 19, 2024 at 11:14:02PM +0300, Manos Pitsidianakis wrote:
> Date: Wed, 19 Jun 2024 23:14:02 +0300
> From: Manos Pitsidianakis
> Subject: [RFC PATCH v3 5/5] DO NOT MERGE: replace TYPE_PL011 with
> x-pl011-rust in arm virt machine
> X-Mailer: git-send-email 2.44.0
>
> Convenience patch for testing the rust device.
>
> Signed-off-by: Manos Pitsidianakis
> ---
> hw/arm/virt.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 3c93c0c0a6..f33b58ae0d 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -912,7 +912,11 @@ static void create_uart(const VirtMachineState *vms, int
> uart,
> int irq = vms->irqmap[uart];
> const char compat[] = "arm,pl011\0arm,primecell";
> const char clocknames[] = "uartclk\0apb_pclk";
> +#ifdef CONFIG_WITH_RUST
> +DeviceState *dev = qdev_new("x-pl011-rust");
> +#else
> DeviceState *dev = qdev_new(TYPE_PL011);
> +#endif
> SysBusDevice *s = SYS_BUS_DEVICE(dev);
> MachineState *ms = MACHINE(vms);
>
I realized that if we want to merge the rust pl011 device, then this
patch or similar enablement support is necessary, otherwise, the rust
code is only used for compilation and cannot actually be run...
This is also an open for the devices that are rewrite in Rust.
I think there should be an option for the user to choose whether to
enable pl011 in C or pl011 in Rust. What do you think?
Perhaps the easiest way to enable rust pl011 is to add an option for
virt machine... But that's certainly not a long-term approach. I think
the ideal way would be to allow rust pl011 to be specified in the
command line via -device, but this approach would mean allowing the
user to create pl011 and would require changes to the current buildin
pl011's creation logic.
-Zhao
Re: [RFC PATCH v3 2/5] rust: add bindgen step as a meson dependency
> > > + # > WARNING: Project specifies a minimum meson_version '>=0.63.0'
> > > but
> > > + # > uses features which were added in newer versions:
> > > + # > * 0.64.0: {'fs.copyfile'}
> > > + # > * 1.0.0: {'dependencies arg in rust.bindgen', 'module rust as
> > > stable module'}
> > > + rust_bindgen = import('rust')
> > > +
> > > + # We need one generated_rs target per target, so give them
> > > + # target-specific names.
> > > + copy = fs.copyfile('rust/wrapper.h',
> > > + target + '_wrapper.h')
> > > + generated_rs = rust_bindgen.bindgen(
> > > +input: copy,
> > > +dependencies: arch_deps + lib_deps,
> > > +output: target + '-generated.rs',
> > > +include_directories: include_directories('.', 'include'),
> > > +args: [
> > > + '--ctypes-prefix', 'core::ffi',
> > > + '--formatter', 'rustfmt',
> > > + '--generate-block',
> > > + '--generate-cstr',
> > > + '--impl-debug',
> > > + '--merge-extern-blocks',
> > > + '--no-doc-comments',
> > > + '--no-include-path-detection',
> > > + '--use-core',
> > > + '--with-derive-default',
> > > + '--allowlist-file', meson.project_source_root() +
> > > '/include/.*',
> > > + '--allowlist-file', meson.project_source_root() + '/.*',
> > > + '--allowlist-file', meson.project_build_root() + '/.*'
> > > +],
> > > + )
> > > +
> > > + if target in rust_targets
> > > +rust_hw = ss.source_set()
> > > +foreach t: rust_targets[target]
> > > + rust_device_cargo = custom_target(t['name'],
> > > + output: t['output'],
> > > + depends: [generated_rs],
> > > + build_always_stale: true,
> > > + command: t['command'])
> > > + rust_dep = declare_dependency(link_args: [
> > > + '-Wl,--whole-archive',
> > > + t['output-path'],
> > > + '-Wl,--no-whole-archive'
> > > + ],
> > > + sources: [rust_device_cargo])
> > > + rust_hw.add(rust_dep)
> > > +endforeach
> > > +rust_hw_config = rust_hw.apply(config_target, strict: false)
> > > +arch_srcs += rust_hw_config.sources()
> > > +arch_deps += rust_hw_config.dependencies()
> > > + endif
> > > + endif
> > > +
> > >lib = static_library('qemu-' + target,
> > > sources: arch_srcs + genh,
> > > dependencies: lib_deps,
> > > diff --git a/rust/.gitignore b/rust/.gitignore
> > > new file mode 100644
> > > index 00..1bf71b1f68
> > > --- /dev/null
> > > +++ b/rust/.gitignore
> > > @@ -0,0 +1,3 @@
> > > +# Ignore any cargo development build artifacts; for qemu-wide builds,
> > > all build
> > > +# artifacts will go to the meson build directory.
> > > +target
> > > diff --git a/rust/meson.build b/rust/meson.build
> > > new file mode 100644
> > > index 00..435abd3e1c
> > > --- /dev/null
> > > +++ b/rust/meson.build
> > > @@ -0,0 +1,129 @@
> > > +# Supported hosts
> > > +rust_supported_oses = {
> > > + 'linux': '-unknown-linux-gnu',
> > > + 'darwin': '-apple-darwin',
> > > + 'windows': '-pc-windows-gnu'
> > > +}
> >
> > Does the current test cover windows and apple? If not, I think we can
> > add these two platforms later on.
>
> Do you mean the test for supported OSes?
Yes.
> This is for future-proofing the Rust integration in general. I haven't been
> able to compile under macos yet because bindgen cannot find the system clang
> header. I also don't have a windows pc to test it on. But it should work
> theoretically under all three.
Yes, they should work. EMM, but there is no particular need for them at
the moment, so just to be safe, we can put these two platforms on hold
for now, and they can be easily added when the tests are covered.
A TODO can remind support for them.
> >
> > > +rust_supported_cpus = ['x86_64', 'aarch64']
> >
> > Similarly, here I have another question, x-pl011-rust in arm virt machine
> > I understand should only run on ARM platform, right? So compiling to
> > x86_64 doesn't seem necessary at the moment?
>
> This refers to the host cpu not the virtualized ones, you can run
> qemu-system-aarch64 under x86_64
Thanks, I see.
> > > +# Future-proof the above definitions against any change in the root
> > > meson.build file:
> > > +foreach rust_os: rust_supported_oses.keys()
> > > + if not supported_oses.contains(rust_os)
> > > +message()
> > > +warning('UNSUPPORTED OS VALUES IN ' + meson.current_source_dir() +
> > > '/meson.build')
> > > +message()
> > > +message('This
Re: [PATCH 0/4] Add support for Zhaoxin Yongfeng CPU model and other improvements
Hi EwanHai, On Tue, Jun 25, 2024 at 05:19:01AM -0400, EwanHai wrote: > Date: Tue, 25 Jun 2024 05:19:01 -0400 > From: EwanHai > Subject: [PATCH 0/4] Add support for Zhaoxin Yongfeng CPU model and other > improvements > X-Mailer: git-send-email 2.34.1 > > This patch series introduces support for the Zhaoxin Yongfeng CPU model and > includes > some improvements and updates related to Zhaoxin and VIA CPUs. The changes > ensure that > QEMU can correctly identify and emulate Zhaoxin CPUs, providing accurate > functionality > and performance characteristics. > > > ### Summary of Changes > > EwanHai (4): > target/i386: Add support for Zhaoxin/VIA CPU vendor identification > target/i386: Add CPUID leaf 0xC000_0001 EDX definitions > target/i386: Introduce Zhaoxin Yongfeng CPU model > target/i386: Update CMPLegacy handling for Zhaoxin and VIA CPUs > > target/i386/cpu.c | 130 -- > target/i386/cpu.h | 38 ++ > 2 files changed, 165 insertions(+), 3 deletions(-) > > ### Known Bugs > > 1. Issue with VMX Preemption Timer Rate on Yongfeng CPU: >- Description: On Yongfeng CPUs, the VMX preemption timer rate is 128, > meaning that > bits 4:0 of MSR_IA32_VMX_MISC_CTLS should be set to 7. However, due to > Intel's rate > being 5, the Linux kernel has hardcoded this value as 5: > `#define VMX_MISC_EMULATED_PREEMPTION_TIMER_RATE 5` >- Impact: This discrepancy can cause incorrect behavior in the VMX > preemption timer on > Yongfeng CPUs. >- Workaround: A patch to correct this issue in the Linux kernel is > currently being > prepared and will be submitted soon. > Thanks for your patch. Is there some spec/datasheet link that people can refer to? Regards, Zhao
Re: [PATCH v3] target/i386/kvm: Refine VMX controls setting for backward compatibility
[snip]
> > Additionally, has_msr_vmx_vmfunc has the similar compat issue. I think
> > it deserves a fix, too.
> >
> > -Zhao
> Thanks for your reply. In fact, I've tried to process has_msr_vmx_vmfunc in
> the same
> way as has_msr_vmx_procbased_ctls in this patch, but when I tested on Linux
> kernel
> 4.19.67, I encountered an "error: failed to set MSR 0x491 to 0x***".
>
> This issue is due to Linux kernel commit 27c42a1bb ("KVM: nVMX: Enable
> VMFUNC
> for the L1 hypervisor", 2017-08-03) exposing VMFUNC to the QEMU guest
> without
> corresponding VMFUNC MSR modification code, leading to an error when QEMU
> attempts
> to set the VMFUNC MSR. This bug affects kernels from 4.14 to 5.2, with a fix
> introduced
> in 5.3 by Paolo (e8a70bd4e "KVM: nVMX: allow setting the VMFUNC controls
> MSR", 2019-07-02).
It looks like this fix was not ported to the 4.19 stable kernel.
> So the fix for has_msr_vmx_vmfunc is clearly different from
> has_msr_vmx_procbased_ctls2.
> However, due to the different kernel support situations, I have not yet come
> up with a suitable
> way to handle the compatibility of has_msr_vmx_procbased_ctls2 across
> different kernel versions.
>
> Therefore, should we consider only fixing has_msr_vmx_procbased_ctls2 this
> time and addressing
> has_msr_vmx_vmfunc in a future patch when the timing is more appropriate?
>
I agree this fix should focus on MSR_IA32_VMX_PROCBASED_CTLS2.
But I think at least we need a comment (maybe a TODO) to note the case of
has_msr_vmx_vmfunc in a followup patch.
Let's wait and see what Paolo will say.
-Zhao
Re: [PATCH v3] target/i386/kvm: Refine VMX controls setting for backward compatibility
On Mon, Jun 24, 2024 at 05:58:06AM -0400, EwanHai wrote:
> Date: Mon, 24 Jun 2024 05:58:06 -0400
> From: EwanHai
> Subject: [PATCH v3] target/i386/kvm: Refine VMX controls setting for
> backward compatibility
> X-Mailer: git-send-email 2.34.1
>
> Commit 4a910e1 ("target/i386: do not set unsupported VMX secondary
> execution controls") implemented a workaround for hosts that have
> specific CPUID features but do not support the corresponding VMX
> controls, e.g., hosts support RDSEED but do not support RDSEED-Exiting.
>
> In detail, commit 4a910e1 introduced a flag `has_msr_vmx_procbased_clts2`.
> If KVM has `MSR_IA32_VMX_PROCBASED_CTLS2` in its msr list, QEMU would
> use KVM's settings, avoiding any modifications to this MSR.
>
> However, this commit (4a910e1) didn't account for cases in older Linux
> kernels(4.17~5.2) where `MSR_IA32_VMX_PROCBASED_CTLS2` is in
> `kvm_feature_msrs`-obtained by ioctl(KVM_GET_MSR_FEATURE_INDEX_LIST),
> but not in `kvm_msr_list`-obtained by ioctl(KVM_GET_MSR_INDEX_LIST).
> As a result,it did not set the `has_msr_vmx_procbased_clts2` flag based
> on `kvm_msr_list` alone, even though KVM does maintain the value of
> this MSR.
>
> This patch supplements the above logic, ensuring that
> `has_msr_vmx_procbased_clts2` is correctly set by checking both MSR
> lists, thus maintaining compatibility with older kernels.
>
> Signed-off-by: EwanHai
> ---
> Changes in v3:
> - Use a more precise version range in the comment, specifically "4.17~5.2"
> instead of "<5.3".
>
> Changes in v2:
> - Adjusted some punctuation in the commit message as per suggestions.
> - Added comments to the newly added code to indicate that it is a
> compatibility fix.
>
> v1 link:
> https://lore.kernel.org/all/20230925071453.14908-1-ewanhai...@zhaoxin.com/
>
> v2 link:
> https://lore.kernel.org/all/20231127034326.257596-1-ewanhai...@zhaoxin.com/
> ---
> target/i386/kvm/kvm.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c
> index 7ad8072748..a7c6c5b2d0 100644
> --- a/target/i386/kvm/kvm.c
> +++ b/target/i386/kvm/kvm.c
> @@ -2386,6 +2386,7 @@ void kvm_arch_do_init_vcpu(X86CPU *cpu)
> static int kvm_get_supported_feature_msrs(KVMState *s)
> {
> int ret = 0;
> +int i;
>
> if (kvm_feature_msrs != NULL) {
> return 0;
> @@ -2420,6 +2421,20 @@ static int kvm_get_supported_feature_msrs(KVMState *s)
> return ret;
> }
>
> + /*
> +* Compatibility fix:
> +* Older Linux kernels (4.17~5.2) report MSR_IA32_VMX_PROCBASED_CTLS2
> +* in KVM_GET_MSR_FEATURE_INDEX_LIST but not in KVM_GET_MSR_INDEX_LIST.
> +* This leads to an issue in older kernel versions where QEMU,
> +* through the KVM_GET_MSR_INDEX_LIST check, assumes the kernel
> +* doesn't maintain MSR_IA32_VMX_PROCBASED_CTLS2, resulting in
> +* incorrect settings by QEMU for this MSR.
> +*/
> +for (i = 0; i < kvm_feature_msrs->nmsrs; i++) {
nit: `i` could be declared here,
for (int i = 0; i < kvm_feature_msrs->nmsrs; i++) {
> +if (kvm_feature_msrs->indices[i] == MSR_IA32_VMX_PROCBASED_CTLS2) {
> +has_msr_vmx_procbased_ctls2 = true;
> +}
> +}
> return 0;
> }
>
> --
> 2.34.1
>
Since the minimum KVM version supported for i386 is v4.5 (docs/system/
target-i386.rst), this fix makes sense, so for this patch,
Reviewed-by: Zhao Liu
Additionally, has_msr_vmx_vmfunc has the similar compat issue. I think
it deserves a fix, too.
-Zhao
Re: [RFC PATCH v3 2/5] rust: add bindgen step as a meson dependency
Hi Manos,
Thanks for your patch. I have a few comments below:
> diff --git a/meson.build b/meson.build
> index 3533889852..2b305e745a 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3876,6 +3876,62 @@ foreach target : target_dirs
> lib_deps += dep.partial_dependency(compile_args: true, includes: true)
>endforeach
>
> + if with_rust and target_type == 'system'
> + # FIXME:
Just nit, FIXME looks a bit ambiguous, is it intended to fix the
following warning? It could be a bit more specific.
> + # > WARNING: Project specifies a minimum meson_version '>=0.63.0' but
> + # > uses features which were added in newer versions:
> + # > * 0.64.0: {'fs.copyfile'}
> + # > * 1.0.0: {'dependencies arg in rust.bindgen', 'module rust as
> stable module'}
> + rust_bindgen = import('rust')
> +
> + # We need one generated_rs target per target, so give them
> + # target-specific names.
> + copy = fs.copyfile('rust/wrapper.h',
> + target + '_wrapper.h')
> + generated_rs = rust_bindgen.bindgen(
> +input: copy,
> +dependencies: arch_deps + lib_deps,
> +output: target + '-generated.rs',
> +include_directories: include_directories('.', 'include'),
> +args: [
> + '--ctypes-prefix', 'core::ffi',
> + '--formatter', 'rustfmt',
> + '--generate-block',
> + '--generate-cstr',
> + '--impl-debug',
> + '--merge-extern-blocks',
> + '--no-doc-comments',
> + '--no-include-path-detection',
> + '--use-core',
> + '--with-derive-default',
> + '--allowlist-file', meson.project_source_root() + '/include/.*',
> + '--allowlist-file', meson.project_source_root() + '/.*',
> + '--allowlist-file', meson.project_build_root() + '/.*'
> +],
> + )
> +
> + if target in rust_targets
> +rust_hw = ss.source_set()
> +foreach t: rust_targets[target]
> + rust_device_cargo = custom_target(t['name'],
> + output: t['output'],
> + depends: [generated_rs],
> + build_always_stale: true,
> + command: t['command'])
> + rust_dep = declare_dependency(link_args: [
> + '-Wl,--whole-archive',
> + t['output-path'],
> + '-Wl,--no-whole-archive'
> + ],
> + sources: [rust_device_cargo])
> + rust_hw.add(rust_dep)
> +endforeach
> +rust_hw_config = rust_hw.apply(config_target, strict: false)
> +arch_srcs += rust_hw_config.sources()
> +arch_deps += rust_hw_config.dependencies()
> + endif
> + endif
> +
>lib = static_library('qemu-' + target,
> sources: arch_srcs + genh,
> dependencies: lib_deps,
> diff --git a/rust/.gitignore b/rust/.gitignore
> new file mode 100644
> index 00..1bf71b1f68
> --- /dev/null
> +++ b/rust/.gitignore
> @@ -0,0 +1,3 @@
> +# Ignore any cargo development build artifacts; for qemu-wide builds, all
> build
> +# artifacts will go to the meson build directory.
> +target
> diff --git a/rust/meson.build b/rust/meson.build
> new file mode 100644
> index 00..435abd3e1c
> --- /dev/null
> +++ b/rust/meson.build
> @@ -0,0 +1,129 @@
> +# Supported hosts
> +rust_supported_oses = {
> + 'linux': '-unknown-linux-gnu',
> + 'darwin': '-apple-darwin',
> + 'windows': '-pc-windows-gnu'
> +}
Does the current test cover windows and apple? If not, I think we can
add these two platforms later on.
> +rust_supported_cpus = ['x86_64', 'aarch64']
Similarly, here I have another question, x-pl011-rust in arm virt machine
I understand should only run on ARM platform, right? So compiling to
x86_64 doesn't seem necessary at the moment?
> +# Future-proof the above definitions against any change in the root
> meson.build file:
> +foreach rust_os: rust_supported_oses.keys()
> + if not supported_oses.contains(rust_os)
> +message()
> +warning('UNSUPPORTED OS VALUES IN ' + meson.current_source_dir() +
> '/meson.build')
> +message()
> +message('This meson.build file claims OS `+' + rust_os + '` is supported
> but')
> +message('it is not included in the global supported OSes list in')
> +message(meson.global_source_root() + '/meson.build.')
> + endif
> +endforeach
> +foreach rust_cpu: rust_supported_cpus
> + if not supported_cpus.contains(rust_cpu)
> +message()
> +warning('UNSUPPORTED CPU VALUES IN ' + meson.current_source_dir() +
> '/meson.build')
> +message()
> +message('This meson.build file claims CPU `+' + rust_cpu + '` is
> supported but')
> +message('it is not included in the global
Re: [RFC PATCH v3 1/5] build-sys: Add rust feature option
[snip]
> diff --git a/meson.build b/meson.build
> index c5360fbd299..ad7dbc0d641 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -290,6 +290,11 @@ foreach lang : all_languages
>endif
> endforeach
> +cargo = not_found
> +if 'RUST_TARGET_TRIPLE' in config_host
> + cargo = find_program('cargo', required: true)
> +endif
> +
As with the original Manos version, it looks like there's no need to
check cargo here? Since patch 2 checks cargo and others in rust/meson.build.
Otherwise, cargo was checked twice.
> # default flags for all hosts
> # We use -fwrapv to tell the compiler that we require a C dialect where
> # left shift of signed integers is well defined and has the expected
> @@ -4239,6 +4244,10 @@ if 'objc' in all_languages
> else
>summary_info += {'Objective-C compiler': false}
> endif
> +summary_info += {'Rust support': cargo.found()}
> +if cargo.found() and config_host['RUST_TARGET_TRIPLE']) !=
> config_host['RUST_HOST_TRIPLE']
> + summary_info += {'Rust target': config_host['RUST_TARGET_TRIPLE']}
> +endif
> option_cflags = (get_option('debug') ? ['-g'] : [])
> if get_option('optimization') != 'plain'
>option_cflags += ['-O' + get_option('optimization')]
>
>
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
Hi maintainers,
Per my communication with Markus, it seems this renaming matches the
"local consistency" principle in (include/hw/boards.h). :-)
So do you think this change is acceptable?
Thanks,
Zhao
On Mon, May 27, 2024 at 09:18:37PM +0800, Zhao Liu wrote:
> Date: Mon, 27 May 2024 21:18:37 +0800
> From: Zhao Liu
> Subject: [PATCH] hw/core: Rename CpuTopology to CPUTopology
> X-Mailer: git-send-email 2.34.1
>
> Use CPUTopology to honor the generic style of CPU capitalization
> abbreviations.
>
> Signed-off-by: Zhao Liu
> ---
> * Split from the previous SMP cache RFC:
>
> https://lore.kernel.org/qemu-devel/20240220092504.726064-2-zhao1@linux.intel.com/
> ---
> hw/s390x/cpu-topology.c | 6 +++---
> include/hw/boards.h | 8
> include/hw/s390x/cpu-topology.h | 6 +++---
> tests/unit/test-smp-parse.c | 14 +++---
> 4 files changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
> index f16bdf65faa0..016f6c1c15ac 100644
> --- a/hw/s390x/cpu-topology.c
> +++ b/hw/s390x/cpu-topology.c
> @@ -86,7 +86,7 @@ bool s390_has_topology(void)
> */
> static void s390_topology_init(MachineState *ms)
> {
> -CpuTopology *smp = >smp;
> +CPUTopology *smp = >smp;
>
> s390_topology.cores_per_socket = g_new0(uint8_t, smp->sockets *
> smp->books * smp->drawers);
> @@ -181,7 +181,7 @@ void s390_topology_reset(void)
> */
> static bool s390_topology_cpu_default(S390CPU *cpu, Error **errp)
> {
> -CpuTopology *smp = _machine->smp;
> +CPUTopology *smp = _machine->smp;
> CPUS390XState *env = >env;
>
> /* All geometry topology attributes must be set or all unset */
> @@ -234,7 +234,7 @@ static bool s390_topology_check(uint16_t socket_id,
> uint16_t book_id,
> uint16_t drawer_id, uint16_t entitlement,
> bool dedicated, Error **errp)
> {
> -CpuTopology *smp = _machine->smp;
> +CPUTopology *smp = _machine->smp;
>
> if (socket_id >= smp->sockets) {
> error_setg(errp, "Unavailable socket: %d", socket_id);
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index 2fa800f11ae4..c1737f2a5736 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -334,7 +334,7 @@ typedef struct DeviceMemoryState {
> } DeviceMemoryState;
>
> /**
> - * CpuTopology:
> + * CPUTopology:
> * @cpus: the number of present logical processors on the machine
> * @drawers: the number of drawers on the machine
> * @books: the number of books in one drawer
> @@ -346,7 +346,7 @@ typedef struct DeviceMemoryState {
> * @threads: the number of threads in one core
> * @max_cpus: the maximum number of logical processors on the machine
> */
> -typedef struct CpuTopology {
> +typedef struct CPUTopology {
> unsigned int cpus;
> unsigned int drawers;
> unsigned int books;
> @@ -357,7 +357,7 @@ typedef struct CpuTopology {
> unsigned int cores;
> unsigned int threads;
> unsigned int max_cpus;
> -} CpuTopology;
> +} CPUTopology;
>
> /**
> * MachineState:
> @@ -409,7 +409,7 @@ struct MachineState {
> const char *cpu_type;
> AccelState *accelerator;
> CPUArchIdList *possible_cpus;
> -CpuTopology smp;
> +CPUTopology smp;
> struct NVDIMMState *nvdimms_state;
> struct NumaState *numa_state;
> };
> diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
> index c064f427e948..ff09c57a4428 100644
> --- a/include/hw/s390x/cpu-topology.h
> +++ b/include/hw/s390x/cpu-topology.h
> @@ -63,17 +63,17 @@ static inline void s390_topology_reset(void)
>
> extern S390Topology s390_topology;
>
> -static inline int s390_std_socket(int n, CpuTopology *smp)
> +static inline int s390_std_socket(int n, CPUTopology *smp)
> {
> return (n / smp->cores) % smp->sockets;
> }
>
> -static inline int s390_std_book(int n, CpuTopology *smp)
> +static inline int s390_std_book(int n, CPUTopology *smp)
> {
> return (n / (smp->cores * smp->sockets)) % smp->books;
> }
>
> -static inline int s390_std_drawer(int n, CpuTopology *smp)
> +static inline int s390_std_drawer(int n, CPUTopology *smp)
> {
> return (n / (smp->cores * smp->sockets * smp->books)) % smp->drawers;
> }
> diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
> index 9fdba24fce56..f51138794ca1 100644
> --- a/tests/unit/test-smp-parse.c
> +++ b/tests/unit/test-s
[PATCH 1/3] target/i386/cpu: Use hex mask to check for valid cache CPUID leaf
Hexadecimal mask is more intuitive comparing to decimal.
Therefore convert the mask of bits 00-04 to hexadecimal value.
Signed-off-by: Zhao Liu
---
target/i386/cpu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 365852cb99e1..c4d4048ec32a 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6452,7 +6452,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
uint32_t count,
* QEMU has its own number of cores/logical cpus,
* set 24..14, 31..26 bit to configured values
*/
-if (*eax & 31) {
+if (*eax & 0x1f) {
int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
*eax &= ~0xFC00;
--
2.34.1
[PATCH 2/3] target/i386/cpu: Check guest_thread_ids_per_pkg for host-cache-info case
The CPUID[4].EAX[bits 25:14] encodes the "maximum number of addressable
IDs for logical processors", which value may be different with the
actual number of threads.
For example, there's a Guest with the topology like: 3 threads per core
and 3 cores per package. Its maximum ids for package level is 15 (0xf),
but it has 9 threads per package.
Therefore, using "threads_per_pkg" to check sharing threads overflow (out
of package scope) is not sufficient.
Use Guest's maximum ids for package level information to compare with
Host's.
Note the original check is stricter, but it can be mathematically proven
that the original check does not contain redundant case (e.g.
guest_thread_ids_per_pkg >= host_thread_ids_per_cache > threads_per_pkg,
which is impossible for the current QEMU APIC ID encoding rule).
Therefore, the behavior of this feature is consistent before and after
the change.
Signed-off-by: Zhao Liu
---
target/i386/cpu.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c4d4048ec32a..c20ff69b7b65 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6453,16 +6453,22 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
uint32_t count,
* set 24..14, 31..26 bit to configured values
*/
if (*eax & 0x1f) {
-int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
+int host_thread_ids_per_cache;
+int guest_thread_ids_per_pkg;
*eax &= ~0xFC00;
*eax |= max_core_ids_in_package(_info) << 26;
-if (host_vcpus_per_cache > threads_per_pkg) {
+
+host_thread_ids_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
+guest_thread_ids_per_pkg =
+max_thread_ids_for_cache(_info,
+ CPU_TOPO_LEVEL_PACKAGE);
+
+if (host_thread_ids_per_cache > guest_thread_ids_per_pkg) {
*eax &= ~0x3FFC000;
/* Share the cache at package level. */
-*eax |= max_thread_ids_for_cache(_info,
-CPU_TOPO_LEVEL_PACKAGE) << 14;
+*eax |= guest_thread_ids_per_pkg << 14;
}
}
} else if (cpu->vendor_cpuid_only && IS_AMD_CPU(env)) {
--
2.34.1
[PATCH 3/3] target/i386/cpu: Add comment about adjusting the Guest cache topo for host-cache-info
The host-cache-info needs the check to ensure the valid maximum
addressable thread IDs.
We don't need to adjust the information in this one field for all cache
topology cases by default, even though Host's cache topology may not
correspond to Guest's CPU topology level.
For example, when a Geust (3 threads per core) runs on a Host with 1
threads per core, the L2 cache topo (L2 per core on Host) obtained by
Guest does not correspond to the Guest's core level. So for the case
where the topology of Guest and Host are very inconsistent, it is not
possible to do a perfect job, so we try to let the Guest have similar
cache topo info as Host, at least in the case of an even distribution
of vCPUs, which can benefit the Guest internal scheduling.
To this end, add a comment to explain why we need to care for this check
and why we don't need to adjust the topology for all cache cases.
Signed-off-by: Zhao Liu
---
target/i386/cpu.c | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index c20ff69b7b65..71300ac6d197 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6463,7 +6463,15 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
uint32_t count,
guest_thread_ids_per_pkg =
max_thread_ids_for_cache(_info,
CPU_TOPO_LEVEL_PACKAGE);
-
+/*
+ * We handle this case because it causes sharing threads to
+ * overflow out of the package scope. In other cases, there
+ * is no need to adjust the cache topology info for the Guest,
+ * as the Host's maximum addressable thread IDs are not out of
+ * bounds in the Guest's APIC ID scope, and are always valid,
+ * even though Host's cache topology may not correspond to
+ * Guest's CPU topology level.
+ */
if (host_thread_ids_per_cache > guest_thread_ids_per_pkg) {
*eax &= ~0x3FFC000;
--
2.34.1
[PATCH 0/3] target/i386/cpu: Misc Cleanup on host-cache-info
Hi, This series is mainly to addresss Igor's comment about if one check in host-cache-info could be removed [1], i.e., whether Guest's cache topology should be self-consistent (able to correspond to Guest's CPU topology level, as we currently do with the Guest cache topo). I originally thought (in the mail thread with Igor) that host-cache-info should allow Guest and Host to have the same topology level information, e.g. if Host shares cache on core level, then via host-cache-info, Guest should also share on core level. But in practice, I gave up on this idea, because in the cache info passthrough case, it should be possible for Guest to get the original Host cache info (including the original threads sharing cache) without further modifying the info to Guest. Therefore, I simply added the comment in PATCH 3 to hopefully illustrate the need for such a check. Hope my explanation is clear enough so that my poor English doesn't bother you! [1]: https://lore.kernel.org/qemu-devel/20240527170317.14520...@imammedo.users.ipa.redhat.com/ Thanks and Best Regards, Zhao --- Zhao Liu (3): target/i386/cpu: Use hex mask to check for valid cache CPUID leaf target/i386/cpu: Check guest_thread_ids_per_pkg for host-cache-info case target/i386/cpu: Add comment about adjusting the Guest cache topo for host-cache-info target/i386/cpu.c | 24 +++- 1 file changed, 19 insertions(+), 5 deletions(-) -- 2.34.1
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
Hi Paolo, On Thu, Jun 13, 2024 at 09:56:39AM +0200, Paolo Bonzini wrote: > Date: Thu, 13 Jun 2024 09:56:39 +0200 > From: Paolo Bonzini > Subject: Re: [RFC PATCH v2 3/5] rust: add PL011 device model > > Il gio 13 giu 2024, 09:13 Daniel P. Berrangé ha > scritto: > > > On Wed, Jun 12, 2024 at 11:27:04PM +0200, Paolo Bonzini wrote: > > > Il mer 12 giu 2024, 22:58 Manos Pitsidianakis < > > > manos.pitsidiana...@linaro.org> ha scritto: > > > > > > > In any case, it is out of scope for this RFC. Introducing wrappers > > would > > > > be a gradual process. > > > > > > > > > > Sure, how would you feel about such bindings being developed on list, and > > > maintained in a (somewhat) long-lived experimental branch? > > > > IMHO any higher level binding APIs for Rust should be acceptable in the > > main QEMU tree as soon as we accept Rust functionality. They can evolve > > in-tree based on the needs of whomever is creating and/or consuming them. > > > > My question is the opposite, should we accept Rust functionality without > proper high level bindings? I am afraid that, if more Rust devices are > contributed, it becomes technical debt to have a mix of idiomatic and C-ish > code. If the answer is no, then this PL011 device has to be developed out > of tree. I think deeper and higher level bindings will have more opens and will likely require more discussion and exploration. So could we explore this direction on another reference Rust device? I also think there won’t be too many Rust devices in the short term. Going back to tweak or enhance existing Rust devices may not require too much effort, once we have a definitive answer. I wonder if x86 could also implement a rust device (like the x86 timer you mentioned before, hw/i386/kvm/i8254.c or hw/timer/i8254.c IIRC) to try this? Or would you recommend another x86 device? :-) I'd be willing to help Manos try it if you think it's fine. Regards, Zhao
Re: [RFC PATCH v2 3/5] rust: add PL011 device model
On Tue, Jun 11, 2024 at 01:33:32PM +0300, Manos Pitsidianakis wrote:
> Date: Tue, 11 Jun 2024 13:33:32 +0300
> From: Manos Pitsidianakis
> Subject: [RFC PATCH v2 3/5] rust: add PL011 device model
> X-Mailer: git-send-email 2.44.0
>
> This commit adds a re-implementation of hw/char/pl011.c in Rust.
>
> It uses generated Rust bindings (produced by `ninja
> aarch64-softmmu-generated.rs`) to
> register itself as a QOM type/class.
>
> How to build:
>
> 1. Make sure rust, cargo and bindgen (cargo install bindgen-cli) are
>installed
> 2. Configure a QEMU build with:
>--enable-system --target-list=aarch64-softmmu --enable-with-rust
> 3. Launching a VM with qemu-system-aarch64 should use the Rust version
>of the pl011 device (unless it is not set up so in hw/arm/virt.c; the
>type of the UART device is hardcoded).
>
>To confirm, inspect `info qom-tree` in the monitor and look for an
>`x-pl011-rust` device.
>
> Signed-off-by: Manos Pitsidianakis
> ---
Hi Manos,
Thanks for your example!
> diff --git a/rust/pl011/Cargo.toml b/rust/pl011/Cargo.toml
> new file mode 100644
> index 00..db74f2b59f
> --- /dev/null
> +++ b/rust/pl011/Cargo.toml
> @@ -0,0 +1,66 @@
...
> +[lints]
> +[lints.rustdoc]
> +broken_intra_doc_links = "deny"
> +redundant_explicit_links = "deny"
> +[lints.clippy]
> +# lint groups
> +correctness = { level = "deny", priority = -1 }
> +suspicious = { level = "deny", priority = -1 }
> +complexity = { level = "deny", priority = -1 }
> +perf = { level = "deny", priority = -1 }
> +cargo = { level = "deny", priority = -1 }
> +nursery = { level = "deny", priority = -1 }
> +style = { level = "deny", priority = -1 }
> +# restriction group
> +dbg_macro = "deny"
> +rc_buffer = "deny"
> +as_underscore = "deny"
> +assertions_on_result_states = "deny"
> +# pedantic group
> +doc_markdown = "deny"
> +expect_fun_call = "deny"
> +borrow_as_ptr = "deny"
> +case_sensitive_file_extension_comparisons = "deny"
> +cast_lossless = "deny"
> +cast_ptr_alignment = "allow"
> +large_futures = "deny"
> +waker_clone_wake = "deny"
> +unused_enumerate_index = "deny"
> +unnecessary_fallible_conversions = "deny"
> +struct_field_names = "deny"
> +manual_hash_one = "deny"
> +into_iter_without_iter = "deny"
> +option_if_let_else = "deny"
> +missing_const_for_fn = "deny"
> +significant_drop_tightening = "deny"
> +multiple_crate_versions = "deny"
> +significant_drop_in_scrutinee = "deny"
> +cognitive_complexity = "deny"
> +missing_safety_doc = "allow"
...
> diff --git a/rust/pl011/rustfmt.toml b/rust/pl011/rustfmt.toml
> new file mode 12
> index 00..39f97b043b
> --- /dev/null
> +++ b/rust/pl011/rustfmt.toml
> @@ -0,0 +1 @@
> +../rustfmt.toml
...
> diff --git a/rust/rustfmt.toml b/rust/rustfmt.toml
> new file mode 100644
> index 00..ebecb99fe0
> --- /dev/null
> +++ b/rust/rustfmt.toml
> @@ -0,0 +1,7 @@
> +edition = "2021"
> +format_generated_files = false
> +format_code_in_doc_comments = true
> +format_strings = true
> +imports_granularity = "Crate"
> +group_imports = "StdExternalCrate"
> +wrap_comments = true
>
About the Rust style, inspired from the discussion on my previous
simpletrace-rust [1], it looks like people prefer the default rust style
and use the default check without custom configurations.
More style requirements are also an open, especially for unstable ones,
and it would be better to split this part into a separate patch, so that
the discussion about style doesn't overshadow the focus on your example.
[1]: https://lore.kernel.org/qemu-devel/zlnbgwk29ds9f...@redhat.com/
Regards,
Zhao
Re: [PATCH 4/4] i386/cpu: Add support for EPYC-Turin model
On Wed, Jun 12, 2024 at 02:12:20PM -0500, Babu Moger wrote: > Date: Wed, 12 Jun 2024 14:12:20 -0500 > From: Babu Moger > Subject: [PATCH 4/4] i386/cpu: Add support for EPYC-Turin model > X-Mailer: git-send-email 2.34.1 > > Adds the support for AMD EPYC zen 5 processors(EPYC-Turin). nit s/Adds/Add > Adds the following new feature bits on top of the feature bits from s/Adds/Add/ > the previous generation EPYC models. > > movdiri: Move Doubleword as Direct Store Instruction > movdir64b : Move 64 Bytes as Direct Store Instruction > avx512-vp2intersect: AVX512 Vector Pair Intersection to a Pair > of Mask Register > avx-vnni : AVX VNNI Instruction > > Signed-off-by: Babu Moger > --- > target/i386/cpu.c | 131 ++ > 1 file changed, 131 insertions(+) Reviewed-by: Zhao Liu
Re: [PATCH 3/4] i386/cpu: Enable perfmon-v2 and RAS feature bits on EPYC-Genoa
On Wed, Jun 12, 2024 at 02:12:19PM -0500, Babu Moger wrote: > Date: Wed, 12 Jun 2024 14:12:19 -0500 > From: Babu Moger > Subject: [PATCH 3/4] i386/cpu: Enable perfmon-v2 and RAS feature bits on > EPYC-Genoa > X-Mailer: git-send-email 2.34.1 > > Following feature bits are added on EPYC-Genoa-v2 model. > > perfmon-v2: Allows guests to make use of the PerfMonV2 features. nit s/Allows/Allow/ > SUCCOR: Software uncorrectable error containment and recovery capability. > The processor supports software containment of uncorrectable > errors > through context synchronizing data poisoning and deferred error > interrupts. > > McaOverflowRecov: MCA overflow recovery support. > > The feature details are available in APM listed below [1]. > [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming > Publication # 24593 Revision 3.41. > > Signed-off-by: Babu Moger > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 > --- > target/i386/cpu.c | 15 +++ > 1 file changed, 15 insertions(+) Reviewed-by: Zhao Liu
Re: [PATCH 2/4] i386/cpu: Add PerfMonV2 feature bit
On Wed, Jun 12, 2024 at 02:12:18PM -0500, Babu Moger wrote:
> Date: Wed, 12 Jun 2024 14:12:18 -0500
> From: Babu Moger
> Subject: [PATCH 2/4] i386/cpu: Add PerfMonV2 feature bit
> X-Mailer: git-send-email 2.34.1
>
> From: Sandipan Das
>
> CPUID leaf 0x8022, i.e. ExtPerfMonAndDbg, advertises new performance
> monitoring features for AMD processors. Bit 0 of EAX indicates support
> for Performance Monitoring Version 2 (PerfMonV2) features. If found to
> be set during PMU initialization, the EBX bits can be used to determine
> the number of available counters for different PMUs. It also denotes the
> availability of global control and status registers.
>
> Add the required CPUID feature word and feature bit to allow guests to
> make use of the PerfMonV2 features.
>
> Signed-off-by: Sandipan Das
> Signed-off-by: Babu Moger
> ---
> target/i386/cpu.c | 26 ++
> target/i386/cpu.h | 4
> 2 files changed, 30 insertions(+)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 86a90b1405..7f1837cdc9 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -1228,6 +1228,22 @@ FeatureWordInfo feature_word_info[FEATURE_WORDS] = {
> .tcg_features = 0,
> .unmigratable_flags = 0,
> },
> +[FEAT_8000_0022_EAX] = {
> +.type = CPUID_FEATURE_WORD,
> +.feat_names = {
> +"perfmon-v2", NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +NULL, NULL, NULL, NULL,
> +},
> +.cpuid = { .eax = 0x8022, .reg = R_EAX, },
> +.tcg_features = 0,
> +.unmigratable_flags = 0,
> +},
> [FEAT_XSAVE] = {
> .type = CPUID_FEATURE_WORD,
> .feat_names = {
> @@ -6998,6 +7014,16 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> *edx = 0;
> }
> break;
> +case 0x8022:
> +*eax = *ebx = *ecx = *edx = 0;
> +/* AMD Extended Performance Monitoring and Debug */
> +if (kvm_enabled() && cpu->enable_pmu &&
> +(env->features[FEAT_8000_0022_EAX] &
> CPUID_8000_0022_EAX_PERFMON_V2)) {
> +*eax = CPUID_8000_0022_EAX_PERFMON_V2;
> +*ebx = kvm_arch_get_supported_cpuid(cs->kvm_state, index, count,
> +R_EBX) & 0xf;
Although only EAX[bit 0] and EBX[bits 0-3] are supported right now, I
think it's better to use “|=” rather than just override the
original *eax and *ebx, which will prevent future mistakes or omissions.
Otherwise,
Reviewed-by: Zhao Liu
Re: [PATCH 1/4] i386/cpu: Add RAS feature bits on EPYC CPU models
On Wed, Jun 12, 2024 at 02:12:17PM -0500, Babu Moger wrote: > Date: Wed, 12 Jun 2024 14:12:17 -0500 > From: Babu Moger > Subject: [PATCH 1/4] i386/cpu: Add RAS feature bits on EPYC CPU models > X-Mailer: git-send-email 2.34.1 > > Add the support for following RAS features bits on AMD guests. > > SUCCOR: Software uncorrectable error containment and recovery capability. > The processor supports software containment of uncorrectable errors > through context synchronizing data poisoning and deferred error > interrupts. > > McaOverflowRecov: MCA overflow recovery support. > > Signed-off-by: Babu Moger > --- > target/i386/cpu.c | 30 ++ > 1 file changed, 30 insertions(+) Reviewed-by: Zhao Liu
Re: [RFC PATCH v1 0/6] Implement ARM PL011 in Rust
On Tue, Jun 11, 2024 at 01:41:57PM +0300, Manos Pitsidianakis wrote: > Date: Tue, 11 Jun 2024 13:41:57 +0300 > From: Manos Pitsidianakis > Subject: Re: [RFC PATCH v1 0/6] Implement ARM PL011 in Rust > > > Currently, pl011 exclusively occupies a cargo as a package. In the > > future, will other Rust implementations utilize the workspace mechanism > > to act as a second package in the same cargo? Or will new cargo be created > > again? > > What do you mean by "new cargo"? I didn't catch that :( > > A workspace would make sense if we have "general" crate libraries that > hardware crates depend on. Thanks Manos! I mean if we spread the rust device across the QEMU submodules, wouldn't we have to create their own cargo directories (aka single-package cargo) for each rust device? However, if the Rust code is all centralized under the /Rust directory, then it can be managed by multiple-packages in cargo workspace. About the "general" crate, I'm not sure whether a base lib to manage external crates is a good idea, like I replied in [1]. [1]: https://lore.kernel.org/qemu-devel/CAJSP0QWLe6yPDE3rPztx=os0g+vkt9w3gykrnu0eqzcaw06...@mail.gmail.com/T/#mfaf9abf06ed82dd7f8ce5e7520bbb4447083b550 > > > > Under a unified Rust directory, using a workspace to manage multiple > > packages looks as if it would be easier to maintain. Decentralized to an > > existing directory, they're all separate cargos, and external dependencies > > tend to become fragmented? > > Hmm potentially yes, but that's a "what if" scenario. Let's worry about that > bridge when we cross it! > Yes!
Re: [RFC PATCH v1 0/6] Implement ARM PL011 in Rust
On Tue, Jun 11, 2024 at 09:18:25AM +0100, Daniel P. Berrangé wrote: > Date: Tue, 11 Jun 2024 09:18:25 +0100 > From: "Daniel P. Berrangé" > Subject: Re: [RFC PATCH v1 0/6] Implement ARM PL011 in Rust > > On Mon, Jun 10, 2024 at 09:22:35PM +0300, Manos Pitsidianakis wrote: > > What are the issues with not using the compiler, rustc, directly? > > - > > [whataretheissueswith] Back to [TOC] > > > > 1. Tooling > >Mostly writing up the build-sys tooling to do so. Ideally we'd > >compile everything without cargo but rustc directly. > > > >If we decide we need Rust's `std` library support, we could > >investigate whether building it from scratch is a good solution. This > >will only build the bits we need in our devices. > > Re-building 'std' for QEMU would be a no-go for many distros who > will expect QEMU to use the distro provided 'std' package. So at > most that would have to be an optional feature. > > > 2. Rust dependencies > >We could go without them completely. I chose deliberately to include > >one dependency in my UART implementation, `bilge`[0], because it has > >an elegant way of representing typed bitfields for the UART's > >registers. > > > > [0]: Article: https://hecatia-elegua.github.io/blog/no-more-bit-fiddling/ > > Crates.io page: https://crates.io/crates/bilge > > Repository: https://github.com/hecatia-elegua/bilge > > > > Should QEMU use third-party dependencies? > > - > > [shouldqemuusethirdparty] Back to [TOC] > > > > In my personal opinion, if we need a dependency we need a strong > > argument for it. A dependency needs a trusted upstream source, a QEMU > > maintainer to make sure it us up-to-date in QEMU etc. > > "strong" is a rather fuzzy term. In C we already have a huge number > of build dependencies > > $ wc -l tests/lcitool/projects/qemu.yml > 127 tests/lcitool/projects/qemu.yml > > we would have many more than that except that we're conservative > about adding deps on things because getting new libraries into > distros is quite painful, or we lag behind where we would want > to be to stick with compat for old distro versions. > > In terms of Rust dependancies, I'd expect us to have fairly arbitrary > dependancies used. If the dep avoids QEMU maintainers having to > re-invent the wheel for something there is already a common crate > for, then it is a good thing to use it. I'd almost go as far as > encouraging use of external crates. Our maintainers should focus tmie > on writing code that's delivers compelling features to QEMU, rather > than re-creating common APIs that already have good crates. So should a base lib be introduced to import and wrap all external dependencies? Sort of like osdep.h, so that specific Rust implementations can't import external third-party libraries directly, but only through the base lib. The advantage of this is that we can unify the management of external dependencies and avoid “potentially/overly arbitrary” importing of specific Rust implementations.
Re: [RFC PATCH v1 0/6] Implement ARM PL011 in Rust
On Tue, Jun 11, 2024 at 09:22:44AM +0100, Daniel P. Berrangé wrote: > Date: Tue, 11 Jun 2024 09:22:44 +0100 > From: "Daniel P. Berrangé" > Subject: Re: [RFC PATCH v1 0/6] Implement ARM PL011 in Rust > > On Mon, Jun 10, 2024 at 09:22:35PM +0300, Manos Pitsidianakis wrote: > > Hello everyone, > > > > This is an early draft of my work on implementing a very simple device, > > in this case the ARM PL011 (which in C code resides in hw/char/pl011.c > > and is used in hw/arm/virt.c). > > looking at the diffstat: > > > .gitignore | 2 + > > .gitlab-ci.d/buildtest.yml | 64 ++-- > > configure | 12 + > > hw/arm/virt.c | 2 +- > > meson.build| 99 ++ > > meson_options.txt | 4 + > > rust/meson.build | 93 ++ > > rust/pl011/.cargo/config.toml | 2 + > > rust/pl011/.gitignore | 2 + > > rust/pl011/Cargo.lock | 120 +++ > > rust/pl011/Cargo.toml | 26 ++ > > rust/pl011/README.md | 42 +++ > > rust/pl011/build.rs| 44 +++ > > rust/pl011/meson.build | 7 + > > rust/pl011/rustfmt.toml| 10 + > > rust/pl011/src/definitions.rs | 95 ++ > > rust/pl011/src/device.rs | 531 ++ > > rust/pl011/src/device_class.rs | 95 ++ > > rust/pl011/src/generated.rs| 5 + > > rust/pl011/src/lib.rs | 575 + > > rust/pl011/src/memory_ops.rs | 38 +++ > > My thought is that if we're going to start implementing devices > or other parts of QEMU, in Rust, then I do not want to see it > placed in a completely separate directory sub-tree. > > In this example, I would expect to have hw/arm/pl011.rs, or hw/arm/pl011/*.rs > so that the device is part of the normal Arm hardware directory structure > and maintainer assignments. It has its advantages. Otherwise, as the number of Rust implementations grows, the same mirror directory as QEMU will have to be rebuilt again in the Rust directory. Further, putting C implementations in the same directory, there is again the question of why it needs to be duplicated :-) . This topic is probably also beyond the scope of this RFC, but it's nice to have a Rust example to start with. Currently, pl011 exclusively occupies a cargo as a package. In the future, will other Rust implementations utilize the workspace mechanism to act as a second package in the same cargo? Or will new cargo be created again? Under a unified Rust directory, using a workspace to manage multiple packages looks as if it would be easier to maintain. Decentralized to an existing directory, they're all separate cargos, and external dependencies tend to become fragmented?
Re: [PATCH v3] i386/cpu: fixup number of addressable IDs for processor cores in the physical package
On Tue, Jun 11, 2024 at 11:23:14AM +0800, Chuang Xu wrote:
> Date: Tue, 11 Jun 2024 11:23:14 +0800
> From: Chuang Xu
> Subject: [PATCH v3] i386/cpu: fixup number of addressable IDs for processor
> cores in the physical package
> X-Mailer: git-send-email 2.24.3 (Apple Git-128)
>
> When QEMU is started with:
> -cpu host,host-cache-info=on,l3-cache=off \
> -smp 2,sockets=1,dies=1,cores=1,threads=2
> Guest can't acquire maximum number of addressable IDs for processor cores in
> the physical package from CPUID[04H].
>
> When creating a CPU topology of 1 core per package, host-cache-info only
> uses the Host's addressable core IDs field (CPUID.04H.EAX[bits 31-26]),
> resulting in a conflict (on the multicore Host) between the Guest core
> topology information in this field and the Guest's actual cores number.
>
> Fix it by removing the unnecessary condition to cover 1 core per package
> case. This is safe because cores_per_pkg will not be 0 and will be at
> least 1.
>
> Fixes: d7caf13b5fcf ("x86: cpu: fixup number of addressable IDs for logical
> processors sharing cache")
> Signed-off-by: Guixiong Wei
> Signed-off-by: Yipeng Yin
> Signed-off-by: Chuang Xu
> ---
> target/i386/cpu.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
Thanks! LGTM,
Reviewed-by: Zhao Liu
Re: [PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC
On Fri, Jun 07, 2024 at 03:47:01PM +0800, Xiaoyao Li wrote: > Date: Fri, 7 Jun 2024 15:47:01 +0800 > From: Xiaoyao Li > Subject: Re: [PATCH] i386/apic: Add hint on boot failure because of > disabling x2APIC > > On 6/7/2024 3:46 PM, Zhao Liu wrote: > > Hi Philippe, > > > > On Fri, Jun 07, 2024 at 08:17:36AM +0200, Philippe Mathieu-Daudé wrote: > > > Date: Fri, 7 Jun 2024 08:17:36 +0200 > > > From: Philippe Mathieu-Daudé > > > Subject: Re: [PATCH] i386/apic: Add hint on boot failure because of > > > disabling x2APIC > > > > > > On 6/6/24 16:08, Zhao Liu wrote: > > > > Currently, the Q35 supports up to 4096 vCPUs (since v9.0), but for TCG > > > > cases, if x2APIC is not actively enabled to boot more than 255 vCPUs ( > > why bother mentioning TCG case? I'm restating the problem scenario for this error message. This is a common use case, which is enough to support adding a simple hint. > you below example is not for TCG. Not? > And the issue is not caused by TCG but > default cpu model doesn't have X2APIC enabled. This is not an issue, and it is normal to report an error in current QEMU. > > > > e.g., qemu-system-i386 -M pc-q35-9.0 -smp 666), the following error is > > > > reported: > > > > > > > > Unexpected error in apic_common_set_id() at > > > > ../hw/intc/apic_common.c:449: > > > > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU > > > > Aborted (core dumped) > > > > > > > > This error can be resolved by setting x2apic=on in -cpu. In order to > > > > better help users deal with this scenario, add the error hint to > > > > instruct users on how to enable the x2apic feature. > > > > > > Why not automatically set x2apic=on in this case instead? > > > > The default CPU model is qemu64 without x2APIC. I think it might be > > necessary to update the version to add x2APIC in qemu64, and I'd like to > > look into it again for any other potential issues. > > > > In addition to adding x2APIC directly to the qemu64, this hint can also > > help some users who want a large number of vCPUs based on other older > > CPU models. Though, it's not very common but this hint would be helpful. > > > > > > Then, the error > > > > report becomes the following: > > > > > > > > Unexpected error in apic_common_set_id() at > > > > ../hw/intc/apic_common.c:448: > > > > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU > > > > Try x2apic=on in -cpu. > > > > Aborted (core dumped) > > > > > > > > Note since @errp is _abort, error_append_hint() can't be applied > > > > on @errp. And in order to separate the exact error message from the > > > > (perhaps effectively) hint, adding a hint via error_append_hint() is > > > > also necessary. Therefore, introduce @local_error in > > > > apic_common_set_id() to handle both the error message and the error > > > > hint. > > > > > > > > Suggested-by: Philippe Mathieu-Daudé > > > > Signed-off-by: Zhao Liu > > > > --- > > > >hw/intc/apic_common.c | 7 ++- > > > >1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > Reviewed-by: Philippe Mathieu-Daudé > > > > Thanks! > > > > >
Re: [PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC
Hi Philippe, On Fri, Jun 07, 2024 at 08:17:36AM +0200, Philippe Mathieu-Daudé wrote: > Date: Fri, 7 Jun 2024 08:17:36 +0200 > From: Philippe Mathieu-Daudé > Subject: Re: [PATCH] i386/apic: Add hint on boot failure because of > disabling x2APIC > > On 6/6/24 16:08, Zhao Liu wrote: > > Currently, the Q35 supports up to 4096 vCPUs (since v9.0), but for TCG > > cases, if x2APIC is not actively enabled to boot more than 255 vCPUs ( > > e.g., qemu-system-i386 -M pc-q35-9.0 -smp 666), the following error is > > reported: > > > > Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:449: > > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU > > Aborted (core dumped) > > > > This error can be resolved by setting x2apic=on in -cpu. In order to > > better help users deal with this scenario, add the error hint to > > instruct users on how to enable the x2apic feature. > > Why not automatically set x2apic=on in this case instead? The default CPU model is qemu64 without x2APIC. I think it might be necessary to update the version to add x2APIC in qemu64, and I'd like to look into it again for any other potential issues. In addition to adding x2APIC directly to the qemu64, this hint can also help some users who want a large number of vCPUs based on other older CPU models. Though, it's not very common but this hint would be helpful. > > Then, the error > > report becomes the following: > > > > Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:448: > > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU > > Try x2apic=on in -cpu. > > Aborted (core dumped) > > > > Note since @errp is _abort, error_append_hint() can't be applied > > on @errp. And in order to separate the exact error message from the > > (perhaps effectively) hint, adding a hint via error_append_hint() is > > also necessary. Therefore, introduce @local_error in > > apic_common_set_id() to handle both the error message and the error > > hint. > > > > Suggested-by: Philippe Mathieu-Daudé > > Signed-off-by: Zhao Liu > > --- > > hw/intc/apic_common.c | 7 ++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > Reviewed-by: Philippe Mathieu-Daudé Thanks!
Re: [PATCH] tracetool: Remove unused vcpu.py script
On Thu, Jun 06, 2024 at 12:26:31PM +0200, Philippe Mathieu-Daudé wrote:
> Date: Thu, 6 Jun 2024 12:26:31 +0200
> From: Philippe Mathieu-Daudé
> Subject: [PATCH] tracetool: Remove unused vcpu.py script
> X-Mailer: git-send-email 2.41.0
>
> vcpu.py is pointless since commit 89aafcf2a7 ("trace:
> remove code that depends on setting vcpu"), remote it.
>
> Signed-off-by: Philippe Mathieu-Daudé
> ---
> meson.build | 1 -
> scripts/tracetool/__init__.py | 8 +
> scripts/tracetool/vcpu.py | 59 ---
> 3 files changed, 1 insertion(+), 67 deletions(-)
> delete mode 100644 scripts/tracetool/vcpu.py
Reviewed-by: Zhao Liu
[PATCH] i386/apic: Add hint on boot failure because of disabling x2APIC
Currently, the Q35 supports up to 4096 vCPUs (since v9.0), but for TCG
cases, if x2APIC is not actively enabled to boot more than 255 vCPUs (
e.g., qemu-system-i386 -M pc-q35-9.0 -smp 666), the following error is
reported:
Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:449:
qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
Aborted (core dumped)
This error can be resolved by setting x2apic=on in -cpu. In order to
better help users deal with this scenario, add the error hint to
instruct users on how to enable the x2apic feature. Then, the error
report becomes the following:
Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:448:
qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
Try x2apic=on in -cpu.
Aborted (core dumped)
Note since @errp is _abort, error_append_hint() can't be applied
on @errp. And in order to separate the exact error message from the
(perhaps effectively) hint, adding a hint via error_append_hint() is
also necessary. Therefore, introduce @local_error in
apic_common_set_id() to handle both the error message and the error
hint.
Suggested-by: Philippe Mathieu-Daudé
Signed-off-by: Zhao Liu
---
hw/intc/apic_common.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index d8fc1e2815fe..c13cdd79943d 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -433,6 +433,7 @@ static void apic_common_set_id(Object *obj, Visitor *v,
const char *name,
APICCommonState *s = APIC_COMMON(obj);
DeviceState *dev = DEVICE(obj);
uint32_t value;
+Error *local_err = NULL;
if (dev->realized) {
qdev_prop_set_after_realize(dev, name, errp);
@@ -444,7 +445,11 @@ static void apic_common_set_id(Object *obj, Visitor *v,
const char *name,
}
if (value >= 255 && !cpu_has_x2apic_feature(>cpu->env)) {
-error_setg(errp, "APIC ID %d requires x2APIC feature in CPU", value);
+error_setg(_err,
+ "APIC ID %d requires x2APIC feature in CPU",
+ value);
+error_append_hint(_err, "Try x2apic=on in -cpu.\n");
+error_propagate(errp, local_err);
return;
}
--
2.34.1
Re: [PATCH v2 0/6] target/i386: Misc cleanup on KVM PV defs and outdated comments
Hi Paolo, Just a ping for this cleanup series. Thanks, Zhao On Mon, May 06, 2024 at 04:51:47PM +0800, Zhao Liu wrote: > Date: Mon, 6 May 2024 16:51:47 +0800 > From: Zhao Liu > Subject: [PATCH v2 0/6] target/i386: Misc cleanup on KVM PV defs and > outdated comments > X-Mailer: git-send-email 2.34.1 > > Hi, > > This is my v2 cleanup series. Compared with v1 [1], only tags (R/b, S/b) > updates, and a typo fix, no code change. > > This series picks cleanup from my previous kvmclock [2] (as other > renaming attempts were temporarily put on hold). > > In addition, this series also include the cleanup on a historically > workaround and recent comment of coco interface [3]. > > Avoiding the fragmentation of these misc cleanups, I consolidated them > all in one series and was able to tackle them in one go! > > [1]: > https://lore.kernel.org/qemu-devel/20240426100716.2111688-1-zhao1@intel.com/ > [2]: > https://lore.kernel.org/qemu-devel/20240329101954.3954987-1-zhao1@linux.intel.com/ > [3]: > https://lore.kernel.org/qemu-devel/2815f0f1-9e20-4985-849c-d74c6cdc9...@intel.com/ > > Thanks and Best Regards, > Zhao > --- > Zhao Liu (6): > target/i386/kvm: Add feature bit definitions for KVM CPUID > target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and > MSR_KVM_SYSTEM_TIME definitions > target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled > target/i386/kvm: Save/load MSRs of kvmclock2 > (KVM_FEATURE_CLOCKSOURCE2) > target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo > target/i386/confidential-guest: Fix comment of > x86_confidential_guest_kvm_type() > > hw/i386/kvm/clock.c | 5 +-- > target/i386/confidential-guest.h | 2 +- > target/i386/cpu.h| 25 + > target/i386/kvm/kvm.c| 63 +++- > 4 files changed, 66 insertions(+), 29 deletions(-) > > -- > 2.34.1 >
[PATCH] docs: i386: pc: Avoid mentioning limit of maximum vCPUs
Different versions of PC machine support different maximum vCPUs, and even different features have limits on the maximum number of vCPUs ( For example, if x2apic is not enabled in the TCG case, the maximum of 255 vCPUs are supported). It is difficult to list the maximum vCPUs under all restrictions. Thus, to avoid confusion, avoid mentioning specific maximum vCPU number limitations here. Suggested-by: Daniel P. Berrangé Signed-off-by: Zhao Liu --- docs/system/target-i386-desc.rst.inc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/docs/system/target-i386-desc.rst.inc b/docs/system/target-i386-desc.rst.inc index 319e540573d3..ae312b1c1e61 100644 --- a/docs/system/target-i386-desc.rst.inc +++ b/docs/system/target-i386-desc.rst.inc @@ -36,7 +36,8 @@ The QEMU PC System emulator simulates the following peripherals: - PCI UHCI, OHCI, EHCI or XHCI USB controller and a virtual USB-1.1 hub. -SMP is supported with up to 255 CPUs (and 4096 CPUs for PC Q35 machine). +SMP is supported with a large number of virtual CPUs (upper limit is +configuration dependent). QEMU uses the PC BIOS from the Seabios project and the Plex86/Bochs LGPL VGA BIOS. -- 2.34.1
Re: [PATCH] stubs/meson: Fix qemuutil build when --disable-system
On Thu, Jun 06, 2024 at 09:41:47AM +0200, Paolo Bonzini wrote:
> Date: Thu, 6 Jun 2024 09:41:47 +0200
> From: Paolo Bonzini
> Subject: Re: [PATCH] stubs/meson: Fix qemuutil build when --disable-system
>
> On 6/5/24 17:25, Zhao Liu wrote:
> > Compiling without system, user, tools or guest-agent fails with the
> > following error message:
> >
> > ./configure --disable-system --disable-user --disable-tools \
> > --disable-guest-agent
> >
> > error message:
> >
> > /usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function
> > `error_printf':
> > /media/liuzhao/data/qemu-cook/build/../util/error-report.c:38: undefined
> > reference to `error_vprintf'
> > /usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function `vreport':
> > /media/liuzhao/data/qemu-cook/build/../util/error-report.c:215: undefined
> > reference to `error_vprintf'
> > collect2: error: ld returned 1 exit status
> >
> > This is because tests/bench and tests/unit both need qemuutil, which
> > requires error_vprintf stub when system is disabled.
> >
> > Add error_vprintf stub into stub_ss for all cases other than disabling
> > system.
>
> Should be "other than enabled system emulation", but...
>
> > -if have_ga
> > - stub_ss.add(files('error-printf.c'))
> > -endif
> > -
> > if have_block or have_user
> > stub_ss.add(files('qtest.c'))
> > stub_ss.add(files('vm-stop.c'))
> > stub_ss.add(files('vmstate.c'))
> > -
> > - # more symbols provided by the monitor
> > - stub_ss.add(files('error-printf.c'))
> > endif
>
> ... these should be left in, since it's possible to build with
> --enable-guest-agent --enable-system.
>
> The best and easiest solution is simply to move error-printf.c to the
> unconditional section at the top of the file. I queued the patch with that
> change.
Thanks!! I can delete my v2 branch now. :-)
Re: [PATCH] target/i386: SEV: do not assume machine->cgs is SEV
On Thu, Jun 06, 2024 at 12:44:09AM +0200, Paolo Bonzini wrote:
> Date: Thu, 6 Jun 2024 00:44:09 +0200
> From: Paolo Bonzini
> Subject: [PATCH] target/i386: SEV: do not assume machine->cgs is SEV
> X-Mailer: git-send-email 2.45.1
>
> There can be other confidential computing classes that are not derived
> from sev-common. Avoid aborting when encountering them.
>
> Signed-off-by: Paolo Bonzini
> ---
> target/i386/sev.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 004c667ac14..97e15f8b7a9 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -1710,7 +1710,9 @@ void sev_es_set_reset_vector(CPUState *cpu)
> {
> X86CPU *x86;
> CPUX86State *env;
> -SevCommonState *sev_common =
> SEV_COMMON(MACHINE(qdev_get_machine())->cgs);
> +ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs;
> +SevCommonState *sev_common = SEV_COMMON(
> +object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON));
SEV_COMMON(object_dynamic_cast()) looks to be twice cast, we can just
force to do conversion with pointer type:
(SevCommonState *) object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON)
> /* Only update if we have valid reset information */
> if (!sev_common || !sev_common->reset_data_valid) {
> --
> 2.45.1
>
>
Re: [PATCH] stubs/meson: Fix qemuutil build when --disable-system
On Wed, Jun 05, 2024 at 11:25:49PM +0800, Zhao Liu wrote:
> Date: Wed, 5 Jun 2024 23:25:49 +0800
> From: Zhao Liu
> Subject: [PATCH] stubs/meson: Fix qemuutil build when --disable-system
> X-Mailer: git-send-email 2.34.1
>
> Compiling without system, user, tools or guest-agent fails with the
> following error message:
>
> ./configure --disable-system --disable-user --disable-tools \
> --disable-guest-agent
>
> error message:
>
> /usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function
> `error_printf':
> /media/liuzhao/data/qemu-cook/build/../util/error-report.c:38: undefined
> reference to `error_vprintf'
> /usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function `vreport':
> /media/liuzhao/data/qemu-cook/build/../util/error-report.c:215: undefined
> reference to `error_vprintf'
> collect2: error: ld returned 1 exit status
>
> This is because tests/bench and tests/unit both need qemuutil, which
> requires error_vprintf stub when system is disabled.
>
> Add error_vprintf stub into stub_ss for all cases other than disabling
> system.
>
> Fixes: 3a15604900c4 ("stubs: include stubs only if needed")
> Reported-by: Daniel P. Berrangé
> Signed-off-by: Zhao Liu
> ---
> stubs/meson.build | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/stubs/meson.build b/stubs/meson.build
> index 3b9d42023cb2..a99522ab6bbf 100644
> --- a/stubs/meson.build
> +++ b/stubs/meson.build
> @@ -45,17 +45,10 @@ if have_block or have_ga
>stub_ss.add(files('qmp-quit.c'))
> endif
>
> -if have_ga
> - stub_ss.add(files('error-printf.c'))
> -endif
> -
> if have_block or have_user
>stub_ss.add(files('qtest.c'))
>stub_ss.add(files('vm-stop.c'))
>stub_ss.add(files('vmstate.c'))
> -
> - # more symbols provided by the monitor
> - stub_ss.add(files('error-printf.c'))
> endif
>
> if have_user
> @@ -76,6 +69,9 @@ if have_system
>stub_ss.add(files('target-monitor-defs.c'))
>stub_ss.add(files('win32-kbd-hook.c'))
>stub_ss.add(files('xen-hw-stub.c'))
> +else
> + # more symbols provided by the monitor
> + stub_ss.add(files('error-printf.c'))
> endif
Oops, it's not a correct fix. error-printf.c should still be added
unconditionally.
>
> if have_system or have_user
> --
> 2.34.1
>
[PATCH] stubs/meson: Fix qemuutil build when --disable-system
Compiling without system, user, tools or guest-agent fails with the
following error message:
./configure --disable-system --disable-user --disable-tools \
--disable-guest-agent
error message:
/usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function `error_printf':
/media/liuzhao/data/qemu-cook/build/../util/error-report.c:38: undefined
reference to `error_vprintf'
/usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function `vreport':
/media/liuzhao/data/qemu-cook/build/../util/error-report.c:215: undefined
reference to `error_vprintf'
collect2: error: ld returned 1 exit status
This is because tests/bench and tests/unit both need qemuutil, which
requires error_vprintf stub when system is disabled.
Add error_vprintf stub into stub_ss for all cases other than disabling
system.
Fixes: 3a15604900c4 ("stubs: include stubs only if needed")
Reported-by: Daniel P. Berrangé
Signed-off-by: Zhao Liu
---
stubs/meson.build | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/stubs/meson.build b/stubs/meson.build
index 3b9d42023cb2..a99522ab6bbf 100644
--- a/stubs/meson.build
+++ b/stubs/meson.build
@@ -45,17 +45,10 @@ if have_block or have_ga
stub_ss.add(files('qmp-quit.c'))
endif
-if have_ga
- stub_ss.add(files('error-printf.c'))
-endif
-
if have_block or have_user
stub_ss.add(files('qtest.c'))
stub_ss.add(files('vm-stop.c'))
stub_ss.add(files('vmstate.c'))
-
- # more symbols provided by the monitor
- stub_ss.add(files('error-printf.c'))
endif
if have_user
@@ -76,6 +69,9 @@ if have_system
stub_ss.add(files('target-monitor-defs.c'))
stub_ss.add(files('win32-kbd-hook.c'))
stub_ss.add(files('xen-hw-stub.c'))
+else
+ # more symbols provided by the monitor
+ stub_ss.add(files('error-printf.c'))
endif
if have_system or have_user
--
2.34.1
Re: [PULL 17/63] stubs: include stubs only if needed
On Tue, Jun 04, 2024 at 11:07:30AM +0100, Daniel P. Berrangé wrote:
> Date: Tue, 4 Jun 2024 11:07:30 +0100
> From: "Daniel P. Berrangé"
> Subject: Re: [PULL 17/63] stubs: include stubs only if needed
>
> On Tue, Apr 23, 2024 at 05:09:05PM +0200, Paolo Bonzini wrote:
> > Currently it is not documented anywhere why some functions need to
> > be stubbed.
> >
> > Group the files in stubs/meson.build according to who needs them, both
> > to reduce the size of the compilation and to clarify the use of stubs.
> >
> > Signed-off-by: Paolo Bonzini
> > Message-ID: <20240408155330.522792-18-pbonz...@redhat.com>
> > Signed-off-by: Paolo Bonzini
> > ---
> > stubs/{monitor.c => monitor-internal.c} | 0
> > stubs/meson.build | 122 +++-
> > 2 files changed, 75 insertions(+), 47 deletions(-)
> > rename stubs/{monitor.c => monitor-internal.c} (100%)
>
> This change breaks the build of many tests in the following situation:
>
> ./configure --disable-system --disable-user --disable-tools
>
> $ makej
> changing dir to build for make ""...
> make[1]: Entering directory '/var/home/berrange/src/virt/qemu/build'
> [1/46] Generating qemu-version.h with a custom command (wrapped by meson to
> capture output)
> [2/46] Linking target tests/bench/qht-bench
> FAILED: tests/bench/qht-bench
> cc -m64 -o tests/bench/qht-bench tests/bench/qht-bench.p/qht-bench.c.o
> -Wl,--as-needed -Wl,--no-undefined -pie -Wl,--whole-archive
> libevent-loop-base.fa libqom.fa -Wl,--no-whole-archive
> -fstack-protector-strong -Wl,-z,relro -Wl,-z,now -Wl,--start-group
> libqemuutil.a libevent-loop-base.fa libqom.fa -lm -pthread
> /usr/lib64/libglib-2.0.so /usr/lib64/libgmodule-2.0.so -Wl,--end-group
> /usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function
> `error_printf':
> /var/home/berrange/src/virt/qemu/build/../util/error-report.c:38:(.text+0x93):
> undefined reference to `error_vprintf'
> /usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function `vreport':
> /var/home/berrange/src/virt/qemu/build/../util/error-report.c:225:(.text+0x195):
> undefined reference to `error_vprintf'
> collect2: error: ld returned 1 exit status
> [3/46] Linking target tests/unit/check-qdict
> FAILED: tests/unit/check-qdict
> cc -m64 -o tests/unit/check-qdict tests/unit/check-qdict.p/check-qdict.c.o
> -Wl,--as-needed -Wl,--no-undefined -pie -Wl,--whole-archive
> libevent-loop-base.fa libqom.fa -Wl,--no-whole-archive
> -fstack-protector-strong -Wl,-z,relro -Wl,-z,now -Wl,--start-group
> libqemuutil.a libevent-loop-base.fa libqom.fa -lm -pthread
> /usr/lib64/libglib-2.0.so /usr/lib64/libgmodule-2.0.so -Wl,--end-group
> /usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function
> `error_printf':
> /var/home/berrange/src/virt/qemu/build/../util/error-report.c:38:(.text+0x93):
> undefined reference to `error_vprintf'
> /usr/bin/ld: libqemuutil.a.p/util_error-report.c.o: in function `vreport':
> /var/home/berrange/src/virt/qemu/build/../util/error-report.c:225:(.text+0x195):
> undefined reference to `error_vprintf'
> collect2: error: ld returned 1 exit status
> [4/46] Linking target tests/unit/check-qstring
> FAILED: tests/unit/check-qstring
> ...snip many more similar errors...
>
error-printf.c should be a common file in stub_ss, since bench test and
unit test both need qemuutil.
It seems the related previous fix 109f1a437f99 ("stubs: Add missing qga
stubs") is not a complete fix.
Re: [PATCH-for-9.0?] docs: i386: pc: Update maximum CPU numbers for PC Q35
[snip] > > > $ qemu-system-i386 -M pc-q35-9.0 -smp 666 > > > Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:447: > > > qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU > > > Abort trap: 6 > > > > For tcg, it needs to set x2apic=on in -cpu. > > Thanks for clarifying. Using error_append_hint() is certainly > better than aborting or asking on the mailing list (from user > perspective) ;) Good idea! Will improve this. ;-)
Re: [RFC v2 3/7] hw/core: Add cache topology options in -smp
Hi Markus and Daniel,
(I replied to both of you because I discussed the idea of cache object
at the end)
On Tue, Jun 04, 2024 at 10:32:14AM +0100, Daniel P. Berrangé wrote:
> Date: Tue, 4 Jun 2024 10:32:14 +0100
> From: "Daniel P. Berrangé"
> Subject: Re: [RFC v2 3/7] hw/core: Add cache topology options in -smp
>
> On Tue, Jun 04, 2024 at 10:54:51AM +0200, Markus Armbruster wrote:
> > Zhao Liu writes:
> >
> > > Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in
> > > -smp to define the cache topology for SMP system.
> > >
> > > Signed-off-by: Zhao Liu
> >
> > [...]
> >
> > > diff --git a/qapi/machine.json b/qapi/machine.json
> > > index 7ac5a05bb9c9..8fa5af69b1bf 100644
> > > --- a/qapi/machine.json
> > > +++ b/qapi/machine.json
> > > @@ -1746,6 +1746,23 @@
> > > #
> > > # @threads: number of threads per core
> > > #
> > > +# @l1d-cache: topology hierarchy of L1 data cache. It accepts the CPU
> > > +# topology enumeration as the parameter, i.e., CPUs in the same
> > > +# topology container share the same L1 data cache. (since 9.1)
> > > +#
> > > +# @l1i-cache: topology hierarchy of L1 instruction cache. It accepts
> > > +# the CPU topology enumeration as the parameter, i.e., CPUs in the
> > > +# same topology container share the same L1 instruction cache.
> > > +# (since 9.1)
> > > +#
> > > +# @l2-cache: topology hierarchy of L2 unified cache. It accepts the CPU
> > > +# topology enumeration as the parameter, i.e., CPUs in the same
> > > +# topology container share the same L2 unified cache. (since 9.1)
> > > +#
> > > +# @l3-cache: topology hierarchy of L3 unified cache. It accepts the CPU
> > > +# topology enumeration as the parameter, i.e., CPUs in the same
> > > +# topology container share the same L3 unified cache. (since 9.1)
> > > +#
> > > # Since: 6.1
> > > ##
> >
> > The new members are all optional. What does "absent" mean? No such
> > cache? Some default topology?
I suppose "absent" means using the the default arch-specific cache topo.
For example, x86 has its own default cache topo. my purpose in providing
this interface is to allow users to override the default (arch-specific)
cache topology.
Daniel suggested in cover letter's reply that I could add the value
“default”, I think omitting it would have the same effect as setting it
to “default”, and omitting it shouldn't be regarded as disabling a
particular cache, since the interface is only about the topology!
> > Is this sufficiently general? Do all machines of interest have a split
> > level 1 cache, a level 2 cache, and a level 3 cache?
The different cache support can be extended by such control fields in
MachineClass:
typedef struct {
...
bool l1_separated_cache_supported;
bool l2_unified_cache_supported;
bool l3_unified_cache_supported;
} SMPCompatProps;
For example, if a machine with l1 unified cache appears, it can add the
"l1_unified_cache_supported", and add "l1" option in QAPI.
Then separate L1 cache has “l1i” and “l1d” options, and unified L1 cache
should have “l1” option.
> Level 4 cache is apparently a thing
>
> https://www.guru3d.com/story/intel-confirms-l4-cache-in-upcoming-meteor-lake-cpus/
>
> but given that any new cache levels will require new code in QEMU to
> wire up, its not a big deal to add new properties at the same time.
>
> That said see my reply just now to the cover letter, where I suggest
> we should have a "caches" property that takes an array of cache
> info objects.
Yes, I agree.
> >
> > Is the CPU topology level the only cache property we'll want to
> > configure here? If the answer isn't "yes", then we should perhaps wrap
> > it in an object, so we can easily add more members later.
>
> Cache size is a piece of info I could see us wanting to express
For the simplicity and clarity, I think it's better to only cover
topology in -smp, including the current CPU topology and the cache
topology I'm working on.
I've thought about the idea of converting the cache into the device,
which in turn could define more properties for the cache.
This one is mostly in connection with the previous qom-topo idea (aka
abstracting CPU topology device [1]):
-device cpu-socket,id=sock0 \
-device cpu-die,id=die0,parent=sock0 \
-device cpu-core,id=core0,parent=die0,nr-threads=2 \
-device cpu-cache,id=cache0,parent=core0,level=l1,type=inst \
-device cpu-cache,id=cache1,parent=core0,level=l1,type
Re: [RFC v2 0/7] Introduce SMP Cache Topology
Hi Daniel,
On Tue, Jun 04, 2024 at 10:29:15AM +0100, Daniel P. Berrangé wrote:
> Date: Tue, 4 Jun 2024 10:29:15 +0100
> From: "Daniel P. Berrangé"
> Subject: Re: [RFC v2 0/7] Introduce SMP Cache Topology
>
> On Thu, May 30, 2024 at 06:15:32PM +0800, Zhao Liu wrote:
> > Hi,
> >
> > Now that the i386 cache model has been able to define the topology
> > clearly, it's time to move on to discussing/advancing this feature about
> > configuring the cache topology with -smp as the following example:
> >
> > -smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32,\
> > l1d-cache=core,l1i-cache=core,l2-cache=core,l3-cache=die
> >
> > With the new cache topology options ("l1d-cache", "l1i-cache",
> > "l2-cache" and "l3-cache"), we could adjust the cache topology via -smp.
>
> Switching to QAPI for a second, your proposal is effectively
>
> { 'enum': 'SMPCacheTopo',
> 'data': [ 'default','socket','die','cluster','module','core','thread'] }
>
>{ 'struct': 'SMPConfiguration',
> 'data': {
>'*cpus': 'int',
>'*drawers': 'int',
>'*books': 'int',
>'*sockets': 'int',
>'*dies': 'int',
>'*clusters': 'int',
>'*modules': 'int',
>'*cores': 'int',
>'*threads': 'int',
>'*maxcpus': 'int',
>'*l1d-cache': 'SMPCacheTopo',
>'*l1i-cache': 'SMPCacheTopo',
>'*l2-cache': 'SMPCacheTopo',
>'*l3-cache': 'SMPCacheTopo',
> } }
>
> I think that would be more natural to express as an array of structs
> thus:
>
> { 'enum': 'SMPCacheTopo',
> 'data': [ 'default','socket','die','cluster','module','core','thread'] }
>
> { 'enum': 'SMPCacheType',
> 'data': [ 'l1d', 'l1i', 'l2', 'l3' ] }
>
> { 'struct': 'SMPCache',
> 'data': {
> 'type': 'SMPCacheType',
> 'topo': 'SMPCacheTopo',
> } }
>
>{ 'struct': 'SMPConfiguration',
> 'data': {
>'*cpus': 'int',
>'*drawers': 'int',
>'*books': 'int',
>'*sockets': 'int',
>'*dies': 'int',
>'*clusters': 'int',
>'*modules': 'int',
>'*cores': 'int',
>'*threads': 'int',
>'*maxcpus': 'int',
>'caches': [ 'SMPCache' ]
> } }
>
> Giving an example in (hypothetical) JSON cli syntax of:
>
> -smp "{'cpus':32,'sockets':2,'dies':2,'modules':2,
> 'cores':2,'threads':2,'maxcpus':32,'caches': [
> {'type':'l1d','topo':'core' },
> {'type':'l1i','topo':'core' },
> {'type':'l2','topo':'core' },
> {'type':'l3','topo':'die' },
> ]}"
Thanks! Looks clean to me and I think it is ok.
Just one further question, for this case where it must be expressed in a
raw JSON string, is there any requirement in QEMU that a simple
command-line friendly variant must be provided that corresponds to it?
> > Open about How to Handle the Default Options
> >
> >
> > (For the detailed description of this series, pls skip this "long"
> > section and review the subsequent content.)
> >
> >
> > Background of OPEN
> > --
> >
> > Daniel and I discussed initial thoughts on cache topology, and there was
> > an idea that the default *cache_topo is on the CORE level [3]:
> >
> > > simply preferring "cores" for everything is a reasonable
> > > default long term plan for everything, unless the specific
> > > architecture target has no concept of "cores".
>
> FYI, when I wrote that I wasn't specifically thinking about cache
> mappings. I just meant that when exposing SMP topology to guests,
> 'cores' is a good default, compared to 'sockets', or 'threads',etc.
>
> Defaults for cache <-> topology mappings should be whatever makes
> most sense to the architecture target/cpu.
Thank you for the additional clarification!
> > Problem with this OPEN
> > --
> >
> > Some arches have their own arch-specific cache topology, such as l1 per
> > core/l2 per core/l3 per die for i386. And as Jeehang proposed for
> > RISC-V, the cache topologies are like: l1/l2 per core and l3 per
> > cluster.
> >
> > Taking L3 as an example, logically there is a difference between the two
> > starting points of user-specified core level and with the default core
> > level.
> >
> > For example,
> >
> > "(user-specified) l3-cache-topo=core" should override i3
Re: [PATCH v2] i386/cpu: fixup number of addressable IDs for processor cores in the physical package
Hi Chuang,
On Mon, Jun 03, 2024 at 04:36:41PM +0800, Chuang Xu wrote:
> Date: Mon, 3 Jun 2024 16:36:41 +0800
> From: Chuang Xu
> Subject: [PATCH v2] i386/cpu: fixup number of addressable IDs for processor
> cores in the physical package
> X-Mailer: git-send-email 2.24.3 (Apple Git-128)
>
> When QEMU is started with:
> -cpu host,host-cache-info=on,l3-cache=off \
> -smp 2,sockets=1,dies=1,cores=1,threads=2
> Guest can't acquire maximum number of addressable IDs for processor cores in
> the physical package from CPUID[04H].
>
> When testing Intel TDX, guest attempts to acquire extended topology from
> CPUID[0bH],
> but because the TDX module doesn't provide the emulation of CPUID[0bH],
> guest will instead acquire extended topology from CPUID[04H]. However,
> due to QEMU's inaccurate emulation of CPUID[04H], one of the vcpus in 2c TDX
> guest would be offline.
I guess this case is based on downstream's TDX patches... Since TDX
hasn't landed in QEMU yet, it's a bit ahead of the curve to elaborate on
TDX-specific case.
Because normal VM will also face the such cache topology error, I think
it could be stated a bit more generically like:
When creating a CPU topology of 1 core per package, host-cache-info only
uses the Host's addressable core IDs field (CPUID.04H.EAX[bits 31-26]),
resulting in a conflict (on the multicore Host) between the Guest core
topology information in this field and the Guest's actual cores number.
Fix it by removing the unnecessary condition to cover 1 core per package
case. This is safe because cores_per_pkg will not be 0 and will be at
least 1.
> Fix it by removing the unnecessary condition.
>
> Fixes: d7caf13b5fcf742e5680c1d3448ba070fc811644 ("x86: cpu: fixup number of
> addressable IDs for logical processors sharing cache")
12 characters (d7caf13b5fcf) is enough. No blank line. ;-)
> Signed-off-by: Guixiong Wei
> Signed-off-by: Yipeng Yin
> Signed-off-by: Chuang Xu
> ---
> target/i386/cpu.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index bc2dceb647..b68f7460db 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6426,10 +6426,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> if (*eax & 31) {
> int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
>
> -if (cores_per_pkg > 1) {
> -*eax &= ~0xFC00;
> -*eax |= max_core_ids_in_package(_info) << 26;
> -}
> +*eax &= ~0xFC00;
> +*eax |= max_core_ids_in_package(_info) << 26;
> if (host_vcpus_per_cache > threads_per_pkg) {
> *eax &= ~0x3FFC000;
>
> --
> 2.20.1
>
Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic
On Tue, Jun 04, 2024 at 10:47:44AM +0200, Markus Armbruster wrote: > Date: Tue, 04 Jun 2024 10:47:44 +0200 > From: Markus Armbruster > Subject: Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration > arch-agnostic > > Zhao Liu writes: > > > Hi Markus, > > > > On Mon, Jun 03, 2024 at 02:25:36PM +0200, Markus Armbruster wrote: > >> Date: Mon, 03 Jun 2024 14:25:36 +0200 > >> From: Markus Armbruster > >> Subject: Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration > >> arch-agnostic > >> > >> Zhao Liu writes: > >> > >> > Cache topology needs to be defined based on CPU topology levels. Thus, > >> > define CPU topology enumeration in qapi/machine.json to make it generic > >> > for all architectures. > >> > > >> > To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT > >> > and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and > >> > CPU_TOPO_LEVEL_SOCKET. > >> > > >> > Also, enumerate additional topology levels for non-i386 arches, and add > >> > helpers for topology enumeration and string conversion. > >> > > >> > Signed-off-by: Zhao Liu > >> > >> [...] > >> > >> > diff --git a/qapi/machine.json b/qapi/machine.json > >> > index bce6e1bbc412..7ac5a05bb9c9 100644 > >> > --- a/qapi/machine.json > >> > +++ b/qapi/machine.json > >> > @@ -1667,6 +1667,46 @@ > >> > '*reboot-timeout': 'int', > >> > '*strict': 'bool' } } > >> > > >> > +## > >> > +# @CPUTopoLevel: > >> > >> I understand you're moving existing enum CPUTopoLevel into the QAPI > >> schema. I think the idiomatic QAPI name would be CpuTopologyLevel. > >> Would you be willing to rename it, or would that be too much churn? > > > > Sure, I'll rename it as you suggested. > > > >> > +# > >> > +# An enumeration of CPU topology levels. > >> > +# > >> > +# @invalid: Invalid topology level, used as a placeholder. > >> > +# > >> > +# @thread: thread level, which would also be called SMT level or logical > >> > +# processor level. The @threads option in -smp is used to configure > >> > +# the topology of this level. > >> > +# > >> > +# @core: core level. The @cores option in -smp is used to configure the > >> > +# topology of this level. > >> > +# > >> > +# @module: module level. The @modules option in -smp is used to > >> > +# configure the topology of this level. > >> > +# > >> > +# @cluster: cluster level. The @clusters option in -smp is used to > >> > +# configure the topology of this level. > >> > +# > >> > +# @die: die level. The @dies option in -smp is used to configure the > >> > +# topology of this level. > >> > +# > >> > +# @socket: socket level, which would also be called package level. The > >> > +# @sockets option in -smp is used to configure the topology of this > >> > +# level. > >> > +# > >> > +# @book: book level. The @books option in -smp is used to configure the > >> > +# topology of this level. > >> > +# > >> > +# @drawer: drawer level. The @drawers option in -smp is used to > >> > +# configure the topology of this level. > >> > >> docs/devel/qapi-code-gen.rst section Documentation markup: > >> > >> For legibility, wrap text paragraphs so every line is at most 70 > >> characters long. > >> > >> Separate sentences with two spaces. > > > > Thank you for pointing this. > > > > About separating sentences, is this what I should be doing? > > > > # @drawer: drawer level. The @drawers option in -smp is used to > > # configure the topology of this level. > > Like this: > > ## > # @CPUTopoLevel: > # > # An enumeration of CPU topology levels. > # > # @invalid: Invalid topology level. > # > # @thread: thread level, which would also be called SMT level or > # logical processor level. The @threads option in -smp is used to > # configure the topology of this level. > # > # @core: core level. The @cores option in -smp is used to configure > # the topology of this level. > # > # @module: module level. The @modules option in -smp is used to > # configure the topology of this level. > # > # @cluster: cluster level. The @clusters option in -smp is used to > # configure the topology of this level. > # > # @die: die level. The @dies option in -smp is used to configure the > # topology of this level. > # > # @socket: socket level, which would also be called package level. > # The @sockets option in -smp is used to configure the topology of > # this level. > # > # @book: book level. The @books option in -smp is used to configure > # the topology of this level. > # > # @drawer: drawer level. The @drawers option in -smp is used to > # configure the topology of this level. > # > # Since: 9.1 > ## > I see, thanks very much for your patience.
Re: [PATCH-for-9.0?] docs: i386: pc: Update maximum CPU numbers for PC Q35
Hi Michael, Daniel, and Philippe,
On Mon, Jun 03, 2024 at 06:39:19PM +0100, Daniel P. Berrangé wrote:
> Date: Mon, 3 Jun 2024 18:39:19 +0100
> From: "Daniel P. Berrangé"
> Subject: Re: [PATCH-for-9.0?] docs: i386: pc: Update maximum CPU numbers
> for PC Q35
>
> On Mon, Jun 03, 2024 at 07:31:47PM +0200, Philippe Mathieu-Daudé wrote:
> > Hi Michael,
> >
> > On 2/6/24 15:30, Michael S. Tsirkin wrote:
> > > On Fri, Apr 12, 2024 at 11:57:35AM +0200, Philippe Mathieu-Daudé wrote:
> > > > Hi Zhao,
> > > >
> > > > On 12/4/24 10:53, Zhao Liu wrote:
> > > > > From: Zhao Liu
> > > > >
> > > > > Commit e4e98c7eebfa ("pc: q35: Bump max_cpus to 4096 vcpus") increases
> > > > > the supported CPUs for PC Q35 machine.
> > > > >
> > > > > Update maximum CPU numbers for PC Q35 in the document.
> > > > >
> > > > > Signed-off-by: Zhao Liu
> > > > > ---
> > > > >docs/system/target-i386-desc.rst.inc | 2 +-
> > > > >1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/docs/system/target-i386-desc.rst.inc
> > > > > b/docs/system/target-i386-desc.rst.inc
> > > > > index 5ebbcda9db4c..319e540573d3 100644
> > > > > --- a/docs/system/target-i386-desc.rst.inc
> > > > > +++ b/docs/system/target-i386-desc.rst.inc
> > > > > @@ -36,7 +36,7 @@ The QEMU PC System emulator simulates the following
> > > > > peripherals:
> > > > >- PCI UHCI, OHCI, EHCI or XHCI USB controller and a virtual
> > > > > USB-1.1
> > > > > hub.
> > > > > -SMP is supported with up to 255 CPUs.
> > > > > +SMP is supported with up to 255 CPUs (and 4096 CPUs for PC Q35
> > > > > machine).
> > > >
> > > > This comment is not accurate since a while, IIUC:
> > > >
> > > > Up to q35-2.7: 255
> > > > q35-2.8: 288
> > > > q35-8.0+: 1024
> > > > q35-9.0: 4096
> > >
> > >
> > > What are you saying here, Philippe? I don't think compat
> > > machine types matter enough to bother with more detail.
> >
> > My point is I find this description confusing w.r.t. how QEMU behaves:
>
> snip
>
> I think the docs should simply avoid mentioning any limit at all. ie
>
>-SMP is supported with up to 255 CPUs.
>+SMP is supported with a large number of virtual CPUs (upper limit is
> configuration dependent)
I agree, the limit may also be different for different scenarios, and it
is not appropriate to describe it in detail.
Sorry I forgot that this patch has been merged in by Thomas for me
(838f82468a12), and I can modify it with a new patch based on Daniel's
suggestion.
Thanks,
Zhao
Re: [PATCH-for-9.0?] docs: i386: pc: Update maximum CPU numbers for PC Q35
On Mon, Jun 03, 2024 at 07:31:47PM +0200, Philippe Mathieu-Daudé wrote:
> Date: Mon, 3 Jun 2024 19:31:47 +0200
> From: Philippe Mathieu-Daudé
> Subject: Re: [PATCH-for-9.0?] docs: i386: pc: Update maximum CPU numbers
> for PC Q35
>
> Hi Michael,
>
> On 2/6/24 15:30, Michael S. Tsirkin wrote:
> > On Fri, Apr 12, 2024 at 11:57:35AM +0200, Philippe Mathieu-Daudé wrote:
> > > Hi Zhao,
> > >
> > > On 12/4/24 10:53, Zhao Liu wrote:
> > > > From: Zhao Liu
> > > >
> > > > Commit e4e98c7eebfa ("pc: q35: Bump max_cpus to 4096 vcpus") increases
> > > > the supported CPUs for PC Q35 machine.
> > > >
> > > > Update maximum CPU numbers for PC Q35 in the document.
> > > >
> > > > Signed-off-by: Zhao Liu
> > > > ---
> > > >docs/system/target-i386-desc.rst.inc | 2 +-
> > > >1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/docs/system/target-i386-desc.rst.inc
> > > > b/docs/system/target-i386-desc.rst.inc
> > > > index 5ebbcda9db4c..319e540573d3 100644
> > > > --- a/docs/system/target-i386-desc.rst.inc
> > > > +++ b/docs/system/target-i386-desc.rst.inc
> > > > @@ -36,7 +36,7 @@ The QEMU PC System emulator simulates the following
> > > > peripherals:
> > > >- PCI UHCI, OHCI, EHCI or XHCI USB controller and a virtual USB-1.1
> > > > hub.
> > > > -SMP is supported with up to 255 CPUs.
> > > > +SMP is supported with up to 255 CPUs (and 4096 CPUs for PC Q35
> > > > machine).
> > >
> > > This comment is not accurate since a while, IIUC:
> > >
> > > Up to q35-2.7: 255
> > > q35-2.8: 288
> > > q35-8.0+: 1024
> > > q35-9.0: 4096
> >
> >
> > What are you saying here, Philippe? I don't think compat
> > machine types matter enough to bother with more detail.
>
> My point is I find this description confusing w.r.t. how QEMU behaves:
>
> $ qemu-system-i386 -M pc-q35-2.8 -smp 666
> qemu-system-i386: Invalid SMP CPUs 666. The max CPUs supported by machine
> 'pc-q35-2.8' is 288
>
> $ qemu-system-i386 -M pc-q35-8.0 -smp 666
> qemu-system-i386: Invalid SMP CPUs 666. The max CPUs supported by machine
> 'pc-q35-8.0' is 288
>
> $ qemu-system-i386 -M pc-q35-9.0 -smp 666
> Unexpected error in apic_common_set_id() at ../hw/intc/apic_common.c:447:
> qemu-system-i386: APIC ID 255 requires x2APIC feature in CPU
> Abort trap: 6
For tcg, it needs to set x2apic=on in -cpu.
Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic
[snip]
> > +CPUTopoInfo cpu_topo_descriptors[] = {
> > +[CPU_TOPO_LEVEL_INVALID] = { .name = "invalid", },
> > +[CPU_TOPO_LEVEL_THREAD] = { .name = "thread", },
> > +[CPU_TOPO_LEVEL_CORE]= { .name = "core",},
> > +[CPU_TOPO_LEVEL_MODULE] = { .name = "module", },
> > +[CPU_TOPO_LEVEL_CLUSTER] = { .name = "cluster", },
> > +[CPU_TOPO_LEVEL_DIE] = { .name = "die", },
> > +[CPU_TOPO_LEVEL_SOCKET] = { .name = "socket", },
> > +[CPU_TOPO_LEVEL_BOOK]= { .name = "book",},
> > +[CPU_TOPO_LEVEL_DRAWER] = { .name = "drawer", },
> > +[CPU_TOPO_LEVEL__MAX]= { .name = NULL, },
> > +};
>
> This looks redundant with generated
>
> const QEnumLookup CPUTopoLevel_lookup = {
> .array = (const char *const[]) {
> [CPU_TOPO_LEVEL_INVALID] = "invalid",
> [CPU_TOPO_LEVEL_THREAD] = "thread",
> [CPU_TOPO_LEVEL_CORE] = "core",
> [CPU_TOPO_LEVEL_MODULE] = "module",
> [CPU_TOPO_LEVEL_CLUSTER] = "cluster",
> [CPU_TOPO_LEVEL_DIE] = "die",
> [CPU_TOPO_LEVEL_SOCKET] = "socket",
> [CPU_TOPO_LEVEL_BOOK] = "book",
> [CPU_TOPO_LEVEL_DRAWER] = "drawer",
> },
> .size = CPU_TOPO_LEVEL__MAX
> };
>
> > +
> > +const char *cpu_topo_to_string(CPUTopoLevel topo)
> > +{
> > +return cpu_topo_descriptors[topo].name;
> > +}
>
> And this with generated CPUTopoLevel_str().
Thanks! I missed these generated helpers.
[snip]
> > +##
> > +# @CPUTopoLevel:
> > +#
> > +# An enumeration of CPU topology levels.
> > +#
> > +# @invalid: Invalid topology level, used as a placeholder.
>
> Placeholder for what?
I was trying to express that when no specific topology level is
specified, it will be initialized to this value by default.
Or what about just deleting this placeholder related words and just
saying it's "Invalid topology level"?
> > +#
> > +# @thread: thread level, which would also be called SMT level or logical
> > +# processor level. The @threads option in -smp is used to configure
> > +# the topology of this level.
> > +#
> > +# @core: core level. The @cores option in -smp is used to configure the
> > +# topology of this level.
> > +#
> > +# @module: module level. The @modules option in -smp is used to
> > +# configure the topology of this level.
> > +#
> > +# @cluster: cluster level. The @clusters option in -smp is used to
> > +# configure the topology of this level.
> > +#
> > +# @die: die level. The @dies option in -smp is used to configure the
> > +# topology of this level.
> > +#
> > +# @socket: socket level, which would also be called package level. The
> > +# @sockets option in -smp is used to configure the topology of this
> > +# level.
> > +#
> > +# @book: book level. The @books option in -smp is used to configure the
> > +# topology of this level.
> > +#
> > +# @drawer: drawer level. The @drawers option in -smp is used to
> > +# configure the topology of this level.
>
> As far as I can tell, -smp is sugar for machine property "smp" of QAPI
> type SMPConfiguration. Should we refer to SMPConfiguration instead of
> -smp?
Yes, SMPConfiguration is better.
Thanks,
Zhao
Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic
Hi Markus, On Mon, Jun 03, 2024 at 02:25:36PM +0200, Markus Armbruster wrote: > Date: Mon, 03 Jun 2024 14:25:36 +0200 > From: Markus Armbruster > Subject: Re: [RFC v2 1/7] hw/core: Make CPU topology enumeration > arch-agnostic > > Zhao Liu writes: > > > Cache topology needs to be defined based on CPU topology levels. Thus, > > define CPU topology enumeration in qapi/machine.json to make it generic > > for all architectures. > > > > To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT > > and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and > > CPU_TOPO_LEVEL_SOCKET. > > > > Also, enumerate additional topology levels for non-i386 arches, and add > > helpers for topology enumeration and string conversion. > > > > Signed-off-by: Zhao Liu > > [...] > > > diff --git a/qapi/machine.json b/qapi/machine.json > > index bce6e1bbc412..7ac5a05bb9c9 100644 > > --- a/qapi/machine.json > > +++ b/qapi/machine.json > > @@ -1667,6 +1667,46 @@ > > '*reboot-timeout': 'int', > > '*strict': 'bool' } } > > > > +## > > +# @CPUTopoLevel: > > I understand you're moving existing enum CPUTopoLevel into the QAPI > schema. I think the idiomatic QAPI name would be CpuTopologyLevel. > Would you be willing to rename it, or would that be too much churn? Sure, I'll rename it as you suggested. > > +# > > +# An enumeration of CPU topology levels. > > +# > > +# @invalid: Invalid topology level, used as a placeholder. > > +# > > +# @thread: thread level, which would also be called SMT level or logical > > +# processor level. The @threads option in -smp is used to configure > > +# the topology of this level. > > +# > > +# @core: core level. The @cores option in -smp is used to configure the > > +# topology of this level. > > +# > > +# @module: module level. The @modules option in -smp is used to > > +# configure the topology of this level. > > +# > > +# @cluster: cluster level. The @clusters option in -smp is used to > > +# configure the topology of this level. > > +# > > +# @die: die level. The @dies option in -smp is used to configure the > > +# topology of this level. > > +# > > +# @socket: socket level, which would also be called package level. The > > +# @sockets option in -smp is used to configure the topology of this > > +# level. > > +# > > +# @book: book level. The @books option in -smp is used to configure the > > +# topology of this level. > > +# > > +# @drawer: drawer level. The @drawers option in -smp is used to > > +# configure the topology of this level. > > docs/devel/qapi-code-gen.rst section Documentation markup: > > For legibility, wrap text paragraphs so every line is at most 70 > characters long. > > Separate sentences with two spaces. Thank you for pointing this. About separating sentences, is this what I should be doing? # @drawer: drawer level. The @drawers option in -smp is used to # configure the topology of this level. Thanks, Zhao
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
On Tue, Jun 04, 2024 at 07:29:15AM +0200, Markus Armbruster wrote:
> Date: Tue, 04 Jun 2024 07:29:15 +0200
> From: Markus Armbruster
> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
>
> Zhao Liu writes:
>
> > On Mon, Jun 03, 2024 at 01:54:15PM +0200, Markus Armbruster wrote:
> >> Date: Mon, 03 Jun 2024 13:54:15 +0200
> >> From: Markus Armbruster
> >> Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
> >>
> >> Zhao Liu writes:
> >>
> >> > Use CPUTopology to honor the generic style of CPU capitalization
> >> > abbreviations.
> >> >
> >> > Signed-off-by: Zhao Liu
> >>
> >> Is CPUFoo really more common than CpuFoo? It isn't in the qapi
> >> schema...
> >
> > Hi Markus, do you think this style needs to be standardized?
> >
> > All-caps cases, like the widely used CPUState.
> >
> > And the common structures declared in include/qemu/typedefs.h, are all
> > using CPU, not Cpu...
> >
> > If you feel this is necessary, I'd be happy to help more places change
> > their names to standardize their style...
>
> The situation is unfortunate[*]. The renaming cure could be worse than
> the disease, though.
>
> In a situation like this, settling for local consistency is often the
> least bad. machine.json is locally consistent: it consistently uses
> CpuFoo.
>
>
> [*] We suck at systematic, disciplined naming.
I see, by local consistency principle, my another patch (adding topology
enumeration in machine.json) should use Cpu...
The CpuTopology that this patch modifies is located in include/hw/boards.h,
where that looks as if this file prefers to use CPUs (defining the
CPUArchIdList and CPUArchId). And there's also another case for all-caps,
SMPCompatProps (using SMP not Smp). So I feel like this patch change
still makes sense... Sorry if I'm being a bit obsessive.
The most confusing thing in include/hw/boards.h is this structure:
typedef struct CPUArchId {
...
CpuInstanceProperties props;
CPUState *cpu;
...
} CPUArchId;
CPU and Cpu are mixed together, but this is also explained by the local
consistency principle, since the CpuInstanceProperties belong to
machine.json. ;-)
Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology
On Mon, Jun 03, 2024 at 01:54:15PM +0200, Markus Armbruster wrote: > Date: Mon, 03 Jun 2024 13:54:15 +0200 > From: Markus Armbruster > Subject: Re: [PATCH] hw/core: Rename CpuTopology to CPUTopology > > Zhao Liu writes: > > > Use CPUTopology to honor the generic style of CPU capitalization > > abbreviations. > > > > Signed-off-by: Zhao Liu > > Is CPUFoo really more common than CpuFoo? It isn't in the qapi > schema... Hi Markus, do you think this style needs to be standardized? All-caps cases, like the widely used CPUState. And the common structures declared in include/qemu/typedefs.h, are all using CPU, not Cpu... If you feel this is necessary, I'd be happy to help more places change their names to standardize their style...
Re: [PATCH V3 2/2] target/i386: Advertise MWAIT iff host supports
On Mon, Jun 03, 2024 at 05:02:22PM -0700, Zide Chen wrote:
> Date: Mon, 3 Jun 2024 17:02:22 -0700
> From: Zide Chen
> Subject: [PATCH V3 2/2] target/i386: Advertise MWAIT iff host supports
> X-Mailer: git-send-email 2.34.1
>
> host_cpu_realizefn() sets CPUID_EXT_MONITOR without consulting host/KVM
> capabilities. This may cause problems:
>
> - If MWAIT/MONITOR is not available on the host, advertising this
> feature to the guest and executing MWAIT/MONITOR from the guest
> triggers #UD and the guest doesn't boot. This is because typically
> #UD takes priority over VM-Exit interception checks and KVM doesn't
> emulate MONITOR/MWAIT on #UD.
>
> - If KVM doesn't support KVM_X86_DISABLE_EXITS_MWAIT, MWAIT/MONITOR
> from the guest are intercepted by KVM, which is not what cpu-pm=on
> intends to do.
>
> In these cases, MWAIT/MONITOR should not be exposed to the guest.
>
> The logic in kvm_arch_get_supported_cpuid() to handle CPUID_EXT_MONITOR
> is correct and sufficient, and we can't set CPUID_EXT_MONITOR after
> x86_cpu_filter_features().
>
> This was not an issue before commit 662175b91ff ("i386: reorder call to
> cpu_exec_realizefn") because the feature added in the accel-specific
> realizefn could be checked against host availability and filtered out.
>
> Additionally, it seems not a good idea to handle guest CPUID leaves in
> host_cpu_realizefn(), and this patch merges host_cpu_enable_cpu_pm()
> into kvm_cpu_realizefn().
>
> Fixes: f5cc5a5c1686 ("i386: split cpu accelerators from cpu.c, using
> AccelCPUClass")
> Fixes: 662175b91ff2 ("i386: reorder call to cpu_exec_realizefn")
> Signed-off-by: Zide Chen
> ---
LGTM,
Reviewed-by: Zhao Liu
[PATCH] target/i386/tcg: Fix RDPID feature check
DisasContext.cpuid_ext_features indicates CPUID.01H.ECX.
Use DisasContext.cpuid_7_0_ecx_features field to check RDPID feature bit
(CPUID_7_0_ECX_RDPID).
Fixes: 6750485bf42a ("target/i386: implement RDPID in TCG")
Inspired-by: Xinyu Li
Signed-off-by: Zhao Liu
---
target/i386/tcg/translate.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/target/i386/tcg/translate.c b/target/i386/tcg/translate.c
index 6dedfe94c04c..0486ab691120 100644
--- a/target/i386/tcg/translate.c
+++ b/target/i386/tcg/translate.c
@@ -3199,7 +3199,7 @@ static void disas_insn_old(DisasContext *s, CPUState
*cpu, int b)
goto illegal_op;
}
if (s->prefix & PREFIX_REPZ) {
-if (!(s->cpuid_ext_features & CPUID_7_0_ECX_RDPID)) {
+if (!(s->cpuid_7_0_ecx_features & CPUID_7_0_ECX_RDPID)) {
goto illegal_op;
}
gen_helper_rdpid(s->T0, tcg_env);
--
2.34.1
Re: [PATCH] target/i386: fix SSE and SSE2 featue check
On Sun, Jun 02, 2024 at 06:09:04PM +0800, lixinyu...@ict.ac.cn wrote:
> Date: Sun, 2 Jun 2024 18:09:04 +0800
> From: lixinyu...@ict.ac.cn
> Subject: [PATCH] target/i386: fix SSE and SSE2 featue check
> X-Mailer: git-send-email 2.34.1
>
> From: Xinyu Li
>
> Featues check of CPUID_SSE and CPUID_SSE2 shoule use cpuid_features,
> rather than cpuid_ext_features
It's better to add a Fixes tag,
Fixes: caa01fadbef1 ("target/i386: add CPUID feature checks to new decoder")
> Signed-off-by: Xinyu Li
> ---
> target/i386/tcg/decode-new.c.inc | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Zhao Liu
Re: [PATCH 2/6] meson: assume x86-64-v2 baseline ISA
On Fri, May 31, 2024 at 11:14:53AM +0200, Paolo Bonzini wrote:
> Date: Fri, 31 May 2024 11:14:53 +0200
> From: Paolo Bonzini
> Subject: [PATCH 2/6] meson: assume x86-64-v2 baseline ISA
> X-Mailer: git-send-email 2.45.1
>
> x86-64-v2 processors were released in 2008, assume that we have one.
> Unfortunately there is no GCC flag to enable all the features
> without disabling what came after; so enable them one by one.
>
> Signed-off-by: Paolo Bonzini
> ---
> meson.build | 10 +++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index 63866071445..19d1fc1f33b 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -336,9 +336,13 @@ if host_arch == 'i386' and not cc.links('''
>qemu_common_flags = ['-march=i486'] + qemu_common_flags
> endif
>
> -# ??? Only extremely old AMD cpus do not have cmpxchg16b.
> -# If we truly care, we should simply detect this case at
> -# runtime and generate the fallback to serial emulation.
> +# Assume x86-64-v2 (minus CMPXCHG16B for 32-bit code)
Is it necessary to state the requirement (x86-64-v2) for x86 host in
some doc?
e.g., docs/system/target-i386.rst.
> +if host_arch == 'i386'
> + qemu_common_flags = ['-mfpmath=sse'] + qemu_common_flags
> +endif
> +if host_arch in ['i386', 'x86_64']
> + qemu_common_flags = ['-mpopcnt', '-msse4.2'] + qemu_common_flags
> +endif
> if host_arch == 'x86_64'
>qemu_common_flags = ['-mcx16'] + qemu_common_flags
> endif
> --
> 2.45.1
Reviewed-by: Zhao Liu
Re: [PATCH 6/6] host/i386: assume presence of POPCNT
On Fri, May 31, 2024 at 11:14:57AM +0200, Paolo Bonzini wrote: > Date: Fri, 31 May 2024 11:14:57 +0200 > From: Paolo Bonzini > Subject: [PATCH 6/6] host/i386: assume presence of POPCNT > X-Mailer: git-send-email 2.45.1 > > QEMU now requires an x86-64-v2 host, which has the POPCNT instruction. > Use it freely in TCG-generated code. > > Signed-off-by: Paolo Bonzini > --- > host/include/i386/host/cpuinfo.h | 1 - > tcg/i386/tcg-target.h| 5 ++--- > util/cpuinfo-i386.c | 1 - > 3 files changed, 2 insertions(+), 5 deletions(-) Reviewed-by: Zhao Liu
Re: [PATCH 5/6] host/i386: assume presence of SSSE3
On Fri, May 31, 2024 at 11:14:56AM +0200, Paolo Bonzini wrote: > Date: Fri, 31 May 2024 11:14:56 +0200 > From: Paolo Bonzini > Subject: [PATCH 5/6] host/i386: assume presence of SSSE3 > X-Mailer: git-send-email 2.45.1 > > QEMU now requires an x86-64-v2 host, which has SSSE3 instructions > (notably, PSHUFB which is used by QEMU's AES implementation). > Do not bother checking it. > > Signed-off-by: Paolo Bonzini > --- > util/cpuinfo-i386.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Zhao Liu
Re: [PATCH 4/6] host/i386: assume presence of SSE2
On Fri, May 31, 2024 at 11:14:55AM +0200, Paolo Bonzini wrote: > Date: Fri, 31 May 2024 11:14:55 +0200 > From: Paolo Bonzini > Subject: [PATCH 4/6] host/i386: assume presence of SSE2 > X-Mailer: git-send-email 2.45.1 > > QEMU now requires an x86-64-v2 host, which has SSE2. > Use it freely in buffer_is_zero. > > Signed-off-by: Paolo Bonzini > --- > host/include/i386/host/cpuinfo.h | 1 - > util/bufferiszero.c | 2 +- > util/cpuinfo-i386.c | 1 - > 3 files changed, 1 insertion(+), 3 deletions(-) Reviewed-by: Zhao Liu
Re: [PATCH 3/6] host/i386: assume presence of CMOV
On Fri, May 31, 2024 at 11:14:54AM +0200, Paolo Bonzini wrote: > Date: Fri, 31 May 2024 11:14:54 +0200 > From: Paolo Bonzini > Subject: [PATCH 3/6] host/i386: assume presence of CMOV > X-Mailer: git-send-email 2.45.1 > > QEMU now requires an x86-64-v2 host, which always has CMOV. > Use it freely in TCG generated code. > > Signed-off-by: Paolo Bonzini > --- > host/include/i386/host/cpuinfo.h | 1 - > util/cpuinfo-i386.c | 1 - > tcg/i386/tcg-target.c.inc| 15 +-- > 3 files changed, 1 insertion(+), 16 deletions(-) Reviewed-by: Zhao Liu
Re: [PATCH 1/6] host/i386: nothing looks at CPUINFO_SSE4
On Fri, May 31, 2024 at 11:14:52AM +0200, Paolo Bonzini wrote: > Date: Fri, 31 May 2024 11:14:52 +0200 > From: Paolo Bonzini > Subject: [PATCH 1/6] host/i386: nothing looks at CPUINFO_SSE4 > X-Mailer: git-send-email 2.45.1 > > The only user was the SSE4.1 variant of buffer_is_zero, which has > been removed; code to compute CPUINFO_SSE4 is dead. > > Signed-off-by: Paolo Bonzini > --- > host/include/i386/host/cpuinfo.h | 1 - > util/cpuinfo-i386.c | 1 - > 2 files changed, 2 deletions(-) > Reviewed-by: Zhao Liu
Re: [PATCH] accel/kvm: Fix two lines with hard-coded tabs
On Fri, May 31, 2024 at 06:09:52PM +0100, Peter Maydell wrote: > Date: Fri, 31 May 2024 18:09:52 +0100 > From: Peter Maydell > Subject: [PATCH] accel/kvm: Fix two lines with hard-coded tabs > X-Mailer: git-send-email 2.34.1 > > In kvm-all.c, two lines have been accidentally indented with > hard-coded tabs rather than spaces. Normalise to match the rest > of the file. > > Signed-off-by: Peter Maydell > --- > accel/kvm/kvm-all.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Reviewed-by: Zhao Liu
Re: [PATCH] machine: allow early use of machine_require_guest_memfd
On Fri, May 31, 2024 at 01:26:36PM +0200, Paolo Bonzini wrote: > Date: Fri, 31 May 2024 13:26:36 +0200 > From: Paolo Bonzini > Subject: [PATCH] machine: allow early use of machine_require_guest_memfd > X-Mailer: git-send-email 2.45.1 > > Ask the ConfidentialGuestSupport object whether to use guest_memfd > for KVM-backend private memory. This bool can be set in instance_init > (or user_complete) so that it is available when the machine is created. > > Signed-off-by: Paolo Bonzini > --- > include/exec/confidential-guest-support.h | 5 + > include/hw/boards.h | 1 - > hw/core/machine.c | 2 +- > 3 files changed, 6 insertions(+), 2 deletions(-) Reviewed-by: Zhao Liu
Re: [PATCH V2 2/3] target/i386: call cpu_exec_realizefn before x86_cpu_filter_features
On Fri, May 31, 2024 at 10:13:47AM -0700, Chen, Zide wrote:
> Date: Fri, 31 May 2024 10:13:47 -0700
> From: "Chen, Zide"
> Subject: Re: [PATCH V2 2/3] target/i386: call cpu_exec_realizefn before
> x86_cpu_filter_features
>
> On 5/30/2024 11:30 PM, Zhao Liu wrote:
> > Hi Zide,
> >
> > On Fri, May 24, 2024 at 01:00:16PM -0700, Zide Chen wrote:
> >> Date: Fri, 24 May 2024 13:00:16 -0700
> >> From: Zide Chen
> >> Subject: [PATCH V2 2/3] target/i386: call cpu_exec_realizefn before
> >> x86_cpu_filter_features
> >> X-Mailer: git-send-email 2.34.1
> >>
> >> cpu_exec_realizefn which calls the accel-specific realizefn may expand
> >> features. e.g., some accel-specific options may require extra features
> >> to be enabled, and it's appropriate to expand these features in accel-
> >> specific realizefn.
> >>
> >> One such example is the cpu-pm option, which may add CPUID_EXT_MONITOR.
> >>
> >> Thus, call cpu_exec_realizefn before x86_cpu_filter_features to ensure
> >> that it won't expose features not supported by the host.
> >>
> >> Fixes: 662175b91ff2 ("i386: reorder call to cpu_exec_realizefn")
> >> Suggested-by: Xiaoyao Li
> >> Signed-off-by: Zide Chen
> >> ---
> >> target/i386/cpu.c | 24
> >> target/i386/kvm/kvm-cpu.c | 1 -
> >> 2 files changed, 12 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> >> index bc2dceb647fa..a1c1c785bd2f 100644
> >> --- a/target/i386/cpu.c
> >> +++ b/target/i386/cpu.c
> >> @@ -7604,6 +7604,18 @@ static void x86_cpu_realizefn(DeviceState *dev,
> >> Error **errp)
> >> }
> >> }
> >>
> >> +/*
> >> + * note: the call to the framework needs to happen after feature
> >> expansion,
> >> + * but before the checks/modifications to ucode_rev, mwait, phys_bits.
> >> + * These may be set by the accel-specific code,
> >> + * and the results are subsequently checked / assumed in this
> >> function.
> >> + */
> >> +cpu_exec_realizefn(cs, _err);
> >> +if (local_err != NULL) {
> >> +error_propagate(errp, local_err);
> >> +return;
> >> +}
> >> +
> >> x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid);
> >
> > For your case, which sets cpu-pm=on via overcommit, then
> > x86_cpu_filter_features() will complain that mwait is not supported.
> >
> > Such warning is not necessary, because the purpose of overcommit (from
> > code) is only to support mwait when possible, not to commit to support
> > mwait in Guest.
> >
> > Additionally, I understand x86_cpu_filter_features() is primarily
> > intended to filter features configured by the user,
>
> Yes, that's why this patches intends to let x86_cpu_filter_features()
> filter out the MWAIT bit which is set from the overcommit option.
HMM, but in fact x86_cpu_filter_features() has already checked the MWAIT
bit set by "-overcommit cpu-pm=on". ;-)
(Pls correct me if I'm wrong) Revisiting what cpu-pm did to MWAIT:
* Firstly, it set MWAIT bit in x86_cpu_expand_features():
x86_cpu_expand_features()
-> x86_cpu_get_supported_feature_word()
-> kvm_arch_get_supported_cpuid()
This MWAIT is based on Host's MWAIT capability. This MWAIT enablement
is fine for next x86_cpu_filter_features() and x86_cpu_filter_features()
is working correctly here!
* Then, MWAIT was secondly set in host_cpu_enable_cpu_pm() regardless
neither Host's support or previous MWAIT enablement result. This is
the root cause of your issue.
Therefore, we should make cpu-pm honor his first MWAIT enablement result
instead of repeatly and unconditionally setting the MWAIT bit again in
host_cpu_enable_cpu_pm().
Additionally, I think the code in x86_cpu_realizefn():
cpu->mwait.ecx |= CPUID_MWAIT_EMX | CPUID_MWAIT_IBE;
has the similar issue because it also should check MWAIT feature bit.
Further, it may be possible to remove cpu->mwait: just check the MWAIT
bit in leaf 5 of cpu_x86_cpuid(), and if MWAIT is present, use host's
mwait info plus CPUID_MWAIT_EMX | CPUID_MWAIT_IBE.
> > and the changes of
> > CPUID after x86_cpu_filter_features() should by default be regarded like
> > "QEMU knows what it is doing".
>
> Sure, we can add feature bits after x86_cpu_filter_features(), but I
> think moving cpu_exec_realizefn() before x86_cpu_filter_features() is
> more generic, and actually this is what QEMU did before commit 662175b91ff2.
>
> - Less redundant code. Specifically, no need to call
> x86_cpu_get_supported_feature_word() again.
> - Potentially there could be other features could be added from the
> accel-specific realizefn, kvm_cpu_realizefn() for example. And these
> features need to be checked against the host availability.
Mainly I don't think this reorder is a direct fix for the problem (I
just analyse it above), also in your case x86_cpu_filter_features() will
print a WARNING when QEMU boots, which I don't think is cpu-pm's intention.
Re: [PATCH V2 3/3] target/i386: Move host_cpu_enable_cpu_pm into kvm_cpu_realizefn()
On Fri, May 24, 2024 at 01:00:17PM -0700, Zide Chen wrote:
> Date: Fri, 24 May 2024 13:00:17 -0700
> From: Zide Chen
> Subject: [PATCH V2 3/3] target/i386: Move host_cpu_enable_cpu_pm into
> kvm_cpu_realizefn()
> X-Mailer: git-send-email 2.34.1
>
> It seems not a good idea to expand features in host_cpu_realizefn,
> which is for host CPU only.
It is restricted by max_features and should be part of the "max" CPU,
and the current target/i386/host-cpu.c should only serve the “host” CPU.
> Additionally, cpu-pm option is KVM
> specific, and it's cleaner to put it in kvm_cpu_realizefn(), together
> with the WAITPKG code.
>
> Fixes: f5cc5a5c1686 ("i386: split cpu accelerators from cpu.c, using
> AccelCPUClass")
> Signed-off-by: Zide Chen
> ---
> target/i386/host-cpu.c| 12
> target/i386/kvm/kvm-cpu.c | 11 +--
> 2 files changed, 9 insertions(+), 14 deletions(-)
>
> diff --git a/target/i386/host-cpu.c b/target/i386/host-cpu.c
> index 280e427c017c..8b8bf5afeccf 100644
> --- a/target/i386/host-cpu.c
> +++ b/target/i386/host-cpu.c
> @@ -42,15 +42,6 @@ static uint32_t host_cpu_phys_bits(void)
> return host_phys_bits;
> }
>
> -static void host_cpu_enable_cpu_pm(X86CPU *cpu)
> -{
> -CPUX86State *env = >env;
> -
> -host_cpuid(5, 0, >mwait.eax, >mwait.ebx,
> - >mwait.ecx, >mwait.edx);
> -env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
> -}
> -
> static uint32_t host_cpu_adjust_phys_bits(X86CPU *cpu)
> {
> uint32_t host_phys_bits = host_cpu_phys_bits();
> @@ -83,9 +74,6 @@ bool host_cpu_realizefn(CPUState *cs, Error **errp)
> X86CPU *cpu = X86_CPU(cs);
> CPUX86State *env = >env;
>
> -if (cpu->max_features && enable_cpu_pm) {
> -host_cpu_enable_cpu_pm(cpu);
> -}
> if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
> uint32_t phys_bits = host_cpu_adjust_phys_bits(cpu);
>
> diff --git a/target/i386/kvm/kvm-cpu.c b/target/i386/kvm/kvm-cpu.c
> index 3adcedf0dbc3..197c892da89a 100644
> --- a/target/i386/kvm/kvm-cpu.c
> +++ b/target/i386/kvm/kvm-cpu.c
> @@ -64,9 +64,16 @@ static bool kvm_cpu_realizefn(CPUState *cs, Error **errp)
> * cpu_common_realizefn() (via xcc->parent_realize)
> */
> if (cpu->max_features) {
> -if (enable_cpu_pm && kvm_has_waitpkg()) {
> -env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
> +if (enable_cpu_pm) {
> +if (kvm_has_waitpkg()) {
> +env->features[FEAT_7_0_ECX] |= CPUID_7_0_ECX_WAITPKG;
> +}
If you agree with my comment in patch 2, here we need a mwait bit check.
> +host_cpuid(5, 0, >mwait.eax, >mwait.ebx,
> + >mwait.ecx, >mwait.edx);
> +env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
> }
> +
> if (cpu->ucode_rev == 0) {
> cpu->ucode_rev =
> kvm_arch_get_supported_msr_feature(kvm_state,
> --
> 2.34.1
>
>
Re: [PATCH V2 2/3] target/i386: call cpu_exec_realizefn before x86_cpu_filter_features
Hi Zide,
On Fri, May 24, 2024 at 01:00:16PM -0700, Zide Chen wrote:
> Date: Fri, 24 May 2024 13:00:16 -0700
> From: Zide Chen
> Subject: [PATCH V2 2/3] target/i386: call cpu_exec_realizefn before
> x86_cpu_filter_features
> X-Mailer: git-send-email 2.34.1
>
> cpu_exec_realizefn which calls the accel-specific realizefn may expand
> features. e.g., some accel-specific options may require extra features
> to be enabled, and it's appropriate to expand these features in accel-
> specific realizefn.
>
> One such example is the cpu-pm option, which may add CPUID_EXT_MONITOR.
>
> Thus, call cpu_exec_realizefn before x86_cpu_filter_features to ensure
> that it won't expose features not supported by the host.
>
> Fixes: 662175b91ff2 ("i386: reorder call to cpu_exec_realizefn")
> Suggested-by: Xiaoyao Li
> Signed-off-by: Zide Chen
> ---
> target/i386/cpu.c | 24
> target/i386/kvm/kvm-cpu.c | 1 -
> 2 files changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index bc2dceb647fa..a1c1c785bd2f 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -7604,6 +7604,18 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
> **errp)
> }
> }
>
> +/*
> + * note: the call to the framework needs to happen after feature
> expansion,
> + * but before the checks/modifications to ucode_rev, mwait, phys_bits.
> + * These may be set by the accel-specific code,
> + * and the results are subsequently checked / assumed in this function.
> + */
> +cpu_exec_realizefn(cs, _err);
> +if (local_err != NULL) {
> +error_propagate(errp, local_err);
> +return;
> +}
> +
> x86_cpu_filter_features(cpu, cpu->check_cpuid || cpu->enforce_cpuid);
For your case, which sets cpu-pm=on via overcommit, then
x86_cpu_filter_features() will complain that mwait is not supported.
Such warning is not necessary, because the purpose of overcommit (from
code) is only to support mwait when possible, not to commit to support
mwait in Guest.
Additionally, I understand x86_cpu_filter_features() is primarily
intended to filter features configured by the user, and the changes of
CPUID after x86_cpu_filter_features() should by default be regarded like
"QEMU knows what it is doing".
I feel adding a check for the CPUID mwait bit in host_cpu_realizefn()
is enough, after all, this bit should be present if host supports mwait
and enable_cpu_pm (in kvm_arch_get_supported_cpuid()).
Thanks,
Zhao
Re: [PATCH 0/2] qapi/qapi-schema: Clarify the dependency relationship
Hi Eric and Markus, Just a gentle poke. What do you think of this ordering? Thanks, Zhao On Fri, May 17, 2024 at 02:27:46PM +0800, Zhao Liu wrote: > Date: Fri, 17 May 2024 14:27:46 +0800 > From: Zhao Liu > Subject: [PATCH 0/2] qapi/qapi-schema: Clarify the dependency relationship > X-Mailer: git-send-email 2.34.1 > > Hi, > > At present, the correctness of the dependencies of JSON files is ensured > by the order in which they are listed, but in general, the mixing of > multiple files and the lack of clear guidelines for ordering them is not > friendly to extending and maintaining. > > Therefore, I have a proposal to manually categorize and sort JSON files > generation order by dependencies/dependency hierarchy, to improve the > readability and maintainability of qapi-schema.json. > > Welcome your feedback! > > Thanks and Best Regards, > Zhao > --- > Zhao Liu (2): > qapi: Reorder and categorize json files in qapi-schema.json > qapi: List block-core.json in qapi-schema.json > > qapi/qapi-schema.json | 100 +- > 1 file changed, 69 insertions(+), 31 deletions(-) > > -- > 2.34.1 >
Re: [PATCH V2 1/3] vl: Allow multiple -overcommit commands
On Mon, May 27, 2024 at 07:19:56AM +0200, Thomas Huth wrote:
> Date: Mon, 27 May 2024 07:19:56 +0200
> From: Thomas Huth
> Subject: Re: [PATCH V2 1/3] vl: Allow multiple -overcommit commands
>
> On 24/05/2024 22.00, Zide Chen wrote:
> > Both cpu-pm and mem-lock are related to system resource overcommit, but
> > they are separate from each other, in terms of how they are realized,
> > and of course, they are applied to different system resources.
> >
> > It's tempting to use separate command lines to specify their behavior.
> > e.g., in the following example, the cpu-pm command is quietly
> > overwritten, and it's not easy to notice it without careful inspection.
> >
> >--overcommit mem-lock=on
> >--overcommit cpu-pm=on
> >
> > Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
> > Suggested-by: Thomas Huth
> > Signed-off-by: Zide Chen
> > ---
> >
> > v2:
> >
> > Thanks to Thomas' suggestion, changed to this better approach, which
> > is more generic and can handle situations like: "enabled the option in
> > the config file, and now you'd like to disable it on the command line
> > again".
> >
> > system/vl.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/system/vl.c b/system/vl.c
> > index a3eede5fa5b8..dfa6cdd9283b 100644
> > --- a/system/vl.c
> > +++ b/system/vl.c
> > @@ -3545,8 +3545,8 @@ void qemu_init(int argc, char **argv)
> > if (!opts) {
> > exit(1);
> > }
> > -enable_mlock = qemu_opt_get_bool(opts, "mem-lock", false);
> > -enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm", false);
> > +enable_mlock = qemu_opt_get_bool(opts, "mem-lock",
> > enable_mlock);
> > +enable_cpu_pm = qemu_opt_get_bool(opts, "cpu-pm",
> > enable_cpu_pm);
> > break;
> > case QEMU_OPTION_compat:
> > {
>
> Reviewed-by: Thomas Huth
>
Hi Thomas,
BTW, do you think it's a good idea to define the overcommit via QAPI way
(defined in a json file)? ;-)
My rough understanding is that all APIs are better to be defined via
QAPI to go JSON compatible.
Re: [PATCH V2 0/3] improve -overcommit cpu-pm=on|off
Hi Zide,
On Wed, May 29, 2024 at 10:31:21AM -0700, Chen, Zide wrote:
> Date: Wed, 29 May 2024 10:31:21 -0700
> From: "Chen, Zide"
> Subject: Re: [PATCH V2 0/3] improve -overcommit cpu-pm=on|off
>
>
>
> On 5/29/2024 5:46 AM, Igor Mammedov wrote:
> > On Tue, 28 May 2024 11:16:59 -0700
> > "Chen, Zide" wrote:
> >
> >> On 5/28/2024 2:23 AM, Igor Mammedov wrote:
> >>> On Fri, 24 May 2024 13:00:14 -0700
> >>> Zide Chen wrote:
> >>>
> Currently, if running "-overcommit cpu-pm=on" on hosts that don't
> have MWAIT support, the MWAIT/MONITOR feature is advertised to the
> guest and executing MWAIT/MONITOR on the guest triggers #UD.
> >>>
> >>> this is missing proper description how do you trigger issue
> >>> with reproducer and detailed description why guest sees MWAIT
> >>> when it's not supported by host.
> >>
> >> If "overcommit cpu-pm=on" and "-cpu host" are present, as shown in the
> > it's bette to provide full QEMU CLI and host/guest kernels used and what
> > hardware was used if it's relevant so others can reproduce problem.
>
> I ever reproduced this on an older Intel Icelake machine, a
> Sapphire Rapids and a Sierra Forest, but I believe this is a x86 generic
> issue, not specific to particular models.
>
> For the CLI, I think the only command line options that matter are
> -overcommit cpu-pm=on: to set enable_cpu_pm
> -cpu host: so that cpu->max_features is set
>
> For QEMU version, as long as it's after this commit: 662175b91ff2
> ("i386: reorder call to cpu_exec_realizefn")
>
> The guest fails to boot:
>
> [ 24.825568] smpboot: x86: Booting SMP configuration:
> [ 24.826377] node #0, CPUs: #1 #2 #3 #4 #5 #6 #7 #8 #9 #10 #11 #12
> #13 #14 #15 #17
> [ 24.985799] node #1, CPUs: #128 #129 #130 #131 #132 #133 #134 #135
> #136 #137 #138 #139 #140 #141 #142 #143 #145
> [ 25.136955] invalid opcode: 1 PREEMPT SMP NOPTI
> [ 25.137790] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 6.8.0 #2
> [ 25.137790] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> rel-1.16.3-0-ga6ed6b701f0a-prebuilt.qemu.org 04/04
> [ 25.137790] RIP: 0010:mwait_idle+0x35/0x80
> [ 25.137790] Code: 6f f0 80 48 02 20 48 8b 10 83 e2 08 75 3e 65 48 8b 15
> 47 d6 56 6f 48 0f ba e2 27 72 41 31 d2 48 89 d8
> [ 25.137790] RSP: :91403e70 EFLAGS: 00010046
> [ 25.137790] RAX: 9140a980 RBX: 9140a980 RCX:
>
> [ 25.137790] RDX: RSI: 97f1ade21b20 RDI:
> 0004
> [ 25.137790] RBP: R08: 0005da4709cb R09:
> 0001
> [ 25.137790] R10: 5da4 R11: 0009 R12:
>
> [ 25.137790] R13: 98573ff90fc0 R14: 9140a038 R15:
> 00093ff0
> [ 25.137790] FS: () GS:97f1ade0()
> knlGS:
> [ 25.137790] CS: 0010 DS: ES: CR0: 80050033
> [ 25.137790] CR2: 97d8aa801000 CR3: 0049e9430001 CR4:
> 00770ef0
> [ 25.137790] DR0: DR1: DR2:
>
> [ 25.137790] DR3: DR6: 07f0 DR7:
> 0400
> [ 25.137790] PKRU: 5554
> [ 25.137790] Call Trace:
> [ 25.137790]
> [ 25.137790] ? die+0x37/0x90
> [ 25.137790] ? do_trap+0xe3/0x110
> [ 25.137790] ? mwait_idle+0x35/0x80
> [ 25.137790] ? do_error_trap+0x6a/0x90
> [ 25.137790] ? mwait_idle+0x35/0x80
> [ 25.137790] ? exc_invalid_op+0x52/0x70
> [ 25.137790] ? mwait_idle+0x35/0x80
> [ 25.137790] ? asm_exc_invalid_op+0x1a/0x20
> [ 25.137790] ? mwait_idle+0x35/0x80
> [ 25.137790] default_idle_call+0x30/0x100
> [ 25.137790] cpuidle_idle_call+0x12c/0x170
> [ 25.137790] ? tsc_verify_tsc_adjust+0x73/0xd0
> [ 25.137790] do_idle+0x7f/0xd0
> [ 25.137790] cpu_startup_entry+0x29/0x30
> [ 25.137790] rest_init+0xcc/0xd0
> [ 25.137790] start_kernel+0x396/0x5d0
> [ 25.137790] x86_64_start_reservations+0x18/0x30
> [ 25.137790] x86_64_start_kernel+0xe7/0xf0
> [ 25.137790] common_startup_64+0x13e/0x148
> [ 25.137790]
> [ 25.137790] Modules linked in:
> [ 25.137790] --[ end trace ]--
> [ 25.137790] invalid opcode: 2 PREEMPT SMP NOPTI
> [ 25.137790] RIP: 0010:mwait_idle+0x35/0x80
> [ 25.137790] Code: 6f f0 80 48 02 20 48 8b 10 83 e2 08 75 3e 65 48 8b 15
> 47 d6 56 6f 48 0f ba e2 27 72 41 31 d2 48 89 d8
>
> >
> >> following, CPUID_EXT_MONITOR is set after x86_cpu_filter_features(), so
> >> that it doesn't have a chance to check MWAIT against host features and
> >> will be advertised to the guest regardless of whether it's supported by
> >> the host or not.
> >>
> >> x86_cpu_realizefn()
> >> x86_cpu_filter_features()
> >> cpu_exec_realizefn()
> >> kvm_cpu_realizefn
> >> host_cpu_realizefn
> >> host_cpu_enable_cpu_pm
> >> env->features[FEAT_1_ECX] |= CPUID_EXT_MONITOR;
> >>
> >>
> >> If it's not supported by the host, executing MONITOR or MWAIT
> >> instructions from the guest triggers #UD, no matter MWAIT_EXITING
> >> control is set or not.
Re: [PATCH V2 1/3] vl: Allow multiple -overcommit commands
On Fri, May 24, 2024 at 01:00:15PM -0700, Zide Chen wrote:
> Date: Fri, 24 May 2024 13:00:15 -0700
> From: Zide Chen
> Subject: [PATCH V2 1/3] vl: Allow multiple -overcommit commands
> X-Mailer: git-send-email 2.34.1
>
> Both cpu-pm and mem-lock are related to system resource overcommit, but
> they are separate from each other, in terms of how they are realized,
> and of course, they are applied to different system resources.
>
> It's tempting to use separate command lines to specify their behavior.
> e.g., in the following example, the cpu-pm command is quietly
> overwritten, and it's not easy to notice it without careful inspection.
>
> --overcommit mem-lock=on
> --overcommit cpu-pm=on
>
> Fixes: c8c9dc42b7ca ("Remove the deprecated -realtime option")
> Suggested-by: Thomas Huth
> Signed-off-by: Zide Chen
> ---
>
> v2:
>
> Thanks to Thomas' suggestion, changed to this better approach, which
> is more generic and can handle situations like: "enabled the option in
> the config file, and now you'd like to disable it on the command line
> again".
>
> system/vl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Zhao Liu
[RFC v2 1/7] hw/core: Make CPU topology enumeration arch-agnostic
Cache topology needs to be defined based on CPU topology levels. Thus,
define CPU topology enumeration in qapi/machine.json to make it generic
for all architectures.
To match the general topology naming style, rename CPU_TOPO_LEVEL_SMT
and CPU_TOPO_LEVEL_PACKAGE to CPU_TOPO_LEVEL_THREAD and
CPU_TOPO_LEVEL_SOCKET.
Also, enumerate additional topology levels for non-i386 arches, and add
helpers for topology enumeration and string conversion.
Signed-off-by: Zhao Liu
---
Changes since RFC v1:
* Use QAPI to enumerate CPU topology levels.
* Drop string_to_cpu_topo() since QAPI will help to parse the topo
levels.
---
MAINTAINERS| 2 ++
hw/core/cpu-topology.c | 36 ++
hw/core/meson.build| 1 +
include/hw/core/cpu-topology.h | 20 +
include/hw/i386/topology.h | 18 +--
qapi/machine.json | 40 ++
target/i386/cpu.c | 30 -
target/i386/cpu.h | 4 ++--
8 files changed, 117 insertions(+), 34 deletions(-)
create mode 100644 hw/core/cpu-topology.c
create mode 100644 include/hw/core/cpu-topology.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 448dc951c509..09173e8c953d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1875,6 +1875,7 @@ R: Yanan Wang
S: Supported
F: hw/core/cpu-common.c
F: hw/core/cpu-sysemu.c
+F: hw/core/cpu-topology.c
F: hw/core/machine-qmp-cmds.c
F: hw/core/machine.c
F: hw/core/machine-smp.c
@@ -1886,6 +1887,7 @@ F: qapi/machine-common.json
F: qapi/machine-target.json
F: include/hw/boards.h
F: include/hw/core/cpu.h
+F: include/hw/core/cpu-topology.h
F: include/hw/cpu/cluster.h
F: include/sysemu/numa.h
F: tests/unit/test-smp-parse.c
diff --git a/hw/core/cpu-topology.c b/hw/core/cpu-topology.c
new file mode 100644
index ..20b5d708cb54
--- /dev/null
+++ b/hw/core/cpu-topology.c
@@ -0,0 +1,36 @@
+/*
+ * QEMU CPU Topology Representation
+ *
+ * Copyright (c) 2024 Intel Corporation
+ *
+ * Authors:
+ * Zhao Liu
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/cpu-topology.h"
+
+typedef struct CPUTopoInfo {
+const char *name;
+} CPUTopoInfo;
+
+CPUTopoInfo cpu_topo_descriptors[] = {
+[CPU_TOPO_LEVEL_INVALID] = { .name = "invalid", },
+[CPU_TOPO_LEVEL_THREAD] = { .name = "thread", },
+[CPU_TOPO_LEVEL_CORE]= { .name = "core",},
+[CPU_TOPO_LEVEL_MODULE] = { .name = "module", },
+[CPU_TOPO_LEVEL_CLUSTER] = { .name = "cluster", },
+[CPU_TOPO_LEVEL_DIE] = { .name = "die", },
+[CPU_TOPO_LEVEL_SOCKET] = { .name = "socket", },
+[CPU_TOPO_LEVEL_BOOK]= { .name = "book",},
+[CPU_TOPO_LEVEL_DRAWER] = { .name = "drawer", },
+[CPU_TOPO_LEVEL__MAX]= { .name = NULL, },
+};
+
+const char *cpu_topo_to_string(CPUTopoLevel topo)
+{
+return cpu_topo_descriptors[topo].name;
+}
diff --git a/hw/core/meson.build b/hw/core/meson.build
index a3d9bab9f42a..71dc396e9bfc 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -13,6 +13,7 @@ hwcore_ss.add(files(
))
common_ss.add(files('cpu-common.c'))
+common_ss.add(files('cpu-topology.c'))
common_ss.add(files('machine-smp.c'))
system_ss.add(when: 'CONFIG_FITLOADER', if_true: files('loader-fit.c'))
system_ss.add(when: 'CONFIG_GENERIC_LOADER', if_true:
files('generic-loader.c'))
diff --git a/include/hw/core/cpu-topology.h b/include/hw/core/cpu-topology.h
new file mode 100644
index ..0e21fe8a9bf8
--- /dev/null
+++ b/include/hw/core/cpu-topology.h
@@ -0,0 +1,20 @@
+/*
+ * QEMU CPU Topology Representation Header
+ *
+ * Copyright (c) 2024 Intel Corporation
+ *
+ * Authors:
+ * Zhao Liu
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or
+ * later. See the COPYING file in the top-level directory.
+ */
+
+#ifndef CPU_TOPOLOGY_H
+#define CPU_TOPOLOGY_H
+
+#include "qapi/qapi-types-machine.h"
+
+const char *cpu_topo_to_string(CPUTopoLevel topo);
+
+#endif /* CPU_TOPOLOGY_H */
diff --git a/include/hw/i386/topology.h b/include/hw/i386/topology.h
index dff49fce1154..c6ff75f23991 100644
--- a/include/hw/i386/topology.h
+++ b/include/hw/i386/topology.h
@@ -39,7 +39,7 @@
* CPUID Fn8000_0008_ECX[ApicIdCoreIdSize[3:0]] is set to apicid_core_width().
*/
-
+#include "hw/core/cpu-topology.h"
#include "qemu/bitops.h"
/*
@@ -62,22 +62,6 @@ typedef struct X86CPUTopoInfo {
unsigned threads_per_core;
} X86CPUTopoInfo;
-/*
- * CPUTopoLevel is the general i386 topology hierarchical representation,
- * ordered by increasing hierarchical relationship.
- * Its enumeration value is not bound to the type value of Intel (CPUID[0x1F])
- * or AMD (CPUID[0x8000
[RFC v2 5/7] i386/cpu: Update cache topology with machine's configuration
User will configure SMP cache topology via -smp.
For this case, update the x86 CPUs' cache topology with user's
configuration in MachineState.
Signed-off-by: Zhao Liu
---
target/i386/cpu.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 3a2dadb4bce0..1bd1860ae625 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -7764,6 +7764,27 @@ static void x86_cpu_realizefn(DeviceState *dev, Error
**errp)
#ifndef CONFIG_USER_ONLY
MachineState *ms = MACHINE(qdev_get_machine());
+
+if (ms->smp_cache.l1d != CPU_TOPO_LEVEL_INVALID) {
+env->cache_info_cpuid4.l1d_cache->share_level = ms->smp_cache.l1d;
+env->cache_info_amd.l1d_cache->share_level = ms->smp_cache.l1d;
+}
+
+if (ms->smp_cache.l1i != CPU_TOPO_LEVEL_INVALID) {
+env->cache_info_cpuid4.l1i_cache->share_level = ms->smp_cache.l1i;
+env->cache_info_amd.l1i_cache->share_level = ms->smp_cache.l1i;
+}
+
+if (ms->smp_cache.l2 != CPU_TOPO_LEVEL_INVALID) {
+env->cache_info_cpuid4.l2_cache->share_level = ms->smp_cache.l2;
+env->cache_info_amd.l2_cache->share_level = ms->smp_cache.l2;
+}
+
+if (ms->smp_cache.l3 != CPU_TOPO_LEVEL_INVALID) {
+env->cache_info_cpuid4.l3_cache->share_level = ms->smp_cache.l3;
+env->cache_info_amd.l3_cache->share_level = ms->smp_cache.l3;
+}
+
qemu_register_reset(x86_cpu_machine_reset_cb, cpu);
if (cpu->env.features[FEAT_1_EDX] & CPUID_APIC || ms->smp.cpus > 1) {
--
2.34.1
[RFC v2 4/7] i386/cpu: Support thread and module level cache topology
Allows cache to be defined at the thread and module level. This
increases flexibility for x86 users to customize their cache topology.
Signed-off-by: Zhao Liu
---
target/i386/cpu.c | 10 ++
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index b11097b5bafd..3a2dadb4bce0 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -241,9 +241,15 @@ static uint32_t max_thread_ids_for_cache(X86CPUTopoInfo
*topo_info,
uint32_t num_ids = 0;
switch (share_level) {
+case CPU_TOPO_LEVEL_THREAD:
+num_ids = 1;
+break;
case CPU_TOPO_LEVEL_CORE:
num_ids = 1 << apicid_core_offset(topo_info);
break;
+case CPU_TOPO_LEVEL_MODULE:
+num_ids = 1 << apicid_module_offset(topo_info);
+break;
case CPU_TOPO_LEVEL_DIE:
num_ids = 1 << apicid_die_offset(topo_info);
break;
@@ -251,10 +257,6 @@ static uint32_t max_thread_ids_for_cache(X86CPUTopoInfo
*topo_info,
num_ids = 1 << apicid_pkg_offset(topo_info);
break;
default:
-/*
- * Currently there is no use case for THREAD and MODULE, so use
- * assert directly to facilitate debugging.
- */
g_assert_not_reached();
}
--
2.34.1
[RFC v2 3/7] hw/core: Add cache topology options in -smp
Add "l1d-cache", "l1i-cache". "l2-cache", and "l3-cache" options in
-smp to define the cache topology for SMP system.
Signed-off-by: Zhao Liu
---
Changes since RFC v1:
* Set has_*_cache field in machine_get_smp(). (JeeHeng)
* Adjust string breaking style in error_setg() for more semantic
sentence breaking conventions. (Jonathan)
* Add more description about cache options. (Markus)
* Now in v2, config->*_cache field stores topology enumeration instead
of string, no need to parse, so just make machine_check_cache_topo()
return boolean.
---
hw/core/machine-smp.c | 146 +
hw/core/machine.c | 20 ++
qapi/machine.json | 23 ++-
system/vl.c| 12
tests/unit/meson.build | 3 +-
5 files changed, 202 insertions(+), 2 deletions(-)
diff --git a/hw/core/machine-smp.c b/hw/core/machine-smp.c
index 5d8d7edcbd3f..c79464cf3d2c 100644
--- a/hw/core/machine-smp.c
+++ b/hw/core/machine-smp.c
@@ -61,6 +61,150 @@ static char *cpu_hierarchy_to_string(MachineState *ms)
return g_string_free(s, false);
}
+static bool machine_check_topo_support(MachineState *ms,
+ CPUTopoLevel topo)
+{
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+if (topo == CPU_TOPO_LEVEL_MODULE && !mc->smp_props.modules_supported) {
+return false;
+}
+
+if (topo == CPU_TOPO_LEVEL_CLUSTER && !mc->smp_props.clusters_supported) {
+return false;
+}
+
+if (topo == CPU_TOPO_LEVEL_DIE && !mc->smp_props.dies_supported) {
+return false;
+}
+
+if (topo == CPU_TOPO_LEVEL_BOOK && !mc->smp_props.books_supported) {
+return false;
+}
+
+if (topo == CPU_TOPO_LEVEL_DRAWER && !mc->smp_props.drawers_supported) {
+return false;
+}
+
+return true;
+}
+
+static bool machine_check_cache_topo(MachineState *ms,
+ CPUTopoLevel topo,
+ Error **errp)
+{
+if (topo == CPU_TOPO_LEVEL__MAX || topo == CPU_TOPO_LEVEL_INVALID) {
+error_setg(errp,
+ "Invalid cache topology level: %s. "
+ "The cache topology should match the "
+ "valid CPU topology level",
+ cpu_topo_to_string(topo));
+return false;
+}
+
+if (!machine_check_topo_support(ms, topo)) {
+error_setg(errp,
+ "Invalid cache topology level: %s. "
+ "The topology level is not supported by this machine",
+ cpu_topo_to_string(topo));
+return false;
+}
+
+return true;
+}
+
+static void machine_parse_smp_cache_config(MachineState *ms,
+ const SMPConfiguration *config,
+ Error **errp)
+{
+MachineClass *mc = MACHINE_GET_CLASS(ms);
+
+/*
+ * The cache topology does not support a default entry similar to
+ * CPU topology with parameters=1. So when the machine explicitly
+ * does not support cache topology, return the error.
+ */
+if (config->has_l1d_cache) {
+if (!mc->smp_props.l1_separated_cache_supported) {
+error_setg(errp,
+ "L1 D-cache topology not supported by this machine");
+return;
+}
+
+if (!machine_check_cache_topo(ms, config->l1d_cache, errp)) {
+return;
+}
+
+ms->smp_cache.l1d = config->l1d_cache;
+}
+
+if (config->has_l1i_cache) {
+if (!mc->smp_props.l1_separated_cache_supported) {
+error_setg(errp,
+ "L1 I-cache topology not supported by this machine");
+return;
+}
+
+if (!machine_check_cache_topo(ms, config->l1i_cache, errp)) {
+return;
+}
+
+ms->smp_cache.l1i = config->l1i_cache;
+}
+
+if (config->has_l2_cache) {
+if (!mc->smp_props.l2_unified_cache_supported) {
+error_setg(errp,
+ "L2 cache topology not supported by this machine");
+return;
+}
+
+if (!machine_check_cache_topo(ms, config->l2_cache, errp)) {
+return;
+}
+
+ms->smp_cache.l2 = config->l2_cache;
+
+/*
+ * Cache topology is initialized by default to CPU_TOPO_LEVEL_INVALID,
+ * which is the lowest level, so such a check is OK, even if the config
+ * doesn't override that field.
+ */
+if (ms->smp_cache.l1d > ms->smp_cache.l2 ||
+ms->smp_cache.l1i > ms->smp_cache.l2) {
+error_setg(errp,
+ "Invalid L2 cache topology. "
+ "
[RFC v2 7/7] qemu-options: Add the cache topology description of -smp
Signed-off-by: Zhao Liu
---
Changes since RFC v1:
* Use "*_cache=topo_level" as -smp example as the original "level"
term for a cache has a totally different meaning. (Jonathan)
---
qemu-options.hx | 50 +++--
1 file changed, 44 insertions(+), 6 deletions(-)
diff --git a/qemu-options.hx b/qemu-options.hx
index 8ca7f34ef0c8..29d8a4b9b68b 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -282,7 +282,8 @@ ERST
DEF("smp", HAS_ARG, QEMU_OPTION_smp,
"-smp
[[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets]\n"
"
[,dies=dies][,clusters=clusters][,modules=modules][,cores=cores]\n"
-" [,threads=threads]\n"
+"
[,threads=threads][,l1d-cache=topo_level][,l1i-cache=topo_level]\n"
+" [,l2-cache=topo_level][,l3-cache=topo_level]\n"
"set the number of initial CPUs to 'n' [default=1]\n"
"maxcpus= maximum number of total CPUs, including\n"
"offline CPUs for hotplug, etc\n"
@@ -294,7 +295,11 @@ DEF("smp", HAS_ARG, QEMU_OPTION_smp,
"modules= number of modules in one cluster\n"
"cores= number of cores in one module\n"
"threads= number of threads in one core\n"
-"Note: Different machines may have different subsets of the CPU topology\n"
+"l1d-cache= topology level of L1 D-cache\n"
+"l1i-cache= topology level of L1 I-cache\n"
+"l2-cache= topology level of L2 cache\n"
+"l3-cache= topology level of L3 cache\n"
+"Note: Different machines may have different subsets of the CPU and cache
topology\n"
" parameters supported, so the actual meaning of the supported
parameters\n"
" will vary accordingly. For example, for a machine type that
supports a\n"
" three-level CPU hierarchy of sockets/cores/threads, the parameters
will\n"
@@ -308,7 +313,7 @@ DEF("smp", HAS_ARG, QEMU_OPTION_smp,
" must be set as 1 in the purpose of correct parsing.\n",
QEMU_ARCH_ALL)
SRST
-``-smp
[[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets][,dies=dies][,clusters=clusters][,modules=modules][,cores=cores][,threads=threads]``
+``-smp
[[cpus=]n][,maxcpus=maxcpus][,drawers=drawers][,books=books][,sockets=sockets][,dies=dies][,clusters=clusters][,modules=modules][,cores=cores][,threads=threads][,l1d-cache=topo_level][,l1i-cache=topo_level][,l2-cache=topo_level][,l3-cache=topo_level]``
Simulate a SMP system with '\ ``n``\ ' CPUs initially present on
the machine type board. On boards supporting CPU hotplug, the optional
'\ ``maxcpus``\ ' parameter can be set to enable further CPUs to be
@@ -322,15 +327,34 @@ SRST
Both parameters are subject to an upper limit that is determined by
the specific machine type chosen.
+CPU topology parameters include '\ ``drawers``\ ', '\ ``books``\ ',
+'\ ``sockets``\ ', '\ ``dies``\ ', '\ ``clusters``\ ', '\ ``modules``\ ',
+'\ ``cores``\ ' and '\ ``threads``\ '. These CPU parameters accept only
+integers and are used to specify the number of specific topology domains
+under the corresponding topology level.
+
To control reporting of CPU topology information, values of the topology
parameters can be specified. Machines may only support a subset of the
-parameters and different machines may have different subsets supported
-which vary depending on capacity of the corresponding CPU targets. So
-for a particular machine type board, an expected topology hierarchy can
+CPU topology parameters and different machines may have different subsets
+supported which vary depending on capacity of the corresponding CPU
targets.
+So for a particular machine type board, an expected topology hierarchy can
be defined through the supported sub-option. Unsupported parameters can
also be provided in addition to the sub-option, but their values must be
set as 1 in the purpose of correct parsing.
+Cache topology parameters include '\ ``l1d-cache``\ ', '\ ``l1i-cache``\ ',
+'\ ``l2-cache``\ ' and '\ ``l3-cache``\ '. These cache topology parameters
+accept the strings of CPU topology levels (such as '\ ``drawer``\ ', '\
``book``\ ',
+'\ ``socket``\ ', '\ ``die``\ ', '\ ``cluster``\ ', '\ ``module``\ ',
+'\ ``core``\ ' or '\ ``thread``\ '). Exactly which topology level strings
+could be accepted as the parameter depends on the machine's support for the
+corresponding CPU topology level.
+
+Machines may also only support a subse
[RFC v2 2/7] hw/core: Define cache topology for machine
Define the cache topology based on CPU topology level for two reasons:
1. In practice, a cache will always be bound to the CPU container
(either private in the CPU container or shared among multiple
containers), and CPU container is often expressed in terms of CPU
topology level.
2. The x86's cache-related CPUIDs encode cache topology based on APIC
ID's CPU topology layout. And the ACPI PPTT table that ARM/RISCV
relies on also requires CPU containers to help indicate the private
shared hierarchy of the cache. Therefore, for SMP systems, it is
natural to use the CPU topology hierarchy directly in QEMU to define
the cache topology.
Currently, separated L1 cache (L1 data cache and L1 instruction cache)
with unified higher-level cache (e.g., unified L2 and L3 caches), is the
most common cache architectures.
Therefore, define the topology for L1 D-cache, L1 I-cache, L2 cache and
L3 cache in machine as the basic cache topology support.
Signed-off-by: Zhao Liu
---
hw/core/machine.c | 5 +
include/hw/boards.h | 25 +
2 files changed, 30 insertions(+)
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 8087026b45da..e31d0f3cb4b0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -1175,6 +1175,11 @@ static void machine_initfn(Object *obj)
ms->smp.cores = 1;
ms->smp.threads = 1;
+ms->smp_cache.l1d = CPU_TOPO_LEVEL_INVALID;
+ms->smp_cache.l1i = CPU_TOPO_LEVEL_INVALID;
+ms->smp_cache.l2 = CPU_TOPO_LEVEL_INVALID;
+ms->smp_cache.l3 = CPU_TOPO_LEVEL_INVALID;
+
machine_copy_boot_config(ms, &(BootConfiguration){ 0 });
}
diff --git a/include/hw/boards.h b/include/hw/boards.h
index c1737f2a5736..e70b2a1bdca2 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -10,6 +10,7 @@
#include "qemu/module.h"
#include "qom/object.h"
#include "hw/core/cpu.h"
+#include "hw/core/cpu-topology.h"
#define TYPE_MACHINE_SUFFIX "-machine"
@@ -145,6 +146,12 @@ typedef struct {
* @books_supported - whether books are supported by the machine
* @drawers_supported - whether drawers are supported by the machine
* @modules_supported - whether modules are supported by the machine
+ * @l1_separated_cache_supported - whether l1 data and instruction cache
+ * topology are supported by the machine
+ * @l2_unified_cache_supported - whether l2 unified cache topology are
+ * supported by the machine
+ * @l3_unified_cache_supported - whether l3 unified cache topology are
+ * supported by the machine
*/
typedef struct {
bool prefer_sockets;
@@ -154,6 +161,9 @@ typedef struct {
bool books_supported;
bool drawers_supported;
bool modules_supported;
+bool l1_separated_cache_supported;
+bool l2_unified_cache_supported;
+bool l3_unified_cache_supported;
} SMPCompatProps;
/**
@@ -359,6 +369,20 @@ typedef struct CPUTopology {
unsigned int max_cpus;
} CPUTopology;
+/**
+ * CPUTopology:
+ * @l1d: the CPU topology hierarchy the L1 data cache is shared at.
+ * @l1i: the CPU topology hierarchy the L1 instruction cache is shared at.
+ * @l2: the CPU topology hierarchy the L2 (unified) cache is shared at.
+ * @l3: the CPU topology hierarchy the L3 (unified) cache is shared at.
+ */
+typedef struct CacheTopology {
+CPUTopoLevel l1d;
+CPUTopoLevel l1i;
+CPUTopoLevel l2;
+CPUTopoLevel l3;
+} CacheTopology;
+
/**
* MachineState:
*/
@@ -410,6 +434,7 @@ struct MachineState {
AccelState *accelerator;
CPUArchIdList *possible_cpus;
CPUTopology smp;
+CacheTopology smp_cache;
struct NVDIMMState *nvdimms_state;
struct NumaState *numa_state;
};
--
2.34.1
[RFC v2 0/7] Introduce SMP Cache Topology
he cache topology for Guest that is nearly identical to Host.
This is necessary in cases where there are bound vCPUs, especially
considering that Guest scheduling often takes into account the cache
topology as well (e.g. Linux cluster aware scheduling, i.e. L2 cache
scheduling).
Previously, we introduced a x86 specific option to adjust the cache
topology:
-cpu x-l2-cache-topo=[core|module] [4]
However, considering the needs of other arches, we re-implemented the
generic cache topology (aslo in response to Michael's [5] and Daniel's
comment [6]) in this series.
Cache Topology Representation
-
We consider to define the cache topology based on CPU topology level for
two reasons:
1. In practice, a cache will always be bound to the CPU container -
"CPU container" indicates to a set of CPUs that refer to a certain
level of CPU topology - where the cache is either private in that
CPU container or shared among multiple containers.
2. The x86's cache-related CPUIDs encode cache topology based on APIC
ID's CPU topology layout. And the ACPI PPTT table that ARM/RISCV
relies on also requires CPU containers (CPU topology) to help
indicate the private shared hierarchy of the cache.
Therefore, for SMP systems, it is natural to use the CPU topology
hierarchy directly in QEMU to define the cache topology.
And currently, separated L1 cache (L1 data cache and L1 instruction
cache) with unified higher-level caches (e.g., unified L2 and L3
caches), is the most common cache architectures.
Thus, we define the topology for L1 D-cache, L1 I-cache, L2 cache and L3
cache in MachineState as the basic cache topology support:
typedef struct CacheTopology {
CPUTopoLevel l1d;
CPUTopoLevel l1i;
CPUTopoLevel l2;
CPUTopoLevel l3;
} CacheTopology;
Machines may also only support a subset of the cache topology
to be configured in -smp by setting the SMP property of MachineClass:
typedef struct {
...
bool l1_separated_cache_supported;
bool l2_unified_cache_supported;
bool l3_unified_cache_supported;
} SMPCompatProps;
Cache Topology Configuration in -smp
Further, we add new parameters to -smp:
* l1d-cache=topo_level
* l1i-cache=topo_level
* l2-cache=topo_level
* l3-cache=topo_level
These cache topology parameters accept the strings of CPU topology
levels (such as "drawer", "book", "socket", "die", "cluster", "module",
"core" or "thread"). Exactly which topology level strings could be
accepted as the parameter depends on the machine's support for the
corresponding CPU topology level.
Unsupported cache topology parameters will cause error.
In this series, we add the cache topology support in -smp for x86 PC
machine.
The following example defines a 3-level cache topology hierarchy (L1
D-cache per core, L1 I-cache per core, L2 cache per core and L3 cache per
die) for PC machine.
-smp 32,sockets=2,dies=2,modules=2,cores=2,threads=2,maxcpus=32,\
l1d-cache=core,l1i-cache=core,l2-cache=core,l3-cache=die
Reference
-
[1]: [ARM] Jonathan's proposal to adjust cache topology:
https://lore.kernel.org/qemu-devel/20230808115713.2613-2-jonathan.came...@huawei.com/
[2]: [RISC-V] Discussion between JeeHeng and Jonathan about cache
topology:
https://lore.kernel.org/qemu-devel/20240131155336.6...@huawei.com/
[3]: Discussion with Daniel about default cache topology:
https://lore.kernel.org/qemu-devel/zktrsddygrfzv...@redhat.com/
[4]: Previous x86 specific cache topology option:
https://lore.kernel.org/qemu-devel/20230914072159.1177582-22-zhao1@linux.intel.com/
[5]: Michael's comment about generic cache topology support:
https://lore.kernel.org/qemu-devel/20231003085516-mutt-send-email-...@kernel.org/
[6]: Daniel's question about how x86 support L2 cache domain (cluster)
configuration:
https://lore.kernel.org/qemu-devel/zcug0uc8kyleq...@redhat.com/
Thanks and Best Regards,
Zhao
---
Changelog:
Main changes since RFC v1:
* Split CpuTopology renaimg out of this RFC.
* Use QAPI to enumerate CPU topology levels.
* Drop string_to_cpu_topo() since QAPI will help to parse the topo
levels.
* Set has_*_cache field in machine_get_smp(). (JeeHeng)
* Use "*_cache=topo_level" as -smp example as the original "level"
term for a cache has a totally different meaning. (Jonathan)
---
Zhao Liu (7):
hw/core: Make CPU topology enumeration arch-agnostic
hw/core: Define cache topology for machine
hw/core: Add cache topology options in -smp
i386/cpu: Support thread and module level cache topology
i386/cpu: Update cache topology with machine's configuration
i386/pc: Support cache topology in -smp for PC machine
qemu-options: Add the cache topology description of -smp
MAINTAINERS| 2 +
hw/core/cpu-topology.c | 36
hw/core/ma
[RFC v2 6/7] i386/pc: Support cache topology in -smp for PC machine
Signed-off-by: Zhao Liu --- hw/i386/pc.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 7b638da7aaa8..2e03b33a4116 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1844,6 +1844,9 @@ static void pc_machine_class_init(ObjectClass *oc, void *data) mc->nvdimm_supported = true; mc->smp_props.dies_supported = true; mc->smp_props.modules_supported = true; +mc->smp_props.l1_separated_cache_supported = true; +mc->smp_props.l2_unified_cache_supported = true; +mc->smp_props.l3_unified_cache_supported = true; mc->default_ram_id = "pc.ram"; pcmc->default_smbios_ep_type = SMBIOS_ENTRY_POINT_TYPE_AUTO; -- 2.34.1
Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo framework
Hi Stefan, On Tue, May 28, 2024 at 10:14:01AM -0400, Stefan Hajnoczi wrote: > Date: Tue, 28 May 2024 10:14:01 -0400 > From: Stefan Hajnoczi > Subject: Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo > framework > > On Tue, May 28, 2024 at 03:53:55PM +0800, Zhao Liu wrote: > > Hi Stefan, > > > > [snip] > > > > > > diff --git a/scripts/simpletrace-rust/.rustfmt.toml > > > > b/scripts/simpletrace-rust/.rustfmt.toml > > > > new file mode 100644 > > > > index ..97a97c24ebfb > > > > --- /dev/null > > > > +++ b/scripts/simpletrace-rust/.rustfmt.toml > > > > @@ -0,0 +1,9 @@ > > > > +brace_style = "AlwaysNextLine" > > > > +comment_width = 80 > > > > +edition = "2021" > > > > +group_imports = "StdExternalCrate" > > > > +imports_granularity = "item" > > > > +max_width = 80 > > > > +use_field_init_shorthand = true > > > > +use_try_shorthand = true > > > > +wrap_comments = true > > > > > > There should be QEMU-wide policy. That said, why is it necessary to > > > customize rustfmt? > > > > Indeed, but QEMU's style for Rust is currently undefined, so I'm trying > > to add this to make it easier to check the style...I will separate it > > out as a style policy proposal. > > Why is a config file necessary? QEMU should use the default Rust style. > There are some that may be overdone, but I think some basic may still be necessary, like "comment_width = 80", "max_width = 80", "wrap_comments". Is it necessary to specify the width? As C. And, "group_imports" and "imports_granularity" (refered from crosvm), can also be used to standardize including styles and improve readability, since importing can be done in many different styles. This fmt config is something like ./script/check_patch.pl for QEMU/linux. Different programs have different practices, so I feel like that's an open too! This certainly also depends on the maintainer's/your preferences, ;-) in what way looks more comfortable/convenient that is the best, completely according to the default is also good.
Re: [RFC 0/6] scripts: Rewrite simpletrace printer in Rust
Hi Stefan and Mads, On Wed, May 29, 2024 at 11:33:42AM +0200, Mads Ynddal wrote: > Date: Wed, 29 May 2024 11:33:42 +0200 > From: Mads Ynddal > Subject: Re: [RFC 0/6] scripts: Rewrite simpletrace printer in Rust > X-Mailer: Apple Mail (2.3774.600.62) > > > >> Maybe later, Rust-simpletrace and python-simpletrace can differ, e.g. > >> the former goes for performance and the latter for scalability. > > > > Rewriting an existing, maintained component without buy-in from the > > maintainers is risky. Mads is the maintainer of simpletrace.py and I am > > the overall tracing maintainer. While the performance improvement is > > nice, I'm a skeptical about the need for this and wonder whether thought > > was put into how simpletrace should evolve. > > > > There are disadvantages to maintaining multiple implementations: > > - File format changes need to be coordinated across implementations to > > prevent compatibility issues. In other words, changing the > > trace-events format becomes harder and discourages future work. > > - Multiple implementations makes life harder for users because they need > > to decide between implementations and understand the trade-offs. > > - There is more maintenance overall. > > > > I think we should have a single simpletrace implementation to avoid > > these issues. The Python implementation is more convenient for > > user-written trace analysis scripts. The Rust implementation has better > > performance (although I'm not aware of efforts to improve the Python > > implementation's performance, so who knows). > > > > I'm ambivalent about why a reimplementation is necessary. What I would > > like to see first is the TCG binary tracing functionality. Find the > > limits of the Python simpletrace implementation and then it will be > > clear whether a Rust reimplementation makes sense. > > > > If Mads agrees, I am happy with a Rust reimplementation, but please > > demonstrate why a reimplementation is necessary first. > > > > Stefan > > I didn't want to shoot down the idea, since it seemed like somebody had a plan > with GSoC. But I actually agree, that I'm not quite convinced. > > I think I'd need to see some data that showed the Python version is > inadequate. > I know Python is not the fastest, but is it so prohibitively slow, that we > cannot make the TCG analysis? I'm not saying it can't be true, but it'd be > nice > to see it demonstrated before making decisions. > > Because, as you point out, there's a lot of downsides to having two versions. > So > the benefits have to clearly outweigh the additional work. > > I have a lot of other questions, but let's maybe start with the core idea > first. > > — > Mads Ynddal > I really appreciate your patience and explanations, and your feedback and review has helped me a lot! Yes, simple repetition creates much maintenance burden (though I'm happy to help with), and the argument for current performance isn't convincing enough. Getting back to the project itself, as you have said, the core is still further support for TCG-related traces, and I'll continue to work on it, and then look back based on such work to see what issues there are with traces or what improvements can be made. Thanks again! Regards, Zhao
Re: [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock feature name
Hi Paolo, Sorry to re-pick this series, is it an acceptable cleanup to separate the current kvmclock/kvmclock2 if the old kvmclock can't be dropped? Thanks, Zhao On Fri, Mar 29, 2024 at 06:19:47PM +0800, Zhao Liu wrote: > Date: Fri, 29 Mar 2024 18:19:47 +0800 > From: Zhao Liu > Subject: [PATCH for-9.1 0/7] target/i386/kvm: Cleanup the kvmclock feature > name > X-Mailer: git-send-email 2.34.1 > > From: Zhao Liu > > Hi list, > > This series is based on Paolo's guest_phys_bits patchset [1]. > > Currently, the old and new kvmclocks have the same feature name > "kvmclock" in FeatureWordInfo[FEAT_KVM]. > > When I tried to dig into the history of this unusual naming and fix it, > I realized that Tim was already trying to rename it, so I picked up his > renaming patch [2] (with a new commit message and other minor changes). > > 13 years age, the same name was introduced in [3], and its main purpose > is to make it easy for users to enable/disable 2 kvmclocks. Then, in > 2012, Don tried to rename the new kvmclock, but the follow-up did not > address Igor and Eduardo's comments about compatibility. > > Tim [2], not long ago, and I just now, were both puzzled by the naming > one after the other. > > So, this series is to push for renaming the new kvmclock feature to > "kvmclock2" and adding compatibility support for older machines (PC 9.0 > and older). > > Finally, let's put an end to decades of doubt about this name. > > > Next Step > = > > This series just separates the two kvmclocks from the naming, and in > subsequent patches I plan to stop setting kvmclock(old kcmclock) by > default as long as KVM supports kvmclock2 (new kvmclock). > > Also, try to deprecate the old kvmclock in KVM side. > > [1]: > https://lore.kernel.org/qemu-devel/20240325141422.1380087-1-pbonz...@redhat.com/ > [2]: > https://lore.kernel.org/qemu-devel/20230908124534.25027-4-twied...@redhat.com/ > [3]: > https://lore.kernel.org/qemu-devel/1300401727-5235-3-git-send-email-glom...@redhat.com/ > [4]: > https://lore.kernel.org/qemu-devel/1348171412-23669-3-git-send-email-...@cloudswitch.com/ > > Thanks and Best Regards, > Zhao > > --- > Tim Wiederhake (1): > target/i386: Fix duplicated kvmclock name in FEAT_KVM > > Zhao Liu (6): > target/i386/kvm: Add feature bit definitions for KVM CPUID > target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and > MSR_KVM_SYSTEM_TIME definitions > target/i386/kvm: Only Save/load kvmclock MSRs when kvmclock enabled > target/i386/kvm: Save/load MSRs of new kvmclock > (KVM_FEATURE_CLOCKSOURCE2) > target/i386/kvm: Add legacy_kvmclock cpu property > target/i386/kvm: Update comment in kvm_cpu_realizefn() > > hw/i386/kvm/clock.c | 5 ++-- > hw/i386/pc.c | 1 + > target/i386/cpu.c | 3 +- > target/i386/cpu.h | 32 + > target/i386/kvm/kvm-cpu.c | 25 - > target/i386/kvm/kvm.c | 59 +-- > 6 files changed, 99 insertions(+), 26 deletions(-) > > -- > 2.34.1 >
Re: [PATCH v2 0/6] target/i386: Misc cleanup on KVM PV defs and outdated comments
Hi mainatainers, Just a friendly ping. Thanks, Zhao On Mon, May 06, 2024 at 04:51:47PM +0800, Zhao Liu wrote: > Date: Mon, 6 May 2024 16:51:47 +0800 > From: Zhao Liu > Subject: [PATCH v2 0/6] target/i386: Misc cleanup on KVM PV defs and > outdated comments > X-Mailer: git-send-email 2.34.1 > > Hi, > > This is my v2 cleanup series. Compared with v1 [1], only tags (R/b, S/b) > updates, and a typo fix, no code change. > > This series picks cleanup from my previous kvmclock [2] (as other > renaming attempts were temporarily put on hold). > > In addition, this series also include the cleanup on a historically > workaround and recent comment of coco interface [3]. > > Avoiding the fragmentation of these misc cleanups, I consolidated them > all in one series and was able to tackle them in one go! > > [1]: > https://lore.kernel.org/qemu-devel/20240426100716.2111688-1-zhao1@intel.com/ > [2]: > https://lore.kernel.org/qemu-devel/20240329101954.3954987-1-zhao1@linux.intel.com/ > [3]: > https://lore.kernel.org/qemu-devel/2815f0f1-9e20-4985-849c-d74c6cdc9...@intel.com/ > > Thanks and Best Regards, > Zhao > --- > Zhao Liu (6): > target/i386/kvm: Add feature bit definitions for KVM CPUID > target/i386/kvm: Remove local MSR_KVM_WALL_CLOCK and > MSR_KVM_SYSTEM_TIME definitions > target/i386/kvm: Only save/load kvmclock MSRs when kvmclock enabled > target/i386/kvm: Save/load MSRs of kvmclock2 > (KVM_FEATURE_CLOCKSOURCE2) > target/i386/kvm: Drop workaround for KVM_X86_DISABLE_EXITS_HTL typo > target/i386/confidential-guest: Fix comment of > x86_confidential_guest_kvm_type() > > hw/i386/kvm/clock.c | 5 +-- > target/i386/confidential-guest.h | 2 +- > target/i386/cpu.h| 25 + > target/i386/kvm/kvm.c| 63 +++- > 4 files changed, 66 insertions(+), 29 deletions(-) > > -- > 2.34.1 >
Re: [PATCH v5 20/23] hw/i386/pc: Remove deprecated pc-i440fx-2.3 machine
On Wed, May 29, 2024 at 07:15:36AM +0200, Philippe Mathieu-Daudé wrote: > Date: Wed, 29 May 2024 07:15:36 +0200 > From: Philippe Mathieu-Daudé > Subject: [PATCH v5 20/23] hw/i386/pc: Remove deprecated pc-i440fx-2.3 > machine > X-Mailer: git-send-email 2.41.0 > > The pc-i440fx-2.3 machine was deprecated for the 8.2 > release (see commit c7437f0ddb "docs/about: Mark the > old pc-i440fx-2.0 - 2.3 machine types as deprecated"), > time to remove it. > > Signed-off-by: Philippe Mathieu-Daudé > --- > docs/about/deprecated.rst | 4 ++-- > docs/about/removed-features.rst | 2 +- > hw/i386/pc.c| 25 - > hw/i386/pc_piix.c | 19 --- > 4 files changed, 3 insertions(+), 47 deletions(-) Reviewed-by: Zhao Liu
Re: [PATCH v5 21/23] hw/i386/pc: Simplify DEFINE_I440FX_MACHINE() macro
On Wed, May 29, 2024 at 07:15:37AM +0200, Philippe Mathieu-Daudé wrote: > Date: Wed, 29 May 2024 07:15:37 +0200 > From: Philippe Mathieu-Daudé > Subject: [PATCH v5 21/23] hw/i386/pc: Simplify DEFINE_I440FX_MACHINE() macro > X-Mailer: git-send-email 2.41.0 > > Last commit removed the last non-NULL use of DEFINE_I440FX_MACHINE > 3rd parameter. 'compatfn' is now obsolete, remove it. > > Suggested-by: Daniel P. Berrangé > Signed-off-by: Philippe Mathieu-Daudé > --- > hw/i386/pc_piix.c | 62 ++- > 1 file changed, 29 insertions(+), 33 deletions(-) Reviewed-by: Zhao Liu
[PATCH 3/8] tests/unit/test-smp-parse: Fix an invalid topology case
Adjust the "cpus" parameter to match the comment configuration.
Signed-off-by: Zhao Liu
---
tests/unit/test-smp-parse.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index c9cbc89c21b9..5d99e0d9234c 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -528,7 +528,7 @@ static const struct SMPTestData data_full_topo_invalid[] = {
* config: -smp 1,drawers=3,books=5,sockets=2,dies=4,\
* clusters=2,cores=7,threads=3,maxcpus=5040
*/
-.config = SMP_CONFIG_WITH_FULL_TOPO(3361, 3, 5, 2, 4, 2, 7, 3, 5040),
+.config = SMP_CONFIG_WITH_FULL_TOPO(1, 3, 5, 2, 4, 2, 7, 3, 5040),
.expect_error = "Invalid SMP CPUs 5040. The max CPUs supported "
"by machine '" SMP_MACHINE_NAME "' is 4096",
},
--
2.34.1
[PATCH 8/8] tests/unit/test-smp-parse: Test the full 8-levels topology hierarchy
With module level, QEMU now support 8-levels topology hierarchy.
Cover "modules" in SMP_CONFIG_WITH_FULL_TOPO related cases.
Signed-off-by: Zhao Liu
---
tests/unit/test-smp-parse.c | 129
1 file changed, 85 insertions(+), 44 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 2ca8530e935e..f9bccb56abc7 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -94,11 +94,11 @@
}
/*
- * Currently QEMU supports up to a 7-level topology hierarchy, which is the
+ * Currently QEMU supports up to a 8-level topology hierarchy, which is the
* QEMU's unified abstract representation of CPU topology.
- * -drawers/books/sockets/dies/clusters/cores/threads
+ * -drawers/books/sockets/dies/clusters/modules/cores/threads
*/
-#define SMP_CONFIG_WITH_FULL_TOPO(a, b, c, d, e, f, g, h, i)\
+#define SMP_CONFIG_WITH_FULL_TOPO(a, b, c, d, e, f, g, h, i, j) \
{ \
.has_cpus = true, .cpus = a,\
.has_drawers = true, .drawers = b,\
@@ -106,9 +106,10 @@
.has_sockets = true, .sockets = d,\
.has_dies = true, .dies = e,\
.has_clusters = true, .clusters = f,\
-.has_cores= true, .cores= g,\
-.has_threads = true, .threads = h,\
-.has_maxcpus = true, .maxcpus = i,\
+.has_modules = true, .modules = g,\
+.has_cores= true, .cores= h,\
+.has_threads = true, .threads = i,\
+.has_maxcpus = true, .maxcpus = j,\
}
/**
@@ -336,10 +337,10 @@ static const struct SMPTestData data_generic_valid[] = {
/*
* Unsupported parameters are always allowed to be set to '1'
* config:
- * -smp 8,drawers=1,books=1,sockets=2,dies=1,clusters=1,cores=2,\
- *threads=2,maxcpus=8
+ * -smp 8,drawers=1,books=1,sockets=2,dies=1,clusters=1,modules=1,\
+ *cores=2,threads=2,maxcpus=8
* expect: cpus=8,sockets=2,cores=2,threads=2,maxcpus=8 */
-.config = SMP_CONFIG_WITH_FULL_TOPO(8, 1, 1, 2, 1, 1, 2, 2, 8),
+.config = SMP_CONFIG_WITH_FULL_TOPO(8, 1, 1, 2, 1, 1, 1, 2, 2, 8),
.expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
.expect_prefer_cores = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
},
@@ -561,32 +562,37 @@ static const struct SMPTestData data_full_topo_invalid[]
= {
{
/*
* config: -smp 200,drawers=3,books=5,sockets=2,dies=4,\
- * clusters=2,cores=7,threads=2,maxcpus=200
+ * clusters=2,modules=3,cores=7,threads=2,\
+ * maxcpus=200
*/
-.config = SMP_CONFIG_WITH_FULL_TOPO(200, 3, 5, 2, 4, 2, 7, 2, 200),
+.config = SMP_CONFIG_WITH_FULL_TOPO(200, 3, 5, 2, 4, 2, 3, 7, 2, 200),
.expect_error = "Invalid CPU topology: "
"product of the hierarchy must match maxcpus: "
"drawers (3) * books (5) * sockets (2) * dies (4) * "
-"clusters (2) * cores (7) * threads (2) "
+"clusters (2) * modules (3) * cores (7) * threads (2) "
"!= maxcpus (200)",
}, {
/*
- * config: -smp 3361,drawers=3,books=5,sockets=2,dies=4,\
- * clusters=2,cores=7,threads=2,maxcpus=3360
+ * config: -smp 2881,drawers=3,books=5,sockets=2,dies=4,\
+ * clusters=2,modules=3,cores=2,threads=2,
+ * maxcpus=2880
*/
-.config = SMP_CONFIG_WITH_FULL_TOPO(3361, 3, 5, 2, 4, 2, 7, 2, 3360),
+.config = SMP_CONFIG_WITH_FULL_TOPO(2881, 3, 5, 2, 4,
+2, 3, 2, 2, 2880),
.expect_error = "Invalid CPU topology: "
"maxcpus must be equal to or greater than smp: "
-"drawers (3) * books (5) * sockets (2) * dies (4) * "
-"clusters (2) * cores (7) * threads (2) "
-"== maxcpus (3360) < smp_cpus (3361)",
+"drawers (3) * books (5) * sockets (2) * "
+"dies (4) * clusters (2) * modules (3) * "
+"cores (2) * threads (2) == maxcpus (2880) "
+"< smp_cpus (2881)",
}, {
/*
* config: -smp 1,drawers=3,books=5,sockets=2,dies=4,\
- * clusters=
[PATCH 4/8] tests/unit/test-smp-parse: Use default parameters=0 when not set in -smp
Since -smp allows parameters=1 whether the level is supported by
machine, to avoid the test scenarios where the parameter defaults to 1
cause some errors to be masked, explicitly set undesired parameters to
0.
Signed-off-by: Zhao Liu
---
tests/unit/test-smp-parse.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 5d99e0d9234c..e3a0a9d12d05 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -436,7 +436,7 @@ static const struct SMPTestData
data_with_clusters_invalid[] = {
static const struct SMPTestData data_with_books_invalid[] = {
{
/* config: -smp 16,books=2,sockets=2,cores=4,threads=2,maxcpus=16 */
-.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 16, F, 1, T, 2, T,
+.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 16, F, 0, T, 2, T,
2, T, 4, T, 2, T, 16),
.expect_error = "Invalid CPU topology: "
"product of the hierarchy must match maxcpus: "
@@ -444,7 +444,7 @@ static const struct SMPTestData data_with_books_invalid[] =
{
"!= maxcpus (16)",
}, {
/* config: -smp 34,books=2,sockets=2,cores=4,threads=2,maxcpus=32 */
-.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 34, F, 1, T, 2, T,
+.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 34, F, 0, T, 2, T,
2, T, 4, T, 2, T, 32),
.expect_error = "Invalid CPU topology: "
"maxcpus must be equal to or greater than smp: "
@@ -456,7 +456,7 @@ static const struct SMPTestData data_with_books_invalid[] =
{
static const struct SMPTestData data_with_drawers_invalid[] = {
{
/* config: -smp 16,drawers=2,sockets=2,cores=4,threads=2,maxcpus=16 */
-.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 16, T, 2, F, 1, T,
+.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 16, T, 2, F, 0, T,
2, T, 4, T, 2, T, 16),
.expect_error = "Invalid CPU topology: "
"product of the hierarchy must match maxcpus: "
@@ -464,7 +464,7 @@ static const struct SMPTestData data_with_drawers_invalid[]
= {
"!= maxcpus (16)",
}, {
/* config: -smp 34,drawers=2,sockets=2,cores=4,threads=2,maxcpus=32 */
-.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 34, T, 2, F, 1, T,
+.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 34, T, 2, F, 0, T,
2, T, 4, T, 2, T, 32),
.expect_error = "Invalid CPU topology: "
"maxcpus must be equal to or greater than smp: "
--
2.34.1
[PATCH 2/8] tests/unit/test-smp-parse: Fix comment of parameters=1 case
SMP_CONFIG_WITH_FULL_TOPO hasn't support module level, so the parameter
should indicate the "clusters".
Additionally, reorder the parameters of -smp to match the topology
hierarchy order.
Signed-off-by: Zhao Liu
---
tests/unit/test-smp-parse.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index fa8e7d83a7b6..c9cbc89c21b9 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -333,7 +333,9 @@ static const struct SMPTestData data_generic_valid[] = {
}, {
/*
* Unsupported parameters are always allowed to be set to '1'
- * config: -smp
8,books=1,drawers=1,sockets=2,modules=1,dies=1,cores=2,threads=2,maxcpus=8
+ * config:
+ * -smp 8,drawers=1,books=1,sockets=2,dies=1,clusters=1,cores=2,\
+ *threads=2,maxcpus=8
* expect: cpus=8,sockets=2,cores=2,threads=2,maxcpus=8 */
.config = SMP_CONFIG_WITH_FULL_TOPO(8, 1, 1, 2, 1, 1, 2, 2, 8),
.expect_prefer_sockets = CPU_TOPOLOGY_GENERIC(8, 2, 2, 2, 8),
--
2.34.1
[PATCH 7/8] tests/unit/test-smp-parse: Test "modules" and "dies" combination case
Since i386 PC machine supports both "modules" and "dies" in -smp, add the
"modules" and "dies" combination test case to match the actual topology
usage scenario.
Signed-off-by: Zhao Liu
---
tests/unit/test-smp-parse.c | 103
1 file changed, 103 insertions(+)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 01832e5eda32..2ca8530e935e 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -445,6 +445,33 @@ static const struct SMPTestData data_with_dies_invalid[] =
{
},
};
+static const struct SMPTestData data_with_modules_dies_invalid[] = {
+{
+/*
+ * config: -smp 200,sockets=3,dies=5,modules=2,cores=4,\
+ * threads=2,maxcpus=200
+ */
+.config = SMP_CONFIG_WITH_MODS_DIES(T, 200, T, 3, T, 5, T,
+2, T, 4, T, 2, T, 200),
+.expect_error = "Invalid CPU topology: "
+"product of the hierarchy must match maxcpus: "
+"sockets (3) * dies (5) * modules (2) * "
+"cores (4) * threads (2) != maxcpus (200)",
+}, {
+/*
+ * config: -smp 242,sockets=3,dies=5,modules=2,cores=4,\
+ * threads=2,maxcpus=240
+ */
+.config = SMP_CONFIG_WITH_MODS_DIES(T, 242, T, 3, T, 5, T,
+2, T, 4, T, 2, T, 240),
+.expect_error = "Invalid CPU topology: "
+"maxcpus must be equal to or greater than smp: "
+"sockets (3) * dies (5) * modules (2) * "
+"cores (4) * threads (2) "
+"== maxcpus (240) < smp_cpus (242)",
+},
+};
+
static const struct SMPTestData data_with_clusters_invalid[] = {
{
/* config: -smp 16,sockets=2,clusters=2,cores=4,threads=2,maxcpus=16 */
@@ -905,6 +932,14 @@ static void machine_with_dies_class_init(ObjectClass *oc,
void *data)
mc->smp_props.dies_supported = true;
}
+static void machine_with_modules_dies_class_init(ObjectClass *oc, void *data)
+{
+MachineClass *mc = MACHINE_CLASS(oc);
+
+mc->smp_props.modules_supported = true;
+mc->smp_props.dies_supported = true;
+}
+
static void machine_with_clusters_class_init(ObjectClass *oc, void *data)
{
MachineClass *mc = MACHINE_CLASS(oc);
@@ -1082,6 +1117,67 @@ static void test_with_dies(const void *opaque)
object_unref(obj);
}
+static void test_with_modules_dies(const void *opaque)
+{
+const char *machine_type = opaque;
+Object *obj = object_new(machine_type);
+MachineState *ms = MACHINE(obj);
+MachineClass *mc = MACHINE_GET_CLASS(obj);
+SMPTestData data = {};
+unsigned int num_modules = 5, num_dies = 3;
+int i;
+
+for (i = 0; i < ARRAY_SIZE(data_generic_valid); i++) {
+data = data_generic_valid[i];
+unsupported_params_init(mc, );
+
+/*
+ * when modules and dies parameters are omitted, they will
+ * be both set as 1.
+ */
+data.expect_prefer_sockets.modules = 1;
+data.expect_prefer_sockets.dies = 1;
+data.expect_prefer_cores.modules = 1;
+data.expect_prefer_cores.dies = 1;
+
+smp_parse_test(ms, , true);
+
+/* when modules and dies parameters are both specified */
+data.config.has_modules = true;
+data.config.modules = num_modules;
+data.config.has_dies = true;
+data.config.dies = num_dies;
+
+if (data.config.has_cpus) {
+data.config.cpus *= num_modules * num_dies;
+}
+if (data.config.has_maxcpus) {
+data.config.maxcpus *= num_modules * num_dies;
+}
+
+data.expect_prefer_sockets.modules = num_modules;
+data.expect_prefer_sockets.dies = num_dies;
+data.expect_prefer_sockets.cpus *= num_modules * num_dies;
+data.expect_prefer_sockets.max_cpus *= num_modules * num_dies;
+
+data.expect_prefer_cores.modules = num_modules;
+data.expect_prefer_cores.dies = num_dies;
+data.expect_prefer_cores.cpus *= num_modules * num_dies;
+data.expect_prefer_cores.max_cpus *= num_modules * num_dies;
+
+smp_parse_test(ms, , true);
+}
+
+for (i = 0; i < ARRAY_SIZE(data_with_modules_dies_invalid); i++) {
+data = data_with_modules_dies_invalid[i];
+unsupported_params_init(mc, );
+
+smp_parse_test(ms, , false);
+}
+
+object_unref(obj);
+}
+
static void test_with_clusters(const void *opaque)
{
const char *machine_type = opaque;
@@ -1398,6 +1494,10 @@ static const TypeInfo smp_machine_types[] = {
.name = MACHINE_TYPE_NAME("smp-with-dies"),
.parent = TYPE_MACHINE,
.class_ini
[PATCH 6/8] tests/unit/test-smp-parse: Test "modules" parameter in -smp
Cover the module cases in test-smp-parse.
Signed-off-by: Zhao Liu
---
tests/unit/test-smp-parse.c | 112 +---
1 file changed, 103 insertions(+), 9 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 2214e47ba9c0..01832e5eda32 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -48,17 +48,19 @@
}
/*
- * Currently a 4-level topology hierarchy is supported on PC machines
- * -sockets/dies/cores/threads
+ * Currently a 5-level topology hierarchy is supported on PC machines
+ * -sockets/dies/modules/cores/threads
*/
-#define SMP_CONFIG_WITH_DIES(ha, a, hb, b, hc, c, hd, d, he, e, hf, f) \
+#define SMP_CONFIG_WITH_MODS_DIES(ha, a, hb, b, hc, c, hd, d, \
+ he, e, hf, f, hg, g)\
{ \
.has_cpus= ha, .cpus= a, \
.has_sockets = hb, .sockets = b, \
.has_dies= hc, .dies= c, \
-.has_cores = hd, .cores = d, \
-.has_threads = he, .threads = e, \
-.has_maxcpus = hf, .maxcpus = f, \
+.has_modules = hd, .modules = d, \
+.has_cores = he, .cores = e, \
+.has_threads = hf, .threads = f, \
+.has_maxcpus = hg, .maxcpus = g, \
}
/*
@@ -345,8 +347,14 @@ static const struct SMPTestData data_generic_valid[] = {
static const struct SMPTestData data_generic_invalid[] = {
{
+/* config: -smp 2,modules=2 */
+.config = SMP_CONFIG_WITH_MODS_DIES(T, 2, F, 0, F, 0, T, 2,
+F, 0, F, 0, F, 0),
+.expect_error = "modules > 1 not supported by this machine's CPU
topology",
+}, {
/* config: -smp 2,dies=2 */
-.config = SMP_CONFIG_WITH_DIES(T, 2, F, 0, T, 2, F, 0, F, 0, F, 0),
+.config = SMP_CONFIG_WITH_MODS_DIES(T, 2, F, 0, T, 2, F, 0,
+F, 0, F, 0, F, 0),
.expect_error = "dies > 1 not supported by this machine's CPU
topology",
}, {
/* config: -smp 2,clusters=2 */
@@ -397,17 +405,39 @@ static const struct SMPTestData data_generic_invalid[] = {
},
};
+static const struct SMPTestData data_with_modules_invalid[] = {
+{
+/* config: -smp 16,sockets=2,modules=2,cores=4,threads=2,maxcpus=16 */
+.config = SMP_CONFIG_WITH_MODS_DIES(T, 16, T, 2, F, 0, T, 2,
+T, 4, T, 2, T, 16),
+.expect_error = "Invalid CPU topology: "
+"product of the hierarchy must match maxcpus: "
+"sockets (2) * modules (2) * cores (4) * threads (2) "
+"!= maxcpus (16)",
+}, {
+/* config: -smp 34,sockets=2,modules=2,cores=4,threads=2,maxcpus=32 */
+.config = SMP_CONFIG_WITH_MODS_DIES(T, 34, T, 2, F, 0, T, 2,
+T, 4, T, 2, T, 32),
+.expect_error = "Invalid CPU topology: "
+"maxcpus must be equal to or greater than smp: "
+"sockets (2) * modules (2) * cores (4) * threads (2) "
+"== maxcpus (32) < smp_cpus (34)",
+},
+};
+
static const struct SMPTestData data_with_dies_invalid[] = {
{
/* config: -smp 16,sockets=2,dies=2,cores=4,threads=2,maxcpus=16 */
-.config = SMP_CONFIG_WITH_DIES(T, 16, T, 2, T, 2, T, 4, T, 2, T, 16),
+.config = SMP_CONFIG_WITH_MODS_DIES(T, 16, T, 2, T, 2, F, 0,
+T, 4, T, 2, T, 16),
.expect_error = "Invalid CPU topology: "
"product of the hierarchy must match maxcpus: "
"sockets (2) * dies (2) * cores (4) * threads (2) "
"!= maxcpus (16)",
}, {
/* config: -smp 34,sockets=2,dies=2,cores=4,threads=2,maxcpus=32 */
-.config = SMP_CONFIG_WITH_DIES(T, 34, T, 2, T, 2, T, 4, T, 2, T, 32),
+.config = SMP_CONFIG_WITH_MODS_DIES(T, 34, T, 2, T, 2, F, 0,
+T, 4, T, 2, T, 32),
.expect_error = "Invalid CPU topology: "
"maxcpus must be equal to or greater than smp: "
"sockets (2) * dies (2) * cores (4) * threads (2) "
@@ -861,6 +891,13 @@ static void machine_generic_invalid_class_init(ObjectClass
*oc, void *data)
mc->max_cpus = MAX_CPUS - 1;
}
+static void machine_with_modules_class_init(ObjectClass *o
[PATCH 5/8] tests/unit/test-smp-parse: Make test cases aware of module level
Currently, -smp supports module level.
It is necessary to consider the effects of module in the test cases to
ensure that the calculations are correct. This is also the preparation
to add module test cases.
Signed-off-by: Zhao Liu
---
tests/unit/test-smp-parse.c | 13 +++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index e3a0a9d12d05..2214e47ba9c0 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -629,6 +629,7 @@ static char *smp_config_to_string(const SMPConfiguration
*config)
".has_sockets = %5s, sockets = %" PRId64 ",\n"
".has_dies = %5s, dies = %" PRId64 ",\n"
".has_clusters = %5s, clusters = %" PRId64 ",\n"
+".has_modules = %5s, modules = %" PRId64 ",\n"
".has_cores= %5s, cores= %" PRId64 ",\n"
".has_threads = %5s, threads = %" PRId64 ",\n"
".has_maxcpus = %5s, maxcpus = %" PRId64 ",\n"
@@ -639,6 +640,7 @@ static char *smp_config_to_string(const SMPConfiguration
*config)
config->has_sockets ? "true" : "false", config->sockets,
config->has_dies ? "true" : "false", config->dies,
config->has_clusters ? "true" : "false", config->clusters,
+config->has_modules ? "true" : "false", config->modules,
config->has_cores ? "true" : "false", config->cores,
config->has_threads ? "true" : "false", config->threads,
config->has_maxcpus ? "true" : "false", config->maxcpus);
@@ -679,6 +681,7 @@ static char *cpu_topology_to_string(const CpuTopology *topo,
".sockets= %u,\n"
".dies = %u,\n"
".clusters = %u,\n"
+".modules= %u,\n"
".cores = %u,\n"
".threads= %u,\n"
".max_cpus = %u,\n"
@@ -688,8 +691,8 @@ static char *cpu_topology_to_string(const CpuTopology *topo,
"}",
topo->cpus, topo->drawers, topo->books,
topo->sockets, topo->dies, topo->clusters,
-topo->cores, topo->threads, topo->max_cpus,
-threads_per_socket, cores_per_socket,
+topo->modules, topo->cores, topo->threads,
+topo->max_cpus, threads_per_socket, cores_per_socket,
has_clusters ? "true" : "false");
}
@@ -732,6 +735,7 @@ static void check_parse(MachineState *ms, const
SMPConfiguration *config,
(ms->smp.sockets == expect_topo->sockets) &&
(ms->smp.dies == expect_topo->dies) &&
(ms->smp.clusters == expect_topo->clusters) &&
+(ms->smp.modules == expect_topo->modules) &&
(ms->smp.cores == expect_topo->cores) &&
(ms->smp.threads == expect_topo->threads) &&
(ms->smp.max_cpus == expect_topo->max_cpus) &&
@@ -812,6 +816,11 @@ static void smp_parse_test(MachineState *ms, SMPTestData
*data, bool is_valid)
/* The parsed results of the unsupported parameters should be 1 */
static void unsupported_params_init(const MachineClass *mc, SMPTestData *data)
{
+if (!mc->smp_props.modules_supported) {
+data->expect_prefer_sockets.modules = 1;
+data->expect_prefer_cores.modules = 1;
+}
+
if (!mc->smp_props.dies_supported) {
data->expect_prefer_sockets.dies = 1;
data->expect_prefer_cores.dies = 1;
--
2.34.1
[PATCH 0/8] tests/unit/test-smp-sparse: Misc Cleanup and Add Module Test
Hi, Since the module support has landed in x86, and it's time to add the module's -smp test cases to cover the relevant code path. This series adds the module tests to ensure that this new level does not break the current topology information calculations. It also includes some misc cleanup. Thanks and Best Regadrs, Zhao --- Zhao Liu (8): tests/unit/test-smp-parse: Fix comments of drawers and books case tests/unit/test-smp-parse: Fix comment of parameters=1 case tests/unit/test-smp-parse: Fix an invalid topology case tests/unit/test-smp-parse: Use default parameters=0 when not set in -smp tests/unit/test-smp-parse: Make test cases aware of module level tests/unit/test-smp-parse: Test "modules" parameter in -smp tests/unit/test-smp-parse: Test "modules" and "dies" combination case tests/unit/test-smp-parse: Test the full 8-levels topology hierarchy tests/unit/test-smp-parse.c | 373 ++-- 1 file changed, 311 insertions(+), 62 deletions(-) -- 2.34.1
[PATCH 1/8] tests/unit/test-smp-parse: Fix comments of drawers and books case
Fix the comments to match the actual configurations.
Signed-off-by: Zhao Liu
---
tests/unit/test-smp-parse.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 9fdba24fce56..fa8e7d83a7b6 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -474,8 +474,8 @@ static const struct SMPTestData data_with_drawers_invalid[]
= {
static const struct SMPTestData data_with_drawers_books_invalid[] = {
{
/*
- * config: -smp 200,drawers=2,books=2,sockets=2,cores=4,\
- * threads=2,maxcpus=200
+ * config: -smp 200,drawers=3,books=5,sockets=2,cores=4,\
+ * threads=2,maxcpus=200
*/
.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 200, T, 3, T, 5, T,
2, T, 4, T, 2, T, 200),
@@ -485,8 +485,8 @@ static const struct SMPTestData
data_with_drawers_books_invalid[] = {
"cores (4) * threads (2) != maxcpus (200)",
}, {
/*
- * config: -smp 242,drawers=2,books=2,sockets=2,cores=4,\
- * threads=2,maxcpus=240
+ * config: -smp 242,drawers=3,books=5,sockets=2,cores=4,\
+ * threads=2,maxcpus=240
*/
.config = SMP_CONFIG_WITH_BOOKS_DRAWERS(T, 242, T, 3, T, 5, T,
2, T, 4, T, 2, T, 240),
--
2.34.1
Re: [RFC 4/6] scripts/simpletrace-rust: Parse and check trace recode file
> > +fn read_trace_records(
> > +events: ,
> > +fobj: ,
> > +analyzer: Formatter,
> > +read_header: bool,
> > +) -> Result>
> > +{
> > +/* backtrace::Backtrace needs this env variable. */
> > +env::set_var("RUST_BACKTRACE", "1");
> > +let bt = Backtrace::new();
> > +let raw_frame = bt.frames().first().unwrap();
> > +let frameinfo = raw_frame.symbols().first().unwrap();
> > +
> > +let dropped_event = Event::build(
> > +"Dropped_Event(uint64_t num_events_dropped)",
> > +frameinfo.lineno().unwrap() + 1,
> > +frameinfo.filename().unwrap().to_str().unwrap(),
> > +)
> > +.unwrap();
>
> Is the backtrace necessary? This trick was used in Python where it's
> part of the standard library, but I don't think there is any need for
> Dropped_Event to refer to this line in the source code.
>
> Maybe Rust has a way to do this at compile-time or you can hardcode
> values instead.
Thanks your reminder, I'll think about the hardcode approach as the
easiest solution...removing the backtrace can help a lot here.
Regards,
Zhao
Re: [RFC 3/6] scripts/simpletrace-rust: Add helpers to parse trace file
> > +fn read_type(mut fobj: ) -> Result
> > +{
> > +let mut tbuf = [0u8; 8];
> > +if let Err(e) = fobj.read_exact( tbuf) {
> > +if e.kind() == ErrorKind::UnexpectedEof {
> > +return Ok(RecordType::Empty);
> > +} else {
> > +return Err(Error::ReadFile(e));
> > +}
> > +}
> > +
> > +/*
> > + * Safe because the layout of the trace record requires us to parse
> > + * the type first, and then there is a check on the validity of the
> > + * record type.
> > + */
> > +let raw_t =
> > +unsafe { std::mem::transmute::<[u8; 8], RecordRawType>(tbuf) };
>
> A safe alternative:
> https://doc.rust-lang.org/std/primitive.u64.html#method.from_ne_bytes?
Thanks! Will use it.
> > +match raw_t.rtype {
> > +RECORD_TYPE_MAPPING => Ok(RecordType::Mapping),
> > +RECORD_TYPE_EVENT => Ok(RecordType::Event),
> > +_ => Err(Error::UnknownRecType(raw_t.rtype)),
> > +}
> > +}
> > +}
[snip]
> > +{
> > +fn read_header(mut fobj: ) -> Result
> > +{
> > +let mut raw_hdr = [0u8; 24];
> > +fobj.read_exact( raw_hdr).map_err(Error::ReadFile)?;
> > +
> > +/*
> > + * Safe because the size of log header (struct LogHeader)
> > + * is 24 bytes, which is ensured by simple trace backend.
> > + */
> > +let hdr =
> > +unsafe { std::mem::transmute::<[u8; 24], LogHeader>(raw_hdr) };
>
> Or u64::from_ne_bytes() for each field.
Will do.
> > +Ok(hdr)
> > +}
> > +}
[snip]
> > +impl ReadHeader for RecordHeader
> > +{
> > +fn read_header(mut fobj: ) -> Result
> > +{
> > +let mut raw_hdr = [0u8; 24];
> > +fobj.read_exact( raw_hdr).map_err(Error::ReadFile)?;
> > +
> > +/*
> > + * Safe because the size of record header (struct RecordHeader)
> > + * is 24 bytes, which is ensured by simple trace backend.
> > + */
> > +let hdr: RecordHeader =
> > +unsafe { std::mem::transmute::<[u8; 24],
> > RecordHeader>(raw_hdr) };
>
> Or u64::from_ne_bytes() and u32::from_ne_bytes() for all fields.
Will do.
Thanks,
Zhao
Re: [RFC 2/6] scripts/simpletrace-rust: Support Event & Arguments in trace module
> > +/*
> > + * Refer to the description of ALLOWED_TYPES in
> > + * scripts/tracetool/__init__.py.
>
> Please don't reference the Python implementation because this will not
> age well. It may bitrot if the Python code changes or if the Python
> implementation is deprecated then the source file will go away
> altogether. Make the Rust implementation self-contained. If there are
> common file format concerns shared by implementations, then move that
> information to a separate document in docs/interop/ (i.e. a simpletrace
> file format specification).
Thanks for your guidance, will do.
> > + */
> > +const ALLOWED_TYPES: [ 20] = [
> > +"int",
> > +"long",
> > +"short",
> > +"char",
> > +"bool",
> > +"unsigned",
> > +"signed",
> > +"int8_t",
> > +"uint8_t",
> > +"int16_t",
> > +"uint16_t",
> > +"int32_t",
> > +"uint32_t",
> > +"int64_t",
> > +"uint64_t",
> > +"void",
> > +"size_t",
> > +"ssize_t",
> > +"uintptr_t",
> > +"ptrdiff_t",
> > +];
> > +
> > +const STRING_TYPES: [ 4] =
> > +["const char*", "char*", "const char *", "char *"];
> > +
> > +/* TODO: Support 'vcpu' property. */
>
> The vcpu property was removed in commit d9a6bad542cd ("docs: remove
> references to TCG tracing"). Is this comment outdated or are you
> planning to bring it back?
Thanks! I have no plan for this, I just follow _VALID_PROPS[] in
scripts/tracetool/__init__.py. As you commented above, I think I should
just ignore it. ;-)
> > +const VALID_PROPS: [ 1] = ["disable"];
[snip]
> > +pub fn build(arg_str: ) -> Result
> > +{
> > +let mut args = Arguments::new();
> > +for arg in arg_str.split(',').map(|s| s.trim()) {
> > +if arg.is_empty() {
> > +return Err(Error::EmptyArg);
> > +}
> > +
> > +if arg == "void" {
> > +continue;
> > +}
> > +
> > +let (arg_type, identifier) = if arg.contains('*') {
> > +/* FIXME: Implement rsplit_inclusive(). */
> > +let p = arg.rfind('*').unwrap();
> > +(
> > +/* Safe because arg contains "*" and p exists. */
> > +unsafe { arg.get_unchecked(..p + 1) },
> > +/* Safe because arg contains "*" and p exists. */
> > +unsafe { arg.get_unchecked(p + 1..) },
> > +)
> > +} else {
> > +arg.rsplit_once(' ').unwrap()
> > +};
>
> Can you write this without unsafe? Maybe rsplit_once(' ') followed by a
> check for (_, '*identifier'). If the identifier starts with '*', then
> arg_type += ' *' and identifier = identifier[1:].
Clever idea! It should work, will try this way.
> > +
> > +validate_c_type(arg_type)?;
> > +args.props.push(ArgProperty::new(arg_type, identifier));
> > +}
> > +Ok(args)
> > +}
> > +}
> > +
[snip]
> > +pub fn build(line_str: , lineno: u32, filename: ) ->
> > Result
> > +{
> > +static RE: Lazy = Lazy::new(|| {
> > +Regex::new(
> > +r#"(?x)
> > +((?P[\w\s]+)\s+)?
> > +(?P\w+)
> > +\((?P[^)]*)\)
> > +\s*
> > +(?:(?:(?P".+),)?\s*(?P".+))?
>
> What is the purpose of fmt_trans?
>
> > +\s*"#,
> > +)
> > +.unwrap()
>
> I wonder if regular expressions help here. It's not easy to read this
> regex and there is a bunch of logic that takes apart the matches
> afterwards. It might even be clearer to use string methods to split
> fields.
Yes, regular matching is a burden here (it's a "lazy simplification" on
my part), and I'll think if it's possible to avoid regular matching with
string methods.
> Please add a comment showing the format that's being parsed:
>
> // [disable] ( [, ] ...) ""
>
OK.
> > +});
> > +
> > +let caps_res = RE.captures(line_str);
> > +if caps_res.is_none() {
> > +return Err(Error::UnknownEvent(line_str.to_owned()));
> > +}
> > +let caps = caps_res.unwrap();
> > +let name = caps.name("name").map_or("", |m| m.as_str());
> > +let props: Vec = if caps.name("props").is_some() {
> > +caps.name("props")
> > +.unwrap()
> > +.as_str()
> > +.split_whitespace()
> > +.map(|s| s.to_string())
> > +.collect()
> > +} else {
> > +Vec::new()
> > +};
> > +let fmt: String =
> > +caps.name("fmt").map_or("", |m| m.as_str()).to_string();
> > +let fmt_trans: String = caps
> > +.name("fmt_trans")
> > +.map_or("", |m| m.as_str())
> > +.to_string();
> > +
> > +if fmt.contains("%m") || fmt_trans.contains("%m") {
> > +return Err(Error::InvalidFormat(
>
Re: [RFC 1/6] scripts/simpletrace-rust: Add the basic cargo framework
Hi Stefan,
[snip]
> > diff --git a/scripts/simpletrace-rust/.rustfmt.toml
> > b/scripts/simpletrace-rust/.rustfmt.toml
> > new file mode 100644
> > index ..97a97c24ebfb
> > --- /dev/null
> > +++ b/scripts/simpletrace-rust/.rustfmt.toml
> > @@ -0,0 +1,9 @@
> > +brace_style = "AlwaysNextLine"
> > +comment_width = 80
> > +edition = "2021"
> > +group_imports = "StdExternalCrate"
> > +imports_granularity = "item"
> > +max_width = 80
> > +use_field_init_shorthand = true
> > +use_try_shorthand = true
> > +wrap_comments = true
>
> There should be QEMU-wide policy. That said, why is it necessary to customize
> rustfmt?
Indeed, but QEMU's style for Rust is currently undefined, so I'm trying
to add this to make it easier to check the style...I will separate it
out as a style policy proposal.
[snip]
> > +trait Analyzer
> > +{
>
> The Python version treats this as an API so that users can write trace
> analysis scripts. I see below that you're using non-doc comments. That
> suggests you don't see this as a public API that people can write trace
> analysis scripts against?
Yes, for the initial version, I'm not exposing it for now. It could be
exposed later if needed and then we can support multiple script builds
by defining multiple workspaces in cargo.
Thanks,
Zhao
> > +/* Called at the start of the trace. */
> > +fn begin() {}
> > +
> > +/* Called if no specific method for processing a trace event. */
> > +fn catchall(
> > + self,
> > +rec_args: &[EventArgPayload],
> > +event: ,
> > +timestamp_ns: u64,
> > +pid: u32,
> > +event_id: u64,
> > +) -> Result;
> > +
> > +/* Called at the end of the trace. */
> > +fn end() {}
> > +
> > +/*
> > + * TODO: Support "variable" parameters (i.e. variants of
> > process_event()
> > + * with different parameters, like **kwargs in python), when we need a
> > + * simpletrace rust module.
> > + */
> > +fn process_event(
> > + self,
> > +rec_args: &[EventArgPayload],
> > +event: ,
> > +event_id: u64,
> > +timestamp_ns: u64,
> > +pid: u32,
> > +) -> Result
> > +{
> > +self.catchall(rec_args, event, timestamp_ns, pid, event_id)
> > +
> > +/*
> > + * TODO: Support custom function hooks (like getattr() in python),
> > + * when we need a simpletrace rust module.
> > + */
> > +}
> > +}
Re: [RFC 0/6] scripts: Rewrite simpletrace printer in Rust
Hi Stefan, On Mon, May 27, 2024 at 03:59:44PM -0400, Stefan Hajnoczi wrote: > Date: Mon, 27 May 2024 15:59:44 -0400 > From: Stefan Hajnoczi > Subject: Re: [RFC 0/6] scripts: Rewrite simpletrace printer in Rust > > On Mon, May 27, 2024 at 04:14:15PM +0800, Zhao Liu wrote: > > Hi maintainers and list, > > > > This RFC series attempts to re-implement simpletrace.py with Rust, which > > is the 1st task of Paolo's GSoC 2024 proposal. > > > > There are two motivations for this work: > > 1. This is an open chance to discuss how to integrate Rust into QEMU. > > 2. Rust delivers faster parsing. > > > > > > Introduction > > > > > > Code framework > > -- > > > > I choose "cargo" to organize the code, because the current > > implementation depends on external crates (Rust's library), such as > > "backtrace" for getting frameinfo, "clap" for parsing the cli, "rex" for > > regular matching, and so on. (Meson's support for external crates is > > still incomplete. [2]) > > > > The simpletrace-rust created in this series is not yet integrated into > > the QEMU compilation chain, so it can only be compiled independently, e.g. > > under ./scripts/simpletrace/, compile it be: > > > > cargo build --release > > Please make sure it's built by .gitlab-ci.d/ so that the continuous > integration system prevents bitrot. You can add a job that runs the > cargo build. Thanks! I'll do this. > > > > The code tree for the entire simpletrace-rust is as follows: > > > > $ script/simpletrace-rust . > > . > > ├── Cargo.toml > > └── src > > └── main.rs // The simpletrace logic (similar to simpletrace.py). > > └── trace.rs // The Argument and Event abstraction (refer to > > // tracetool/__init__.py). > > > > My question about meson v.s. cargo, I put it at the end of the cover > > letter (the section "Opens on Rust Support"). > > > > The following two sections are lessons I've learned from this Rust > > practice. > > > > > > Performance > > --- > > > > I did the performance comparison using the rust-simpletrace prototype with > > the python one: > > > > * On the i7-10700 CPU @ 2.90GHz machine, parsing and outputting a 35M > > trace binary file for 10 times on each item: > > > > AVE (ms) Rust v.s. Python > > Rust (stdout) 12687.16114.46% > > Python (stdout) 14521.85 > > > > Rust (file) 1422.44264.99% > > Python (file) 3769.37 > > > > - The "stdout" lines represent output to the screen. > > - The "file" lines represent output to a file (via "> file"). > > > > This Rust version contains some optimizations (including print, regular > > matching, etc.), but there should be plenty of room for optimization. > > > > The current performance bottleneck is the reading binary trace file, > > since I am parsing headers and event payloads one after the other, so > > that the IO read overhead accounts for 33%, which can be further > > optimized in the future. > > Performance will become more important when large amounts of TCG data is > captured, as described in the project idea: > https://wiki.qemu.org/Internships/ProjectIdeas/TCGBinaryTracing > > While I can't think of a time in the past where simpletrace.py's > performance bothered me, improving performance is still welcome. Just > don't spend too much time on performance (and making the code more > complex) unless there is a real need. Yes, I agree that it shouldn't be over-optimized. The logic in the current Rust version is pretty much a carbon copy of the Python version, without additional complex logic introduced, but the improvements in x2.6 were obtained by optimizing IO: * reading the event configuration file, where I called the buffered interface, * and the output formatted trace log, which I output all via std_out.lock() followed by write_all(). So, just the simple tweak of the interfaces brings much benefits. :-) > > Security > > > > > > This is an example. > > > > Rust is very strict about type-checking, and it found timestamp reversal > > issue in simpletrace-rust [3] (sorry, haven't gotten around to digging > > deeper with more time)...in this RFC, I workingaround it by allowing > > negative values. And the python version, just silently covered this > > issue up. > >
Re: [RFC 0/6] scripts: Rewrite simpletrace printer in Rust
Hi Mads, On Mon, May 27, 2024 at 12:49:06PM +0200, Mads Ynddal wrote: > Date: Mon, 27 May 2024 12:49:06 +0200 > From: Mads Ynddal > Subject: Re: [RFC 0/6] scripts: Rewrite simpletrace printer in Rust > X-Mailer: Apple Mail (2.3774.600.62) > > Hi, > > Interesting work. I don't have any particular comments for the code, but I > wanted to address a few of the points here. > > > 2. Rust delivers faster parsing. > > For me, the point of simpletrace.py is not to be the fastest at parsing, but > rather to open the door for using Python libraries like numpy, matplotlib, > etc. > for analysis. > > There might be room for improvement in the Python version, especially in > minimizing memory usage, when parsing large traces. Thanks for pointing this out, the Python version is also very extensible and easy to develop. Perhaps ease of scalability vs. performance could be the difference that the two versions focus on? > > Security > > > > > > This is an example. > > > > Rust is very strict about type-checking, and it found timestamp reversal > > issue in simpletrace-rust [3] (sorry, haven't gotten around to digging > > deeper with more time)...in this RFC, I workingaround it by allowing > > negative values. And the python version, just silently covered this > > issue up. > > I'm not particularly worried about the security of the Python version. We're > not > doing anything obviously exploitable. I agree with this, this tool is mainly for parsing. I think one of the starting points for providing a Rust version was also to explore whether this could be an opportunity to integrate Rust into QEMU. Thanks, Zhao
Re: [PATCH] x86: cpu: fixup number of addressable IDs for processor cores in the physical package
Hi Chuang,
On Mon, May 27, 2024 at 11:13:33AM +0800, Chuang Xu wrote:
> Date: Mon, 27 May 2024 11:13:33 +0800
> From: Chuang Xu
> Subject: [PATCH] x86: cpu: fixup number of addressable IDs for processor
> cores in the physical package
According to the usual practice of QEMU commits, people tend to use
"i386/cpu" as the subject prefix, which indicates the code path.
> X-Mailer: git-send-email 2.24.3 (Apple Git-128)
>
> When QEMU is started with:
> -cpu host,host-cache-info=on,l3-cache=off \
Just a discussion, "l3-cache=off" doesn't work in host cache pssthu
case, do you have a specific need that you don't want to see l3 cache?
> -smp 2,sockets=1,dies=1,cores=1,threads=2
> Guest can't acquire maximum number of addressable IDs for processor cores in
> the physical package from CPUID[04H].
>
> This bug was introduced in commit d7caf13b5fcf742e5680c1d3448ba070fc811644.
> Fix it by changing the judgement condition to a >= 1.
Pls add a "Fixes" tag like:
Fixes: d7caf13b5fcf ("x86: cpu: fixup number of addressable IDs for logical
processors sharing cache")
Since this is a historical issue that deserves to be ported to the
stable branch, you can cc stable list by:
Cc: qemu-sta...@nongnu.org
> Signed-off-by: Chuang Xu
As the patch sender, it's better to put your signature on the last line.
;-)
> Signed-off-by: Guixiong Wei
> Signed-off-by: Yipeng Yin
> ---
> target/i386/cpu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index cd16cb893d..0369c01153 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -6097,7 +6097,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> uint32_t count,
> if (*eax & 31) {
> int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
> int vcpus_per_socket = cs->nr_cores * cs->nr_threads;
> -if (cs->nr_cores > 1) {
> +if (cs->nr_cores >= 1) {
Like Igor suggested, this condition could be removed since cs->nr_cores can't
be 0.
> *eax &= ~0xFC00;
> *eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
> }
...the code is outdated, pls rebase on the latest master branch.
Regards,
Zhao
Re: [PATCH] x86: cpu: fixup number of addressable IDs for processor cores in the physical package
Hi Igor,
On Mon, May 27, 2024 at 05:03:17PM +0200, Igor Mammedov wrote:
> Date: Mon, 27 May 2024 17:03:17 +0200
> From: Igor Mammedov
> Subject: Re: [PATCH] x86: cpu: fixup number of addressable IDs for
> processor cores in the physical package
> X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-redhat-linux-gnu)
>
> On Mon, 27 May 2024 11:13:33 +0800
> Chuang Xu wrote:
>
> > When QEMU is started with:
> > -cpu host,host-cache-info=on,l3-cache=off \
> > -smp 2,sockets=1,dies=1,cores=1,threads=2
> > Guest can't acquire maximum number of addressable IDs for processor cores in
> > the physical package from CPUID[04H].
>
> please add commit message, what you are actually seeing
> and expected values as well.
> And if guest complains about it also include related dmesg output.
>
>
> > This bug was introduced in commit d7caf13b5fcf742e5680c1d3448ba070fc811644.
> > Fix it by changing the judgement condition to a >= 1.
> >
> > Signed-off-by: Chuang Xu
> > Signed-off-by: Guixiong Wei
> > Signed-off-by: Yipeng Yin
> > ---
> > target/i386/cpu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> > index cd16cb893d..0369c01153 100644
> > --- a/target/i386/cpu.c
> > +++ b/target/i386/cpu.c
> > @@ -6097,7 +6097,7 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index,
> > uint32_t count,
> > if (*eax & 31) {
> > int host_vcpus_per_cache = 1 + ((*eax & 0x3FFC000) >> 14);
> > int vcpus_per_socket = cs->nr_cores * cs->nr_threads;
>
> in light of dies and recent modules shouldn't we also account for them here?
cs->nr_cores now account dies and modules (as its comment said "Number
of cores within this CPU package"). The code for this patch is a bit
outdated, although the issue is still here on the latest master.
> > -if (cs->nr_cores > 1) {
> > +if (cs->nr_cores >= 1) {
> > *eax &= ~0xFC00;
> > *eax |= (pow2ceil(cs->nr_cores) - 1) << 26;
> > }
> above and also following condition
>
> if (host_vcpus_per_cache > vcpus_per_socket) {
> ...
> *eax |= (pow2ceil(vcpus_per_socket) - 1) << 14;
>
> Makes me think, do we really have to have both conditionals,
> Why not just drop conditions and always encode both values
> to ones configured on '-smp' CLI?
The first condition "cores_per_pkg >= 1" can be removed in this patch
since cs->nr_cores won't be 0. :-)
About the 2nd condition "host_vcpus_per_cache > vcpus_per_socket", more
work is needed for cleanup...removal is also desirable, but not direct
removal, otherwise, Guest's L1/L2/L3 cache topology will default to
package level.
Currently with host_vcpus_per_cache <= threads_per_pkg, QEMU will
directly give Guest the Host's EAX[bits 25 -14], which is also
inaccurate, especially if there is a big difference between Guest and
Host CPU topology.
The correct way to do it is to parse the specific level of cache
topology on Host (core/module/die/package), and then encode Guest's
EAX[bits 25 -14] according to the specific items configured in -smp.
By cleaning up in this way, the second condition can be removed
naturally.
The host-cache-info cleanup was originally planned for me as well, I
think I can do this after Chuang's fix.
Thanks,
Zhao
[PATCH] hw/core: Rename CpuTopology to CPUTopology
Use CPUTopology to honor the generic style of CPU capitalization
abbreviations.
Signed-off-by: Zhao Liu
---
* Split from the previous SMP cache RFC:
https://lore.kernel.org/qemu-devel/20240220092504.726064-2-zhao1@linux.intel.com/
---
hw/s390x/cpu-topology.c | 6 +++---
include/hw/boards.h | 8
include/hw/s390x/cpu-topology.h | 6 +++---
tests/unit/test-smp-parse.c | 14 +++---
4 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/hw/s390x/cpu-topology.c b/hw/s390x/cpu-topology.c
index f16bdf65faa0..016f6c1c15ac 100644
--- a/hw/s390x/cpu-topology.c
+++ b/hw/s390x/cpu-topology.c
@@ -86,7 +86,7 @@ bool s390_has_topology(void)
*/
static void s390_topology_init(MachineState *ms)
{
-CpuTopology *smp = >smp;
+CPUTopology *smp = >smp;
s390_topology.cores_per_socket = g_new0(uint8_t, smp->sockets *
smp->books * smp->drawers);
@@ -181,7 +181,7 @@ void s390_topology_reset(void)
*/
static bool s390_topology_cpu_default(S390CPU *cpu, Error **errp)
{
-CpuTopology *smp = _machine->smp;
+CPUTopology *smp = _machine->smp;
CPUS390XState *env = >env;
/* All geometry topology attributes must be set or all unset */
@@ -234,7 +234,7 @@ static bool s390_topology_check(uint16_t socket_id,
uint16_t book_id,
uint16_t drawer_id, uint16_t entitlement,
bool dedicated, Error **errp)
{
-CpuTopology *smp = _machine->smp;
+CPUTopology *smp = _machine->smp;
if (socket_id >= smp->sockets) {
error_setg(errp, "Unavailable socket: %d", socket_id);
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 2fa800f11ae4..c1737f2a5736 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -334,7 +334,7 @@ typedef struct DeviceMemoryState {
} DeviceMemoryState;
/**
- * CpuTopology:
+ * CPUTopology:
* @cpus: the number of present logical processors on the machine
* @drawers: the number of drawers on the machine
* @books: the number of books in one drawer
@@ -346,7 +346,7 @@ typedef struct DeviceMemoryState {
* @threads: the number of threads in one core
* @max_cpus: the maximum number of logical processors on the machine
*/
-typedef struct CpuTopology {
+typedef struct CPUTopology {
unsigned int cpus;
unsigned int drawers;
unsigned int books;
@@ -357,7 +357,7 @@ typedef struct CpuTopology {
unsigned int cores;
unsigned int threads;
unsigned int max_cpus;
-} CpuTopology;
+} CPUTopology;
/**
* MachineState:
@@ -409,7 +409,7 @@ struct MachineState {
const char *cpu_type;
AccelState *accelerator;
CPUArchIdList *possible_cpus;
-CpuTopology smp;
+CPUTopology smp;
struct NVDIMMState *nvdimms_state;
struct NumaState *numa_state;
};
diff --git a/include/hw/s390x/cpu-topology.h b/include/hw/s390x/cpu-topology.h
index c064f427e948..ff09c57a4428 100644
--- a/include/hw/s390x/cpu-topology.h
+++ b/include/hw/s390x/cpu-topology.h
@@ -63,17 +63,17 @@ static inline void s390_topology_reset(void)
extern S390Topology s390_topology;
-static inline int s390_std_socket(int n, CpuTopology *smp)
+static inline int s390_std_socket(int n, CPUTopology *smp)
{
return (n / smp->cores) % smp->sockets;
}
-static inline int s390_std_book(int n, CpuTopology *smp)
+static inline int s390_std_book(int n, CPUTopology *smp)
{
return (n / (smp->cores * smp->sockets)) % smp->books;
}
-static inline int s390_std_drawer(int n, CpuTopology *smp)
+static inline int s390_std_drawer(int n, CPUTopology *smp)
{
return (n / (smp->cores * smp->sockets * smp->books)) % smp->drawers;
}
diff --git a/tests/unit/test-smp-parse.c b/tests/unit/test-smp-parse.c
index 9fdba24fce56..f51138794ca1 100644
--- a/tests/unit/test-smp-parse.c
+++ b/tests/unit/test-smp-parse.c
@@ -120,8 +120,8 @@
*/
typedef struct SMPTestData {
SMPConfiguration config;
-CpuTopology expect_prefer_sockets;
-CpuTopology expect_prefer_cores;
+CPUTopology expect_prefer_sockets;
+CPUTopology expect_prefer_cores;
const char *expect_error;
} SMPTestData;
@@ -643,7 +643,7 @@ static char *smp_config_to_string(const SMPConfiguration
*config)
}
/* Use the different calculation than machine_topo_get_threads_per_socket(). */
-static unsigned int cpu_topology_get_threads_per_socket(const CpuTopology
*topo)
+static unsigned int cpu_topology_get_threads_per_socket(const CPUTopology
*topo)
{
/* Check the divisor to avoid invalid topology examples causing SIGFPE. */
if (!topo->drawers || !topo->books || !topo->sockets) {
@@ -654,7 +654,7 @@ static unsigned int
cpu_topology_get_threads_per_socket(const CpuTopology *topo)
}
/* Use the different calculation than machine_topo_get_cores_per_socket(). */
-static unsigned i
Re: [PATCH v9] arm/kvm: Enable support for KVM_ARM_VCPU_PMU_V3_FILTER
On Mon, May 27, 2024 at 02:41:01PM +0800, Shaoqin Huang wrote: > Date: Mon, 27 May 2024 14:41:01 +0800 > From: Shaoqin Huang > Subject: Re: [PATCH v9] arm/kvm: Enable support for > KVM_ARM_VCPU_PMU_V3_FILTER > > Hi Zhao, > > Thanks for your proposed idea. If you are willing to take the PMU Filter > Enabling work, you can do it. I won't update this series anymore due to the > QAPI restriction. I really appreciate if you can implement that. > Welcome Shaoqin, I'll cc you when I'm done with the first version (it'll take some time). There are also some issues that I might take up and revisit, such as whether to place the kvm-pmu-filter in -cpu or in -kvm. Anyway, hopefully eventually we can implement this feature for QEMU and users can benefit from it! Thanks, Zhao
Re: [PATCH V11 3/8] hw/acpi: Update ACPI GED framework to support vCPU Hotplug
On Wed, May 22, 2024 at 10:11:06PM +0100, Salil Mehta via wrote: > Date: Wed, 22 May 2024 22:11:06 +0100 > From: Salil Mehta via > Subject: [PATCH V11 3/8] hw/acpi: Update ACPI GED framework to support vCPU > Hotplug > X-Mailer: git-send-email 2.34.1 > > ACPI GED (as described in the ACPI 6.4 spec) uses an interrupt listed in the > _CRS object of GED to intimate OSPM about an event. Later then demultiplexes > the > notified event by evaluating ACPI _EVT method to know the type of event. Use > ACPI GED to also notify the guest kernel about any CPU hot(un)plug events. > > ACPI CPU hotplug related initialization should only happen if ACPI_CPU_HOTPLUG > support has been enabled for particular architecture. Add > cpu_hotplug_hw_init() > stub to avoid compilation break. > > Co-developed-by: Keqian Zhu > Signed-off-by: Keqian Zhu > Signed-off-by: Salil Mehta > Reviewed-by: Jonathan Cameron > Reviewed-by: Gavin Shan > Reviewed-by: David Hildenbrand > Reviewed-by: Shaoqin Huang > Tested-by: Vishnu Pajjuri > Tested-by: Xianglai Li > Tested-by: Miguel Luis > Reviewed-by: Vishnu Pajjuri > --- > hw/acpi/acpi-cpu-hotplug-stub.c| 6 ++ > hw/acpi/cpu.c | 6 +- > hw/acpi/generic_event_device.c | 17 + > include/hw/acpi/generic_event_device.h | 4 > 4 files changed, 32 insertions(+), 1 deletion(-) > Reviewed-by: Zhao Liu
