Implementing map-in - OpenBIOS

21 Jul 2019


      On Sun, 21 Jul 2019, Mark Cave-Ayland wrote:
...
On 21/07/2019 15:16, BALATON Zoltan wrote:
...
OK I came up with the attched patch (on top of your previous series and set-args
patch) which is I think about what you've suggested. I kept ob_pci_map for existing
callers but renamed it to ob_pci_map_in to keep symmetry between ob_pci_map and
ob_pci_unmap with ob_pci_map now only doing mapping as its name suggests (although I
think this should still be simpler) and ob_pci_bus_map_in is doing its own address
decoding now. The previous ob_pci_map (now called ob_pci_map_in which is a C helper
for some of what map-in should do) can be cleaned up as you like. Also cleaned up
some white space (tab vs. spaces) but that's excluded from this patch for brevity.
Now I'm really confused - I had a quick look at your patch and compared it with
Good that you're only confused now. :-) I was and still am confused about 
it all the time so please bear with me.
...
pci_config_*() words and they show that you can't memory map PCI config space on a
Mac. So how did you get to the conclusion earlier in the thread that this was what
was required?
Apparently I was confused... Now I realise that this usage of map-in in 
FCode ROM really wants to map the IO BAR1. My confusion probably comes 
from that after mapping it it uses config-b[!@] to tweak some regs so I 
thought it may want config space. (I'm still confused though after reading 
the description of map-in and addresses in the PCI binding doc. And we 
probably will need access to config space if we want to implement config-* 
words in Forth. We could add callbacks to C for those but that's not the 
nicest way.)
...
...
But it does not seem to work yet:
0 > " /pci@f2000000/ATY" open-dev to my-self
...
...
ob_pci_bar_map_in idx=1fc5ac54
pci_bus_addr_to_host_addr space=2 ba=0x82000000
ob_pci_map: phys=0x82000000 size=16384
ob_pci_bar_map_in idx=1fc5ac54
pci_bus_addr_to_host_addr space=2 ba=0x81000000
ob_pci_map: phys=0x81000000 size=16777216
?ok
Here we see 2 MMIO BARs being mapped: one of 16K and another of 16M. And this happens
as soon as you open the device? I think you'll have to explain more about your
command line and how you are getting OpenBIOS to execute the FCode, since I can't
tell if this is OpenBIOS or the FCode.
No FCode here, just started OpenBIOS with patched openbios-elf and typed 
commands that I've seen in the FCode. All this comes from OpenBIOS, don't 
ask me why it calls this for open-dev.
...
...
0 > my-unit? ok
3 > . 7800? ok
2 > . 0? ok
1 > . 0? ok
0 > my-space? ok
1 > . 7800? ok
0 > my-address? ok
2 > . 0? ok
1 > . 0? ok
0 > my-address h# 1000014 my-space + h# 100? ok
4 > .s <4> 0 0 1007814 100
?ok
4 > " map-in" $call-parent
...
...
ob_pci_bar_map_in idx=1fc5ac54
pci_bus_addr_to_host_addr space=1 ba=0x0
ob_pci_map: phys=0xf2000000 size=256
?ok
This is trying to map an IO BAR with 256 bytes, but the phys address isn't going to
be correct because OpenBIOS copies arch->io_base into *io_base (and also *mem_base
for MMIO) during PCI configuration and advances that copy to hold the start of the
next free address.
Where does OpenBIOS copy and advance these values and what do *io_base and 
*mem_base refer to here? I assume the map-in word runs after the device 
tree is already constucted so these should already be mapped and map-in 
might just need to return a virt address for the already mapped bars.
...
At a very minimum you're going to have to teach the PCI map-in how to use these
values instead of making the assumption that the BAR has already been mapped by
Which values?
...
ob_pci_configure_bar() and the address can be calculated by adding the BAR address to
arch->io_base or arch->mem_base according to the space type. This is how
pci_bus_addr_to_host_addr() currently works.
My patch called pci_bus_addr_to_host_addr in ob_pci_bus_map_in with the 
space decoded from phys.hi on the stack yet the virt address came out 
wrong at the end.
Now I've tried to do instead what SLOF's map-in does:
https://github.com/aik/SLOF/blob/master/slof/fs/pci-config-bridge.fs#L59
and ignore everything except phys.hi and get bar address from that like:
size = POP();
     phys[0] = POP();
     phys[1] = POP();
     phys[2] = POP();
PCI_DPRINTF("%s idx=%p, hi=%"PRIx32", mid=%"PRIx32", lo=%"PRIx32", size="
                 FMT_ucell"\n", __func__, idx, phys[0], phys[1], phys[2], size);
     pci_decode_phys_addr(phys, &flags, &space_code, &dev, &reg, &addr);
     /* Check if reg is actually a Base Address Reg */
     if (reg < PCI_BASE_ADDR_0 ||
         (reg > PCI_BASE_ADDR_5 && reg != PCI_ROM_ADDRESS)) {
         PUSH(-1);
         return;
     }
     addr = pci_config_read32(reg, dev);
     PCI_DPRINTF("%s bar=%"PRIx32"\n", __func__, addr);
     virt = ob_pci_map(addr, size);
but then bar==-1 (even for MMIO regions which were mapped before) so it 
looks like it's not mapped at this point so map-in should program the BAR 
reg or call another function for that?
...
The other issue is that the PCI enumeration code will already have mapped the ATI
BARs once so you'll have to come up with a solution so that if you detect an FCode
ROM then the existing PCI configuration code is ignored. Otherwise you'll find that
you have mapped the 16M MMIO BAR twice which is going to take quite a bit of MMIO
memory and lead to more confusion.
Does it map then unmap the region without freeing the allocated MMIO 
space? Or if it's still mapped we should just return a pointer to the 
already mapped space. Could we just get that from assigned-addresses?
...
Finally one other thing: I'm finding this thread really hard to follow - too much
information is spread across too many emails, and the subject line isn't really
helping me understand what topic we're following or where I can find related
information. May I suggest that it makes sense to break this down into a series of
smaller, more concise threads?
I've tried to do that but forgot to update the subject. This thread is now 
about implementing map-in. The goal is still to run FCode ROMs but first 
step is to get map-in working.
Thank you,
BALATON Zoltan