Re: [PATCH] xen/arm: Validate generic timer frequency

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Julien, Henry,

On 10/10/2023 16:48, Julien Grall wrote:
> 
> 
> (+ Henry)
> 
> Hi Michal,
> 
> On 29/09/2023 08:38, Julien Grall wrote:
>> Hi Michal,
>>
>> On 28/09/2023 13:34, Michal Orzel wrote:
>>> Generic timer dt node property "clock-frequency" (refer Linux dt binding
>>> Documentation/devicetree/bindings/timer/arm,arch_timer.yaml) is used to
>>> override the incorrect value set by firmware in CNTFRQ_EL0. If the value
>>> of this property is 0 (i.e. by mistake), Xen would continue to work and
>>> use the value from the sysreg instead. The logic is thus incorrect and
>>> results in inconsistency when creating timer node for domUs:
>>>   - libxl domUs: clock_frequency member of struct xen_arch_domainconfig
>>>     is set to 0 and libxl ignores setting the "clock-frequency" property,
>>>   - dom0less domUs: "clock-frequency" property is taken from dt_host and
>>>     thus set to 0.
>>>
>>> Property "clock-frequency" is used to override the value from sysreg,
>>> so if it is also invalid, there is nothing we can do and we shall panic
>>> to let user know about incorrect setting. Going even further, we operate
>>> under assumption that the frequency must be at least 1KHz (i.e. cpu_khz
>>> not 0) in order for Xen to boot. Therefore, introduce a new helper
>>> validate_timer_frequency() to verify this assumption and use it to check
>>> the frequency obtained either from dt property or from sysreg.
>>>
>>> Signed-off-by: Michal Orzel <michal.orzel@xxxxxxx>
>>
>> Acked-by: Julien Grall <jgrall@xxxxxxxxxx>
> 
> Going through the list of pending patches, I noticed Henry wasn't CCed.
> Is this patch intended for Xen 4.18? If so, can you provide some
> reasoning why would want it?
Strictly speaking, lack of "for-4.18" prefix indicates that I did not 
particularly aim for 4.18.
However, I find it useful, so I will leave it up to Henry to decide if we want 
that or not.

Benefits:
- fixes the invalid logic the consequence of which might result in 
inconsistency when booting
  the same OS as libxl domU and dom0less domU.
- prevents spreading out the error condition (i.e. rate < 1KHZ) that can lead 
to e.g. Xen inability to schedule,
  by panicing with proper msg
Risks:
- early init code, no critical path, in case of an error, panic returned with 
proper msg - no risks AFAICT

~Michal