| Message ID | 20180116142337.24942-2-julien.grall@linaro.org |
|---|---|
| State | Accepted |
| Commit | 7500495155aacce437878cb576f45224ae984f40 |
| Headers | show |
| Series | xen/arm64: Branch predictor hardening (XSA-254 variant 2) | expand |
On Tue, 16 Jan 2018, Julien Grall wrote: > Once Xen knows what features/workarounds present on the platform, it > might be necessary to configure each online CPU. > > Introduce a new callback "enable" that will be called on each online CPU to > configure the "capability". > > The code is based on Linux v4.14 (where cpufeature.c comes from), the > explanation of why using stop_machine_run is kept as we have similar > problem in the future. > > Lastly introduce enable_errata_workaround that will be called once CPUs > have booted and before the hardware domain is created. > > This is part of XSA-254. > > Signed-of-by: Julien Grall <julien.grall@linaro.org> If you took the code from Linux, you need to add the original Signed-off-by from the Linux commit. Aside from that: Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/cpuerrata.c | 6 ++++++ > xen/arch/arm/cpufeature.c | 29 +++++++++++++++++++++++++++++ > xen/arch/arm/setup.c | 1 + > xen/include/asm-arm/cpuerrata.h | 1 + > xen/include/asm-arm/cpufeature.h | 3 +++ > 5 files changed, 40 insertions(+) > > diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c > index fe9e9facbe..772587c05a 100644 > --- a/xen/arch/arm/cpuerrata.c > +++ b/xen/arch/arm/cpuerrata.c > @@ -64,6 +64,12 @@ void check_local_cpu_errata(void) > { > update_cpu_capabilities(arm_errata, "enabled workaround for"); > } > + > +void __init enable_errata_workarounds(void) > +{ > + enable_cpu_capabilities(arm_errata); > +} > + > /* > * Local variables: > * mode: C > diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c > index 479c9fb011..525b45e22f 100644 > --- a/xen/arch/arm/cpufeature.c > +++ b/xen/arch/arm/cpufeature.c > @@ -19,6 +19,7 @@ > #include <xen/types.h> > #include <xen/init.h> > #include <xen/smp.h> > +#include <xen/stop_machine.h> > #include <asm/cpufeature.h> > > DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); > @@ -40,6 +41,34 @@ void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, > } > > /* > + * Run through the enabled capabilities and enable() it on all active > + * CPUs. > + */ > +void __init enable_cpu_capabilities(const struct arm_cpu_capabilities *caps) > +{ > + for ( ; caps->matches; caps++ ) > + { > + if ( !cpus_have_cap(caps->capability) ) > + continue; > + > + if ( caps->enable ) > + { > + int ret; > + > + /* > + * Use stop_machine_run() as it schedules the work allowing > + * us to modify PSTATE, instead of on_each_cpu() which uses > + * an IPI, giving us a PSTATE that disappears when we > + * return. > + */ > + ret = stop_machine_run(caps->enable, (void *)caps, NR_CPUS); > + /* stop_machine_run should never fail at this stage of the boot. */ > + BUG_ON(ret); > + } > + } > +} > + > +/* > * Local variables: > * mode: C > * c-file-style: "BSD" > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index 16a3b1be8e..032a6a882d 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -849,6 +849,7 @@ void __init start_xen(unsigned long boot_phys_offset, > * stop_machine (tasklets initialized via an initcall). > */ > apply_alternatives_all(); > + enable_errata_workarounds(); > > /* Create initial domain 0. */ > /* The vGIC for DOM0 is exactly emulating the hardware GIC */ > diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h > index 8b158429c7..7de68361ff 100644 > --- a/xen/include/asm-arm/cpuerrata.h > +++ b/xen/include/asm-arm/cpuerrata.h > @@ -5,6 +5,7 @@ > #include <asm/alternative.h> > > void check_local_cpu_errata(void); > +void enable_errata_workarounds(void); > > #ifdef CONFIG_HAS_ALTERNATIVE > > diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h > index f00b6dbd39..21c65e198c 100644 > --- a/xen/include/asm-arm/cpufeature.h > +++ b/xen/include/asm-arm/cpufeature.h > @@ -74,6 +74,7 @@ struct arm_cpu_capabilities { > const char *desc; > u16 capability; > bool (*matches)(const struct arm_cpu_capabilities *); > + int (*enable)(void *); /* Called on every active CPUs */ > union { > struct { /* To be used for eratum handling only */ > u32 midr_model; > @@ -85,6 +86,8 @@ struct arm_cpu_capabilities { > void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, > const char *info); > > +void enable_cpu_capabilities(const struct arm_cpu_capabilities *caps); > + > #endif /* __ASSEMBLY__ */ > > #endif > -- > 2.11.0 >
Hi Stefano, On 16/01/18 23:55, Stefano Stabellini wrote: > On Tue, 16 Jan 2018, Julien Grall wrote: >> Once Xen knows what features/workarounds present on the platform, it >> might be necessary to configure each online CPU. >> >> Introduce a new callback "enable" that will be called on each online CPU to >> configure the "capability". >> >> The code is based on Linux v4.14 (where cpufeature.c comes from), the >> explanation of why using stop_machine_run is kept as we have similar >> problem in the future. >> >> Lastly introduce enable_errata_workaround that will be called once CPUs >> have booted and before the hardware domain is created. >> >> This is part of XSA-254. >> >> Signed-of-by: Julien Grall <julien.grall@linaro.org> > > If you took the code from Linux, you need to add the original > Signed-off-by from the Linux commit. Aside from that: There are multiple commits touching this function. So I followed what we did in similar situation. By that I mean, mentioning the code was taken from Linux and not gathered the signed-off-by. If you really want, I can gather all the signed-off-by of the commit touching this function. Cheers, > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > >> --- >> xen/arch/arm/cpuerrata.c | 6 ++++++ >> xen/arch/arm/cpufeature.c | 29 +++++++++++++++++++++++++++++ >> xen/arch/arm/setup.c | 1 + >> xen/include/asm-arm/cpuerrata.h | 1 + >> xen/include/asm-arm/cpufeature.h | 3 +++ >> 5 files changed, 40 insertions(+) >> >> diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c >> index fe9e9facbe..772587c05a 100644 >> --- a/xen/arch/arm/cpuerrata.c >> +++ b/xen/arch/arm/cpuerrata.c >> @@ -64,6 +64,12 @@ void check_local_cpu_errata(void) >> { >> update_cpu_capabilities(arm_errata, "enabled workaround for"); >> } >> + >> +void __init enable_errata_workarounds(void) >> +{ >> + enable_cpu_capabilities(arm_errata); >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c >> index 479c9fb011..525b45e22f 100644 >> --- a/xen/arch/arm/cpufeature.c >> +++ b/xen/arch/arm/cpufeature.c >> @@ -19,6 +19,7 @@ >> #include <xen/types.h> >> #include <xen/init.h> >> #include <xen/smp.h> >> +#include <xen/stop_machine.h> >> #include <asm/cpufeature.h> >> >> DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); >> @@ -40,6 +41,34 @@ void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, >> } >> >> /* >> + * Run through the enabled capabilities and enable() it on all active >> + * CPUs. >> + */ >> +void __init enable_cpu_capabilities(const struct arm_cpu_capabilities *caps) >> +{ >> + for ( ; caps->matches; caps++ ) >> + { >> + if ( !cpus_have_cap(caps->capability) ) >> + continue; >> + >> + if ( caps->enable ) >> + { >> + int ret; >> + >> + /* >> + * Use stop_machine_run() as it schedules the work allowing >> + * us to modify PSTATE, instead of on_each_cpu() which uses >> + * an IPI, giving us a PSTATE that disappears when we >> + * return. >> + */ >> + ret = stop_machine_run(caps->enable, (void *)caps, NR_CPUS); >> + /* stop_machine_run should never fail at this stage of the boot. */ >> + BUG_ON(ret); >> + } >> + } >> +} >> + >> +/* >> * Local variables: >> * mode: C >> * c-file-style: "BSD" >> diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c >> index 16a3b1be8e..032a6a882d 100644 >> --- a/xen/arch/arm/setup.c >> +++ b/xen/arch/arm/setup.c >> @@ -849,6 +849,7 @@ void __init start_xen(unsigned long boot_phys_offset, >> * stop_machine (tasklets initialized via an initcall). >> */ >> apply_alternatives_all(); >> + enable_errata_workarounds(); >> >> /* Create initial domain 0. */ >> /* The vGIC for DOM0 is exactly emulating the hardware GIC */ >> diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h >> index 8b158429c7..7de68361ff 100644 >> --- a/xen/include/asm-arm/cpuerrata.h >> +++ b/xen/include/asm-arm/cpuerrata.h >> @@ -5,6 +5,7 @@ >> #include <asm/alternative.h> >> >> void check_local_cpu_errata(void); >> +void enable_errata_workarounds(void); >> >> #ifdef CONFIG_HAS_ALTERNATIVE >> >> diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h >> index f00b6dbd39..21c65e198c 100644 >> --- a/xen/include/asm-arm/cpufeature.h >> +++ b/xen/include/asm-arm/cpufeature.h >> @@ -74,6 +74,7 @@ struct arm_cpu_capabilities { >> const char *desc; >> u16 capability; >> bool (*matches)(const struct arm_cpu_capabilities *); >> + int (*enable)(void *); /* Called on every active CPUs */ >> union { >> struct { /* To be used for eratum handling only */ >> u32 midr_model; >> @@ -85,6 +86,8 @@ struct arm_cpu_capabilities { >> void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, >> const char *info); >> >> +void enable_cpu_capabilities(const struct arm_cpu_capabilities *caps); >> + >> #endif /* __ASSEMBLY__ */ >> >> #endif >> -- >> 2.11.0 >>
> On 17 Jan 2018, at 10:31, Julien Grall <julien.grall@linaro.org> wrote: > > Hi Stefano, > > On 16/01/18 23:55, Stefano Stabellini wrote: >> On Tue, 16 Jan 2018, Julien Grall wrote: >>> Once Xen knows what features/workarounds present on the platform, it >>> might be necessary to configure each online CPU. >>> >>> Introduce a new callback "enable" that will be called on each online CPU to >>> configure the "capability". >>> >>> The code is based on Linux v4.14 (where cpufeature.c comes from), the >>> explanation of why using stop_machine_run is kept as we have similar >>> problem in the future. >>> >>> Lastly introduce enable_errata_workaround that will be called once CPUs >>> have booted and before the hardware domain is created. >>> >>> This is part of XSA-254. >>> >>> Signed-of-by: Julien Grall <julien.grall@linaro.org> >> If you took the code from Linux, you need to add the original >> Signed-off-by from the Linux commit. Aside from that: > > There are multiple commits touching this function. So I followed what we did in similar situation. By that I mean, mentioning the code was taken from Linux and not gathered the signed-off-by. > > If you really want, I can gather all the signed-off-by of the commit touching this function. If there are a lot of then, you may want to consider adding a README.source file instead. There are a few examples in tree and they are also mentioned in CONTRIBUTING files Lars <html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class=""><br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 17 Jan 2018, at 10:31, Julien Grall <<a href="mailto:julien.grall@linaro.org" class="">julien.grall@linaro.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Hi Stefano,</span><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">On 16/01/18 23:55, Stefano Stabellini wrote:</span><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">On Tue, 16 Jan 2018, Julien Grall wrote:<br class=""><blockquote type="cite" class="">Once Xen knows what features/workarounds present on the platform, it<br class="">might be necessary to configure each online CPU.<br class=""><br class="">Introduce a new callback "enable" that will be called on each online CPU to<br class="">configure the "capability".<br class=""><br class="">The code is based on Linux v4.14 (where cpufeature.c comes from), the<br class="">explanation of why using stop_machine_run is kept as we have similar<br class="">problem in the future.<br class=""><br class="">Lastly introduce enable_errata_workaround that will be called once CPUs<br class="">have booted and before the hardware domain is created.<br class=""><br class="">This is part of XSA-254.<br class=""><br class="">Signed-of-by: Julien Grall <<a href="mailto:julien.grall@linaro.org" class="">julien.grall@linaro.org</a>><br class=""></blockquote>If you took the code from Linux, you need to add the original<br class="">Signed-off-by from the Linux commit. Aside from that:<br class=""></blockquote><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">There are multiple commits touching this function. So I followed what we did in similar situation. By that I mean, mentioning the code was taken from Linux and not gathered the signed-off-by.</span><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">If you really want, I can gather all the signed-off-by of the commit touching this function.</span><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><br class=""></div><div>If there are a lot of then, you may want to consider adding a README.source file instead. There are a few examples in tree and they are also mentioned in CONTRIBUTING files</div><div>Lars</div><br class=""></body></html>
Hi Lars, On 17/01/18 12:23, Lars Kurth wrote: > > >> On 17 Jan 2018, at 10:31, Julien Grall <julien.grall@linaro.org >> <mailto:julien.grall@linaro.org>> wrote: >> >> Hi Stefano, >> >> On 16/01/18 23:55, Stefano Stabellini wrote: >>> On Tue, 16 Jan 2018, Julien Grall wrote: >>>> Once Xen knows what features/workarounds present on the platform, it >>>> might be necessary to configure each online CPU. >>>> >>>> Introduce a new callback "enable" that will be called on each online >>>> CPU to >>>> configure the "capability". >>>> >>>> The code is based on Linux v4.14 (where cpufeature.c comes from), the >>>> explanation of why using stop_machine_run is kept as we have similar >>>> problem in the future. >>>> >>>> Lastly introduce enable_errata_workaround that will be called once CPUs >>>> have booted and before the hardware domain is created. >>>> >>>> This is part of XSA-254. >>>> >>>> Signed-of-by: Julien Grall <julien.grall@linaro.org >>>> <mailto:julien.grall@linaro.org>> >>> If you took the code from Linux, you need to add the original >>> Signed-off-by from the Linux commit. Aside from that: >> >> There are multiple commits touching this function. So I followed what >> we did in similar situation. By that I mean, mentioning the code was >> taken from Linux and not gathered the signed-off-by. >> >> If you really want, I can gather all the signed-off-by of the commit >> touching this function. > > If there are a lot of then, you may want to consider adding > a README.source file instead. There are a few examples in tree and they > are also mentioned in CONTRIBUTING files I am not sure to understand your suggestion here. I spotted only one CONTRIBUTING file and it only list the license. Regarding README.source, this is covering file and contain the same mention as in the commit message. As this is a single function. Isn't the commit message enough? Lastly, we do have quite a bit of code in Xen coming from Linux (or other project). A lot of them are not listed in README.source/CONTRIBUTING. But only mention in the commit message (not necessarily with Signed-off-by tag). So I am quite interested to know what is the normal procedure here. Cheers,
Hi Julien, > On 17 Jan 2018, at 12:31, Julien Grall <julien.grall@linaro.org> wrote: > >>>>> >>>> If you took the code from Linux, you need to add the original >>>> Signed-off-by from the Linux commit. Aside from that: >>> >>> There are multiple commits touching this function. So I followed what we did in similar situation. By that I mean, mentioning the code was taken from Linux and not gathered the signed-off-by. >>> >>> If you really want, I can gather all the signed-off-by of the commit touching this function. >> If there are a lot of then, you may want to consider adding a README.source file instead. There are a few examples in tree and they are also mentioned in CONTRIBUTING files > > I am not sure to understand your suggestion here. I spotted only one CONTRIBUTING file and it only list the license. I was suggesting to create a README.source in the xen arm tree seomwhere where you can list imports. > Regarding README.source, this is covering file and contain the same mention as in the commit message. As this is a single function. Isn't the commit message enough? From a legal viewpoint it is enough. The reason why we created the README.source file is because it is very easy to miss code imports when they are mentioned in commit messages. Normally this isn't a problem: only if we ever have to relicense the code or if someone does code archeology When we relicensed the ACPI builder, it created all sorts of problems as outlined in https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects <https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects> > Lastly, we do have quite a bit of code in Xen coming from Linux (or other project). A lot of them are not listed in README.source/CONTRIBUTING. But only mention in the commit message (not necessarily with Signed-off-by tag). That is true: which is why I have started fixing these, whenever I found them and moved such information to README.source. > So I am quite interested to know what is the normal procedure here. I think: * Doing this via Signed-off-by tag is sufficient for a one-off-import * I would prefer if people added import related info README.source files (and also in the commit message) Does this make sense? Lars <html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Julien,<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 17 Jan 2018, at 12:31, Julien Grall <<a href="mailto:julien.grall@linaro.org" class="">julien.grall@linaro.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><blockquote type="cite" class=""><br class=""></blockquote>If you took the code from Linux, you need to add the original<br class="">Signed-off-by from the Linux commit. Aside from that:<br class=""></blockquote><br class="">There are multiple commits touching this function. So I followed what we did in similar situation. By that I mean, mentioning the code was taken from Linux and not gathered the signed-off-by.<br class=""><br class="">If you really want, I can gather all the signed-off-by of the commit touching this function.<br class=""></blockquote>If there are a lot of then, you may want to consider adding a README.source file instead. There are a few examples in tree and they are also mentioned in CONTRIBUTING files<br class=""></blockquote><br class="">I am not sure to understand your suggestion here. I spotted only one CONTRIBUTING file and it only list the license.<br class=""></div></div></blockquote><div><br class=""></div><div>I was suggesting to create a README.source in the xen arm tree seomwhere where you can list imports.</div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div class="">Regarding README.source, this is covering file and contain the same mention as in the commit message. As this is a single function. Isn't the commit message enough?<br class=""></div></div></blockquote><div><br class=""></div><div>From a legal viewpoint it is enough.</div><div>The reason why we created the README.source file is because it is very easy to miss code imports when they are mentioned in commit messages.</div><div><br class=""></div><div>Normally this isn't a problem: only if we ever have to relicense the code or if someone does code archeology </div><div>When we relicensed the ACPI builder, it created all sorts of problems as outlined in <a href="https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects" class="">https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects</a></div><div><br class=""></div><blockquote type="cite" class=""><div class=""><div class="">Lastly, we do have quite a bit of code in Xen coming from Linux (or other project). A lot of them are not listed in README.source/CONTRIBUTING. But only mention in the commit message (not necessarily with Signed-off-by tag).<br class=""></div></div></blockquote><div><br class=""></div>That is true: which is why I have started fixing these, whenever I found them and moved such information to README.source. </div><div><br class=""><blockquote type="cite" class=""><div class=""><div class="">So I am quite interested to know what is the normal procedure here.<br class=""></div></div></blockquote><div><br class=""></div>I think:</div><div>* Doing this via Signed-off-by tag is sufficient for a one-off-import</div><div>* I would prefer if people added import related info README.source files (and also in the commit message)</div><div><br class=""></div>Does this make sense?<div class=""><br class=""></div><div class="">Lars</div></body></html>
On Wed, 17 Jan 2018, Lars Kurth wrote: > Regarding README.source, this is covering file and contain the same mention as in the commit message. As this is a single function. Isn't the commit message > enough? > > > From a legal viewpoint it is enough. If that is enough from a legal viewpoint, then it is enough for me. However, from a legal viewpoint, I thought we needed to explicitly mention all the original signed-off-bys because Julien is not actually the copyright holder for that function, hence, we need to add the signed-off-bys of all the missing copyright holders.
On Wed, 17 Jan 2018, Stefano Stabellini wrote: > On Wed, 17 Jan 2018, Lars Kurth wrote: > > Regarding README.source, this is covering file and contain the same mention as in the commit message. As this is a single function. Isn't the commit message > > enough? > > > > > > From a legal viewpoint it is enough. > > If that is enough from a legal viewpoint, then it is enough for me. > > However, from a legal viewpoint, I thought we needed to explicitly > mention all the original signed-off-bys because Julien is not actually > the copyright holder for that function, hence, we need to add the > signed-off-bys of all the missing copyright holders. Actually, reading again the Developer’s Certificate of Origin, it states: "The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file" so I think Lars is right. In that case, there is no need to resubmit this series, I'll commit to staging as is. If tests go well, I'll backport it to the stable trees.
On 17/01/18 14:31, Lars Kurth wrote: > Hi Julien, Hi Lars, >> On 17 Jan 2018, at 12:31, Julien Grall <julien.grall@linaro.org >> <mailto:julien.grall@linaro.org>> wrote: >> >>>>>> >>>>> If you took the code from Linux, you need to add the original >>>>> Signed-off-by from the Linux commit. Aside from that: >>>> >>>> There are multiple commits touching this function. So I followed >>>> what we did in similar situation. By that I mean, mentioning the >>>> code was taken from Linux and not gathered the signed-off-by. >>>> >>>> If you really want, I can gather all the signed-off-by of the commit >>>> touching this function. >>> If there are a lot of then, you may want to consider adding >>> a README.source file instead. There are a few examples in tree and >>> they are also mentioned in CONTRIBUTING files >> >> I am not sure to understand your suggestion here. I spotted only one >> CONTRIBUTING file and it only list the license. > > I was suggesting to create a README.source in the xen arm tree seomwhere > where you can list imports. > >> Regarding README.source, this is covering file and contain the same >> mention as in the commit message. As this is a single function. Isn't >> the commit message enough? > > From a legal viewpoint it is enough. > The reason why we created the README.source file is because it is very > easy to miss code imports when they are mentioned in commit messages. I understand that. > > Normally this isn't a problem: only if we ever have to relicense the > code or if someone does code archeology > When we relicensed the ACPI builder, it created all sorts of problems as > outlined in > https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects > >> Lastly, we do have quite a bit of code in Xen coming from Linux (or >> other project). A lot of them are not listed in >> README.source/CONTRIBUTING. But only mention in the commit message >> (not necessarily with Signed-off-by tag). > > That is true: which is why I have started fixing these, whenever I found > them and moved such information to README.source. > >> So I am quite interested to know what is the normal procedure here. > > I think: > * Doing this via Signed-off-by tag is sufficient for a one-off-import > * I would prefer if people added import related info README.source files > (and also in the commit message) > > Does this make sense? I think it makes sense. Do you expect to see the README.source per-directory? Or one at top level? Cheers, > > Lars
(+ Security team) Hi Stefano, On 17/01/18 21:47, Stefano Stabellini wrote: > On Wed, 17 Jan 2018, Stefano Stabellini wrote: >> On Wed, 17 Jan 2018, Lars Kurth wrote: >>> Regarding README.source, this is covering file and contain the same mention as in the commit message. As this is a single function. Isn't the commit message >>> enough? >>> >>> >>> From a legal viewpoint it is enough. >> >> If that is enough from a legal viewpoint, then it is enough for me. >> >> However, from a legal viewpoint, I thought we needed to explicitly >> mention all the original signed-off-bys because Julien is not actually >> the copyright holder for that function, hence, we need to add the >> signed-off-bys of all the missing copyright holders. > > Actually, reading again the Developer’s Certificate of Origin, it > states: > > "The contribution is based upon previous work that, to the best of my knowledge, is covered under an appropriate open source license and I have the right under that license to submit that work with modifications, whether created in whole or in part by me, under the same open source license (unless I am permitted to submit under a different license), as indicated in the file" > > so I think Lars is right. In that case, there is no need to resubmit > this series, I'll commit to staging as is. If tests go well, I'll > backport it to the stable trees. Thank you! I have created branches with patches backported up to Xen 4.8. With minor changes: - Xen 4.10: No changes - Xen 4.9: * minor conflict in some files * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) - Xen 4.8: * conflict in some files (one medium as the number of "features" is different) * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) The branches can be found on xenbits [1] : xsa-254-sp2-X.XX where X.XX is the version of Xen. Xen 4.7 and earlier does not have cpufeature/cpuerrata infrastructure and will require backport. The only difficulty here should be finding the list of commits required. Also, we probably want to update the XSA pointing to the patches. So if someone wants to backport to Xen 4.7 (or earlier) they can. Any opinions? Cheers, [1] https://xenbits.xen.org/git-http/people/julieng/xen-unstable.git
On Thu, 18 Jan 2018, Julien Grall wrote: > (+ Security team) > > Hi Stefano, > > On 17/01/18 21:47, Stefano Stabellini wrote: > > On Wed, 17 Jan 2018, Stefano Stabellini wrote: > > > On Wed, 17 Jan 2018, Lars Kurth wrote: > > > > Regarding README.source, this is covering file and contain the > > > > same mention as in the commit message. As this is a single function. > > > > Isn't the commit message > > > > enough? > > > > > > > > > > > > From a legal viewpoint it is enough. > > > > > > If that is enough from a legal viewpoint, then it is enough for me. > > > > > > However, from a legal viewpoint, I thought we needed to explicitly > > > mention all the original signed-off-bys because Julien is not actually > > > the copyright holder for that function, hence, we need to add the > > > signed-off-bys of all the missing copyright holders. > > > > Actually, reading again the Developer’s Certificate of Origin, it > > states: > > > > "The contribution is based upon previous work that, to the best of my > > knowledge, is covered under an appropriate open source license and I have > > the right under that license to submit that work with modifications, whether > > created in whole or in part by me, under the same open source license > > (unless I am permitted to submit under a different license), as indicated in > > the file" > > > > so I think Lars is right. In that case, there is no need to resubmit > > this series, I'll commit to staging as is. If tests go well, I'll > > backport it to the stable trees. > Thank you! I have created branches with patches backported up to Xen 4.8. With > minor changes: > > - Xen 4.10: No changes > - Xen 4.9: > * minor conflict in some files > * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) > - Xen 4.8: > * conflict in some files (one medium as the number of "features" is > different) > * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) > > The branches can be found on xenbits [1] : xsa-254-sp2-X.XX where X.XX is the > version of Xen. > > Xen 4.7 and earlier does not have cpufeature/cpuerrata infrastructure and will > require backport. The only difficulty here should be finding the list of > commits required. > > Also, we probably want to update the XSA pointing to the patches. So if > someone wants to backport to Xen 4.7 (or earlier) they can. Any opinions? Thank you, Julien. Ideally, I would like to do the backports after OSSTest passes its tests on those changes. In practice, for the sake of mitigating SP2 as soon as possible, tomorrow (Friday) I might do the backports anyway, if OSSTest is still behind on other problems. I don't think that backporting cpufeature/cpuerrata to 4.7 should be too convoluted, I'll give that a go as well. Once done, I'll provide the list of commits to the xen security list so that the XSA advisory can be updated appropriately. Cheers, Stefano > Cheers, > > [1] https://xenbits.xen.org/git-http/people/julieng/xen-unstable.git
>>> On 18.01.18 at 21:28, <sstabellini@kernel.org> wrote: > Thank you, Julien. Ideally, I would like to do the backports after > OSSTest passes its tests on those changes. In practice, for the sake of > mitigating SP2 as soon as possible, tomorrow (Friday) I might do the > backports anyway, if OSSTest is still behind on other problems. Please avoid touching the 4.8 tree for the moment, until 4.8.3 has been tagged. Thanks, Jan
On Fri, 19 Jan 2018, Jan Beulich wrote: > >>> On 18.01.18 at 21:28, <sstabellini@kernel.org> wrote: > > Thank you, Julien. Ideally, I would like to do the backports after > > OSSTest passes its tests on those changes. In practice, for the sake of > > mitigating SP2 as soon as possible, tomorrow (Friday) I might do the > > backports anyway, if OSSTest is still behind on other problems. > > Please avoid touching the 4.8 tree for the moment, until 4.8.3 has > been tagged. Many thanks for the head's up, I'll wait.
Hi Julien, > On 18 Jan 2018, at 10:56, Julien Grall <julien.grall@linaro.org> wrote: > > On 17/01/18 14:31, Lars Kurth wrote: >> Hi Julien, > > Hi Lars, > ... >> Normally this isn't a problem: only if we ever have to relicense the code or if someone does code archeology >> When we relicensed the ACPI builder, it created all sorts of problems as outlined in https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects <https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects> >>> Lastly, we do have quite a bit of code in Xen coming from Linux (or other project). A lot of them are not listed in README.source/CONTRIBUTING. But only mention in the commit message (not necessarily with Signed-off-by tag). >> That is true: which is why I have started fixing these, whenever I found them and moved such information to README.source. >>> So I am quite interested to know what is the normal procedure here. >> I think: >> * Doing this via Signed-off-by tag is sufficient for a one-off-import >> * I would prefer if people added import related info README.source files (and also in the commit message) >> Does this make sense? > > I think it makes sense. Do you expect to see the README.source per-directory? Or one at top level? I think one somewhere at the top level of the ARM tree is sufficient. It depends on how often you import code. It shouldn't block the series. Just something to do at some point Lars <html><head><meta http-equiv="Content-Type" content="text/html; charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; line-break: after-white-space;" class="">Hi Julien,<br class=""><div><br class=""><blockquote type="cite" class=""><div class="">On 18 Jan 2018, at 10:56, Julien Grall <<a href="mailto:julien.grall@linaro.org" class="">julien.grall@linaro.org</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><span style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">On 17/01/18 14:31, Lars Kurth wrote:</span><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><blockquote type="cite" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">Hi Julien,<br class=""></blockquote><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">Hi Lars,</span><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote>... <br class=""><blockquote type="cite" class=""><div class=""><blockquote type="cite" style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-size-adjust: auto; -webkit-text-stroke-width: 0px;" class="">Normally this isn't a problem: only if we ever have to relicense the code or if someone does code archeology<br class="">When we relicensed the ACPI builder, it created all sorts of problems as outlined in<span class="Apple-converted-space"> </span><a href="https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects" class="">https://www.slideshare.net/xen_com_mgr/ossa17-mixed-license-foss-projects</a><br class=""><blockquote type="cite" class="">Lastly, we do have quite a bit of code in Xen coming from Linux (or other project). A lot of them are not listed in README.source/CONTRIBUTING. But only mention in the commit message (not necessarily with Signed-off-by tag).<br class=""></blockquote>That is true: which is why I have started fixing these, whenever I found them and moved such information to README.source.<br class=""><blockquote type="cite" class="">So I am quite interested to know what is the normal procedure here.<br class=""></blockquote>I think:<br class="">* Doing this via Signed-off-by tag is sufficient for a one-off-import<br class="">* I would prefer if people added import related info README.source files (and also in the commit message)<br class="">Does this make sense?<br class=""></blockquote><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""><span style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;" class="">I think it makes sense. Do you expect to see the README.source per-directory? Or one at top level?</span><br style="font-family: Menlo-Regular; font-size: 11px; font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px;" class=""></div></blockquote><br class=""></div><div>I think one somewhere at the top level of the ARM tree is sufficient. It depends on how often you import code. It shouldn't block the series. Just something to do at some point</div><div>Lars</div><br class=""></body></html>
On Thu, 18 Jan 2018, Julien Grall wrote: > (+ Security team) > > Hi Stefano, > > On 17/01/18 21:47, Stefano Stabellini wrote: > > On Wed, 17 Jan 2018, Stefano Stabellini wrote: > > > On Wed, 17 Jan 2018, Lars Kurth wrote: > > > > Regarding README.source, this is covering file and contain the > > > > same mention as in the commit message. As this is a single function. > > > > Isn't the commit message > > > > enough? > > > > > > > > > > > > From a legal viewpoint it is enough. > > > > > > If that is enough from a legal viewpoint, then it is enough for me. > > > > > > However, from a legal viewpoint, I thought we needed to explicitly > > > mention all the original signed-off-bys because Julien is not actually > > > the copyright holder for that function, hence, we need to add the > > > signed-off-bys of all the missing copyright holders. > > > > Actually, reading again the Developer’s Certificate of Origin, it > > states: > > > > "The contribution is based upon previous work that, to the best of my > > knowledge, is covered under an appropriate open source license and I have > > the right under that license to submit that work with modifications, whether > > created in whole or in part by me, under the same open source license > > (unless I am permitted to submit under a different license), as indicated in > > the file" > > > > so I think Lars is right. In that case, there is no need to resubmit > > this series, I'll commit to staging as is. If tests go well, I'll > > backport it to the stable trees. > Thank you! I have created branches with patches backported up to Xen 4.8. With > minor changes: > > - Xen 4.10: No changes > - Xen 4.9: > * minor conflict in some files > * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) > - Xen 4.8: > * conflict in some files (one medium as the number of "features" is > different) > * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) > > The branches can be found on xenbits [1] : xsa-254-sp2-X.XX where X.XX is the > version of Xen. > > Xen 4.7 and earlier does not have cpufeature/cpuerrata infrastructure and will > require backport. The only difficulty here should be finding the list of > commits required. > > Also, we probably want to update the XSA pointing to the patches. So if > someone wants to backport to Xen 4.7 (or earlier) they can. Any opinions? These are the commits for the XSA 254 mitigation for the arm64 architecture: staging-4.10 b829d42829c1ff626a02756acae4dd482fc20c9a 0f7a4faafb2d79920cc63457cfca3e03990af4cc d1f4283a1d8405a480b4121e1efcfaec8bbdbffa cae6e1572f39a1906be0fc3bdaf49fe514c6a9c0 928112900e5b4a92ccebb2eea11665fd76aa0f0d 728fadb586a2a14a244dabd70463bcc1654ecc85 staging-4.9 2ec7ccbffc6b788f65e55498e4347c1ee3a44b01 50450c1f33dc72f2138a671d738934f796be3318 3790833ef16b95653424ec9b145e460ec1a56d16 fba48eff18c02d716c95b92df804a755620be82e 9f79e8d846e8413c828f5fc7cc6ac733728dff00 a2567d6b54b7b187ecc0165021b6dd07dafaf06a staging-4.8 946dd2eefae2faeecbeb9662e66935c8070f64f5 85990bf53addcdb0ce8e458a3d8fad199710ac59 cf0b584c8c5030588bc47a3614ad860af7482c53 44139fed7c794eb4e47a9bb93061e325bd57fe8c 6f6786ef0d7f7025860d360f6b1267193ffd1b27 For staging-4.7, I made the backports and tested them as well. They look correct. However, given that it was more complex than initially though, I would appreciate if you could give it a look as well (I haven't pushed it staging-4.7 yet): git://xenbits.xen.org/people/sstabellini/xen-unstable.git staging-4.7-xsa254 Thanks! - Stefano
Hi Stefano, On 24 January 2018 at 22:14, Stefano Stabellini <sstabellini@kernel.org> wrote: > On Thu, 18 Jan 2018, Julien Grall wrote: >> (+ Security team) >> >> Hi Stefano, >> >> On 17/01/18 21:47, Stefano Stabellini wrote: >> > On Wed, 17 Jan 2018, Stefano Stabellini wrote: >> > > On Wed, 17 Jan 2018, Lars Kurth wrote: >> > > > Regarding README.source, this is covering file and contain the >> > > > same mention as in the commit message. As this is a single function. >> > > > Isn't the commit message >> > > > enough? >> > > > >> > > > >> > > > From a legal viewpoint it is enough. >> > > >> > > If that is enough from a legal viewpoint, then it is enough for me. >> > > >> > > However, from a legal viewpoint, I thought we needed to explicitly >> > > mention all the original signed-off-bys because Julien is not actually >> > > the copyright holder for that function, hence, we need to add the >> > > signed-off-bys of all the missing copyright holders. >> > >> > Actually, reading again the Developer’s Certificate of Origin, it >> > states: >> > >> > "The contribution is based upon previous work that, to the best of my >> > knowledge, is covered under an appropriate open source license and I have >> > the right under that license to submit that work with modifications, whether >> > created in whole or in part by me, under the same open source license >> > (unless I am permitted to submit under a different license), as indicated in >> > the file" >> > >> > so I think Lars is right. In that case, there is no need to resubmit >> > this series, I'll commit to staging as is. If tests go well, I'll >> > backport it to the stable trees. >> Thank you! I have created branches with patches backported up to Xen 4.8. With >> minor changes: >> >> - Xen 4.10: No changes >> - Xen 4.9: >> * minor conflict in some files >> * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) >> - Xen 4.8: >> * conflict in some files (one medium as the number of "features" is >> different) >> * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) >> >> The branches can be found on xenbits [1] : xsa-254-sp2-X.XX where X.XX is the >> version of Xen. >> >> Xen 4.7 and earlier does not have cpufeature/cpuerrata infrastructure and will >> require backport. The only difficulty here should be finding the list of >> commits required. >> >> Also, we probably want to update the XSA pointing to the patches. So if >> someone wants to backport to Xen 4.7 (or earlier) they can. Any opinions? > > These are the commits for the XSA 254 mitigation for the arm64 > architecture: > > staging-4.10 > b829d42829c1ff626a02756acae4dd482fc20c9a > 0f7a4faafb2d79920cc63457cfca3e03990af4cc > d1f4283a1d8405a480b4121e1efcfaec8bbdbffa > cae6e1572f39a1906be0fc3bdaf49fe514c6a9c0 > 928112900e5b4a92ccebb2eea11665fd76aa0f0d > 728fadb586a2a14a244dabd70463bcc1654ecc85 > > staging-4.9 > 2ec7ccbffc6b788f65e55498e4347c1ee3a44b01 > 50450c1f33dc72f2138a671d738934f796be3318 > 3790833ef16b95653424ec9b145e460ec1a56d16 > fba48eff18c02d716c95b92df804a755620be82e > 9f79e8d846e8413c828f5fc7cc6ac733728dff00 > a2567d6b54b7b187ecc0165021b6dd07dafaf06a > > staging-4.8 > 946dd2eefae2faeecbeb9662e66935c8070f64f5 > 85990bf53addcdb0ce8e458a3d8fad199710ac59 > cf0b584c8c5030588bc47a3614ad860af7482c53 > 44139fed7c794eb4e47a9bb93061e325bd57fe8c > 6f6786ef0d7f7025860d360f6b1267193ffd1b27 Something looks quite odd. The commit message have two cherry-pick commit ID. Why didn't you just merged the branches I provided? > > For staging-4.7, I made the backports and tested them as well. They look > correct. However, given that it was more complex than initially though, > I would appreciate if you could give it a look as well (I haven't pushed > it staging-4.7 yet): > > git://xenbits.xen.org/people/sstabellini/xen-unstable.git staging-4.7-xsa254 I will have a look. Cheers,
On Wed, 24 Jan 2018, Julien Grall wrote: > Hi Stefano, > > On 24 January 2018 at 22:14, Stefano Stabellini <sstabellini@kernel.org> wrote: > > On Thu, 18 Jan 2018, Julien Grall wrote: > >> (+ Security team) > >> > >> Hi Stefano, > >> > >> On 17/01/18 21:47, Stefano Stabellini wrote: > >> > On Wed, 17 Jan 2018, Stefano Stabellini wrote: > >> > > On Wed, 17 Jan 2018, Lars Kurth wrote: > >> > > > Regarding README.source, this is covering file and contain the > >> > > > same mention as in the commit message. As this is a single function. > >> > > > Isn't the commit message > >> > > > enough? > >> > > > > >> > > > > >> > > > From a legal viewpoint it is enough. > >> > > > >> > > If that is enough from a legal viewpoint, then it is enough for me. > >> > > > >> > > However, from a legal viewpoint, I thought we needed to explicitly > >> > > mention all the original signed-off-bys because Julien is not actually > >> > > the copyright holder for that function, hence, we need to add the > >> > > signed-off-bys of all the missing copyright holders. > >> > > >> > Actually, reading again the Developer’s Certificate of Origin, it > >> > states: > >> > > >> > "The contribution is based upon previous work that, to the best of my > >> > knowledge, is covered under an appropriate open source license and I have > >> > the right under that license to submit that work with modifications, whether > >> > created in whole or in part by me, under the same open source license > >> > (unless I am permitted to submit under a different license), as indicated in > >> > the file" > >> > > >> > so I think Lars is right. In that case, there is no need to resubmit > >> > this series, I'll commit to staging as is. If tests go well, I'll > >> > backport it to the stable trees. > >> Thank you! I have created branches with patches backported up to Xen 4.8. With > >> minor changes: > >> > >> - Xen 4.10: No changes > >> - Xen 4.9: > >> * minor conflict in some files > >> * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) > >> - Xen 4.8: > >> * conflict in some files (one medium as the number of "features" is > >> different) > >> * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) > >> > >> The branches can be found on xenbits [1] : xsa-254-sp2-X.XX where X.XX is the > >> version of Xen. > >> > >> Xen 4.7 and earlier does not have cpufeature/cpuerrata infrastructure and will > >> require backport. The only difficulty here should be finding the list of > >> commits required. > >> > >> Also, we probably want to update the XSA pointing to the patches. So if > >> someone wants to backport to Xen 4.7 (or earlier) they can. Any opinions? > > > > These are the commits for the XSA 254 mitigation for the arm64 > > architecture: > > > > staging-4.10 > > b829d42829c1ff626a02756acae4dd482fc20c9a > > 0f7a4faafb2d79920cc63457cfca3e03990af4cc > > d1f4283a1d8405a480b4121e1efcfaec8bbdbffa > > cae6e1572f39a1906be0fc3bdaf49fe514c6a9c0 > > 928112900e5b4a92ccebb2eea11665fd76aa0f0d > > 728fadb586a2a14a244dabd70463bcc1654ecc85 > > > > staging-4.9 > > 2ec7ccbffc6b788f65e55498e4347c1ee3a44b01 > > 50450c1f33dc72f2138a671d738934f796be3318 > > 3790833ef16b95653424ec9b145e460ec1a56d16 > > fba48eff18c02d716c95b92df804a755620be82e > > 9f79e8d846e8413c828f5fc7cc6ac733728dff00 > > a2567d6b54b7b187ecc0165021b6dd07dafaf06a > > > > staging-4.8 > > 946dd2eefae2faeecbeb9662e66935c8070f64f5 > > 85990bf53addcdb0ce8e458a3d8fad199710ac59 > > cf0b584c8c5030588bc47a3614ad860af7482c53 > > 44139fed7c794eb4e47a9bb93061e325bd57fe8c > > 6f6786ef0d7f7025860d360f6b1267193ffd1b27 > > Something looks quite odd. The commit message have two cherry-pick commit ID. > > Why didn't you just merged the branches I provided? Basically I did the backports on my own, then I double-checked that they matched your own version of the backports. I did it for safety: this way we can be quite sure that the backports are good, or both of us did exactly the same mistakes :-) It was very helpful to have branches to compare against, thank you for that. > > > > For staging-4.7, I made the backports and tested them as well. They look > > correct. However, given that it was more complex than initially though, > > I would appreciate if you could give it a look as well (I haven't pushed > > it staging-4.7 yet): > > > > git://xenbits.xen.org/people/sstabellini/xen-unstable.git staging-4.7-xsa254 > > I will have a look. Thanks again!
Hi, On 24/01/18 22:43, Stefano Stabellini wrote: > On Wed, 24 Jan 2018, Julien Grall wrote: >> Hi Stefano, >> >> On 24 January 2018 at 22:14, Stefano Stabellini <sstabellini@kernel.org> wrote: >>> On Thu, 18 Jan 2018, Julien Grall wrote: >>>> (+ Security team) >>>> >>>> Hi Stefano, >>>> >>>> On 17/01/18 21:47, Stefano Stabellini wrote: >>>>> On Wed, 17 Jan 2018, Stefano Stabellini wrote: >>>>>> On Wed, 17 Jan 2018, Lars Kurth wrote: >>>>>>> Regarding README.source, this is covering file and contain the >>>>>>> same mention as in the commit message. As this is a single function. >>>>>>> Isn't the commit message >>>>>>> enough? >>>>>>> >>>>>>> >>>>>>> From a legal viewpoint it is enough. >>>>>> >>>>>> If that is enough from a legal viewpoint, then it is enough for me. >>>>>> >>>>>> However, from a legal viewpoint, I thought we needed to explicitly >>>>>> mention all the original signed-off-bys because Julien is not actually >>>>>> the copyright holder for that function, hence, we need to add the >>>>>> signed-off-bys of all the missing copyright holders. >>>>> >>>>> Actually, reading again the Developer’s Certificate of Origin, it >>>>> states: >>>>> >>>>> "The contribution is based upon previous work that, to the best of my >>>>> knowledge, is covered under an appropriate open source license and I have >>>>> the right under that license to submit that work with modifications, whether >>>>> created in whole or in part by me, under the same open source license >>>>> (unless I am permitted to submit under a different license), as indicated in >>>>> the file" >>>>> >>>>> so I think Lars is right. In that case, there is no need to resubmit >>>>> this series, I'll commit to staging as is. If tests go well, I'll >>>>> backport it to the stable trees. >>>> Thank you! I have created branches with patches backported up to Xen 4.8. With >>>> minor changes: >>>> >>>> - Xen 4.10: No changes >>>> - Xen 4.9: >>>> * minor conflict in some files >>>> * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) >>>> - Xen 4.8: >>>> * conflict in some files (one medium as the number of "features" is >>>> different) >>>> * compilation failure in cpuerrata.c (__virt_to_mfn does not exist) >>>> >>>> The branches can be found on xenbits [1] : xsa-254-sp2-X.XX where X.XX is the >>>> version of Xen. >>>> >>>> Xen 4.7 and earlier does not have cpufeature/cpuerrata infrastructure and will >>>> require backport. The only difficulty here should be finding the list of >>>> commits required. >>>> >>>> Also, we probably want to update the XSA pointing to the patches. So if >>>> someone wants to backport to Xen 4.7 (or earlier) they can. Any opinions? >>> >>> These are the commits for the XSA 254 mitigation for the arm64 >>> architecture: >>> >>> staging-4.10 >>> b829d42829c1ff626a02756acae4dd482fc20c9a >>> 0f7a4faafb2d79920cc63457cfca3e03990af4cc >>> d1f4283a1d8405a480b4121e1efcfaec8bbdbffa >>> cae6e1572f39a1906be0fc3bdaf49fe514c6a9c0 >>> 928112900e5b4a92ccebb2eea11665fd76aa0f0d >>> 728fadb586a2a14a244dabd70463bcc1654ecc85 >>> >>> staging-4.9 >>> 2ec7ccbffc6b788f65e55498e4347c1ee3a44b01 >>> 50450c1f33dc72f2138a671d738934f796be3318 >>> 3790833ef16b95653424ec9b145e460ec1a56d16 >>> fba48eff18c02d716c95b92df804a755620be82e >>> 9f79e8d846e8413c828f5fc7cc6ac733728dff00 >>> a2567d6b54b7b187ecc0165021b6dd07dafaf06a >>> >>> staging-4.8 >>> 946dd2eefae2faeecbeb9662e66935c8070f64f5 >>> 85990bf53addcdb0ce8e458a3d8fad199710ac59 >>> cf0b584c8c5030588bc47a3614ad860af7482c53 >>> 44139fed7c794eb4e47a9bb93061e325bd57fe8c >>> 6f6786ef0d7f7025860d360f6b1267193ffd1b27 >> >> Something looks quite odd. The commit message have two cherry-pick commit ID. >> >> Why didn't you just merged the branches I provided? > > Basically I did the backports on my own, then I double-checked that they > matched your own version of the backports. I did it for safety: this way > we can be quite sure that the backports are good, or both of us did > exactly the same mistakes :-) > It was very helpful to have branches to compare against, thank you for > that. I also double checked it yesterday because I wasn't sure what you did :). > > >>> >>> For staging-4.7, I made the backports and tested them as well. They look >>> correct. However, given that it was more complex than initially though, >>> I would appreciate if you could give it a look as well (I haven't pushed >>> it staging-4.7 yet): >>> >>> git://xenbits.xen.org/people/sstabellini/xen-unstable.git staging-4.7-xsa254 >> >> I will have a look. > > Thanks again! This looks good to me. Thank you for backporting them to 4.7. Cheers,
On Thu, 25 Jan 2018, Julien Grall wrote: > Hi, > > On 24/01/18 22:43, Stefano Stabellini wrote: > > On Wed, 24 Jan 2018, Julien Grall wrote: > > > Hi Stefano, > > > > > > On 24 January 2018 at 22:14, Stefano Stabellini <sstabellini@kernel.org> > > > wrote: > > > > On Thu, 18 Jan 2018, Julien Grall wrote: > > > > > (+ Security team) > > > > > > > > > > Hi Stefano, > > > > > > > > > > On 17/01/18 21:47, Stefano Stabellini wrote: > > > > > > On Wed, 17 Jan 2018, Stefano Stabellini wrote: > > > > > > > On Wed, 17 Jan 2018, Lars Kurth wrote: > > > > > > > > Regarding README.source, this is covering file and > > > > > > > > contain the > > > > > > > > same mention as in the commit message. As this is a single > > > > > > > > function. > > > > > > > > Isn't the commit message > > > > > > > > enough? > > > > > > > > > > > > > > > > > > > > > > > > From a legal viewpoint it is enough. > > > > > > > > > > > > > > If that is enough from a legal viewpoint, then it is enough for > > > > > > > me. > > > > > > > > > > > > > > However, from a legal viewpoint, I thought we needed to explicitly > > > > > > > mention all the original signed-off-bys because Julien is not > > > > > > > actually > > > > > > > the copyright holder for that function, hence, we need to add the > > > > > > > signed-off-bys of all the missing copyright holders. > > > > > > > > > > > > Actually, reading again the Developer’s Certificate of Origin, it > > > > > > states: > > > > > > > > > > > > "The contribution is based upon previous work that, to the best of > > > > > > my > > > > > > knowledge, is covered under an appropriate open source license and I > > > > > > have > > > > > > the right under that license to submit that work with modifications, > > > > > > whether > > > > > > created in whole or in part by me, under the same open source > > > > > > license > > > > > > (unless I am permitted to submit under a different license), as > > > > > > indicated in > > > > > > the file" > > > > > > > > > > > > so I think Lars is right. In that case, there is no need to resubmit > > > > > > this series, I'll commit to staging as is. If tests go well, I'll > > > > > > backport it to the stable trees. > > > > > Thank you! I have created branches with patches backported up to Xen > > > > > 4.8. With > > > > > minor changes: > > > > > > > > > > - Xen 4.10: No changes > > > > > - Xen 4.9: > > > > > * minor conflict in some files > > > > > * compilation failure in cpuerrata.c (__virt_to_mfn does not > > > > > exist) > > > > > - Xen 4.8: > > > > > * conflict in some files (one medium as the number of > > > > > "features" is > > > > > different) > > > > > * compilation failure in cpuerrata.c (__virt_to_mfn does not > > > > > exist) > > > > > > > > > > The branches can be found on xenbits [1] : xsa-254-sp2-X.XX where X.XX > > > > > is the > > > > > version of Xen. > > > > > > > > > > Xen 4.7 and earlier does not have cpufeature/cpuerrata infrastructure > > > > > and will > > > > > require backport. The only difficulty here should be finding the list > > > > > of > > > > > commits required. > > > > > > > > > > Also, we probably want to update the XSA pointing to the patches. So > > > > > if > > > > > someone wants to backport to Xen 4.7 (or earlier) they can. Any > > > > > opinions? > > > > > > > > These are the commits for the XSA 254 mitigation for the arm64 > > > > architecture: > > > > > > > > staging-4.10 > > > > b829d42829c1ff626a02756acae4dd482fc20c9a > > > > 0f7a4faafb2d79920cc63457cfca3e03990af4cc > > > > d1f4283a1d8405a480b4121e1efcfaec8bbdbffa > > > > cae6e1572f39a1906be0fc3bdaf49fe514c6a9c0 > > > > 928112900e5b4a92ccebb2eea11665fd76aa0f0d > > > > 728fadb586a2a14a244dabd70463bcc1654ecc85 > > > > > > > > staging-4.9 > > > > 2ec7ccbffc6b788f65e55498e4347c1ee3a44b01 > > > > 50450c1f33dc72f2138a671d738934f796be3318 > > > > 3790833ef16b95653424ec9b145e460ec1a56d16 > > > > fba48eff18c02d716c95b92df804a755620be82e > > > > 9f79e8d846e8413c828f5fc7cc6ac733728dff00 > > > > a2567d6b54b7b187ecc0165021b6dd07dafaf06a > > > > > > > > staging-4.8 > > > > 946dd2eefae2faeecbeb9662e66935c8070f64f5 > > > > 85990bf53addcdb0ce8e458a3d8fad199710ac59 > > > > cf0b584c8c5030588bc47a3614ad860af7482c53 > > > > 44139fed7c794eb4e47a9bb93061e325bd57fe8c > > > > 6f6786ef0d7f7025860d360f6b1267193ffd1b27 > > > > > > Something looks quite odd. The commit message have two cherry-pick commit > > > ID. > > > > > > Why didn't you just merged the branches I provided? > > > > Basically I did the backports on my own, then I double-checked that they > > matched your own version of the backports. I did it for safety: this way > > we can be quite sure that the backports are good, or both of us did > > exactly the same mistakes :-) > > It was very helpful to have branches to compare against, thank you for > > that. > > I also double checked it yesterday because I wasn't sure what you did :). > > > > > > > > > > > > > For staging-4.7, I made the backports and tested them as well. They look > > > > correct. However, given that it was more complex than initially though, > > > > I would appreciate if you could give it a look as well (I haven't pushed > > > > it staging-4.7 yet): > > > > > > > > git://xenbits.xen.org/people/sstabellini/xen-unstable.git > > > > staging-4.7-xsa254 > > > > > > I will have a look. > > > > Thanks again! > > This looks good to me. Thank you for backporting them to 4.7. Thank you! I pushed the branch, these are the relevant commits for 4.7: fd884d6 xen/arm64: Implement branch predictor hardening for affected Cortex-A CPUs 50c68df xen/arm64: Add skeleton to harden the branch predictor aliasing attacks 1bdcc9f xen/arm: cpuerrata: Add MIDR_ALL_VERSIONS 2914ef5 xen/arm64: Add missing MIDR values for Cortex-A72, A73 and A75 62b9706 xen/arm: Introduce enable callback to enable a capabilities on each online CPU 624abdc xen/arm: Detect silicon revision and set cap bits accordingly d7b73ed xen/arm: cpufeature: Provide an helper to check if a capability is supported 112c49c xen/arm: Add cpu_hwcap bitmap a5b0fa4 xen/arm: Add macros to handle the MIDR
diff --git a/xen/arch/arm/cpuerrata.c b/xen/arch/arm/cpuerrata.c index fe9e9facbe..772587c05a 100644 --- a/xen/arch/arm/cpuerrata.c +++ b/xen/arch/arm/cpuerrata.c @@ -64,6 +64,12 @@ void check_local_cpu_errata(void) { update_cpu_capabilities(arm_errata, "enabled workaround for"); } + +void __init enable_errata_workarounds(void) +{ + enable_cpu_capabilities(arm_errata); +} + /* * Local variables: * mode: C diff --git a/xen/arch/arm/cpufeature.c b/xen/arch/arm/cpufeature.c index 479c9fb011..525b45e22f 100644 --- a/xen/arch/arm/cpufeature.c +++ b/xen/arch/arm/cpufeature.c @@ -19,6 +19,7 @@ #include <xen/types.h> #include <xen/init.h> #include <xen/smp.h> +#include <xen/stop_machine.h> #include <asm/cpufeature.h> DECLARE_BITMAP(cpu_hwcaps, ARM_NCAPS); @@ -40,6 +41,34 @@ void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, } /* + * Run through the enabled capabilities and enable() it on all active + * CPUs. + */ +void __init enable_cpu_capabilities(const struct arm_cpu_capabilities *caps) +{ + for ( ; caps->matches; caps++ ) + { + if ( !cpus_have_cap(caps->capability) ) + continue; + + if ( caps->enable ) + { + int ret; + + /* + * Use stop_machine_run() as it schedules the work allowing + * us to modify PSTATE, instead of on_each_cpu() which uses + * an IPI, giving us a PSTATE that disappears when we + * return. + */ + ret = stop_machine_run(caps->enable, (void *)caps, NR_CPUS); + /* stop_machine_run should never fail at this stage of the boot. */ + BUG_ON(ret); + } + } +} + +/* * Local variables: * mode: C * c-file-style: "BSD" diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index 16a3b1be8e..032a6a882d 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -849,6 +849,7 @@ void __init start_xen(unsigned long boot_phys_offset, * stop_machine (tasklets initialized via an initcall). */ apply_alternatives_all(); + enable_errata_workarounds(); /* Create initial domain 0. */ /* The vGIC for DOM0 is exactly emulating the hardware GIC */ diff --git a/xen/include/asm-arm/cpuerrata.h b/xen/include/asm-arm/cpuerrata.h index 8b158429c7..7de68361ff 100644 --- a/xen/include/asm-arm/cpuerrata.h +++ b/xen/include/asm-arm/cpuerrata.h @@ -5,6 +5,7 @@ #include <asm/alternative.h> void check_local_cpu_errata(void); +void enable_errata_workarounds(void); #ifdef CONFIG_HAS_ALTERNATIVE diff --git a/xen/include/asm-arm/cpufeature.h b/xen/include/asm-arm/cpufeature.h index f00b6dbd39..21c65e198c 100644 --- a/xen/include/asm-arm/cpufeature.h +++ b/xen/include/asm-arm/cpufeature.h @@ -74,6 +74,7 @@ struct arm_cpu_capabilities { const char *desc; u16 capability; bool (*matches)(const struct arm_cpu_capabilities *); + int (*enable)(void *); /* Called on every active CPUs */ union { struct { /* To be used for eratum handling only */ u32 midr_model; @@ -85,6 +86,8 @@ struct arm_cpu_capabilities { void update_cpu_capabilities(const struct arm_cpu_capabilities *caps, const char *info); +void enable_cpu_capabilities(const struct arm_cpu_capabilities *caps); + #endif /* __ASSEMBLY__ */ #endif