On 03.01.2010, at 18:29, Michael S. Tsirkin wrote: > On Sun, Jan 03, 2010 at 05:09:32PM +0100, Alexander Graf wrote: >> >> On 03.01.2010, at 16:45, Michael S. Tsirkin wrote: >> >>> On Sun, Jan 03, 2010 at 02:50:45AM +0100, Alexander Graf wrote: >>>> Different host buses may have different layouts for config space accessors. >>>> >>>> The Mac U3 for example uses the following define to access Type 0 (directly >>>> attached) devices: >>>> >>>> #define MACRISC_CFA0(devfn, off) \ >>>> ((1 << (unsigned int)PCI_SLOT(dev_fn)) \ >>>> | (((unsigned int)PCI_FUNC(dev_fn)) << 8) \ >>>> | (((unsigned int)(off)) & 0xFCUL)) >>>> >>>> Obviously, this is different from the encoding we interpret in qemu. In >>>> order to let host controllers take some influence there, we can just >>>> introduce a converter function that takes whatever accessor we have and >>>> makes a qemu understandable one out of it. >>>> >>>> This patch is the groundwork for Uninorth PCI config space fixes. >>>> >>>> Signed-off-by: Alexander Graf <ag...@suse.de> >>>> CC: Michael S. Tsirkin <m...@redhat.com> >>> >>> Thanks! >>> >>> It always bothered me that the code in pci_host uses x86 encoding and >>> other architectures are converted to x86. As long as we are adding >>> redirection, maybe we should get rid of this, and make get_config_reg >>> return register and devfn separately, so we don't need to encode/decode >>> back and forth? >> >> Hmm, when touching code I'm not 100% sure what I'm doing I usually try to >> build on stuff that is there already. That's exactly what happened here. >> >> I'm personally not against defining the x86 format as qemu default. That way >> everyone knows what to deal with. I'm not sure if PCI defines anything that >> couldn't be represented by the x86 encoding in 32 bits. I actually doubt it. >> So it seems like a pretty good fit, especially given that all the other code >> is already in place. >> >>> OTOH if we are after a quick fix, won't it be simpler to have >>> unin_pci simply use its own routines instead of pci_host_conf_register_mmio >>> etc? >> >> Hm, I figured this is less work :-). > > Let me explain the idea: we have: > > static void pci_host_config_writel_ioport(void *opaque, > uint32_t addr, uint32_t val) > { > PCIHostState *s = opaque; > > PCI_DPRINTF("%s addr %"PRIx32 " val %"PRIx32"\n", __func__, addr, > val); > s->config_reg = val; > } > > static uint32_t pci_host_config_readl_ioport(void *opaque, uint32_t > addr) > { > PCIHostState *s = opaque; > uint32_t val = s->config_reg; > > PCI_DPRINTF("%s addr %"PRIx32" val %"PRIx32"\n", __func__, addr, > val); > return val; > } > > void pci_host_conf_register_ioport(pio_addr_t ioport, PCIHostState *s) > { > register_ioport_write(ioport, 4, 4, pci_host_config_writel_ioport, > s); > register_ioport_read(ioport, 4, 4, pci_host_config_readl_ioport, s); > } > > If instead of a simple config_reg = val we translate to pc format > here, the rest will work. No?
Well, that'd mean I'd have to implement a config_reg read function and do the conversion backwards again there. Or I could just save it off in the unin state ... hmm ... Either way, let's better get this done right. I'd rather want to have a proper framework in place in case someone else stumbles across something similar. Alex