From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41490) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zbqi0-0004bg-Cf for qemu-devel@nongnu.org; Tue, 15 Sep 2015 09:55:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Zbqhw-0005U1-0T for qemu-devel@nongnu.org; Tue, 15 Sep 2015 09:55:44 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:19879) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Zbqhv-0005TZ-QQ for qemu-devel@nongnu.org; Tue, 15 Sep 2015 09:55:39 -0400 Date: Tue, 15 Sep 2015 09:55:28 -0400 From: Konrad Rzeszutek Wilk Message-ID: <20150915135528.GG9134@l.oracle.com> References: <1441905361-31967-21-git-send-email-stefano.stabellini@eu.citrix.com> <55F69ADF.40404@redhat.com> <20150915132531.GC9134@l.oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PULL 21/29] xen/pt: Sync up the dev.config and data values. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefano Stabellini Cc: Paolo Bonzini , xen-devel@lists.xensource.com, qemu-devel@nongnu.org, peter.maydell@linaro.org On Tue, Sep 15, 2015 at 02:28:51PM +0100, Stefano Stabellini wrote: > On Tue, 15 Sep 2015, Konrad Rzeszutek Wilk wrote: > > On Tue, Sep 15, 2015 at 11:07:02AM +0100, Stefano Stabellini wrote: > > > CC Konrad > > > > > > On Mon, 14 Sep 2015, Paolo Bonzini wrote: > > > > On 10/09/2015 19:15, Stefano Stabellini wrote: > > > > > + > > > > > + switch (reg->size) { > > > > > + case 1: rc = xen_host_pci_get_byte(&s->real_device, offset, (uint8_t *)&val); > > > > > > > > A bit ugly, and it relies on the host being little endian. > > > > > > > > > + break; > > > > > + case 2: rc = xen_host_pci_get_word(&s->real_device, offset, (uint16_t *)&val); > > > > > > > > Same here. > > > > > > cpu_to_le32? > > > > > > But in practice, Xen being little endian only, I doubt that xen_pt_config_init.c > > > would actually work on be. > > > > > > > > > > > + break; > > > > > + case 4: rc = xen_host_pci_get_long(&s->real_device, offset, &val); > > > > > + break; > > > > > + default: assert(1); > > > > > > > > This should be assert(0) or, better, abort(). > > > > OK. Stefano, do you want me to: > > > > 1). Rebase the patches on top of your tag? > > Patches are already upstream, so no worries about rebasing. > > > > 2). Send an follow up patch to change this to abort()? (and wherever else I used > > assert(..)? > > Yes, that would be good. > > > > 3). Wait till Paolo is done going through the patchset and then revisit 1) or 2)? > > I don't know if Paolo has any more comments. > > Regarding his previous comment on little-endian, as I wrote I am not > sure if sprinkling around some cpu_to_le32 would actually make > hw/xen/xen_pt_config_init.c much better. I'll leave that to you. I got some other outstanding things I need to get done so will be as most lazy as possible in regards to this and just send you a patch :-)