Re: [Qemu-devel] [Xen-devel] [PATCH] qemu: define a TOM register to report the base of PCI

[Michael S. Tsirkin]

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:address@hidden
> >> Sent: Friday, February 22, 2013 5:04 PM
> >> To: Hao, Xudong
> >> Cc: address@hidden; Zhang, Xiantao; address@hidden;
> >> address@hidden; address@hidden; address@hidden
> >> 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 <address@hidden> 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