From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50041) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fBEAE-0007vg-Gw for qemu-devel@nongnu.org; Wed, 25 Apr 2018 02:44:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fBEAA-0001Qp-St for qemu-devel@nongnu.org; Wed, 25 Apr 2018 02:44:26 -0400 From: Markus Armbruster References: <20180424214550.32549-1-lersek@redhat.com> <20180424214550.32549-3-lersek@redhat.com> Date: Wed, 25 Apr 2018 08:44:15 +0200 In-Reply-To: <20180424214550.32549-3-lersek@redhat.com> (Laszlo Ersek's message of "Tue, 24 Apr 2018 23:45:46 +0200") Message-ID: <87muxraips.fsf@dusky.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain Subject: Re: [Qemu-devel] [PATCH 2/6] qapi: handle the riscv CpuInfoArch in query-cpus-fast List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Laszlo Ersek Cc: qemu-devel@nongnu.org, Riku Voipio , Sagar Karandikar , Peter Crosthwaite , Bastian Koppelmann , qemu-stable@nongnu.org, Palmer Dabbelt , Laurent Vivier , Michael Clark , Paolo Bonzini , Richard Henderson , Luiz Capitulino , Viktor Mihajlovski Laszlo Ersek writes: > Commit 25fa194b7b11 added the @riscv enum constant to @CpuInfoArch (used > in both @CpuInfo and @CpuInfoFast -- the return types of the @query-cpus > and @query-cpus-fast commands, respectively), and assigned, in both return > structures, the @CpuInfoRISCV sub-structure to the new enum value. > > However, qmp_query_cpus_fast() would not populate either the @arch field > or the @CpuInfoRISCV sub-structure, when TARGET_RISCV was defined; only > qmp_query_cpus() would. > > In theory, there are two ways to fix this: > > (a) Fill in both the @arch field and the @CpuInfoRISCV sub-structure in > qmp_query_cpus_fast(), by copying the logic from qmp_query_cpus(). > > (b) Assign @CpuInfoOther to the @riscv enum constant in @CpuInfoFast, and > populate only the @arch field in qmp_query_cpus_fast(). > > Approach (b) seems more robust, because: > > - clearly there has never been an attempt to get actual RISV CPU state > from qmp_query_cpus_fast(), so its lack of RISCV support is not actually > a problem, > > - getting CPU state without interrupting KVM looks like an exceptional > thing to do (only S390X does it currently). > > Cc: Bastian Koppelmann > Cc: Eric Blake > Cc: Laurent Vivier > Cc: Markus Armbruster > Cc: Michael Clark > Cc: Palmer Dabbelt > Cc: Paolo Bonzini > Cc: Peter Crosthwaite > Cc: Richard Henderson > Cc: Riku Voipio > Cc: Sagar Karandikar > Cc: qemu-stable@nongnu.org > Fixes: 25fa194b7b11901561532e435beb83d046899f7a > Signed-off-by: Laszlo Ersek > --- > > Notes: > PATCHv1: > > - new patch > > qapi/misc.json | 2 +- > cpus.c | 2 ++ > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/qapi/misc.json b/qapi/misc.json > index 5636f4a14997..104d013adba6 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -565,23 +565,23 @@ > { 'union': 'CpuInfoFast', > 'base': {'cpu-index': 'int', 'qom-path': 'str', > 'thread-id': 'int', '*props': 'CpuInstanceProperties', > 'arch': 'CpuInfoArch' }, > 'discriminator': 'arch', > 'data': { 'x86': 'CpuInfoOther', > 'sparc': 'CpuInfoOther', > 'ppc': 'CpuInfoOther', > 'mips': 'CpuInfoOther', > 'tricore': 'CpuInfoOther', > 's390': 'CpuInfoS390', > - 'riscv': 'CpuInfoRISCV', > + 'riscv': 'CpuInfoOther', > 'other': 'CpuInfoOther' } } Why do CpuInfoFast's variants match CpuInfo's for s390, but not the others? Your commit message has an educated guess: "looks like an exceptional thing to do (only S390X does it currently)". But why guess when we can ask authors of commit ce74ee3dea6? Luiz and Victor, please advise. > ## > # @query-cpus-fast: > # > # Returns information about all virtual CPUs. This command does not > # incur a performance penalty and should be used in production > # instead of query-cpus. > # > # Returns: list of @CpuInfoFast > # > diff --git a/cpus.c b/cpus.c > index 1a9a2edee1f2..60563a6d54ec 100644 > --- a/cpus.c > +++ b/cpus.c > @@ -2225,22 +2225,24 @@ CpuInfoFastList *qmp_query_cpus_fast(Error **errp) > #elif defined(TARGET_SPARC) > info->value->arch = CPU_INFO_ARCH_SPARC; > #elif defined(TARGET_MIPS) > info->value->arch = CPU_INFO_ARCH_MIPS; > #elif defined(TARGET_TRICORE) > info->value->arch = CPU_INFO_ARCH_TRICORE; > #elif defined(TARGET_S390X) > s390_cpu = S390_CPU(cpu); > env = &s390_cpu->env; > info->value->arch = CPU_INFO_ARCH_S390; > info->value->u.s390.cpu_state = env->cpu_state; > +#elif defined(TARGET_RISCV) > + info->value->arch = CPU_INFO_ARCH_RISCV; > #else > info->value->arch = CPU_INFO_ARCH_OTHER; > #endif > if (!cur_item) { > head = cur_item = info; > } else { > cur_item->next = info; > cur_item = info; > } > }