[Qemu-devel] Re: [PATCH V6 13/32] pci_host: consolidate pci config address access.

[Isaku Yamahata]

On Tue, Nov 03, 2009 at 03:45:12PM +0200, Michael S. Tsirkin wrote:
> > --- a/hw/pci_host.c
> > +++ b/hw/pci_host.c
> > @@ -32,6 +32,114 @@ do { printf("pci_host_data: " fmt , ## __VA_ARGS__); } 
> > while (0)
> >  #define PCI_DPRINTF(fmt, ...)
> >  #endif
> >  
> > +static void pci_host_config_writel(void *opaque, target_phys_addr_t addr,
> > +                                   uint32_t val)
> > +{
> > +    PCIHostState *s = opaque;
> > +
> > +#ifdef TARGET_WORDS_BIGENDIAN
> > +    val = bswap32(val);
> > +#endif
> 
> I know you just copied it, but isn't this just
>       val = le32_to_cpu(val);
> 
> ?

Makes sense.


> > +static void pci_host_config_writel_noswap(void *opaque,
> > +                                          target_phys_addr_t addr,
> > +                                          uint32_t val)
> > +{
> > +    PCIHostState *s = opaque;
> > +
> > +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> > +                __func__, addr, val);
> > +    s->config_reg = val;
> > +}
> > +
> > +static uint32_t pci_host_config_readl_noswap(void *opaque,
> > +                                             target_phys_addr_t addr)
> > +{
> > +    PCIHostState *s = opaque;
> > +    uint32_t val = s->config_reg;
> > +
> > +    PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %"PRIx32"\n",
> > +                __func__, addr, val);
> > +    return val;
> > +}
> > +
> > +static CPUWriteMemoryFunc * const pci_host_config_write_noswap[] = {
> > +    &pci_host_config_writel_noswap,
> > +    &pci_host_config_writel_noswap,
> > +    &pci_host_config_writel_noswap,
> > +};
> > +
> > +static CPUReadMemoryFunc * const pci_host_config_read_noswap[] = {
> > +    &pci_host_config_readl_noswap,
> > +    &pci_host_config_readl_noswap,
> > +    &pci_host_config_readl_noswap,
> > +};
> > +
> > +int pci_host_config_register_io_memory_noswap(PCIHostState *s)
> 
> This name is clearly too long,
> as a result when you use it you run over the 80 character
> limit. Let's not fix it, let's make name shorter.
> io_memory -> memory?
> 
> > +{
> > +    return cpu_register_io_memory(pci_host_config_read_noswap,
> > +                                  pci_host_config_write_noswap, s);
> > +}
> 
> Do you happen to know whether no swap is really needed?  I suspect some
> of these places really only happen to work because everyone uses intel,
> so they simply would not work on ppc host.

I just followed the original code not to break it.
I dug into the commit logs a bit.
Liu, Aurelian and Alexander, could you give me a comment on
whether those functions needs byte swap (le32_to_cpu()) or not.
  - ppcie500_cfgaddr_{write, read}l() in ppce500_pci.c
  - pci_unin_config_{write, read}l() in unin_pci.c

ppce500_pci.c case:
The related commit is 74c62ba88902575be9ac3badf95d773470884b1c.
The comment on the top in the file says that it is derived from ppc4xx_pci.c.
ppc4xx_pci.c has byte swap, on the other hand ppce500_pci.c doesn't.
So I'm inclined to suspect that the byte swap was dropped accidently.
However I don't know its pci host bridge spec.


unin_pci.c case:
I don't know its spec neither.
The following changeset seems to remove the byte swap deliberately.
Alexander, could you please give us comment on it?

commit 783a20dcb51f7197c56f77c8012fa4abe8a23391
Author: blueswir1 <address@hidden>
Date:   Sat Mar 7 20:53:18 2009 +0000

    Activate uninorth AGP bridge
    
    Linux tries to poke the AGP bridge port and is pretty sad when it can't,
    so let's activate the old code again and throw out the bit modifications,
    as we don't really do anything with the values anyways.
    
    Signed-off-by: Alexander Graf <address@hidden>
    
    
    git-svn-id: svn://svn.savannah.nongnu.org/qemu/address@hidden 
c046a42c-6fe2-441c-8c8c-71466251a162

-- 
yamahata