From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56588) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAjlL-0006LW-DZ for qemu-devel@nongnu.org; Thu, 02 Jul 2015 15:03:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZAjlG-00076B-C4 for qemu-devel@nongnu.org; Thu, 02 Jul 2015 15:03:07 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47755) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZAjlG-000763-6Q for qemu-devel@nongnu.org; Thu, 02 Jul 2015 15:03:02 -0400 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id D1DA819F221 for ; Thu, 2 Jul 2015 19:03:01 +0000 (UTC) References: <1435842022-13980-1-git-send-email-mst@redhat.com> <5595876E.2040803@redhat.com> <20150702205556-mutt-send-email-mst@redhat.com> From: Paolo Bonzini Message-ID: <55958AE2.1020600@redhat.com> Date: Thu, 2 Jul 2015 21:02:58 +0200 MIME-Version: 1.0 In-Reply-To: <20150702205556-mutt-send-email-mst@redhat.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] virtio-pci: implement cfg capability List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: qemu-devel@nongnu.org On 02/07/2015 21:00, Michael S. Tsirkin wrote: > On Thu, Jul 02, 2015 at 08:48:14PM +0200, Paolo Bonzini wrote: >> >> >> On 02/07/2015 15:00, Michael S. Tsirkin wrote: >>> + cfg = (void *)(proxy->pci_dev.config + proxy->config_cap); >>> + off = le32_to_cpu(cfg->cap.offset); >>> + len = le32_to_cpu(cfg->cap.length); >>> + >>> + if ((len == 1 || len == 2 || len == 4)) { >>> + address_space_write(&proxy->modern_as, off, >>> + MEMTXATTRS_UNSPECIFIED, >>> + cfg->pci_cfg_data, len); >>> + } >> >> This parses pci_cfg_data in target endianness I think. You just want to >> move the little-endian value from the config cap to the little-endian >> value in the modern_as, so you need to use ldl_le_p and >> address_space_stl_le. > > but isn't that two byteswaps? why isn't this same as memcpy? It is a memcpy if you write to RAM, but the MMIO ops take an unsigned integer, so you can still have one byteswap hiding; you have to be careful when you have this kind of "forwarder", and the simplest way to do it is to use an ldl/stl pair with the same endianness. Paolo >>> + } >>> +} >>> + >>> +static uint32_t virtio_read_config(PCIDevice *pci_dev, >>> + uint32_t address, int len) >>> +{ >>> + VirtIOPCIProxy *proxy = DO_UPCAST(VirtIOPCIProxy, pci_dev, pci_dev); >>> + struct virtio_pci_cfg_cap *cfg; >>> + >>> + if (proxy->config_cap && >>> + ranges_overlap(address, len, proxy->config_cap + offsetof(struct virtio_pci_cfg_cap, >>> + pci_cfg_data), >>> + sizeof cfg->pci_cfg_data)) { >>> + uint32_t off; >>> + uint32_t len; >>> + >>> + cfg = (void *)(proxy->pci_dev.config + proxy->config_cap); >>> + off = le32_to_cpu(cfg->cap.offset); >>> + len = le32_to_cpu(cfg->cap.length); >>> + >>> + if ((len == 1 || len == 2 || len == 4)) { >>> + address_space_read(&proxy->modern_as, off, >>> + MEMTXATTRS_UNSPECIFIED, >>> + cfg->pci_cfg_data, len); >> >> Same here, use address_space_ldl_le to read into an int, and stl_le_p to >> write into cfg->pci_cfg_data. >> >> The best way to check it, of course, is to write a unit test! :) But >> you could also use a Linux BE guest on LE host. >> >> Everything else looks good. >> >> Paolo