On Thu, 11 May 2023, Mark Cave-Ayland wrote:
On 27/04/2023 15:21, BALATON Zoltan wrote:
On Wed, 26 Apr 2023, Mark Cave-Ayland wrote:
On 20/04/2023 18:18, BALATON Zoltan wrote:
On Thu, 20 Apr 2023, Mark Cave-Ayland wrote:
On 20/04/2023 13:54, BALATON Zoltan wrote: (cut)
Thank you for the apology, but if a maintainer rejects a patch for valid technical reasons or because the patch introduces a regression is to be expected as part of the review process.
I can accept if my patch is rejected because it would break something or if a reviewer suggests a better way to do that which is not much more work to implement (what I called reasonable review because asking to rewrite whole PCI handling in OpenBIOS or QOM=ifying ISA handling to fix a PCI IDE contoller is clearly beyond that) but often the case was not because of a regression or valid technical reason but more because of different views or you think it isn't a good approach even if it works. Of course "valid technical reasons" is debatable so it's hard to argue because it may be different for you and me. This is the case for this series too. It does simplify code, it does not introduce regressions yet you just don't like it without valud technical reasons.
Use of targeted attacks and strong language such as "demands" and "perfection" are a reflection of your personal perception of the review process, which I believe is in stark contrast to evidence of the technical discussions contained within the archives.
Maybe I got upset by repeatedly having the same resistence against any change I want to make and made me feel I wasted time and effort trying to improve this. Also English is not my native language and I do state facts without caring much about formality which may come across as rude sometimes but it's not meant offensive just saying what it is and saying that you demand perfection to accept a change does not seem to me attacking just what my experience was. Otherwise it's hard to explain why you don't accept changes that would fix some issues while not introducing new problems.
In case you decide to change your mind or somebody else can step in as a maintainer to more sensibly manage this project, my ideas for improvements that I'd consider to elaborate on would be:
- Finish this pci method simplification by moving some more methods to
Forth
I've already answered this one.
If I got your answer correctly you said this is not a good idea because you prefer to keep everyting in pci.c even if that's cluttering that file and doing some things from Forth would be simpler, because you think Forth is harder to maintain than C so OpenBIOS should have less Forth not more. Does that summarise your answer or did I misunderstand it?
No, I'm afraid you've misunderstood my reply completely.
So then can you plase explain again why this series is not acceptable?
I'm not sure there's much point in repeating myself again, so here is the link from before: https://mail.coreboot.org/hyperkitty/list/openbios@openbios.org/message/5QOG....
Rereading that again I still only get what I summarised above: There's no "valid technical reason" or regression against my change, just that you don't think it's a good idea because it introduces more Forth and you want to keep PCI handling in C even if that's more complex. But you say above this is not at all what you said and I'm completely misunderstood it. At this point I don't thing there's a reason to waste more of my time arguing with you about that and trying to convince you to judge patches without bias.
I should add that I originally wrote the series as a precursor to a patch that I have waiting here which solves the problem where the NDRV is inadvertently picked up by the ATI VGA device, which was a bug that you previously reported.
Given the length of this thread, unless there are any new revelations then I plan to push this soon.
I still think your patches are less clean and the first patches adding a bridge function just to pass an argument should not be needed as there's alreasy an argument passed to node methods and I've also demonstrated in this series that all this could be done much simpler without all that complexity but you're still defending your patches and reasy to ignore all this talk so probably no point to continue.
- Implement missing register access words to get FCode ROMs working
Segher already replied to this, explaining how the implementation should call the associated FCode tokens. I don't believe there has been another version of the patch submitted that addresses his comments?
The first version I've submitted of this was done during testing passing through a card to MacOS with QEMU and later I've realised it wasn't right and also replied with that on my own patch. I then haven't done a new version because the person who was testing this disappeared and I was busy doing other things so haven't yet got around trying to impelement it again. I may want to do it in the future but the difficulty of making any change does not encourage me to do so.
Obviously we all contribute our time/effort as we see fit in a community project, so I leave it to you if you wish to submit an updated version of the patch based upon Segher's reply.
I may eventually come back to that but currently not interested in it.
That's fine with me, I'll review it if/when it appears.
- Get rid of fw_cfg and hard coded machine parameters and pass an FDT
from QEMU instead as done my spapr and SLOF or pegasos2 and VOF; I've already ported SLOF's FDT parser to OpenBIOS but because it's impossible to convince you I did not finish this. This would simplify arch/ppc/qemu a lot by removing hard coded device trees and lift the need to change OpenBIOS together with QEMU as then device tree changes could be made in QEMU alone. For example fixing G5 mac99 can be possible without any change in OpenBIOS and thus lessen maintenance need for OpenBIOS.
When I asked how this would work with the device trees already included in OpenBIOS, in particular i) how you would bind Forth words into the device nodes in the FDT and ii) how this would work for non-QEMU builds without duplication there was no follow-up proposal.
As I said these are ideas that need to be elaborated so I may not be able to answer every detail before I get there, but if the ideas are shot down even before attempting it, I'll never get there. I think at least SLOF proves it's possible to do that and this series already explored at least two ways to bind methods to device nodes so the above should not be unsolvable problems, even if we don't yet have a clear understanding of every detail. So citing it as reason to not even consider the idea is not progressive.
The thing is we already know that passing an FDT image is possible, the big unknown is how such a binding with Forth words would work. This is a such a critical part of the solution I can't see how you would invest any significant time into this without proposing a solution.
I would go through with it and find out if I knew this could be accepted at the end but if the result would be that I submit something then you look at it and say OK but I don't like this so won't be merged that really discourages me to even try it in the first place. So unless you say you're ready to accept such a solution if it works I will only work on this to see if it's possible but that's very low priority. Even now when the series is not yet ready you're already bringing up theoretical problems to justify why you would not take it which does not really help to make progress. Theoretical problems like this next one:
The other thing to remember is that OpenBIOS is intended to run on real hardware where possible: I don't particularly want 2 separate mechanisms for building device trees in OpenBIOS since that then takes twice as long to debug. Will you remember to test both scenarios before you submit patches? Will I?
This is a non existing problem for several reasons:
- OpenBIOS does not currently run on real hardware so I doubt you'd test on
it.
- It already works differently on QEMU as fw_cfg does not exist elsewhere.
- We already have not 2 but at least 4 different versions just for ppc.
What I propose to chnage is only QEMU and the change would just replace the hard coded device tree fragments currently in arch/ppc/qemu with something passed from QEMU to keep it together with the machines and remove duplicated info from OpenBIOS so they don't have to be kept in sync.
I shall ask you one last time: how will the Forth word bindings work? Until you can provide an outline of this, I am going to ignore this from now on. I
I probably don't get your question. What Forth word bindings are you asking about?
should add that there are people who use OpenBIOS and use it on FPGA hardware against QEMU as a reference, so this is a genuine and not theoretical use case.
What config does that use? Does it have anything to do with arch/ppc/qemu which is all I want to change?
As you can see from this the bar to make such a feature worthwhile is fairly high, so then the next question is does such a feature save us time and effort? How often do we find that we need to change DT properties in OpenBIOS to match QEMU? I'd say it is very rare these days.
What I see is that you're trying to make the bar high while in fact it should not be that high to keep everything working. I think it would save us effort because it would make OpenBIOS code much simpler so it should not need that much maintenance.
How much effort would it have saved us over the past couple of years? I'm genuinely curious.
That's hard to tell but I think if we had spent less time arguing about theoretical problems and could make small steps ahead we were able to go somewhere and not being stuck at the same place. Moving the hard coded device tree to QEMU removes the need to maintain it in OpenBIOS and also removes the need to update OpenBIOS if changes are made in QEMU so it should make your job easier as you would need to spend less time maintaining OpenBIOS.
I could see that there could be potential to bring libfdt into OpenBIOS since
I have never proposed to bring libfdt into OpenBIOS and it's not needed. SLOF does not depend on libfdt and it's not needed to unflatten the device tree which can be don fully in Forth. After that point everything works the same as now where we have a device tree constucted in arch/ppc/qemu/init.c. I think you actually don't even get my proposal but you're sure it can't work and you don't want to merge it whatever it may be. That's not a good basis for cooperation.
I do get your proposal, but currently it has gaps and I'm yet to be convinced the benefits outweigh the drawbacks. Whilst you accuse me of not listening to
Unfortunately my experience (evidenced by this thread again) shows that you're hard to be convinced about anythnig that even slightly doesn't match your views so I'm not much motivated to spend time with it if the likely result would be that it won't be considered and quickly dismissed for whatever reason (valid of not, technical or not). I may still do that eventually but this makes it very low priority to me.
you, how many times have I asked that you *just in this thread* to explain how the FDT to Forth bindings would work? Because that is the key as to whether this approach is feasible or not.
There's no libfdt so there are also no Forth bindings to libfdt. FDT is also not used in OpenBIOS other than building the initial device tree that is now hard coded in arch/ppc/qemu/init.c which does not need libfdt and can be done entirely in Forth as SLOF already did that. I've ported the same fdt.fs from SLOF to OpenFirware and demonstrated this works. I've also ported it to OpenBIOS to at least compile but did not finish this. If this works with SLOF and OpenFirmware then it is also possible in OpenBIOS. I don't think there are unsolvable problems with this approach so I don't see what you think could be infeasible.
The suggestion of introducing libfdt was considering that if FDTs can be made to work with Forth in a way that isn't inconvenient to the developer, it may be worth standardising its use within OpenBIOS. But for now it's not worth continuing this discussion until you can demonstrate how the key part of the proposal should work.
Having Forth bindings to libfdt is hardly a key parf of my proposal. In fact it isn't even part of my proposal just something you've imagined and keep mixing it in here. I don't want to use libfdt in OpenBIOS, neither from Forth not from C. Libfdt is more suited to work with device trees from C and the proposal moves building most of the device tree to QEMU where you can use libfdt. (This is only for arch/ppc/qemu for now, what you do with other machnies is up to you later.) The FDT that QEMU builds can be parsed with SLOF's fdt.fs to create an unflattened device tree in Forth then you can access it the same way as you access it now. At no point libfdt is needed for this and this would reduce the need to touch the device tree from C and get rid of a lot of embedded forth words in C that you recommended against.
then we can source the FDT image locally if there is no hypervisor, but again you end up back at the same critical place: how can the Forth word binding work, and in a way that isn't detrimental to developers working in Forth and C? A full solution isn't required, but you'll need a demonstrable proof-of-concept to show developers how this will work.
I did make a proof of concept in OpenFirmware for this where passing the FDT from QEMU and unflattening it worked but I could not make a single OFW ROM work for all PPC machines as OpenFirmware is meant to be for a single machine so I could not put machine support in packages that would only work after machine init. But the interesting part of getting the device tree is done there if you want to have a look. Obviously OpenFirmware has no C so the solution also does not require any changes to the C parts of OpenBIOS. Working with the resulting device tree is the same as now, the only change would be to add a step in startup to construct the initial device tree from the FDT placed in memory by QEMU as it's done in spapr which would replace the hard coded machine info and device tree fragments currently in arch/ppc/qemu so these would not have to be maintained in OpenBIOS and would be together with the machines they describe in QEMU so if something is changed in that machine then the device tre can be updated there and OpenBIOS likely does not need any further changes or maybe only adding a driver if it has to work with the device added. QEMU already depends on libfdt so you can use that to construct the device tree or load it from a file like how sam460ex does for Linux boot.
Yes, I've already seen this.
The point was that if the first reaction to every patch I submit will be that you don't agree and try to find how to prove it's wrong and yout idea is better, then it's really hard to contribute anything. This has certainly been my experience so far, not only in OpenBIOS but also in QEMU. The exceptions are cases where the proposed patch matched exacrly your ideas but even then you like to change it a bit and not just accept it as it is.
I don't understand this at all? Generally most reviewers will request minor changes to patches, as both you and I have done in the past.
As for the above problem, this would be solved when we get there. What I have now is that I can pass an FDT to OpenBIOS which is parsed to create an unflattened version. It's been a while I've tried it so maybe for OpenBIOS some more changes would be needed but it worked at least with Open Firmware, you can look it up on that mailing list where I've documented that experiment, Maybe the problems you mention don't even exist as this would jist replace the included device trees with one created by QEMU that contain the root node, cpus, memory and top level of pci nodes but no PCI devices (this is what's now hard coded in atch/ppc/qemu/init.c) then the rest is done the same way as now. So what I propose is simply instead of keeping a list of hardcoded stuff in OpenBIOS, read those from an FDT which is passed from QEMU. This way we don't have duplicated info about machines in both QEMU and OpenBIOS so 1) you don't have to maintain this in OpenBIOS and 2) no need to chahge both QEMU and OpenBIOS as the info is then only in the machine model in QEMU.
(already covered above, but again consider how often do we need to do this?)
(Related to this is passing VGA driver through ROM to lessen dependency on fw_cfg as a first step and move towards real FCode ROM as it works on real hardware so instead of hacks do what real hardware does. But that's another change that met resistence from you.)
As already explained on the QEMU list, the problem here is that the NDRV binary can only be built within a classic MacOS guest which means you would end up with the problems of distributing a binary blob from an external project. Since QEMU already has a mechanism to support this, it makes sense to leverage that via fw_cfg injection into OpenBIOS to avoid a dependency problem, which was also the intention of the original author (Ben).
This again isn't a problem that couldn't be solved if you wanted. What's more, it isn't even a problem yet before we can run FCodes. At this stage all I proposed was to pass the existing NDRV from QEMU to OpenBIOS putting it in the card ROM instead of downloading it from fw_cfg. This would allow simplifying OpenBIOS as then you can get rid of code to download the file and keep the NDRV dependency in QEMU.
But then you still have to build a ROM file with an embedded binary blob from an external source. I should add that the current mechanism doesn't prevent passing in external PCI option ROMs from real cards if needed, and it wouldn't have been accepted if it did.
Later if you don't want to introduce dependency on the NDRV in OpenBIOS to build the FCode ROM then do the other way around and let the NDRV depend on fcode-utils to compile the binary and the forth part to create an FCode ROM (after all that's a separate project anyway to provide a ROM for QEMU's graphics card). Then the FCode ROM will be included in QEMU the same way the ndrv is now included.
If you'd tried this, you'd immediately discover the problem trying to run fcode-utils in the non-POSIX NDRV build environment. And as previously discussed the longer-term aim is to generate a proper PCI option ROM from the FCode VGA driver, but there isn't a tool in either fcode-utils or OpenBIOS to generate the final ROM image (yet).
I think some people are using fcode-utils to decompile modify and compile patched ROMs for graphics cards for old Macs so this certainly works.
Do you have a link with an example somewhere? I believe romheaders can parse an existing ROM, but I wasn't aware it could create a new one.
I thing this may be in this forum discussion somewhere: https://forums.macrumors.com/threads/question-how-powerful-of-a-graphics-car...
I don't see any insurmountable problem here, you just like to present these as one to block progress with these and keep to your ideas.
Indeed they are not insurmountable, but I've responded to them above for you, in case you would like to work on and/or discuss this further.
I could try to explain the ideas in more detail but if your first reaction is just that you don't think this can be a good solution then even spending time with detailing the ideas is wasted time so I'll just stop now.
I've replied to your points above for completeness, since your representation above is misleading and implies that I am neglecting my role as maintainer.
Let me calrify again, I did not say you're neglecting your role. What I said is that you're doing it in a way which does not let anybody else contribute to the project (or make it unnecessarily hard) which holds back progress and discourages contributions. I think this should change. The best would be if you could change but if you can't then maybe somebody else could do a better job. But I've tried to explain that several times, this is the last attempt to do that again before I give up.
There is nothing wrong with having ideas as to how the project should proceed, but as you see above these ideas were often missing follow-up patches or critical detail. And one of the roles of a maintainer is to point out such problems before anyone invests a significant amount of time or effort to avoid disappointment.
On the other hand if you shot down ideas at the beginning and don't even accept simple steps towards a bigger goal deciding in advance that it won't work without even trying then people are not encouraged to pursue ideas as it will just be wasted time if they can't improve the project in the end.
How is pointing out the problems with a proposal "shooting it down"? If you can't demonstrate how to resolve the indicated problems then it strongly suggests the proposal isn't going to work.
Pointing out a real problem helps. Bringing up theoretical problems as a way to justify why this would not work or even if it did work why you would not want to merge it which is what you seem to do most of the time does not help.
Finally your comments about holding the project hostage are completely unacceptable:
I know my words may sound harsh but I could not put it better what the experience I got trying to contribute to projects you're maintaining was most of the time. You listen to what I say then just completely ignore it and do the same thing again and again. I can't even avoid it because we seem to be interested in the same parts of QEMU so we'll inevitably cross roads eventually. I can quit trying to change OpenBIOS and Mac emulation in QEMU but I won't completely leave QEMU development so this may happen again and again unfortunately. I think my ideas are reasonable and would improve the project but I can't even try it because you don't believe in it so don't even let it happen.
I believe I've replied (yet again) to the various points raised in your message.
Except why this series is not acceptable, which I still don't get as you said I did not understand your answer but that's what I got from your reply so I still don't know why this simple chnage is not appropriate then.
I've reposted the link explaining why I don't think this is a suitable approach above.
if a maintainer points out problems with your ideas or patches, it is not because of a whim but because there is a good reason e.g. the PCI bus id patches were refused because they broke booting NetBSD. If you find that you are constantly fighting with maintainers when proposing ideas and/or patches, I can only suggest taking some time out for a period of introspection to try and understand why this is the case.
I would do that if that happened and I can take reasonable criticism and improve the patches if needed but I find I only constantly have to fight with you, not with other maintainers so I believe it's your standards or views are unreasonably high not my approach is completely wrong. If the problems pointed out are real problems then the review helps. If they are only stem from constant disagreement and different views or look to me like excuses to not consider the changes then I have a hard time accepting that.
From what I can see all my arguments above are technical: if not, what have I missed?
(I've actually debugged that NetBSD issue back then and found it only boots now because it happens to work by chance as some memory that it used wasn't overwritten before but the slightly changed memory layout after the patch ended up overlapping something the guest used so that patch did not actually broke anything that wasn't already broken just exposed and existing ptoblem so there wasn't much to fix in that patch and I wasn't willing to debug and fix unrelated problems, especially that the latest NetBSD version still booted, it was only an old, likely unused version that had a problem.)
If you don't want to fix the issue, then that's fine. However I'm not willing to merge a patch that breaks things, particularly as we have no idea how many installations of NetBSD will be affected. Even more so, since I'm the maintainer then it will be me that gets the complaints both on and off-list.
I suspect there are no installations of that old NetBSD that could break. Anybody wanting to use NetBSD could likely update to the latest which did work. If there are some users who rely on that and you get reports, you could have just reverted the change. That would have been a more practical approach.
Could they? Real life experience says that this is often not the case. Alternatively as a project we should simply aim to improve functionality without introducing unnecessary regressions for our existing users.
But I don't care about this issue any more. Not sure you've noticed but I've added another machine in QEMU that can run more MorphOS versions than mac99 and also runs AmigaOS quite well which does not support mac99 and now has sound output as well while we're still waiting for screamer to be finished for mac99. So I'm not any more interested in improving mac99. I still have some ideas and may conribute to help people who want to run MacOS but I don't want to fight with you so I'd only consider spending time with it if you also allow changes to be made.
Yes, I've seen the work you've done introducing the new machines into QEMU. AFAICT the only new functionality outstanding for OpenBIOS and mac99 are the patches to allow executing a real ATI FCode ROM, and those have been either already merged or awaiting changes.
There are potentially much more but we're not getting anywhere with that if we always argue on small changes. I can run an ATI FCode ROM with my patches but that does not help anybody wanting to pass through real cards or test ROMs without breaking hardware. To run FCode ROMs you'd need:
- config access patches - fix map-in patch - register access patches (not yet written, the one I had is wrong) - for some NVIDIA ROMs: fix : within if (this is what people patch out in above forum thread as some older Apple OF also has problem with that)
Apart from that moving device tree creation from OpenBIOS to QEMU as proposed above would allow improving machines in the future without needing change in OpenBIOS such as separating G4 and G5 mac99 and fixing the latter to be able to boot something other than Linux. All these are prevented by current state of code which is hard to maintain and would take a lot of your time even if you already dont' have enough time for these.
Another interesting imrpovement would be to fix QEMU machines to be able to work with Apple ROMs which would need screamer to be mearged and the I2C emulation in macio ovehauled as those are neede for the ROM to even start as it detects RAM via I2C SPD data and playing the startup chime which is part of macio POST. I had patches for all these but not all of them are finished. I can get to OF prompt with g3beige now with Apple ROM but it does not boot yet.
Regards, BALATON Zoltan