Merging our x86 HALs

[Colin Finck]

Hi all,

I have been looking into our HALs recently on the promise that it is a
huge mess that needs fixing. Well, as a start I could imagine merging
our 6 possible x86 HALs (Legacy, ACPI, APIC, ACPI+APIC, SMP, SMP+ACPI)
into a single one, even if Windows ships individual ones. I see many
advantages of that:

* Less duplications and reduced mess: Right now, the APIC HAL hangs at
HalpCalibrateStallExecution during boot, a function that has been fixed
and universally implemented in all non-APIC HALs. The APIC HAL also
duplicates HalpInitializePICs as HalpInitializeLegacyPIC.
If you look at the SMP code, it didn't even receive the last build
system changes and has conflicting implementations for APIC functions.
A single x86 HAL would ensure that all possible configurations are
maintained.

* Future-proof: How is one going to implement newer features like x2APIC
with a structure like that? Would it get another HAL, be integrated into
the APIC HAL, or what?
We wouldn't have such problems with a single x86 HAL.

* Less setup work and testing: Currently, 1st stage setup detects the
computer type and installs the appropriate HAL. As such, every
additional HAL needs to be added to 1st stage setup code.
The user is also able to select a custom HAL during setup, even if it
wouldn't work on the machine. We should give neither the user nor the
setup the ability to decide. The HAL itself knows best at boot-up what
features to enable and what not.

* Convenience: The same ReactOS installation could be used on several
different x86 computers.


So is this the way to go or do I miss something important?


Best regards,

Colin

[Javier Agustìn Fernàndez Arroyo]

But this breaks modularity, isnt It?

And HAL might become huge

El 10/12/2017 19:11, "Colin Finck" <colin(a)reactos.org> escribió:

>   Hi all,
>
>  I have been looking into our HALs recently on the promise that it is a
>  huge mess that needs fixing. Well, as a start I could imagine merging
>  our 6 possible x86 HALs (Legacy, ACPI, APIC, ACPI+APIC, SMP, SMP+ACPI)
>  into a single one, even if Windows ships individual ones. I see many
>  advantages of that:
>
>  * Less duplications and reduced mess: Right now, the APIC HAL hangs at
>  HalpCalibrateStallExecution during boot, a function that has been fixed
>  and universally implemented in all non-APIC HALs. The APIC HAL also
>  duplicates HalpInitializePICs as HalpInitializeLegacyPIC.
>  If you look at the SMP code, it didn't even receive the last build
>  system changes and has conflicting implementations for APIC functions.
>  A single x86 HAL would ensure that all possible configurations are
>  maintained.
>
>  * Future-proof: How is one going to implement newer features like x2APIC
>  with a structure like that? Would it get another HAL, be integrated into
>  the APIC HAL, or what?
>  We wouldn't have such problems with a single x86 HAL.
>
>  * Less setup work and testing: Currently, 1st stage setup detects the
>  computer type and installs the appropriate HAL. As such, every
>  additional HAL needs to be added to 1st stage setup code.
>  The user is also able to select a custom HAL during setup, even if it
>  wouldn't work on the machine. We should give neither the user nor the
>  setup the ability to decide. The HAL itself knows best at boot-up what
>  features to enable and what not.
>
>  * Convenience: The same ReactOS installation could be used on several
>  different x86 computers.
>
>
>  So is this the way to go or do I miss something important?
>
>
>  Best regards,
>
>  Colin
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[David Quintana (gigaherz)]

Colin: Are we talking merge and decide which method to use at runtime (boot
flags or something), or compile-time merging of the source code but
generating multiple final hals based on each possible configuration?

I'm assuming the former.

elhoir: Given the fact that some of the HAL implementations are already
broken due to code duplication, I think modularity in this case has proven
itself to be a BAD choice.

On 10 December 2017 at 19:21, Javier Agustìn Fernàndez Arroyo <
elhoir(a)gmail.com&gt; wrote:

>   But this breaks modularity, isnt It?
>
>  And HAL might become huge
>
>  El 10/12/2017 19:11, "Colin Finck" &lt;colin(a)reactos.org&gt; escribió:
>
> >   Hi all,
> >
> >  I have been looking into our HALs recently on the promise that it is a
> >  huge mess that needs fixing. Well, as a start I could imagine merging
> >  our 6 possible x86 HALs (Legacy, ACPI, APIC, ACPI+APIC, SMP, SMP+ACPI)
> >  into a single one, even if Windows ships individual ones. I see many
> >  advantages of that:
> >
> >  * Less duplications and reduced mess: Right now, the APIC HAL hangs at
> >  HalpCalibrateStallExecution during boot, a function that has been fixed
> >  and universally implemented in all non-APIC HALs. The APIC HAL also
> >  duplicates HalpInitializePICs as HalpInitializeLegacyPIC.
> >  If you look at the SMP code, it didn't even receive the last build
> >  system changes and has conflicting implementations for APIC functions.
> >  A single x86 HAL would ensure that all possible configurations are
> >  maintained.
> >
> >  * Future-proof: How is one going to implement newer features like x2APIC
> >  with a structure like that? Would it get another HAL, be integrated into
> >  the APIC HAL, or what?
> >  We wouldn't have such problems with a single x86 HAL.
> >
> >  * Less setup work and testing: Currently, 1st stage setup detects the
> >  computer type and installs the appropriate HAL. As such, every
> >  additional HAL needs to be added to 1st stage setup code.
> >  The user is also able to select a custom HAL during setup, even if it
> >  wouldn't work on the machine. We should give neither the user nor the
> >  setup the ability to decide. The HAL itself knows best at boot-up what
> >  features to enable and what not.
> >
> >  * Convenience: The same ReactOS installation could be used on several
> >  different x86 computers.
> >
> >
> >  So is this the way to go or do I miss something important?
> >
> >
> >  Best regards,
> >
> >  Colin
> >
> >  _______________________________________________
> >  Ros-dev mailing list
> >  Ros-dev(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev 
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>

[Colin Finck]

Am 10.12.2017 um 19:38 schrieb David Quintana (gigaherz):
>   Colin: Are we talking merge and decide which method to
> use at runtime 
Exactly! We don't even need boot flags: Just like the setup currently
detects an ACPI-compliant computer, the HAL could do this at boot-up.
It's also not too hard to detect the presence of an APIC.

I think a universal HAL for every x86 machine wouldn't be noticeably
larger than an ACPI+SMP HAL.


- Colin

[Hermès BÉLUSCA-MAÏTO]

I personally think it's a bit "against" the philosophy of HALs, namely
having a lightweight hardware abstraction layer code for different platforms.
If you basically put all the HALs into one, then you obtain bloated stuff (which remains
in memory for the whole life of the OS). Example: standard HAL is 1MB vs. ACPI HAL which
is few kB.
A bit more work and you could even get a monolithic kernel! Nah joking xD ... but not
completely.

Note that if Windows nowadays has only one hal, it's because they now support
basically only one "architecture"/platform, namely, ACPI multiprocessor (to put
it simple). It has its pros, but also a lot of cons.

To solve the original problem you have encountered in our code, just introduce
common/generic .c files containing the code that is similar everywhere, even at the level
of all the hals, or at the level of (let's say) a given CPU "type" (x86,
x64...), then there are the other .c that implement the different flavours of the
procedures that depend on the specific arch/platform.

Like this:

HALs
+---- Generic code
+---- HAL for a given arch #1 (e.g. x86)
|     +---- Generic code for this arch
|     +---- Code for standard (non-ACPI) HAL
|     +---- Code for ACPI HAL
|     +---- Code for a different HAL flavour (platform)?
|     +---- ...
|
+---- HAL for arch #2
|     +---- Generic code
|     +---- Code for platform
|     +---- Code for second platform
|     +---- ...
|
+---- etc...

This is very clear and maintainable.

H.

>   -----Message d'origine-----
>  De : Ros-dev [mailto:[e-mail]] De la part de Colin Finck
>  Envoyé : dimanche 10 décembre 2017 19:55
>  À : ros-dev(a)reactos.org
>  Objet : Re: [ros-dev] Merging our x86 HALs
>  
>  Am 10.12.2017 um 19:38 schrieb David Quintana (gigaherz):
> >   Colin: Are we talking merge and decide which
> > method to use at runtime 
>  
>  Exactly! We don't even need boot flags: Just like the setup currently detects an
>  ACPI-compliant computer, the HAL could do this at boot-up.
>  It's also not too hard to detect the presence of an APIC.
>  
>  I think a universal HAL for every x86 machine wouldn't be noticeably larger
>  than an ACPI+SMP HAL.
>  
>  
>  - Colin
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Hermès BÉLUSCA-MAÏTO]

Also, there is (at least parts of) HKLM\HARDWARE tree that is built at installation time
from info obtained from the HAL that is suitable for your platform, and that will be used
for the rest of the life of your Windows/ReactOS/... installation. So if you then think
that you'll be able to use "the same ReactOS installation [...] on several
different x86 computers" (to quote you), it'll be a bit more complicated than
that!!!!

Besides this, I've a question about your observation that in the APIC hal (not ACPI)
there's different implementation of HalpCalibrateStallExecution and HalpInitializePICs
/ HalpInitializeLegacyPIC . Isn't it precisely because these stuff are completely
different from the standard PICs used in platforms for which the standard HAL (and
possibly the ACPI HAL) are used?

For your x2APIC question, if this shares a good stuff wrt. APIC, then either it would be
inside the same APIC HAL, otherwise there could be two HALs, APIC and the other one, but
made from common "generic" code + different code that depend on the APIC vs.
x2APIC.

>   The user is also able to select a custom HAL during
> setup, even if it wouldn't work on the machine. We should give neither the user nor
> the setup the ability to decide. The HAL itself knows best at boot-up what features to
> enable and what not. 
Actually we should, because the detection might not work (of course in our simple case
"ACPI UP/MP" vs. "Standard", it's simple, but think about other
platforms where there can be subtle differences), AND the fact that an advanced user may
want to specify one's HAL. And normally it's not the setup that decides about the
HAL, but the bootloader. The bootloader (ntldr / winldr) also has the capability of
detecting the HAL to use and load, unless being specified otherwise by a switch in the
command-lines in boot.ini (ntldr) or somewhere in the BCD (bootmgr/winldr).

H.

>   -----Message d'origine-----
>  De : Ros-dev [mailto:[e-mail]] De la part de Hermès
>  BÉLUSCA-MAÏTO
>  Envoyé : lundi 11 décembre 2017 01:18
>  À : 'ReactOS Development List'
>  Objet : Re: [ros-dev] Merging our x86 HALs
>  
>  I personally think it's a bit "against" the philosophy of HALs, namely
> having a
>  lightweight hardware abstraction layer code for different platforms.
>  If you basically put all the HALs into one, then you obtain bloated stuff (which
>  remains in memory for the whole life of the OS). Example: standard HAL is
>  1MB vs. ACPI HAL which is few kB.
>  A bit more work and you could even get a monolithic kernel! Nah joking xD ...
>  but not completely.
>  
>  Note that if Windows nowadays has only one hal, it's because they now support
>  basically only one "architecture"/platform, namely, ACPI multiprocessor (to
> put
>  it simple). It has its pros, but also a lot of cons.
>  
>  To solve the original problem you have encountered in our code, just introduce
>  common/generic .c files containing the code that is similar everywhere, even
>  at the level of all the hals, or at the level of (let's say) a given CPU
> "type" (x86,
>  x64...), then there are the other .c that implement the different flavours of the
>  procedures that depend on the specific arch/platform.
>  
>  Like this:
>  
>  HALs
>  +---- Generic code
>  +---- HAL for a given arch #1 (e.g. x86)
>  |     +---- Generic code for this arch
>  |     +---- Code for standard (non-ACPI) HAL
>  |     +---- Code for ACPI HAL
>  |     +---- Code for a different HAL flavour (platform)?
>  |     +---- ...
>  |
>  +---- HAL for arch #2
>  |     +---- Generic code
>  |     +---- Code for platform
>  |     +---- Code for second platform
>  |     +---- ...
>  |
>  +---- etc...
>  
>  This is very clear and maintainable.
>  
>  H.
>  
> >   -----Message d'origine-----
> >  De : Ros-dev [mailto:[e-mail]] De la part de Colin
> >  Finck Envoyé : dimanche 10 décembre 2017 19:55 À : ros-dev(a)reactos.org
> >  Objet : Re: [ros-dev] Merging our x86 HALs
> >
> >  Am 10.12.2017 um 19:38 schrieb David Quintana (gigaherz):
> > >   Colin: Are we talking merge and decide which
> > > method to use at
> > >  runtime 
> >  Exactly! We don't even need boot flags: Just like the setup currently
> >  detects an ACPI-compliant computer, the HAL could do this at boot-up.
> >  It's also not too hard to detect the presence of an APIC.
> >
> >  I think a universal HAL for every x86 machine wouldn't be noticeably
> >  larger than an ACPI+SMP HAL.
> >
> >
> >  - Colin
> >
> >  _______________________________________________
> >  Ros-dev mailing list
> >  Ros-dev(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev 
>  
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Hermès BÉLUSCA-MAÏTO]

Actually, this is parts of the HKLM\SYSTEM\ControlSet00x\Enum tree that are subject to the
remark I made about HKLM\HARDWARE in my previous mail, while the HARDWARE hive is actually
volatile (thus regenerated each time at boot).

H.

>   -----Message d'origine-----
>  De : Ros-dev [mailto:[e-mail]] De la part de Hermès
>  BÉLUSCA-MAÏTO
>  Envoyé : lundi 11 décembre 2017 02:03
>  À : 'ReactOS Development List'
>  Objet : Re: [ros-dev] Merging our x86 HALs
>  
>  Also, there is (at least parts of) HKLM\HARDWARE tree that is built at
>  installation time from info obtained from the HAL that is suitable for your
>  platform, and that will be used for the rest of the life of your
>  Windows/ReactOS/... installation. So if you then think that you'll be able to use
>  "the same ReactOS installation [...] on several different x86 computers" (to
>  quote you), it'll be a bit more complicated than that!!!!
>  
>  Besides this, I've a question about your observation that in the APIC hal (not
>  ACPI) there's different implementation of HalpCalibrateStallExecution and
>  HalpInitializePICs / HalpInitializeLegacyPIC . Isn't it precisely because these
>  stuff are completely different from the standard PICs used in platforms for
>  which the standard HAL (and possibly the ACPI HAL) are used?
>  
>  For your x2APIC question, if this shares a good stuff wrt. APIC, then either it
>  would be inside the same APIC HAL, otherwise there could be two HALs, APIC
>  and the other one, but made from common "generic" code + different code
>  that depend on the APIC vs. x2APIC.
>  
> >   The user is also able to select a custom HAL
> > during setup, even if it wouldn't 
>  work on the machine. We should give neither
> the user nor the setup the ability
>  to decide. The HAL itself knows best at boot-up what features to enable and
>  what not.
>  
>  Actually we should, because the detection might not work (of course in our
>  simple case "ACPI UP/MP" vs. "Standard", it's simple, but think
> about other
>  platforms where there can be subtle differences), AND the fact that an
>  advanced user may want to specify one's HAL. And normally it's not the setup
>  that decides about the HAL, but the bootloader. The bootloader (ntldr / winldr)
>  also has the capability of detecting the HAL to use and load, unless being
>  specified otherwise by a switch in the command-lines in boot.ini (ntldr) or
>  somewhere in the BCD (bootmgr/winldr).
>  
>  H.
>  
> >   -----Message d'origine-----
> >  De : Ros-dev [mailto:[e-mail]] De la part de Hermès
> >  BÉLUSCA-MAÏTO Envoyé : lundi 11 décembre 2017 01:18 À : 'ReactOS
> >  Development List'
> >  Objet : Re: [ros-dev] Merging our x86 HALs
> >
> >  I personally think it's a bit "against" the philosophy of HALs, namely
> >  having a lightweight hardware abstraction layer code for different platforms.
> >  If you basically put all the HALs into one, then you obtain bloated
> >  stuff (which remains in memory for the whole life of the OS). Example:
> >  standard HAL is 1MB vs. ACPI HAL which is few kB.
> >  A bit more work and you could even get a monolithic kernel! Nah joking xD ...
> >  but not completely.
> >
> >  Note that if Windows nowadays has only one hal, it's because they now
> >  support basically only one "architecture"/platform, namely, ACPI
> >  multiprocessor (to put it simple). It has its pros, but also a lot of cons.
> >
> >  To solve the original problem you have encountered in our code, just
> >  introduce common/generic .c files containing the code that is similar
> >  everywhere, even at the level of all the hals, or at the level of
> >  (let's say) a given CPU "type" (x86, x64...), then there are the other
> >  .c that implement the different flavours of the procedures that depend on the 
> specific arch/platform.
> >  
> >  Like this:
> >
> >  HALs
> >  +---- Generic code
> >  +---- HAL for a given arch #1 (e.g. x86)
> >  |     +---- Generic code for this arch
> >  |     +---- Code for standard (non-ACPI) HAL
> >  |     +---- Code for ACPI HAL
> >  |     +---- Code for a different HAL flavour (platform)?
> >  |     +---- ...
> >  |
> >  +---- HAL for arch #2
> >  |     +---- Generic code
> >  |     +---- Code for platform
> >  |     +---- Code for second platform
> >  |     +---- ...
> >  |
> >  +---- etc...
> >
> >  This is very clear and maintainable.
> >
> >  H.
> >
> >  > -----Message d'origine-----
> >  > De : Ros-dev [mailto:[e-mail]] De la part de
> >  > Colin Finck Envoyé : dimanche 10 décembre 2017 19:55 À :
> >  > ros-dev(a)reactos.org Objet : Re: [ros-dev] Merging our x86 HALs
> >  >
> >  > Am 10.12.2017 um 19:38 schrieb David Quintana (gigaherz):
> >  > > Colin: Are we talking merge and decide which method to use at
> >  > > runtime
> >  >
> >  > Exactly! We don't even need boot flags: Just like the setup
> >  > currently detects an ACPI-compliant computer, the HAL could do this at 
> boot-up.
> > >   It's
> > > also not too hard to detect the presence of an APIC.
> > >
> > >  I think a universal HAL for every x86 machine wouldn't be noticeably
> > >  larger than an ACPI+SMP HAL.
> > >
> > >
> > >  - Colin
> > >
> > >  _______________________________________________
> > >  Ros-dev mailing list
> > >  Ros-dev(a)reactos.org
> > >  http://www.reactos.org/mailman/listinfo/ros-dev 
> >
> >  _______________________________________________
> >  Ros-dev mailing list
> >  Ros-dev(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev 
>  
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Colin Finck]

Am 11.12.2017 um 01:18 schrieb Hermès BÉLUSCA-MAÏTO:> If you basically
put all the HALs into one, then you obtain bloated stuff (which remains
in memory for the whole life of the OS). Example: standard HAL is 1MB
vs. ACPI HAL which is few kBHave you actually checked what makes up this
difference?
Hint: hal/halx86/legacy/bus/pci_vendors.ids


>   Note that if Windows nowadays has only one hal,
> it's because they now support basically only one "architecture"/platform,
> namely, ACPI multiprocessor (to put it simple). It has its pros, but also a lot of cons.
That doesn't mean we need to do the same. We can have one HAL for all
(Pentium and newer) x86 platforms. The overhead of additional checks at
boot-up is negligible. That should be a solution for 99% of the people
out there. The rest may still go and trim down our HAL to their needs.

But let's not pretend we can maintain multiple x86 HALs for all x86
computers out there. Do you really want to test X HALs with Y different
systems? Ensure that a legacy HAL runs on a modern ACPI system? What
would be the point?


>   Besides this, I've a question about your
> observation that in the APIC hal (not ACPI) there's different implementation of
> HalpCalibrateStallExecution and HalpInitializePICs / HalpInitializeLegacyPIC . Isn't
> it precisely because these stuff are completely different from the standard PICs used in
> platforms for which the standard HAL (and possibly the ACPI HAL) are used? 
Absolutely not! You need to reprogram the standard PICs also on an APIC
system, and this is precisely what both functions do. Put them into a
diff tool to see for yourself.

The same goes for timers. Even with the introduction of ACPI Timers,
Local APIC Timers, and Time-Stamp Counters, you still need a traditional
one (like RTC or PIT) for calibration at system startup. Simply because
the newer ones don't run at a known fixed frequency.
The Legacy HAL successfully employs an algorithm based on the RTC while
the APIC HAL unsuccessfully tries to use the PIT.


>   Actually we should, because the detection might not
> work (of course in our simple case "ACPI UP/MP" vs. "Standard",
> it's simple, but think about other platforms where there can be subtle differences)
Tell me about a single one we cannot detect and which is worth to
support. I don't recall that we ever recommended our testers to choose a
different HAL at setup.


>   And normally it's not the setup that decides about
> the HAL, but the bootloader. 
That defies your previous point about the setup initializing the
registry depending on the HAL.
If we can let the user select a Legacy HAL in the boot loader after
installing with an ACPI HAL, it is also technically possible to have one
HAL that encompasses both.


- Colin

[Colin Finck]

Thanks for the numerous replies! Let's stop at this point and not talk
this topic to death ;)
Even if we don't know yet how many HALs are to be kept, we agree on
reducing the number of HALs from the current 6. There is also no point
in a final decision on this until we have code to back it.

I have begun by deduplicating some code:
https://github.com/reactos/reactos/pull/193
It has also turned out that HalpCalibrateStallExecution was not broken
in the APIC HAL, but relied on the I/O APIC. Unlike the Local APIC, this
one is disabled by default in VirtualBox. When enabling it, the APIC HAL
fails at a later stage.
However, we may still merge both implementations of
HalpCalibrateStallExecution into a generic one at some point.


Cheers,

Colin