From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D78AFC47088 for ; Wed, 26 May 2021 21:44:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B0797613F0 for ; Wed, 26 May 2021 21:44:38 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234180AbhEZVqJ (ORCPT ); Wed, 26 May 2021 17:46:09 -0400 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]:60436 "EHLO us-smtp-delivery-124.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233324AbhEZVqI (ORCPT ); Wed, 26 May 2021 17:46:08 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1622065476; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=rIbuICSI1ktz0RlZpnrNSN6+3S5I7E3xm9TeMuD/DdU=; b=YI7vVFOvwNN/B0mCo/UcS48p46HnDdKtgHGP67BHmPAKNiuDLdkujh+HrpoQ4ZPJdgRXmU KKjZavrhrSgrHExb9/nOLciUO0z04I2fGUgaNqX/C1MappYnya4Ipv69xg6Z8W+6Csw8Ys M3+Mvp6ZhR1JRo39nd6qNzrfE3tmQG4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-414-xU8nADqYPZCboDvZWnH7yw-1; Wed, 26 May 2021 17:44:34 -0400 X-MC-Unique: xU8nADqYPZCboDvZWnH7yw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 62FC5501E5; Wed, 26 May 2021 21:44:33 +0000 (UTC) Received: from localhost (ovpn-114-21.rdu2.redhat.com [10.10.114.21]) by smtp.corp.redhat.com (Postfix) with ESMTP id 66C4561156; Wed, 26 May 2021 21:44:32 +0000 (UTC) Date: Wed, 26 May 2021 17:44:24 -0400 From: Eduardo Habkost To: Valeriy Vdovin Cc: qemu-devel@nongnu.org, Marcel Apfelbaum , Eric Blake , Markus Armbruster , Paolo Bonzini , Marcelo Tosatti , Richard Henderson , Thomas Huth , Laurent Vivier , kvm@vger.kernel.org, Denis Lunev , Vladimir Sementsov-Ogievskiy Subject: Re: [PATCH v7 1/1] qapi: introduce 'query-cpu-model-cpuid' action Message-ID: <20210526214424.ndk2dwu2crae64y7@habkost.net> References: <20210504122639.18342-1-valeriy.vdovin@virtuozzo.com> <20210504122639.18342-2-valeriy.vdovin@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210504122639.18342-2-valeriy.vdovin@virtuozzo.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org On Tue, May 04, 2021 at 03:26:39PM +0300, Valeriy Vdovin wrote: > Introducing new qapi method 'query-cpu-model-cpuid'. This method can be used to > get virtualized cpu model info generated by QEMU during VM initialization in > the form of cpuid representation. > > Diving into more details about virtual cpu generation: QEMU first parses '-cpu' > command line option. From there it takes the name of the model as the basis for > feature set of the new virtual cpu. After that it uses trailing '-cpu' options, > that state if additional cpu features should be present on the virtual cpu or > excluded from it (tokens '+'/'-' or '=on'/'=off'). > After that QEMU checks if the host's cpu can actually support the derived > feature set and applies host limitations to it. > After this initialization procedure, virtual cpu has it's model and > vendor names, and a working feature set and is ready for identification > instructions such as CPUID. > > Currently full output for this method is only supported for x86 cpus. > > To learn exactly how virtual cpu is presented to the guest machine via CPUID > instruction, new qapi method can be used. By calling 'query-cpu-model-cpuid' > method, one can get a full listing of all CPUID leafs with subleafs which are > supported by the initialized virtual cpu. > > Other than debug, the method is useful in cases when we would like to > utilize QEMU's virtual cpu initialization routines and put the retrieved > values into kernel CPUID overriding mechanics for more precise control > over how various processes perceive its underlying hardware with > container processes as a good example. > > Output format: > The output is a plain list of leaf/subleaf agrument combinations, that > return 4 words in registers EAX, EBX, ECX, EDX. > > Use example: > qmp_request: { > "execute": "query-cpu-model-cpuid" > } > > qmp_response: [ > { > "eax": 1073741825, > "edx": 77, > "leaf": 1073741824, > "ecx": 1447775574, > "ebx": 1263359563, > "subleaf": 0 > }, > { > "eax": 16777339, > "edx": 0, > "leaf": 1073741825, > "ecx": 0, > "ebx": 0, > "subleaf": 0 > }, > { > "eax": 13, > "edx": 1231384169, > "leaf": 0, > "ecx": 1818588270, > "ebx": 1970169159, > "subleaf": 0 > }, > { > "eax": 198354, > "edx": 126614527, > .... > > Signed-off-by: Valeriy Vdovin This breaks --disable-kvm builds (see below[1]), but I like the simplicity of this solution. I think it will be an acceptable and welcome mechanism if we name and document it as KVM-specific. A debugging command like this that returns the raw CPUID data directly from the KVM tables would be very useful for automated testing of our KVM CPUID initialization code. We have some test cases for CPU configuration code, but they trust what the CPU objects tell us and won't catch mistakes in target/i386/kvm.c CPUID code. [1] Build error when using --disable-kvm: [449/821] Linking target qemu-system-x86_64 FAILED: qemu-system-x86_64 c++ -o qemu-system-x86_64 qemu-system-x86_64.p/softmmu_main.c.o libcommon.fa.p/hw_char_virtio-console.c.o [...] /usr/bin/ld: libqemu-x86_64-softmmu.fa.p/meson-generated_.._qapi_qapi-commands-machine-target.c.o: in function `qmp_marshal_query_cpu_model_cpuid': /home/ehabkost/rh/proj/virt/qemu/build/qapi/qapi-commands-machine-target.c:278: undefined reference to `qmp_query_cpu_model_cpuid' collect2: error: ld returned 1 exit status > > --- > v2: - Removed leaf/subleaf iterators. > - Modified cpu_x86_cpuid to return false in cases when count is > greater than supported subleaves. > v3: - Fixed structure name coding style. > - Added more comments > - Ensured buildability for non-x86 targets. > v4: - Fixed cpu_x86_cpuid return value logic and handling of 0xA leaf. > - Fixed comments. > - Removed target check in qmp_query_cpu_model_cpuid. > v5: - Added error handling code in qmp_query_cpu_model_cpuid > v6: - Fixed error handling code. Added method to query_error_class > v7: - Changed implementation in favor of cached cpuid_data for > KVM_SET_CPUID2 > qapi/machine-target.json | 51 ++++++++++++++++++++++++++++++++++++++ > target/i386/kvm/kvm.c | 45 ++++++++++++++++++++++++++++++--- > tests/qtest/qmp-cmd-test.c | 1 + > 3 files changed, 93 insertions(+), 4 deletions(-) > > diff --git a/qapi/machine-target.json b/qapi/machine-target.json > index e7811654b7..ad816a50b6 100644 > --- a/qapi/machine-target.json > +++ b/qapi/machine-target.json > @@ -329,3 +329,54 @@ > ## > { 'command': 'query-cpu-definitions', 'returns': ['CpuDefinitionInfo'], > 'if': 'defined(TARGET_PPC) || defined(TARGET_ARM) || defined(TARGET_I386) || defined(TARGET_S390X) || defined(TARGET_MIPS)' } > + > +## > +# @CpuidEntry: > +# > +# A single entry of a CPUID response. > +# > +# CPUID instruction accepts 'leaf' argument passed in EAX register. > +# A 'leaf' is a single group of information about the CPU, that is returned > +# to the caller in EAX, EBX, ECX and EDX registers. A few of the leaves will > +# also have 'subleaves', the group of information would partially depend on the > +# value passed in the ECX register. The value of ECX is reflected in the 'subleaf' > +# field of this structure. > +# > +# @leaf: CPUID leaf or the value of EAX prior to CPUID execution. > +# @subleaf: value of ECX for leaf that has varying output depending on ECX. Instead of having to explain what "leaf" and "subleaf" means, maybe it would be simpler to just call them "in_eax" and "in_ecx"? > +# @eax: eax > +# @ebx: ebx > +# @ecx: ecx > +# @edx: edx > +# > +# Since: 6.1 > +## > +{ 'struct': 'CpuidEntry', > + 'data': { 'leaf' : 'uint32', > + 'subleaf' : 'uint32', I would make subleaf/in_ecx an optional field. We don't need to return it unless KVM_CPUID_FLAG_SIGNIFCANT_INDEX is set. > + 'eax' : 'uint32', > + 'ebx' : 'uint32', > + 'ecx' : 'uint32', > + 'edx' : 'uint32' > + }, > + 'if': 'defined(TARGET_I386)' } > + > +## > +# @query-cpu-model-cpuid: I would choose a name that indicates that the command is KVM-specific, like "query-kvm-cpuid" or "query-kvm-cpuid-table". > +# > +# Returns description of a virtual CPU model, created by QEMU after cpu > +# initialization routines. The resulting information is a reflection of a parsed > +# '-cpu' command line option, filtered by available host cpu features. I don't think "description of a virtual CPU model" is an accurate description of this. I would document it as "returns raw data from the KVM CPUID table for the first VCPU". I wonder if the "The resulting information is a reflection of a parsed [...] cpu features." part is really necessary. If you believe people don't understand how "-cpu" works, this is not exactly the right place to explain that. If you want to clarify what exactly is returned, maybe something like the following would work? "Returns raw data from the KVM CPUID table for the first VCPU. The KVM CPUID table defines the response to the CPUID instruction when executed by the guest operating system." > +# > +# Returns: @CpuModelCpuidDescription > +# > +# Example: > +# > +# -> { "execute": "query-cpu-model-cpuid" } > +# <- { "return": 'CpuModelCpuidDescription' } > +# > +# Since: 6.1 > +## > +{ 'command': 'query-cpu-model-cpuid', > + 'returns': ['CpuidEntry'], > + 'if': 'defined(TARGET_I386)' } > diff --git a/target/i386/kvm/kvm.c b/target/i386/kvm/kvm.c > index 7fe9f52710..edc4262efb 100644 > --- a/target/i386/kvm/kvm.c > +++ b/target/i386/kvm/kvm.c > @@ -20,6 +20,7 @@ > > #include > #include "standard-headers/asm-x86/kvm_para.h" > +#include "qapi/qapi-commands-machine-target.h" > > #include "cpu.h" > #include "sysemu/sysemu.h" > @@ -1464,16 +1465,48 @@ static Error *invtsc_mig_blocker; > > #define KVM_MAX_CPUID_ENTRIES 100 > > +struct CPUIDEntriesInfo { > + struct kvm_cpuid2 cpuid; > + struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES]; > +}; You don't need this new struct definition, as (&cpuid_data.cpuid.entries[0]) and (&cpuid_data.entries[0]) are exactly the same. a kvm_cpuid2 pointer would be enough. > + > +struct CPUIDEntriesInfo *cpuid_data_cached; > + > +CpuidEntryList * > +qmp_query_cpu_model_cpuid(Error **errp) > +{ > + int i; > + struct kvm_cpuid_entry2 *kvm_entry; > + CpuidEntryList *head = NULL, **tail = &head; > + CpuidEntry *entry; > + > + if (!cpuid_data_cached) { > + error_setg(errp, "cpuid_data cache not ready"); > + return NULL; I would return a more meaningful error message. Nobody except the developers who wrote and reviewed this code knows what "cpuid_data cache" means. A message like "VCPU was not initialized yet" would make more sense. > + } > + > + for (i = 0; i < cpuid_data_cached->cpuid.nent; ++i) { > + kvm_entry = &cpuid_data_cached->entries[i]; > + entry = g_malloc0(sizeof(*entry)); > + entry->leaf = kvm_entry->function; > + entry->subleaf = kvm_entry->index; > + entry->eax = kvm_entry->eax; > + entry->ebx = kvm_entry->ebx; > + entry->ecx = kvm_entry->ecx; > + entry->edx = kvm_entry->edx; > + QAPI_LIST_APPEND(tail, entry); > + } > + > + return head; > +} > + > int kvm_arch_init_vcpu(CPUState *cs) > { > - struct { > - struct kvm_cpuid2 cpuid; > - struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES]; > - } cpuid_data; > /* > * The kernel defines these structs with padding fields so there > * should be no extra padding in our cpuid_data struct. > */ > + struct CPUIDEntriesInfo cpuid_data; > QEMU_BUILD_BUG_ON(sizeof(cpuid_data) != > sizeof(struct kvm_cpuid2) + > sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES); > @@ -1833,6 +1866,10 @@ int kvm_arch_init_vcpu(CPUState *cs) > if (r) { > goto fail; > } > + if (!cpuid_data_cached) { > + cpuid_data_cached = g_malloc0(sizeof(cpuid_data)); > + memcpy(cpuid_data_cached, &cpuid_data, sizeof(cpuid_data)); You are going to copy more entries than necessary, but on the other hand I like the simplicity of not having to calculate the struct size before allocating. > + } Now I'm wondering if we want to cache the CPUID tables for all VCPUs (not just the first one). Being a debugging command, maybe it's an acceptable compromise to copy the data only from one VCPU. If the need to return data for other VCPUs arise, we can extend the command later. > > if (has_xsave) { > env->xsave_buf = qemu_memalign(4096, sizeof(struct kvm_xsave)); > diff --git a/tests/qtest/qmp-cmd-test.c b/tests/qtest/qmp-cmd-test.c > index c98b78d033..f5a926b61b 100644 > --- a/tests/qtest/qmp-cmd-test.c > +++ b/tests/qtest/qmp-cmd-test.c > @@ -46,6 +46,7 @@ static int query_error_class(const char *cmd) > { "query-balloon", ERROR_CLASS_DEVICE_NOT_ACTIVE }, > { "query-hotpluggable-cpus", ERROR_CLASS_GENERIC_ERROR }, > { "query-vm-generation-id", ERROR_CLASS_GENERIC_ERROR }, > + { "query-cpu-model-cpuid", ERROR_CLASS_GENERIC_ERROR }, > { NULL, -1 } > }; > int i; > -- > 2.17.1 > -- Eduardo