Re: Deadcode discussion based on Arm NS phys timer

[Julien Grall <julien@xxxxxxx>]

On 24/10/2022 10:07, Michal Orzel wrote:
> Hello,
Hi Michal,

> Recently I came across a deadcode in Xen Arm arch timer code. Briefly speaking, 
> we are routing
> the NS phys timer (CNTP) IRQ to Xen, even though Xen does not make use of it 
> (as it uses the hypervisor timer CNTHP).
> This timer is fully emulated, which means that there is nothing that can 
> trigger such IRQ. This code is
> a left over from early days, where the CNTHP was buggy on some models and we 
> had to use the CNTP instead.
>
> As far as the problem itself is not really interesting, it raises a question of 
> what to do with a deadcode,
> as there might be/are other deadcode places in Xen.
There are multiple definition of deadcode. Depending on which one you 
chose, then this could cover IS_ENABLED() and possibly #ifdef. So this 
would result to a lot of places impacted with the decision.
So can you clarify what you mean by deadcode?

> One may say that it is useful to keep it, because one day,
> someone might need it when dealing with yet another broken HW. Such person 
> would still need to modify the other
> part of the code (e.g. reprogram_timer), but there would be less work required 
> overall. Personally, I'm not in favor of
> such approach, because we should not really support possible scenarios with 
> broken HW (except for erratas listing known issues).
The difference between "broken HW" and "HW with known errata" is a bit 
unclear to me. Can you clarify how you would make the difference here?
In particular, at which point do you consider that the HW should not be 
supported by Xen?
> Also, as part of the certification/FUSA process, there should be no deadcode 
> and we should have explanation for every block of code.
See above. What are you trying to cover by deadcode? Would protecting 
code with IS_ENABLED() (or #ifdef) ok?
> There are different ways to deal with a deadcode: > 1. Get rid of it completely
> 2. Leave it as it is
> 3. Admit that it can be useful one day and:
>    3.1. protect it with #if 0
>    3.2. protect it with a new Kconfig option (disabled by default) using #ifdef
>    3.3. protect it with a new Kconfig option (disabled by default) using 
> IS_ENABLED (to make sure code always compile)
>    3.4. protect it with a command line option (allowing to choose the timer to 
> be used by Xen)
> ...
>
> Let me know what you think.
Before answering the question, I would need some clarifications on your aim.

Cheers,

--
Julien Grall