From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35941) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7T0g-0001d4-LF for qemu-devel@nongnu.org; Thu, 08 Aug 2013 12:24:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V7T0a-0006mH-Ku for qemu-devel@nongnu.org; Thu, 08 Aug 2013 12:24:22 -0400 Received: from cantor2.suse.de ([195.135.220.15]:50121 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V7T0a-0006m6-Bl for qemu-devel@nongnu.org; Thu, 08 Aug 2013 12:24:16 -0400 Message-ID: <5203C62C.2040303@suse.de> Date: Thu, 08 Aug 2013 18:24:12 +0200 From: =?UTF-8?B?QW5kcmVhcyBGw6RyYmVy?= MIME-Version: 1.0 References: <1375938949-22622-1-git-send-email-rusty@rustcorp.com.au> <1375938949-22622-2-git-send-email-rusty@rustcorp.com.au> <87li4cgvh1.fsf@codemonkey.ws> <5203AB19.9070505@suse.de> <87r4e4p4wj.fsf@codemonkey.ws> In-Reply-To: <87r4e4p4wj.fsf@codemonkey.ws> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 1/7] virtio: allow byte swapping for vring and config access List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Peter Maydell , Rusty Russell , qemu-devel@nongnu.org Am 08.08.2013 17:40, schrieb Anthony Liguori: > Andreas F=C3=A4rber writes: >> Am 08.08.2013 15:31, schrieb Anthony Liguori: >>> We have a mechanism to do weak functions via stubs/. I think it woul= d >>> be better to do cpu_get_byteswap() as a stub function and then overlo= ad >>> it in the ppc64 code. >> >> If this as your name indicates is a per-CPU function then it should go >> into CPUClass. Interesting question is, what is virtio supposed to do = if >> we have two ppc CPUs, one is Big Endian, the other is Little Endian. >=20 > PPC64 is big endian. AFAIK, there is no such thing as a little endian > PPC64 processor. >=20 > This is just a processor mode where loads/stores are byte lane swapped. > Hence the name 'cpu_get_byteswap'. It's just asking whether the > load/stores are being swapped or not. Exactly, just read it as "is in ... Endian mode". On the CPUs I am more familiar with (e.g., 970), this used to be controlled via an MSR bit, which as CPUPPCState::msr exists per CPUState. I did not check on real hardware, but from the QEMU code this would allow for the mixed-endian scenario described above. > At least for PPC64, it's not possible to enable/disable byte lane > swapping for individual CPUs. It's done through a system-wide hcall. What is offending me is only the following: If we name it cpu_get_byteswap() as proposed by you, then its first argument should be a CPUState *cpu. Its value would be read from the derived type's state, such as the MSR bit in the code path that you wanted duplicated. The function implementing that register-reading would be a hook in CPUClass, with a default implementation in qom/cpu.c rather than a fallback in stubs/. To access CPUClass, CPUState cannot be NULL, as brought up by Stefano for cpu_do_unassigned_access(); not following that pattern prevents mixing CPU architectures, which my large refactorings have partially been about. Cf. my guest-memory-dump refactoring. If it is just some random global value, then please don't call it cpu_*(). Since sPAPR is not a target of its own, I don't see how/where you want to implement that hcall query as per-target function either, that might rather call for a QEMUMachine hook? I don't care or argue about byte lanes here, I am just trying to keep API design consistent and not regressing on the way to heterogeneous emulation. Regards, Andreas > FWIW, I think most bi-endian architectures are this way too so I think > this is equally applicable to ARM. Peter, is that right? >=20 > Regards, >=20 > Anthony Liguori >=20 >> We'd need to check current_cpu then, which for Xen is always NULL. >> >> Andreas --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=C3=B6rffer; HRB 16746 AG N=C3=BC= rnberg