|
From: | Alexander Graf |
Subject: | [Qemu-devel] Re: [PATCH V6 13/32] pci_host: consolidate pci config address access. |
Date: | Wed, 4 Nov 2009 12:50:51 +0100 |
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); +#endifI 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
[Prev in Thread] | Current Thread | [Next in Thread] |