On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
> On Sun, Jan 03, 2010 at 06:50:15PM +0100, Alexander Graf wrote:
>>
>> On 03.01.2010, at 18:44, Michael S. Tsirkin wrote:
>>
>> > On Sun, Jan 03, 2010 at 06:40:52PM +0100, Alexander Graf wrote:
>> >>
>> >> 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 ...
>> >
>> > Right.
>> >
>> >> 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
>> >
>> > There's already ugliness with swap/noswap versions in there. Anything
>> > that claims to be a proper framework would need to address that as well
>> > IMO.
>>
>> Hm, so you'd rather wait for 5 years to have an all-in-one rework than
>> incrementally improving the existing code?
>
> Not really, incremental improvements are good. But what you posted does
> not seem to clean up most code, what it really does is fixes ppc
> unin_pci. My feeling was since there's only one user for now maybe it
> is better to just have device specific code rather than complicate
> generic code. Makes sense?
>
> We need to see several users before we add yet another level of
> indirection. If there is a level of indirection that solves the
> swap/noswap ugliness as well that would show this is a good abstraction.
> I think I even know what a good abstraction would be: decode address on
> write and keep it in decoded form.
I think Sparc64 PBM configuration cycles are also a bit different.
It's described in UltraSPARC User's Manual 805-0087, p.325.
Currently both QEMU and OpenBIOS just use something common, but as
soon as Linux boot gets so far as to probe PCI bus, we'd have to fix
that.
