Michael S. Tsirkin
2013-Feb-23 20:41 UTC
Re: [Qemu-devel] [PATCH] qemu: define a TOM register to report the base of PCI
On Fri, Feb 22, 2013 at 07:52:26PM +0100, Andreas Färber wrote:> Am 22.02.2013 16:37, schrieb Hao, Xudong: > >> -----Original Message----- > >> From: Jan Beulich [mailto:JBeulich@suse.com] > >> Sent: Friday, February 22, 2013 5:04 PM > >> To: Hao, Xudong > >> Cc: stefano.stabellini@eu.citrix.com; Zhang, Xiantao; xen-devel@lists.xen.org; > >> qemu-devel@nongnu.org; mst@redhat.com; aliguori@us.ibm.com > >> Subject: Re: [Xen-devel] [PATCH] qemu: define a TOM register to report the > >> base of PCI > >> > >>>>> On 22.02.13 at 04:23, Xudong Hao <xudong.hao@intel.com> wrote: > >>> @@ -203,6 +251,16 @@ static int i440fx_initfn(PCIDevice *dev) > >>> > >>> d->dev.config[I440FX_SMRAM] = 0x02; > >>> > >>> + /* Emulate top of memory, here use 0xe0000000 as default val*/ > >>> + if (xen_enabled()) { > >>> + d->dev.config[I440FX_PCI_HOLE_BASE] = 0xf0; > >>> + } else { > >>> + d->dev.config[I440FX_PCI_HOLE_BASE] = 0xe0; > >>> + } > >>> + d->dev.config[I440FX_PCI_HOLE_BASE + 1] = 0x00; > >>> + d->dev.config[I440FX_PCI_HOLE_BASE + 2] = 0x00; > >>> + d->dev.config[I440FX_PCI_HOLE_BASE + 3] = 0x00; > >>> + > >>> cpu_smm_register(&i440fx_set_smm, d); > >>> return 0; > >>> } > >> > >> Isn''t this the wrong way round (big endian, when it needs to be > >> little)? > >> > > Jan, > > > > Here it should be little endian since the following config reading is little endian, I''ll correct it. Thanks your comments. > > Your colleague David Woodhouse has just prepared some i440fx cleanups. > Please use dev->config instead of the indirect d->dev.config. > > Given Jan''s endianness comment, would alignment allow you to simply > write as follows? > > uint32_t addr = xen_enabled() ? 0xe0000000 : 0xf0000000; > *(uint32_t *)&dev->config[I440FX_PCI_HOLE_BASE] = cpu_to_le32(addr); > Then the byte-swapping would be explicit and the address in one piece > grep''able. Alternatively: > > cpu_to_le32s(&addr); > for (i = 0; i < 4; i++) { > dev->config[... + i] = ((uint8_t *)&addr)[i]; > }Please don''t do either of these things, do not hack endian-ness manually, we have APIs for pci endian-ness. Please use pci_set_long and friends for pci config accesses.> > Anyway, please use "piix: " in the subject rather than "qemu: " if this > is supposed to go into upstream qemu.git. > > Regards, > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg