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

[Julien Grall <julien@xxxxxxx>]

Hi,

On 11/10/2023 09:59, Julien Grall wrote:
> On 11/10/2023 08:01, Henry Wang wrote:
> > 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.
> I have seen a mix. Hence why I asked in case this was expected. To avoid 
> confusion during the freeze period, I tend to also tag e-mail for the 
> next release (e.g. for-next/for-4.19). So the intention is clear.
> > > 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>
> Thanks. I will commit the patch later today.
Now committed. Sorry for the delay.

Cheers,

--
Julien Grall