[Qemu-devel] Re: [PATCH 1/6] Make config space accessor host bus trapable

[Blue Swirl]

On Sun, Jan 3, 2010 at 7:18 PM, Blue Swirl <address@hidden> wrote:
> On Sun, Jan 3, 2010 at 6:06 PM, Michael S. Tsirkin <address@hidden> 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 <address@hidden>
>>> >>>>>> CC: Michael S. Tsirkin <address@hidden>
>>> >>>>>
>>> >>>>> 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.

That time is very close, with latest QEMU and OpenBIOS, PCI probe
starts (with GDB tricks for calibrate_delay, which won't be needed
after %tick_cmpr is fixed):

[sparc64] Kernel already loaded

PROMLIB: Sun IEEE Boot Prom 'OBP 3.10.24 1999/01/01 01:01'
PROMLIB: Root node compatible: sun4u
Linux version 2.6.32 (address@hidden) (gcc version 4.2.4) #3 Sat Jan 9
20:36:42 UTC 2010
bootconsole [earlyprom0] enabled
ARCH: SUN4U
Ethernet address: 52:54:00:12:34:56
Kernel: Using 1 locked TLB entries for main kernel image.
Remapping the kernel... done.
OF stdout device is: 
/address@hidden,0/address@hidden/address@hidden,1/address@hidden/address@hidden
PROM: Built device tree with 32165 bytes of memory.
Top of RAM: 0x7e80000, Total RAM: 0x7e80000
Memory hole size: 0MB
Zone PFN ranges:
  Normal   0x00000000 -> 0x00003f40
Movable zone start PFN for each node
early_node_map[1] active PFN ranges
    0: 0x00000000 -> 0x00003f40
On node 0 totalpages: 16192
  Normal zone: 127 pages used for memmap
  Normal zone: 0 pages reserved
  Normal zone: 16065 pages, LIFO batch:3
Booting Linux...
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 16065
Kernel command line: root=/dev/hdc1 console=ttyS0 -p debug
PID hash table entries: 512 (order: -1, 4096 bytes)
Dentry cache hash table entries: 16384 (order: 4, 131072 bytes)
Inode-cache hash table entries: 8192 (order: 3, 65536 bytes)
Memory: 118480k available (1472k kernel code, 488k data, 104k init)
[fffff80000000000,0000000007e80000]
SLUB: Genslabs=14, HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
Hierarchical RCU implementation.
NR_IRQS:255
clocksource: mult[a0000] shift[16]
clockevent: mult[19999999] shift[32]
Console: colour dummy device 80x25
Calibrating delay loop (skipped) preset value.. 5000.00 BogoMIPS (lpj=10000000)
Mount-cache hash table entries: 512
/pci: PCI IO[1fe02000000] MEM[1ff00000000]
/pci: SABRE PCI Bus Module ver[0:0]
PCI: Scanning PBM /pci

There it hangs because the probed address is bad:
(gdb)
0x000000000043a1a8      75              __asm__ __volatile__("membar #Sync\n\t"
1: x/i $pc
0x43a1a8 <pci_config_read16+40>:        lduha  [ %o0 ] (29), %o4
(gdb) p $o0
$12 = 0x1fe01000806