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

[Alexander Graf]

On 04.11.2009, at 07:14, Isaku Yamahata wrote:

> 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?

Phew - I frankly don't remember. Something broke because some bits  
were strangely mangled and the way it's not it worked more than it did  
before :-). The whole Uninorth thing is still utterly broken, so  
please don't take anything from there as example.

I do know that Hollis was working a lot about LE/BE issues on PPC in  
Qemu, so he's probably the best person to CC and ask here.


Alex