From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57712) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvQne-0004EZ-Iy for qemu-devel@nongnu.org; Fri, 13 Jun 2014 08:41:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvQnU-0003FN-TJ for qemu-devel@nongnu.org; Fri, 13 Jun 2014 08:41:42 -0400 Received: from cantor2.suse.de ([195.135.220.15]:55361 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvQnU-0003F6-K6 for qemu-devel@nongnu.org; Fri, 13 Jun 2014 08:41:32 -0400 Message-ID: <539AF17A.6020205@suse.de> Date: Fri, 13 Jun 2014 14:41:30 +0200 From: Alexander Graf MIME-Version: 1.0 References: <20140613111703.22108.14322.stgit@bahia.local> <20140613112226.22108.50811.stgit@bahia.local> <539AE49F.3000708@suse.de> <20140613141416.61114390@bahia.local> In-Reply-To: <20140613141416.61114390@bahia.local> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v8 11/20] virtio: add endian-ambivalent support to VirtIODevice List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz Cc: Kevin Wolf , Peter Maydell , Stefan Hajnoczi , "Michael S. Tsirkin" , Rusty Russell , qemu-devel@nongnu.org, Juan Quintela , aneesh.kumar@linux.vnet.ibm.com, Anthony Liguori , Amit Shah , Paolo Bonzini , =?ISO-8859-1?Q?Andreas_F=E4rber?= On 13.06.14 14:14, Greg Kurz wrote: > On Fri, 13 Jun 2014 13:46:39 +0200 > Alexander Graf wrote: > >> On 13.06.14 13:23, Greg Kurz wrote: >>> Some CPU families can dynamically change their endianness. This means we >>> can have little endian ppc or big endian arm guests for example. This has >>> an impact on legacy virtio data structures since they are target endian. >>> We hence introduce a new property to track the endianness of each virtio >>> device. It is reasonnably assumed that endianness won't change while the >>> device is in use : we hence capture the device endianness when it gets >>> reset. >>> >>> We migrate this property in a subsection, after the device descriptor. This >>> means the load code must not rely on it until it is restored. As a consequence, >>> the vring sanity checks had to be moved after the call to vmstate_load_state(). >>> We enforce paranoia by poisoning the property at the begining of virtio_load(). >>> >>> Signed-off-by: Greg Kurz >>> --- >>> >>> Changes since last version (virtio migration RFC): >>> - endianness cached at device reset time >>> - reworked migration support >>> >>> hw/virtio/virtio-pci.c | 8 ++-- >>> hw/virtio/virtio.c | 100 +++++++++++++++++++++++++++++++++++++++----- >>> include/hw/virtio/virtio.h | 12 ++++- >>> 3 files changed, 102 insertions(+), 18 deletions(-) >>> >>> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c >>> index 390c8d2..2ffceb8 100644 >>> --- a/hw/virtio/virtio-pci.c >>> +++ b/hw/virtio/virtio-pci.c >>> @@ -406,13 +406,13 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr, >>> break; >>> case 2: >>> val = virtio_config_readw(vdev, addr); >>> - if (virtio_is_big_endian()) { >>> + if (virtio_is_big_endian(vdev)) { >>> val = bswap16(val); >>> } >>> break; >>> case 4: >>> val = virtio_config_readl(vdev, addr); >>> - if (virtio_is_big_endian()) { >>> + if (virtio_is_big_endian(vdev)) { >>> val = bswap32(val); >>> } >>> break; >>> @@ -440,13 +440,13 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr, >>> virtio_config_writeb(vdev, addr, val); >>> break; >>> case 2: >>> - if (virtio_is_big_endian()) { >>> + if (virtio_is_big_endian(vdev)) { >>> val = bswap16(val); >>> } >>> virtio_config_writew(vdev, addr, val); >>> break; >>> case 4: >>> - if (virtio_is_big_endian()) { >>> + if (virtio_is_big_endian(vdev)) { >>> val = bswap32(val); >>> } >>> virtio_config_writel(vdev, addr, val); >>> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >>> index 16b73d9..6235df8 100644 >>> --- a/hw/virtio/virtio.c >>> +++ b/hw/virtio/virtio.c >>> @@ -545,6 +545,24 @@ void virtio_set_status(VirtIODevice *vdev, uint8_t val) >>> vdev->status = val; >>> } >>> >>> +static void virtio_set_endian_target_default(VirtIODevice *vdev) >>> +{ >>> +#ifdef TARGET_WORDS_BIGENDIAN >>> + vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG; >>> +#else >>> + vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE; >>> +#endif >>> +} >>> + >>> +static void virtio_set_endian_cpu(VirtIODevice *vdev, CPUState *cpu) >>> +{ >>> + if (cpu_virtio_is_big_endian(cpu)) { >>> + vdev->device_endian = VIRTIO_DEVICE_ENDIAN_BIG; >>> + } else { >>> + vdev->device_endian = VIRTIO_DEVICE_ENDIAN_LITTLE; >>> + } >>> +} >>> + >>> void virtio_reset(void *opaque) >>> { >>> VirtIODevice *vdev = opaque; >>> @@ -552,6 +570,13 @@ void virtio_reset(void *opaque) >>> int i; >>> >>> virtio_set_status(vdev, 0); >>> + if (current_cpu) { >>> + /* Guest initiated reset */ >>> + virtio_set_endian_cpu(vdev, current_cpu); >>> + } else { >>> + /* System reset */ >>> + virtio_set_endian_target_default(vdev); >>> + } >>> >>> if (k->reset) { >>> k->reset(vdev); >>> @@ -840,6 +865,28 @@ void virtio_notify_config(VirtIODevice *vdev) >>> virtio_notify_vector(vdev, vdev->config_vector); >>> } >>> >>> +static bool virtio_device_endian_needed(void *opaque) >>> +{ >>> + VirtIODevice *vdev = opaque; >>> + >>> + assert(vdev->device_endian != VIRTIO_DEVICE_ENDIAN_UNKNOWN); >>> +#ifdef TARGET_WORDS_BIGENDIAN >>> + return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_LITTLE; >>> +#else >>> + return vdev->device_endian == VIRTIO_DEVICE_ENDIAN_BIG; >>> +#endif >> if (target_words_bigendian()) { >> return virtio_is_big_endian(); >> } else { >> return !virtio_is_big_endian(); >> } >> >> Then we have one less dependency on TARGET_WORDS_BIGENDIAN which means a >> step towards compiling virtio once ;). >> >> >> Alex >> > Heh ! It used to be target_words_bigendian() in the first place and I turned > it into TARGET_WORDS_BIGENDIAN to avoid "hey virtio isn't soon to be common > why don't you sort this out at compile time?" remarks. :) > > Since we are definitely not on a fastpath here, I'll gladly fix this. Yeah, just combine it with virtio_set_endian_target_default. static enum foo virtio_default_endian(vdev) { if (target_words_bigendian()) { return VIRTIO_DEVICE_ENDIAN_BIG; .... } Instead of calling virtio_set_endian_target_default, you do vdev->device_endian = virtio_default_endian(vdev); and static bool virtio_biendian_section_needed(vdev) { assert(unknown); return vdev->device_endian != virtio_default_endian(vdev); } Alex