On Fri, 14 Jun 2019, Mark Cave-Ayland wrote:
On 13/06/2019 21:01, BALATON Zoltan wrote:
- If QEMU always supplies the driver as card ROM which will then be installed, when
is this fallback necessary at all? Wouldn't it be simpler to just remove it altogether? On QEMU it's not needed, other configs probably don't work anyway and if they worked you may get the same problem as in the QEMU case before this patch when card is missing so getting rid of this fallback would simplify code and may only break real hardware case which is likely already broken anyway.
or
- If you think fallback still makes sense and is used by some real case then the
correct test may be to check if TCX card is present or not, not if running on QEMU or not. Maybe you should do this when setting up the card during sbus scan. The drivers/sbus.c seems to create device nodes when some slots are occupied (? is this correct? are these cards hardwired to these slots or what if they are not in the expected slot or something else is there). Anyway, you could check if these nodes are existing but ROM is not valid to decide to install the driver fallback which seems more correct. Or you could try to fall back when mapping the ROM or interpreting it has failed.
And this patchset implements option 1 :)
Sort of, but instead of removing the fall back altogether and thus simplifying code it only disables it on QEMU which does not get rid of the the code but makes it even harder to follow by having a QEMU driver now only used when not on QEMU. So maybe a comment would be useful to explain this at least. If this work around is still useful for Temlib then that's a valid use case that might justify keeping it but otherwise I'd be for simpifyng code instead of keeping around now unused and unneeded hacks that may have helped in the past but now just makes it more difficult to follow the code. (Mixing different QEMU and OpenBIOS versions may not be a frequent use case either and may not work due to changes in fw_cfg anyway.) So if there's nothing left needing it any more it would be better to remove such fall backs/work arounds, rather than keep everything in fear of breaking some theoretical use case as that results in more and more complex code.
But that's just my view and I don't really mind either way, so don't take this as saying not to apply your patches.
But I think a proper fix for the problem would be to fix FCode ROM support instead of adding fallbacks for everything. This reminds me of the QEMU VGA ndrv fallback which is also installed for every VGA device regardless if it works with them or not and I had to disable it to try a card ROM or driver with the ati-vga device. Along the same lines, that fallback (and the fw_cfg hack to download the driver) could also be removed if QEMU set the card ROM to the ndrv but the problem with that is that this is the std vga device and we would need to patch ROM only when this is used in a ppc mac emulation for which there're no functions in QEMU yet. But changing QEMU to allow board code to specify card ROM may be cleaner than the current way of patching in the ndrv for all vga devices and that would allow removing a lot of hacks from OpenBIOS: the fw_cfg download, ndrv installation and vga-ndrv? switch and would make it simpler until proper FCode support could be implemented some time later.
I hope at least some of what I said makes sense.
Indeed, although this is a little off-topic since SBus used by SPARC is separate from PCI. Note that you can disable the VGA ndrv driver via -prom-env 'vga-ndrv?=false' on the command line if you need it for testing.
Yes, this is off-topic but I was reminded of this as this is similar problem and that's what I've done as explained at the end of this page:
https://osdn.net/projects/qmiga/wiki/SubprojectAti
It is already possible to provide an option ROM to the VGA device in QEMU, the slightly tricky part is that you need to add some routines to QEMU to dynamically create the option ROM by creating a "dummy" option ROM header and then injecting the FCode payload. Then it should just be a case of mapping to ROM into the PCI device, applying a small patch to OpenBIOS and then you should be good.
I've found that FCode ROMs don't work but vga_config_cb() in openbios/drivers/pci.c seems to have other workarounds which installs an NDRV or a driver executable directly supplied as a card ROM where MacOS expects them. This is the same as how the QEMU,ndrv.bin hack installs the driver in openbios/drivers/vga.fs so the latter may not be needed if QEMU would supply QEMU,ndrv.bin as the card ROM. This is what I meant above and I think I've tried to explain this before but not sure we're on the same page yet.
I don't think we need to create option ROM in QEMU (expecially no FCode header would be needed as FCode ROMs don't work anyway), we just need a way to set the card ROM to QEMU,ndrv.bin when the card is in a mac_oldworld or mac_newworld machine. We can set option rom with romfile but that's for the card globally and not for the machine so we can't use that because the pc machines still need the vgabios option ROM, only mac machines need different ROM. But there seem to be no way to set it when the card is created through the -vga option. Maybe if the board code could set the property of the card before realizing it somehow that could work but I don't know how to do that and could not figure out an easy way last I've looked at it.
Regards, BALATON Zoltan