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

[Hollis Blanchard]

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.

> > +
> > +    /* XXX register_savevm() */
> >   
> 
> Should be easy enough to add a register_savevm() function, no?

It should be, but since exactly 0 other PPC 4xx devices implement this,
I won't be able to test it. Should I implement it and leave a comment in
there that says "untested"?

-- 
Hollis Blanchard
IBM Linux Technology Center