Re: [PATCH v2 1/8] MAINTAINERS: Mark SH4 hardware orphan

[Philippe Mathieu-Daudé]

On 6/10/20 1:08 PM, Aleksandar Markovic wrote:
> пон, 8. јун 2020. у 11:05 Philippe Mathieu-Daudé <f4bug@amsat.org> је
> написао/ла:
>>
>> Aurelien Jarno expressed his desire to orphan the SH4 hardware [*]:
>>
>>   I don't mind being [...] removed from there.
>>   I do not really have time to work on that.
>>
>> Mark the SH4 emulated hardware orphan.
>>
>> Many thanks to Aurelien for his substantial contributions to QEMU,
>> and for maintaining the SH4 hardware for various years!
>>
>> [*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg708400.html
>>
>> Message-Id: <20200601214125.GA1924990@aurel32.net>
>> Acked-by: Aurelien Jarno <aurelien@aurel32.net>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
> 
> The basic idea of the patch (as read from the title and the commit
> message) is good and positive.
> 
> The problem is that the patch does something different than the commit
> message says - pretending that it just orphans something. Which is not
> good. Actually, very clumsy and bad.

I can improve my English (actually I am trying, I started to take
lessons 2 years ago). Remember it is not easy for non native English
speaker to elaborate non-technical topics.
As a side note, it is also hard sometimes to understand contributors who
uses a very advanced English level with fancy words, as we have to take
the time to translate to understand. Even if we learn new word, this is
not often the best use of our time.

> It creates a whole new subsection in MAINTAINERS file (not said in the
> commit message), without any consistency with the current organization
> in the file. That new subsection looks completely misplaced, living
> with "TCG CPUs" neighbours. On top of that, it creates a new
> precedent, leaving many unanswered questions, like: Should other
> targets follow the same pattern?

If you look at git-log history, you'll see at the beginning there
were not much separations in directories. Everything was altogether.
Adding a new machine meant add the TCG emulation code and the hardware.
The contributor adding TCG emulation was doing the harder work,
usually doing it for a particular machine (hardware). Then other
contributors could add other machines, and were maintaining only
their machines. See commit b6f97c14f59 importing the short
MAINTAINERS from SVN. It was already split in 3 broad categories:
- TCG cores
- Machines / Hardware
- Subsystems (the rest)

In October 2010 (fd5d5c566af0) Anthony Liguori rewrote MAINTAINERS
using Linux style tags.

In January 2011 (42f5a7e9367) Aurelien Jarno clarified the
difference between the TCG host part (backend, under the tcg/
directory) VS the target part (frontend, under target/).

Then the project grew, and eventually in May 2012 Paolo Bonzini
started to clean, by moving files into "subsystem" directories
(see 5e8861a0361..c353f261946). In March 2013 he followed by
splitting various hardware files from the hw/ directory
(see 0d09e41a51a..47b43a1f414).

In November 2013 (f05b328c9d8) Stefan Hajnoczi clarified the
importance of the 'block' subsystem (later completed in
commit e7c6e631b1 in April 2015).

2015 still, Paolo keeps cleaning (c17652ee409 cover the
TCG backend disassembler, 062710000db introduce Peter
Maydell for the ARM hardware).

In October 2018 Thomas Huth added the target/ folder
(commit fcf5ef2ab).

Recently last year (6347e1f1cc7) Markus Armbruster tried
to better describe the TCG frontend ("Overall TCG CPUs")
and backend ("TCG target") differences.


So while the difference between TCG emulation (or other
accelerators, KVM, ...) versus the hardware emulation is
not perfect, there is a separation (and the community
failed at clarifying it).

You can see when you were introduced as maintainer for the
MIPS target (commit c92023bfd) you were also added as
maintainer of the MIPSSIM and Fuloong 2E machines.

In commit 485cd9820 you volunteered to maintain the Malta
machine, so I believe you have a good understanding about
the difference between target code and hardware code.

> 
> I personally think that creating a new subsection is just a code
> churn, waste of everybody's time on unimportant things.

Eh... I read that after spending 1h writing the previous
paragraph. Yes, you are right.

> 
> Wouldn't it be simpler that you just changed statuses of all Aurelien
> sh4 sections to "Orphaned", as he already said he approves, and leave
> sh4 sections reorganization to a future maintainer?
> 
> If you really want to reorganize sh4 sections, these changes should be
> in a separate patch. "Orphaning" patch should contain only changes of
> statuses.

Good idea! Maybe if you send a patch to show me how to do it, it
will be easier to understand what you mean.

Thanks for also caring about the SH4 hardware, the code was
prone to bitrot and needed maintainer to look after it.

Regards,

Phil.

> 
> Regards,
> Aleksandar
> 
>>  MAINTAINERS | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6e7890ce82..49d90c70de 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -299,9 +299,7 @@ SH4 TCG CPUs
>>  M: Aurelien Jarno <aurelien@aurel32.net>
>>  S: Odd Fixes
>>  F: target/sh4/
>> -F: hw/sh4/
>>  F: disas/sh4.c
>> -F: include/hw/sh4/
>>
>>  SPARC TCG CPUs
>>  M: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> @@ -1948,6 +1946,14 @@ F: hw/*/*xive*
>>  F: include/hw/*/*xive*
>>  F: docs/*/*xive*
>>
>> +SH4 Hardware
>> +S: Orphan
>> +F: hw/sh4/
>> +F: hw/char/sh_serial.c
>> +F: hw/intc/sh_intc.c
>> +F: hw/timer/sh_timer.c
>> +F: include/hw/sh4/
>> +
>>  Subsystems
>>  ----------
>>  Audio
>> --
>> 2.21.3
>>
>>
>