| Message ID | 61bbf544-74ac-b698-425a-d1db80acab43@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/time: CF-clobber annotations | expand |
On 01/03/2022 11:06, Jan Beulich wrote: > With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall") > in place we can further arrange for ENDBR removal from the functions no > longer subject to indirect calls. Note that plt_tsc is left untouched, > for not holding any pointer eligible for ENDBR removal. I'd be tempted to include it, for consistency sake alone. It is less likely to go wrong in the future if another hook is introduced. > Signed-off-by: Jan Beulich <jbeulich@suse.com> With the commit message, I'm not not certain if this is linked to the previous patch. Overall it looks fine, but I'd like to get clarity on this point. > --- > I did consider converting most of the plt_* to const (plt_hpet and > plt_pmtimer cannot be converted), but this would entail quite a few > further changes. It's all initdata. const is not terribly interesting, especially if it is invasive and incomplete to do. ~Andrew
On 01.03.2022 14:18, Andrew Cooper wrote: > On 01/03/2022 11:06, Jan Beulich wrote: >> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall") >> in place we can further arrange for ENDBR removal from the functions no >> longer subject to indirect calls. Note that plt_tsc is left untouched, >> for not holding any pointer eligible for ENDBR removal. > > I'd be tempted to include it, for consistency sake alone. > > It is less likely to go wrong in the future if another hook is introduced. Can do, sure. >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > With the commit message, I'm not not certain if this is linked to the > previous patch. > > Overall it looks fine, but I'd like to get clarity on this point. Converting read_tsc() was discussed (with Roger) earlier on, so I'd view this as a separate change. As said in reply to your comments on the 1st patch, how exactly read_tsc() looks like is orthogonal to the changes here at least as long as it doesn't live in .text. Jan
On 01/03/2022 14:18, Jan Beulich wrote: > On 01.03.2022 14:18, Andrew Cooper wrote: >> On 01/03/2022 11:06, Jan Beulich wrote: >>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall") >>> in place we can further arrange for ENDBR removal from the functions no >>> longer subject to indirect calls. Note that plt_tsc is left untouched, >>> for not holding any pointer eligible for ENDBR removal. >> I'd be tempted to include it, for consistency sake alone. >> >> It is less likely to go wrong in the future if another hook is introduced. > Can do, sure. > >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> With the commit message, I'm not not certain if this is linked to the >> previous patch. >> >> Overall it looks fine, but I'd like to get clarity on this point. > Converting read_tsc() was discussed (with Roger) earlier on, so I'd > view this as a separate change. As said in reply to your comments on > the 1st patch, how exactly read_tsc() looks like is orthogonal to > the changes here at least as long as it doesn't live in .text. Ok. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
On 01/03/2022 14:24, Andrew Cooper wrote: > On 01/03/2022 14:18, Jan Beulich wrote: >> On 01.03.2022 14:18, Andrew Cooper wrote: >>> On 01/03/2022 11:06, Jan Beulich wrote: >>>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall") >>>> in place we can further arrange for ENDBR removal from the functions no >>>> longer subject to indirect calls. Note that plt_tsc is left untouched, >>>> for not holding any pointer eligible for ENDBR removal. >>> I'd be tempted to include it, for consistency sake alone. >>> >>> It is less likely to go wrong in the future if another hook is introduced. >> Can do, sure. >> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> With the commit message, I'm not not certain if this is linked to the >>> previous patch. >>> >>> Overall it looks fine, but I'd like to get clarity on this point. >> Converting read_tsc() was discussed (with Roger) earlier on, so I'd >> view this as a separate change. As said in reply to your comments on >> the 1st patch, how exactly read_tsc() looks like is orthogonal to >> the changes here at least as long as it doesn't live in .text. > Ok. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Actually, you probably want to move plt_src into __ro_after_init along with this change. ~Andrew
On 01.03.2022 15:24, Andrew Cooper wrote: > On 01/03/2022 14:18, Jan Beulich wrote: >> On 01.03.2022 14:18, Andrew Cooper wrote: >>> On 01/03/2022 11:06, Jan Beulich wrote: >>>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall") >>>> in place we can further arrange for ENDBR removal from the functions no >>>> longer subject to indirect calls. Note that plt_tsc is left untouched, >>>> for not holding any pointer eligible for ENDBR removal. >>> I'd be tempted to include it, for consistency sake alone. >>> >>> It is less likely to go wrong in the future if another hook is introduced. >> Can do, sure. >> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> With the commit message, I'm not not certain if this is linked to the >>> previous patch. >>> >>> Overall it looks fine, but I'd like to get clarity on this point. >> Converting read_tsc() was discussed (with Roger) earlier on, so I'd >> view this as a separate change. As said in reply to your comments on >> the 1st patch, how exactly read_tsc() looks like is orthogonal to >> the changes here at least as long as it doesn't live in .text. > > Ok. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> Thanks. I'll take it this includes annotating plt_tsc as well, just as a precautionary measure (and as you did suggest; still visible in context above). Jan
On 01.03.2022 15:35, Andrew Cooper wrote: > On 01/03/2022 14:24, Andrew Cooper wrote: >> On 01/03/2022 14:18, Jan Beulich wrote: >>> On 01.03.2022 14:18, Andrew Cooper wrote: >>>> On 01/03/2022 11:06, Jan Beulich wrote: >>>>> With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall") >>>>> in place we can further arrange for ENDBR removal from the functions no >>>>> longer subject to indirect calls. Note that plt_tsc is left untouched, >>>>> for not holding any pointer eligible for ENDBR removal. >>>> I'd be tempted to include it, for consistency sake alone. >>>> >>>> It is less likely to go wrong in the future if another hook is introduced. >>> Can do, sure. >>> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> With the commit message, I'm not not certain if this is linked to the >>>> previous patch. >>>> >>>> Overall it looks fine, but I'd like to get clarity on this point. >>> Converting read_tsc() was discussed (with Roger) earlier on, so I'd >>> view this as a separate change. As said in reply to your comments on >>> the 1st patch, how exactly read_tsc() looks like is orthogonal to >>> the changes here at least as long as it doesn't live in .text. >> Ok. Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com> > > Actually, you probably want to move plt_src into __ro_after_init along > with this change. I'd view this as an independent change. Perhaps it would make for a better change if we went through and converted from __read_mostly for a bunch of items all in one go. Jan
--- a/xen/arch/x86/time.c +++ b/xen/arch/x86/time.c @@ -375,7 +375,7 @@ static void cf_check resume_pit(struct p outb(0, PIT_CH2); /* MSB of count */ } -static struct platform_timesource __initdata plt_pit = +static struct platform_timesource __initdata_cf_clobber plt_pit = { .id = "pit", .name = "PIT", @@ -483,7 +483,7 @@ static void cf_check resume_hpet(struct hpet_resume(NULL); } -static struct platform_timesource __initdata plt_hpet = +static struct platform_timesource __initdata_cf_clobber plt_hpet = { .id = "hpet", .name = "HPET", @@ -528,7 +528,7 @@ static s64 __init cf_check init_pmtimer( return adjust_elapsed(rdtsc_ordered() - start, elapsed, target); } -static struct platform_timesource __initdata plt_pmtimer = +static struct platform_timesource __initdata_cf_clobber plt_pmtimer = { .id = "acpi", .name = "ACPI PM Timer", @@ -683,7 +683,7 @@ static void cf_check resume_xen_timer(st write_atomic(&xen_timer_last, 0); } -static struct platform_timesource __initdata plt_xen_timer = +static struct platform_timesource __initdata_cf_clobber plt_xen_timer = { .id = "xen", .name = "XEN PV CLOCK", @@ -780,7 +780,7 @@ static uint64_t cf_check read_hyperv_tim return hv_scale_tsc(tsc, scale, offset); } -static struct platform_timesource __initdata plt_hyperv_timer = +static struct platform_timesource __initdata_cf_clobber plt_hyperv_timer = { .id = "hyperv", .name = "HYPER-V REFERENCE TSC",
With bed9ae54df44 ("x86/time: switch platform timer hooks to altcall") in place we can further arrange for ENDBR removal from the functions no longer subject to indirect calls. Note that plt_tsc is left untouched, for not holding any pointer eligible for ENDBR removal. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- I did consider converting most of the plt_* to const (plt_hpet and plt_pmtimer cannot be converted), but this would entail quite a few further changes.