This is untested and probably should be marked RFC as it's mainly to show what I was suggesting as an alternative fix for the decode-unit problem discovered by Mark. This could be taken further to adopt the decode-unit and encode-unit words from openfirmware to reduce clutter in pci.c even more which I may attempt if this direction is accepted. My config words patch will also need to be rebased but let this part settle fitst and I can submit the rest as a follow up or a v2 for this series later.
Regards,
BALATON Zoltan (3): pci: Use forth to add trivial open/close methods pci: Add is-pci-bus word to install some methods from forth pci: Fix calling decode-unit without active-package
drivers/pci.c | 78 +++++--------------------------------------------- drivers/pci.fs | 10 +++++++ 2 files changed, 17 insertions(+), 71 deletions(-)
There's already a forth word to add trivial open/close methods to a device node which we can use to simpilfy pci.c and reduce the clutter a bit.
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu --- drivers/pci.c | 26 +------------------------- 1 file changed, 1 insertion(+), 25 deletions(-)
diff --git a/drivers/pci.c b/drivers/pci.c index f30e427..779a8a4 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -52,7 +52,6 @@
DECLARE_UNNAMED_NODE( ob_pci_bus_node, INSTALL_OPEN, 2*sizeof(int) ); DECLARE_UNNAMED_NODE( ob_pci_bridge_node, INSTALL_OPEN, 2*sizeof(int) ); -DECLARE_UNNAMED_NODE( ob_pci_simple_node, 0, 2*sizeof(int) );
const pci_arch_t *arch;
@@ -175,18 +174,6 @@ static inline void pci_decode_pci_addr(pci_addr addr, int *flags, } }
-static void -ob_pci_open(int *idx) -{ - int ret=1; - RET ( -ret ); -} - -static void -ob_pci_close(int *idx) -{ -} - /* ( str len -- phys.lo phys.mid phys.hi ) */
static void @@ -455,8 +442,6 @@ ob_pci_dma_sync(int *idx) }
NODE_METHODS(ob_pci_bus_node) = { - { "open", ob_pci_open }, - { "close", ob_pci_close }, { "decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bus_map_in }, @@ -477,8 +462,6 @@ ob_pci_bridge_map_in(int *idx) }
NODE_METHODS(ob_pci_bridge_node) = { - { "open", ob_pci_open }, - { "close", ob_pci_close }, { "decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bridge_map_in }, @@ -489,11 +472,6 @@ NODE_METHODS(ob_pci_bridge_node) = { { "dma-sync", ob_pci_dma_sync }, };
-NODE_METHODS(ob_pci_simple_node) = { - { "open", ob_pci_open }, - { "close", ob_pci_close }, -}; - static void pci_set_bus_range(const pci_config_t *config) { phandle_t dev = find_dev(config->path); @@ -1863,9 +1841,7 @@ static phandle_t ob_configure_pci_device(const char* parent_path, }
/* if devices haven't supplied open/close words then supply them with simple defaults */ - if (!find_package_method("open", phandle) && !find_package_method("close", phandle)) { - BIND_NODE_METHODS(phandle, ob_pci_simple_node); - } + fword("is-open");
/* scan bus behind bridge device */ //if (htype & PCI_HEADER_TYPE_BRIDGE && class == PCI_BASE_CLASS_BRIDGE) {
The pci bus and bridge nodes have trivial dma related words that just call their parent. It is simpler to install these from forth, there's even a word to do that. This both allows removing some clutter from pci.c and introspecting these words from forth with the 'see' word that only showed it calls some C function before this patch but one can now see what these words actually do.
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu --- drivers/pci.c | 41 +---------------------------------------- drivers/pci.fs | 10 ++++++++++ 2 files changed, 11 insertions(+), 40 deletions(-)
diff --git a/drivers/pci.c b/drivers/pci.c index 779a8a4..55b959c 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -411,45 +411,10 @@ ob_pci_bus_map_in(int *idx) PUSH(virt); }
-static void -ob_pci_dma_alloc(int *idx) -{ - call_parent_method("dma-alloc"); -} - -static void -ob_pci_dma_free(int *idx) -{ - call_parent_method("dma-free"); -} - -static void -ob_pci_dma_map_in(int *idx) -{ - call_parent_method("dma-map-in"); -} - -static void -ob_pci_dma_map_out(int *idx) -{ - call_parent_method("dma-map-out"); -} - -static void -ob_pci_dma_sync(int *idx) -{ - call_parent_method("dma-sync"); -} - NODE_METHODS(ob_pci_bus_node) = { { "decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bus_map_in }, - { "dma-alloc", ob_pci_dma_alloc }, - { "dma-free", ob_pci_dma_free }, - { "dma-map-in", ob_pci_dma_map_in }, - { "dma-map-out", ob_pci_dma_map_out }, - { "dma-sync", ob_pci_dma_sync }, };
/* ( pci-addr.lo pci-addr.mid pci-addr.hi size -- virt ) */ @@ -465,11 +430,6 @@ NODE_METHODS(ob_pci_bridge_node) = { { "decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bridge_map_in }, - { "dma-alloc", ob_pci_dma_alloc }, - { "dma-free", ob_pci_dma_free }, - { "dma-map-in", ob_pci_dma_map_in }, - { "dma-map-out", ob_pci_dma_map_out }, - { "dma-sync", ob_pci_dma_sync }, };
static void pci_set_bus_range(const pci_config_t *config) @@ -1812,6 +1772,7 @@ static phandle_t ob_configure_pci_device(const char* parent_path, } else { BIND_NODE_METHODS(phandle, ob_pci_bus_node); } + fword("is-pci-bus"); break; }
diff --git a/drivers/pci.fs b/drivers/pci.fs index a7b56e1..3c3752e 100644 --- a/drivers/pci.fs +++ b/drivers/pci.fs @@ -37,4 +37,14 @@ then ;
+\ Install methods for a pci bus node +: is-pci-bus + \ 'is-open' will be called by pci.c + " dma-alloc" is-call-parent + " dma-free" is-call-parent + " dma-map-in" is-call-parent + " dma-map-out" is-call-parent + " dma-sync" is-call-parent + ; + [THEN]
The decode-unit and encode-unit methods should be static methods which can be called without an active instance. But decode-unit has to insert the bus number in phys.hi which is available when the device node is created. Instead of trying to get the bus-range property of the current device, which fails if this method is called from a different package, store the bus number in the device when it is created and add a small forth wrapper to add the bus number so the C function does not need to care about it.
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu --- NOTE: There should be a way to do the feval in last hunk within is-pci-bus so that line would not be needed here and could be put in pci.fs instead but I couldn't figure out how to put a colon definition within another so it's only evaluated when the outer one is executed. Does somebody with forth knowledge have an idea?
drivers/pci.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/pci.c b/drivers/pci.c index 55b959c..a50dc11 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -182,7 +182,6 @@ ob_pci_decode_unit(int *idx) ucell hi, mid, lo; const char *arg = pop_fstr_copy(); int dev, fn, reg, ss, n, p, t; - int bus, len; char *ptr;
PCI_DPRINTF("ob_pci_decode_unit idx=%p\n", idx); @@ -274,9 +273,7 @@ ob_pci_decode_unit(int *idx) } free((char*)arg);
- bus = get_int_property(get_cur_dev(), "bus-range", &len); - - hi = n | p | t | (ss << 24) | (bus << 16) | (dev << 11) | (fn << 8) | reg; + hi = n | p | t | (ss << 24) | (dev << 11) | (fn << 8) | reg;
PUSH(lo); PUSH(mid); @@ -412,7 +409,7 @@ ob_pci_bus_map_in(int *idx) }
NODE_METHODS(ob_pci_bus_node) = { - { "decode-unit", ob_pci_decode_unit }, + { "pci-decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bus_map_in }, }; @@ -427,7 +424,7 @@ ob_pci_bridge_map_in(int *idx) }
NODE_METHODS(ob_pci_bridge_node) = { - { "decode-unit", ob_pci_decode_unit }, + { "pci-decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bridge_map_in }, }; @@ -1773,6 +1770,8 @@ static phandle_t ob_configure_pci_device(const char* parent_path, BIND_NODE_METHODS(phandle, ob_pci_bus_node); } fword("is-pci-bus"); + PUSH(*bus_num); + feval(": decode-unit pci-decode-unit lbsplit nip ( bus-num ) literal swap bljoin ;"); break; }
The decode-unit and encode-unit methods should be static methods which can be called without an active instance. But decode-unit has to insert the bus number in phys.hi which is available when the device node is created. Instead of trying to get the bus-range property of the current device, which fails if this method is called from a different package, add a small forth wrapper that adds the bus number so the C function does not need to care about it.
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu --- Found how to move eval to is-pci-bus so this supersedes the previous version.
drivers/pci.c | 10 ++++------ drivers/pci.fs | 3 ++- 2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/pci.c b/drivers/pci.c index 55b959c..28e7321 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -182,7 +182,6 @@ ob_pci_decode_unit(int *idx) ucell hi, mid, lo; const char *arg = pop_fstr_copy(); int dev, fn, reg, ss, n, p, t; - int bus, len; char *ptr;
PCI_DPRINTF("ob_pci_decode_unit idx=%p\n", idx); @@ -274,9 +273,7 @@ ob_pci_decode_unit(int *idx) } free((char*)arg);
- bus = get_int_property(get_cur_dev(), "bus-range", &len); - - hi = n | p | t | (ss << 24) | (bus << 16) | (dev << 11) | (fn << 8) | reg; + hi = n | p | t | (ss << 24) | (dev << 11) | (fn << 8) | reg;
PUSH(lo); PUSH(mid); @@ -412,7 +409,7 @@ ob_pci_bus_map_in(int *idx) }
NODE_METHODS(ob_pci_bus_node) = { - { "decode-unit", ob_pci_decode_unit }, + { "pci-decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bus_map_in }, }; @@ -427,7 +424,7 @@ ob_pci_bridge_map_in(int *idx) }
NODE_METHODS(ob_pci_bridge_node) = { - { "decode-unit", ob_pci_decode_unit }, + { "pci-decode-unit", ob_pci_decode_unit }, { "encode-unit", ob_pci_encode_unit }, { "pci-map-in", ob_pci_bridge_map_in }, }; @@ -1772,6 +1769,7 @@ static phandle_t ob_configure_pci_device(const char* parent_path, } else { BIND_NODE_METHODS(phandle, ob_pci_bus_node); } + PUSH(*bus_num); fword("is-pci-bus"); break; } diff --git a/drivers/pci.fs b/drivers/pci.fs index 3c3752e..52d956a 100644 --- a/drivers/pci.fs +++ b/drivers/pci.fs @@ -38,7 +38,8 @@ ;
\ Install methods for a pci bus node -: is-pci-bus +: is-pci-bus ( bus-num -- ) + s" : decode-unit pci-decode-unit lbsplit nip ( bus-num ) literal swap bljoin ; " evaluate \ 'is-open' will be called by pci.c " dma-alloc" is-call-parent " dma-free" is-call-parent
On 18/03/2023 21:37, BALATON Zoltan wrote:
This is untested and probably should be marked RFC as it's mainly to show what I was suggesting as an alternative fix for the decode-unit problem discovered by Mark. This could be taken further to adopt the decode-unit and encode-unit words from openfirmware to reduce clutter in pci.c even more which I may attempt if this direction is accepted. My config words patch will also need to be rebased but let this part settle fitst and I can submit the rest as a follow up or a v2 for this series later.
Regards,
BALATON Zoltan (3): pci: Use forth to add trivial open/close methods pci: Add is-pci-bus word to install some methods from forth pci: Fix calling decode-unit without active-package
drivers/pci.c | 78 +++++--------------------------------------------- drivers/pci.fs | 10 +++++++ 2 files changed, 17 insertions(+), 71 deletions(-)
Thanks for posting an RFC for discussion. There are two separate meanings to the word "simplify" in the context of this patch: the first is being concise, in which Forth can often provide a very compact solution as demonstrated in patch 3. The second is being understandable to developers which improves maintainability in the long run.
I think it's easy to argue that patch 3 meets the first criteria, but as mentioned before I'm not particularly keen on this approach because it starts splitting the core PCI methods out of pci.c and into pci.fs whilst mixing languages and runtime/compile time semantics, and also as per my previous comment you'll end up embedding strings of Forth which is really something to be avoided.
I can see how the first couple of patches can appear opaque, but then it's easy to see that they work in the same way as the existing BIND_NODE_METHODS macro. And from a developer perspective this is something that "just works" in that you use the new macro, and you end up with the phandle as a C function parameter.
If there were already a significant amount of Forth code in the PCI bindings then a Forth solution such as yours would be a better approach, however for now my preference would be to use the approach from my original series since the glue code provides a much more intuitive solution given that the PCI bindings are written in C.
ATB,
Mark.
On Thu, 23 Mar 2023, Mark Cave-Ayland wrote:
On 18/03/2023 21:37, BALATON Zoltan wrote:
This is untested and probably should be marked RFC as it's mainly to show what I was suggesting as an alternative fix for the decode-unit problem discovered by Mark. This could be taken further to adopt the decode-unit and encode-unit words from openfirmware to reduce clutter in pci.c even more which I may attempt if this direction is accepted. My config words patch will also need to be rebased but let this part settle fitst and I can submit the rest as a follow up or a v2 for this series later.
Regards,
BALATON Zoltan (3): pci: Use forth to add trivial open/close methods pci: Add is-pci-bus word to install some methods from forth pci: Fix calling decode-unit without active-package
drivers/pci.c | 78 +++++--------------------------------------------- drivers/pci.fs | 10 +++++++ 2 files changed, 17 insertions(+), 71 deletions(-)
Thanks for posting an RFC for discussion. There are two separate meanings to the word "simplify" in the context of this patch: the first is being concise, in which Forth can often provide a very compact solution as demonstrated in patch 3. The second is being understandable to developers which improves maintainability in the long run.
What is it that's not understandable in these methods? I can understand it but cannot understand yours. Since forth is a fundamental part of Open Firmware and we already have a lot of it in OpenBIOS people who maintain it are expected to be at least somewhat familiar with forth so it should not be an issue to use it. I think this series simplifies this in both sense of the word and avoids introducing additional complexity unlike your approach.
I think it's easy to argue that patch 3 meets the first criteria, but as mentioned before I'm not particularly keen on this approach because it starts splitting the core PCI methods out of pci.c and into pci.fs whilst mixing languages and runtime/compile time semantics, and also as per my previous comment you'll end up embedding strings of Forth which is really something to be avoided.
We already have a lot of embedded forth strings all over the place, also in pci.c. Look at ebus_config_cb() for example. So why is the one line added by this series worse than those already there? Splitting methods between forth and C may also not be a problem. My original proposal was to move all pci bus methods to forth and I'm willing to do that if this was accepted but this series only demostrated the idea because I don't want to spend time on patches that will be discarded. The decode/encode unit words can be taken from openfirmware and I had a patch to implement map-in in forth which is currently broken and needs to be fixed anyway. The rest are just trivial call parents so at the end we could get rid of most of these. Leaving some in C when it's easier to do that should also not be a problem. We have two languages in OpenBIOS and we can use both and we already do that everywhere. You also mix forth and C otherwise you would not need to patch the bridge between them.
I can see how the first couple of patches can appear opaque, but then it's easy to see that they work in the same way as the existing BIND_NODE_METHODS macro. And from a developer perspective this is something that "just works" in that you use the new macro, and you end up with the phandle as a C function parameter.
I'm still not convinced that is needed. The node methods already take a parameter. What is that? Can you get the needed info from that parameter? Or can you do the same that openfirmware does but from C? Passing a phandle to a method seems like a hack to me.
If there were already a significant amount of Forth code in the PCI bindings then a Forth solution such as yours would be a better approach, however for now my preference would be to use the approach from my original series since the glue code provides a much more intuitive solution given that the PCI bindings are written in C.
I disagree but since nobody else here seems to have an opinion or care about OpenBIOS in the end you do what you want. Forth and C are mixed in OpenBIOS also in pci.c already so using that to make pci.c less cluttered and easier to read by adding some very simple forth that's also easily understandable would make this easier to maintain but if this did not convince you I can't do much else.
Regards, BALATON Zoltan
Hi!
On Fri, Mar 24, 2023 at 12:20:01AM +0100, BALATON Zoltan wrote:
If there were already a significant amount of Forth code in the PCI bindings then a Forth solution such as yours would be a better approach, however for now my preference would be to use the approach from my original series since the glue code provides a much more intuitive solution given that the PCI bindings are written in C.
I disagree but since nobody else here seems to have an opinion or care about OpenBIOS in the end you do what you want. Forth and C are mixed in OpenBIOS also in pci.c already so using that to make pci.c less cluttered and easier to read by adding some very simple forth that's also easily understandable would make this easier to maintain but if this did not convince you I can't do much else.
I care. I find doing as much as possible in C incredibly clumsy, just like you it seems -- you are not alone :-)
But, I don't maintain this code. Whoever does the work gets to make the decisions. I just throw nuts from the peanut gallery sometimes.
Segher
On Fri, 24 Mar 2023, Segher Boessenkool wrote:
On Fri, Mar 24, 2023 at 12:20:01AM +0100, BALATON Zoltan wrote:
If there were already a significant amount of Forth code in the PCI bindings then a Forth solution such as yours would be a better approach, however for now my preference would be to use the approach from my original series since the glue code provides a much more intuitive solution given that the PCI bindings are written in C.
I disagree but since nobody else here seems to have an opinion or care about OpenBIOS in the end you do what you want. Forth and C are mixed in OpenBIOS also in pci.c already so using that to make pci.c less cluttered and easier to read by adding some very simple forth that's also easily understandable would make this easier to maintain but if this did not convince you I can't do much else.
I care. I find doing as much as possible in C incredibly clumsy, just like you it seems -- you are not alone :-)
But, I don't maintain this code. Whoever does the work gets to make the decisions. I just throw nuts from the peanut gallery sometimes.
I would do some work but apparently I'm not allowed to... Another point is that Mark's proposed changes are something he would never merge if I submitted it so he should be equally critical with his own work and make a cleaner change. What I proposed as an alternative does the same simpler and we have at least 1 and a half votes for it against the 1 vote for Mark's series but if it's still not good enough then do it in C but in a cleaner way, without patching the Forth-C bridge which should also be possible. But I really don't see what would be the advantage of keeping these methods in C which just makes pci.c more cluttered and harder to read and maintain.
Regards, BALATON Zoltan
On 01/04/2023 12:36, BALATON Zoltan wrote:
On Fri, 24 Mar 2023, Segher Boessenkool wrote:
On Fri, Mar 24, 2023 at 12:20:01AM +0100, BALATON Zoltan wrote:
If there were already a significant amount of Forth code in the PCI bindings then a Forth solution such as yours would be a better approach, however for now my preference would be to use the approach from my original series since the glue code provides a much more intuitive solution given that the PCI bindings are written in C.
I disagree but since nobody else here seems to have an opinion or care about OpenBIOS in the end you do what you want. Forth and C are mixed in OpenBIOS also in pci.c already so using that to make pci.c less cluttered and easier to read by adding some very simple forth that's also easily understandable would make this easier to maintain but if this did not convince you I can't do much else.
I care. I find doing as much as possible in C incredibly clumsy, just like you it seems -- you are not alone :-)
But, I don't maintain this code. Whoever does the work gets to make the decisions. I just throw nuts from the peanut gallery sometimes.
I would do some work but apparently I'm not allowed to... Another point is that Mark's proposed changes are something he would never merge if I submitted it so he should be equally critical with his own work and make a cleaner change.
This is just getting ridiculous now.
What I proposed as an alternative does the same simpler and we have at least 1 and a half votes for it against the 1 vote for Mark's series but if it's still not good enough then do it in C but in a cleaner way, without patching the Forth-C bridge which should also be possible. But I really don't see what would be the advantage of keeping these methods in C which just makes pci.c more cluttered and harder to read and maintain.
I've already explained in my previous response why the approach you are proposing is not simpler or cleaner. Certainly for historical reasons OpenBIOS has a large C component, however if you are interested in spending time converting drivers from C to Forth then feel free to post a proposal to the list.
ATB,
Mark.
On Thu, 20 Apr 2023, Mark Cave-Ayland wrote:
On 01/04/2023 12:36, BALATON Zoltan wrote:
On Fri, 24 Mar 2023, Segher Boessenkool wrote:
On Fri, Mar 24, 2023 at 12:20:01AM +0100, BALATON Zoltan wrote:
If there were already a significant amount of Forth code in the PCI bindings then a Forth solution such as yours would be a better approach, however for now my preference would be to use the approach from my original series since the glue code provides a much more intuitive solution given that the PCI bindings are written in C.
I disagree but since nobody else here seems to have an opinion or care about OpenBIOS in the end you do what you want. Forth and C are mixed in OpenBIOS also in pci.c already so using that to make pci.c less cluttered and easier to read by adding some very simple forth that's also easily understandable would make this easier to maintain but if this did not convince you I can't do much else.
I care. I find doing as much as possible in C incredibly clumsy, just like you it seems -- you are not alone :-)
But, I don't maintain this code. Whoever does the work gets to make the decisions. I just throw nuts from the peanut gallery sometimes.
I would do some work but apparently I'm not allowed to... Another point is that Mark's proposed changes are something he would never merge if I submitted it so he should be equally critical with his own work and make a cleaner change.
This is just getting ridiculous now.
What I proposed as an alternative does the same simpler and we have at least 1 and a half votes for it against the 1 vote for Mark's series but if it's still not good enough then do it in C but in a cleaner way, without patching the Forth-C bridge which should also be possible. But I really don't see what would be the advantage of keeping these methods in C which just makes pci.c more cluttered and harder to read and maintain.
I've already explained in my previous response why the approach you are proposing is not simpler or cleaner. Certainly for historical reasons OpenBIOS has a large C component, however if you are interested in spending time converting drivers from C to Forth then feel free to post a proposal to the list.
Why should I waste my time if the response will just be that you don't like it and will ignore it? That's why I've asked in advance if you would accept such change and only did minimal version first before spending any more time with it, as I'd only do that if the time will not be wasted. Otherwise I have better things to do in that time so I'll just refrain from contributing to OpenBIOS at all instead. But that leaves noone else besides you to do the things needed to:
- allow running FCode ROMs which is needed to pass through GPUs to MacOS - Fix the mac99 emulation so it works with more guests - Fix the G5 mac99 emulation so it can boot anything else than Linux
My patches aimed to solve the above step by step but you don't even let me do a small step without either doing it all at once or following your ideas only.
I still think it is a problem that the way you're holding hostage OpenBIOS development and Mac emulation in QEMU prevents any progress with these because you don't have time to work on it but also don't let others to work on it. This isn't wnat a maintainer should do but you don't seem to understand that and when I try to explain it to you you take it personal and get offended. This probably scares away potential contributors and I remember at least one case where somebody tried to submit a change to QEMU mos6522 model to fix an issue but you did the same to him that you usually do with my chnages (so I know it's not personal) and that change was not accepteed after ending up arguing about it at the end and the problem may still not be fixed. I don't know the reasons why you're doing this. I guess sometimes maybe you have local changes you don't want to rebase or have different ideas on how a problem should be solved or you don't trust anybody's judgement but yours but as a maintainer you should impartially consider if a proposed change is making progress and does not break or make existing code worse and not force your ideas on everybody. Also demanding extensive clean ups to be done by a contributor just to accept a small change that you regularly do (and tha above request is going that direction again) is unreasonable and jusd holds back any progress. This thread is now a good summary of all of this again.
I agree this is getting ridiculous so I just stop trying to work with you until you change your ways or this is resolved somehow. If you can't help the project to progress, you could consider stepping down as a maintainer. You could and should stay and provide your expertise for review and testing but you should not be the person who alone decides what change can be accepted. So I think if there was somebody more impartial and less strict as a maintainer and your patches were judged under the same terms as any other contributor that would help the project in general.
You don't have to reply, I don't think there's any point to discuss this further. I've tried to explain this several times and if you still don't think I have a point then there isn't much to add to that. I'd like to say again that I have nothing against you personally and sorry again if I've offended you in the past. My intention is not to fight or offend you, just to explain the above which might be difficult when I'm trying to make some progress and constantly getting thrown back by this even in areas unrelated to OpenBIOS or mac99 like the vt82c686 changes the last time. This makes it more difficult to work on these than it should be and have fun with it.
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
- Implement missing register access words to get FCode ROMs working
- 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. (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.)
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.
Regards, BALATON Zoltan
On 20/04/2023 13:54, BALATON Zoltan wrote:
(cut)
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.
- 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?
- 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.
(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).
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. 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.
Finally your comments about holding the project hostage are completely unacceptable: 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.
ATB,
Mark.
On Thu, 20 Apr 2023, Mark Cave-Ayland wrote:
On 20/04/2023 13:54, BALATON Zoltan wrote:
(cut)
I do appreciate you're only replying to the ponts below and willing to continue constructive discussion.
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?
- 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.
- 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 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.
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.
(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. 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. 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.
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.
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.
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.
(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.)
Regards, BALATON Zoltan
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)
I do appreciate you're only replying to the ponts below and willing to continue constructive discussion.
If you feel that the points above were not in keeping with a technical discussion, then perhaps an apology is in order?
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.
- 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.
- 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.
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?
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.
I could see that there could be potential to bring libfdt into OpenBIOS since 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.
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 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.
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.
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.
ATB,
Mark.
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)
I do appreciate you're only replying to the ponts below and willing to continue constructive discussion.
If you feel that the points above were not in keeping with a technical discussion, then perhaps an apology is in order?
If you feel an apology is needed then I'm happy to say sorry again but what I meant was that the above were just describing the current state and past so discussing it further would not move us forward. As I said before I don't have anything against you personally, only criticising the way you manage these projects which makes it almost impossible to make any changes for potential contributors which results in no progress and I think it's because you don't have time to do everthing you want yet don't let others to do anything you don't think is perfect enough and that results in stalled projects that are almost dead. Also this would scare away all potential contributors who don't want to keep up with your demands. That's why I though it might be good for everyone if you gave the maintainership to someone else which would relieve you from some of your burdens and may allow the project to go somewhere and you would not have to feel responsible for it.
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?
- 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.
- 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.
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.
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.
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.
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.
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.
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.
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.
Regards, BALATON Zoltan
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)
I do appreciate you're only replying to the ponts below and willing to continue constructive discussion.
If you feel that the points above were not in keeping with a technical discussion, then perhaps an apology is in order?
If you feel an apology is needed then I'm happy to say sorry again but what I meant was that the above were just describing the current state and past so discussing it further would not move us forward. As I said before I don't have anything against you personally, only criticising the way you manage these projects which makes it almost impossible to make any changes for potential contributors which results in no progress and I think it's because you don't have time to do everthing you want yet don't let others to do anything you don't think is perfect enough and that results in stalled projects that are almost dead. Also this would scare away all potential contributors who don't want to keep up with your demands. That's why I though it might be good for everyone if you gave the maintainership to someone else which would relieve you from some of your burdens and may allow the project to go somewhere and you would not have to feel responsible for it.
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. 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.
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....
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.
- 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 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.
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.
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 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.
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.
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 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.
ATB,
Mark.
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
-
BALATON Zoltan -
Mark Cave-Ayland -
Segher Boessenkool