| Message ID | 20230928123435.2802-1-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/arm: Validate generic timer frequency | expand |
> On 28 Sep 2023, at 13:34, Michal Orzel <michal.orzel@amd.com> 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@amd.com> > --- Reviewed-by: Luca Fancellu <luca.fancellu@arm.com>
On Thu, 27 Sep 2023, 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@amd.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/time.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c > index 3535bd8ac7c7..04b07096b165 100644 > --- a/xen/arch/arm/time.c > +++ b/xen/arch/arm/time.c > @@ -101,6 +101,17 @@ static void __init preinit_acpi_xen_time(void) > static void __init preinit_acpi_xen_time(void) { } > #endif > > +static void __init validate_timer_frequency(void) > +{ > + /* > + * ARM ARM does not impose any strict limit on the range of allowable > + * system counter frequencies. However, we operate under the assumption > + * that cpu_khz must not be 0. > + */ > + if ( !cpu_khz ) > + panic("Timer frequency is less than 1 KHz\n"); > +} > + > /* Set up the timer on the boot CPU (early init function) */ > static void __init preinit_dt_xen_time(void) > { > @@ -122,6 +133,7 @@ static void __init preinit_dt_xen_time(void) > if ( res ) > { > cpu_khz = rate / 1000; > + validate_timer_frequency(); > timer_dt_clock_frequency = rate; > } > } > @@ -137,7 +149,10 @@ void __init preinit_xen_time(void) > preinit_acpi_xen_time(); > > if ( !cpu_khz ) > + { > cpu_khz = (READ_SYSREG(CNTFRQ_EL0) & CNTFRQ_MASK) / 1000; > + validate_timer_frequency(); > + } > > res = platform_init_time(); > if ( res ) > -- > 2.25.1 >
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@amd.com> Acked-by: Julien Grall <jgrall@amazon.com> Cheers,
(+ 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@amd.com> > > Acked-by: Julien Grall <jgrall@amazon.com> 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? Cheers,
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@amd.com> >> >> Acked-by: Julien Grall <jgrall@amazon.com> > > 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
Hi Michal, Julien, > On Oct 11, 2023, at 14:54, Michal Orzel <michal.orzel@amd.com> 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@amd.com> >>> >>> Acked-by: Julien Grall <jgrall@amazon.com> >> >> 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@arm.com> Kind regards, Henry > > ~Michal
On 11/10/2023 08:01, Henry Wang wrote: > Hi Michal, Julien, > >> On Oct 11, 2023, at 14:54, Michal Orzel <michal.orzel@amd.com> 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@amd.com> >>>> >>>> Acked-by: Julien Grall <jgrall@amazon.com> >>> >>> 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@arm.com> Thanks. I will commit the patch later today. Cheers,
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@amd.com> 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@amd.com> >>>>> >>>>> Acked-by: Julien Grall <jgrall@amazon.com> >>>> >>>> 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@arm.com> > > Thanks. I will commit the patch later today. Now committed. Sorry for the delay. Cheers,
diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c index 3535bd8ac7c7..04b07096b165 100644 --- a/xen/arch/arm/time.c +++ b/xen/arch/arm/time.c @@ -101,6 +101,17 @@ static void __init preinit_acpi_xen_time(void) static void __init preinit_acpi_xen_time(void) { } #endif +static void __init validate_timer_frequency(void) +{ + /* + * ARM ARM does not impose any strict limit on the range of allowable + * system counter frequencies. However, we operate under the assumption + * that cpu_khz must not be 0. + */ + if ( !cpu_khz ) + panic("Timer frequency is less than 1 KHz\n"); +} + /* Set up the timer on the boot CPU (early init function) */ static void __init preinit_dt_xen_time(void) { @@ -122,6 +133,7 @@ static void __init preinit_dt_xen_time(void) if ( res ) { cpu_khz = rate / 1000; + validate_timer_frequency(); timer_dt_clock_frequency = rate; } } @@ -137,7 +149,10 @@ void __init preinit_xen_time(void) preinit_acpi_xen_time(); if ( !cpu_khz ) + { cpu_khz = (READ_SYSREG(CNTFRQ_EL0) & CNTFRQ_MASK) / 1000; + validate_timer_frequency(); + } res = platform_init_time(); if ( res )
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@amd.com> --- xen/arch/arm/time.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)