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

[Henry Wang <Henry.Wang@xxxxxxx>]

Hi Michal, Julien,

> On Oct 11, 2023, at 14:54, Michal Orzel <michal.orzel@xxxxxxx> wrote:
> 
> 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

Thanks for the explanation, this looks like an improvement for the
current code to me so I am fine with including it in 4.18 

Release-acked-by: Henry Wang <Henry.Wang@xxxxxxx>

Kind regards,
Henry

> 
> ~Michal