qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 1/1] IBM PowerPC 4xx 32-bit PCI controller emula


From: Aurelien Jarno
Subject: Re: [Qemu-devel] [PATCH 1/1] IBM PowerPC 4xx 32-bit PCI controller emulation
Date: Tue, 2 Dec 2008 23:13:00 +0100
User-agent: Mutt/1.5.13 (2006-08-11)

On Tue, Dec 02, 2008 at 02:43:38PM -0600, Hollis Blanchard wrote:
> On Tue, 2008-12-02 at 14:22 -0600, Anthony Liguori wrote:
> > Hollis Blanchard wrote:
> > 
> > > +#include "pci.h"
> > > +#include "pci_host.h"
> > > +#include "bswap.h"
> > > +
> > > +#undef DEBUG
> > > +#ifdef DEBUG
> > > +#define DPRINTF(fmt, args...) do { printf(fmt, ##args); } while (0)
> > > +#else
> > > +#define DPRINTF(fmt, args...)
> > > +#endif /* DEBUG */
> > >   
> > 
> > This is a GCC-ism that's deprecated.  The proper syntax (C99) is:
> > 
> > #define DPRINTF(fmt, ...) do { printf(fmt, ## __VA_ARGS__); } while (0)
> 
> Will do.
> 
> > > +struct ppc4xx_pci_t {
> > > +    struct pci_master_map pmm[PPC44x_PCI_NR_PMMS];
> > > +    struct pci_target_map ptm[PPC44x_PCI_NR_PTMS];
> > > +
> > > +    PCIHostState pci_state;
> > > +};
> > > +typedef struct ppc4xx_pci_t ppc4xx_pci_t;
> > >   
> > 
> > It would be better to use QEMU style type naming.
> 
> I copied this style from ppc405_uc.c, but I will ChangeItLikeThis.
> 
> > > +static void pci4xx_cfgaddr_writel(void *opaque, target_phys_addr_t addr,
> > > +                                  uint32_t value)
> > > +{
> > > +    ppc4xx_pci_t *ppc4xx_pci = opaque;
> > > +
> > > +#ifdef TARGET_WORDS_BIGENDIAN
> > > +    value = bswap32(value);
> > > +#endif
> > >
> > >   
> > 
> > Is this byte swapping correct?
> 
> I hate this byte swapping, because as we've discussed at great length in
> the past, "TARGET_WORDS_BIGENDIAN" never should have existed.
> 
> That said, as far as I can tell it is correct for the current state of
> qemu. I changed it to this style due to Aurelien's previous request. As
> I noted in the patch description, I can only test on
> big-endian/big-endian guest/host.
> 

To summarize the discussion we had last time, this is actually needed
depending on how the PCI controller is connected to the CPU, it's not
dependent on the CPU itself. But we put it here in QEMU, as we currently
don't model buses.

It reflects all byte swapping that is done between the CPU and the
controller, either by the chipset (which can be dependent on the device
or address range), by the bus being wired reversed, or whatever.

If your controller works with byte-swapping enabled, it means that
byte-swapping is necessary for a big-endian guest. It may or may not be
necessary for a little endian guest, but I guess we have no way to try.
The other solution to find the answer it looking at the docs, get a
headache, and maybe no answer. Not sure we want that.

So I think this code is ok, at least until we get more details or 
until a bus model is developed in QEMU.

-- 
  .''`.  Aurelien Jarno             | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   address@hidden         | address@hidden
   `-    people.debian.org/~aurel32 | www.aurel32.net




reply via email to

[Prev in Thread] Current Thread [Next in Thread]