From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59557) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvQyg-0007oK-KO for qemu-devel@nongnu.org; Fri, 13 Jun 2014 08:53:15 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1WvQyY-0007R2-1e for qemu-devel@nongnu.org; Fri, 13 Jun 2014 08:53:06 -0400 Received: from e06smtp10.uk.ibm.com ([195.75.94.106]:46075) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1WvQyX-0007Qv-M7 for qemu-devel@nongnu.org; Fri, 13 Jun 2014 08:52:57 -0400 Received: from /spool/local by e06smtp10.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Fri, 13 Jun 2014 13:52:55 +0100 Received: from b06cxnps4075.portsmouth.uk.ibm.com (d06relay12.portsmouth.uk.ibm.com [9.149.109.197]) by d06dlp03.portsmouth.uk.ibm.com (Postfix) with ESMTP id 657321B08051 for ; Fri, 13 Jun 2014 13:53:19 +0100 (BST) Received: from d06av06.portsmouth.uk.ibm.com (d06av06.portsmouth.uk.ibm.com [9.149.37.217]) by b06cxnps4075.portsmouth.uk.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id s5DCqqqA31981818 for ; Fri, 13 Jun 2014 12:52:52 GMT Received: from d06av06.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av06.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id s5DDqq3m020126 for ; Fri, 13 Jun 2014 07:52:52 -0600 Date: Fri, 13 Jun 2014 14:52:46 +0200 From: Greg Kurz Message-ID: <20140613145246.7d012445@bahia.local> In-Reply-To: <539AF17A.6020205@suse.de> References: <20140613111703.22108.14322.stgit@bahia.local> <20140613112226.22108.50811.stgit@bahia.local> <539AE49F.3000708@suse.de> <20140613141416.61114390@bahia.local> <539AF17A.6020205@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII 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: Alexander Graf 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 , Andreas =?UTF-8?B?RsOkcmJlcg==?= On Fri, 13 Jun 2014 14:41:30 +0200 Alexander Graf wrote: > > 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 > It sure looks better. Thanks ! -- Gregory Kurz kurzgreg@fr.ibm.com gkurz@linux.vnet.ibm.com Software Engineer @ IBM/Meiosys http://www.ibm.com Tel +33 (0)562 165 496 "Anarchy is about taking complete responsibility for yourself." Alan Moore.