| Message ID | 20180522174254.27551-8-julien.grall@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: SSBD (aka Spectre-v4) mitigation (XSA-263) | expand |
You might want to CC Konrad next time on this patch On Tue, 22 May 2018, Julien Grall wrote: > This is part of XSA-263. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > I am aware of the missing commit message here. I wanted to send the > series on the ML to get some feedback first. > --- > xen/arch/arm/alternative.c | 35 +++++++++++++---------------------- > 1 file changed, 13 insertions(+), 22 deletions(-) > > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c > index 9ffdc475d6..bd62183def 100644 > --- a/xen/arch/arm/alternative.c > +++ b/xen/arch/arm/alternative.c > @@ -97,12 +97,16 @@ static u32 get_alt_insn(const struct alt_instr *alt, > /* > * The region patched should be read-write to allow __apply_alternatives > * to replacing the instructions when necessary. > + * > + * @update_offset: Offset between the region patched and the writable > + * region for the update. 0 if the patched region is writable. > */ > -static int __apply_alternatives(const struct alt_region *region) > +static int __apply_alternatives(const struct alt_region *region, > + paddr_t update_offset) > { > const struct alt_instr *alt; > - const u32 *replptr; > - u32 *origptr; > + const u32 *replptr, *origptr; > + u32 *updptr; > > printk(XENLOG_INFO "alternatives: Patching with alt table %p -> %p\n", > region->begin, region->end); > @@ -118,6 +122,7 @@ static int __apply_alternatives(const struct alt_region *region) > BUG_ON(alt->alt_len != alt->orig_len); > > origptr = ALT_ORIG_PTR(alt); > + updptr = (void *)origptr + update_offset; > replptr = ALT_REPL_PTR(alt); > > nr_inst = alt->alt_len / sizeof(insn); > @@ -125,7 +130,7 @@ static int __apply_alternatives(const struct alt_region *region) > for ( i = 0; i < nr_inst; i++ ) > { > insn = get_alt_insn(alt, origptr + i, replptr + i); > - *(origptr + i) = cpu_to_le32(insn); > + *(updptr + i) = cpu_to_le32(insn); > } > > /* Ensure the new instructions reached the memory and nuke */ > @@ -162,9 +167,6 @@ static int __apply_alternatives_multi_stop(void *unused) > paddr_t xen_size = _end - _start; > unsigned int xen_order = get_order_from_bytes(xen_size); > void *xenmap; > - struct virtual_region patch_region = { > - .list = LIST_HEAD_INIT(patch_region.list), > - }; > > BUG_ON(patched); > > @@ -178,30 +180,19 @@ static int __apply_alternatives_multi_stop(void *unused) > BUG_ON(!xenmap); > > /* > - * If we generate a new branch instruction, the target will be > - * calculated in this re-mapped Xen region. So we have to register > - * this re-mapped Xen region as a virtual region temporarily. > - */ > - patch_region.start = xenmap; > - patch_region.end = xenmap + xen_size; > - register_virtual_region(&patch_region); > - > - /* > * Find the virtual address of the alternative region in the new > * mapping. > * alt_instr contains relative offset, so the function > * __apply_alternatives will patch in the re-mapped version of > * Xen. > */ > - region.begin = (void *)__alt_instructions - (void *)_start + xenmap; > - region.end = (void *)__alt_instructions_end - (void *)_start + xenmap; > + region.begin = __alt_instructions; > + region.end = __alt_instructions_end; > > - ret = __apply_alternatives(®ion); > + ret = __apply_alternatives(®ion, xenmap - (void *)_start); > /* The patching is not expected to fail during boot. */ > BUG_ON(ret != 0); > > - unregister_virtual_region(&patch_region); > - > vunmap(xenmap); > > /* Barriers provided by the cache flushing */ > @@ -235,7 +226,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en > .end = end, > }; > > - return __apply_alternatives(®ion); > + return __apply_alternatives(®ion, 0); > } > > /* > -- > 2.11.0 >
On Fri, 25 May 2018, 22:54 Stefano Stabellini, <sstabellini@kernel.org> wrote: > You might want to CC Konrad next time on this patch > May I ask why? This code falls under Arm maintainership. Cheers, > On Tue, 22 May 2018, Julien Grall wrote: > > This is part of XSA-263. > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > --- > > I am aware of the missing commit message here. I wanted to send the > > series on the ML to get some feedback first. > > --- > > xen/arch/arm/alternative.c | 35 +++++++++++++---------------------- > > 1 file changed, 13 insertions(+), 22 deletions(-) > > > > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c > > index 9ffdc475d6..bd62183def 100644 > > --- a/xen/arch/arm/alternative.c > > +++ b/xen/arch/arm/alternative.c > > @@ -97,12 +97,16 @@ static u32 get_alt_insn(const struct alt_instr *alt, > > /* > > * The region patched should be read-write to allow __apply_alternatives > > * to replacing the instructions when necessary. > > + * > > + * @update_offset: Offset between the region patched and the writable > > + * region for the update. 0 if the patched region is writable. > > */ > > -static int __apply_alternatives(const struct alt_region *region) > > +static int __apply_alternatives(const struct alt_region *region, > > + paddr_t update_offset) > > { > > const struct alt_instr *alt; > > - const u32 *replptr; > > - u32 *origptr; > > + const u32 *replptr, *origptr; > > + u32 *updptr; > > > > printk(XENLOG_INFO "alternatives: Patching with alt table %p -> > %p\n", > > region->begin, region->end); > > @@ -118,6 +122,7 @@ static int __apply_alternatives(const struct > alt_region *region) > > BUG_ON(alt->alt_len != alt->orig_len); > > > > origptr = ALT_ORIG_PTR(alt); > > + updptr = (void *)origptr + update_offset; > > replptr = ALT_REPL_PTR(alt); > > > > nr_inst = alt->alt_len / sizeof(insn); > > @@ -125,7 +130,7 @@ static int __apply_alternatives(const struct > alt_region *region) > > for ( i = 0; i < nr_inst; i++ ) > > { > > insn = get_alt_insn(alt, origptr + i, replptr + i); > > - *(origptr + i) = cpu_to_le32(insn); > > + *(updptr + i) = cpu_to_le32(insn); > > } > > > > /* Ensure the new instructions reached the memory and nuke */ > > @@ -162,9 +167,6 @@ static int __apply_alternatives_multi_stop(void > *unused) > > paddr_t xen_size = _end - _start; > > unsigned int xen_order = get_order_from_bytes(xen_size); > > void *xenmap; > > - struct virtual_region patch_region = { > > - .list = LIST_HEAD_INIT(patch_region.list), > > - }; > > > > BUG_ON(patched); > > > > @@ -178,30 +180,19 @@ static int __apply_alternatives_multi_stop(void > *unused) > > BUG_ON(!xenmap); > > > > /* > > - * If we generate a new branch instruction, the target will be > > - * calculated in this re-mapped Xen region. So we have to > register > > - * this re-mapped Xen region as a virtual region temporarily. > > - */ > > - patch_region.start = xenmap; > > - patch_region.end = xenmap + xen_size; > > - register_virtual_region(&patch_region); > > - > > - /* > > * Find the virtual address of the alternative region in the new > > * mapping. > > * alt_instr contains relative offset, so the function > > * __apply_alternatives will patch in the re-mapped version of > > * Xen. > > */ > > - region.begin = (void *)__alt_instructions - (void *)_start + > xenmap; > > - region.end = (void *)__alt_instructions_end - (void *)_start + > xenmap; > > + region.begin = __alt_instructions; > > + region.end = __alt_instructions_end; > > > > - ret = __apply_alternatives(®ion); > > + ret = __apply_alternatives(®ion, xenmap - (void *)_start); > > /* The patching is not expected to fail during boot. */ > > BUG_ON(ret != 0); > > > > - unregister_virtual_region(&patch_region); > > - > > vunmap(xenmap); > > > > /* Barriers provided by the cache flushing */ > > @@ -235,7 +226,7 @@ int apply_alternatives(const struct alt_instr > *start, const struct alt_instr *en > > .end = end, > > }; > > > > - return __apply_alternatives(®ion); > > + return __apply_alternatives(®ion, 0); > > } > > > > /* > > -- > > 2.11.0 > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel <br><br><div class="gmail_quote"><div dir="ltr">On Fri, 25 May 2018, 22:54 Stefano Stabellini, <<a href="mailto:sstabellini@kernel.org">sstabellini@kernel.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">You might want to CC Konrad next time on this patch<br></blockquote></div><div><br></div><div>May I ask why? This code falls under Arm maintainership.</div><div><br></div><div>Cheers,</div><div><br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <br> On Tue, 22 May 2018, Julien Grall wrote:<br> > This is part of XSA-263.<br> > <br> > Signed-off-by: Julien Grall <<a href="mailto:julien.grall@arm.com" target="_blank">julien.grall@arm.com</a>><br> > <br> > ---<br> > I am aware of the missing commit message here. I wanted to send the<br> > series on the ML to get some feedback first.<br> > ---<br> > xen/arch/arm/alternative.c | 35 +++++++++++++----------------------<br> > 1 file changed, 13 insertions(+), 22 deletions(-)<br> > <br> > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c<br> > index 9ffdc475d6..bd62183def 100644<br> > --- a/xen/arch/arm/alternative.c<br> > +++ b/xen/arch/arm/alternative.c<br> > @@ -97,12 +97,16 @@ static u32 get_alt_insn(const struct alt_instr *alt,<br> > /*<br> > * The region patched should be read-write to allow __apply_alternatives<br> > * to replacing the instructions when necessary.<br> > + *<br> > + * @update_offset: Offset between the region patched and the writable<br> > + * region for the update. 0 if the patched region is writable.<br> > */<br> > -static int __apply_alternatives(const struct alt_region *region)<br> > +static int __apply_alternatives(const struct alt_region *region,<br> > + paddr_t update_offset)<br> > {<br> > const struct alt_instr *alt;<br> > - const u32 *replptr;<br> > - u32 *origptr;<br> > + const u32 *replptr, *origptr;<br> > + u32 *updptr;<br> > <br> > printk(XENLOG_INFO "alternatives: Patching with alt table %p -> %p\n",<br> > region->begin, region->end);<br> > @@ -118,6 +122,7 @@ static int __apply_alternatives(const struct alt_region *region)<br> > BUG_ON(alt->alt_len != alt->orig_len);<br> > <br> > origptr = ALT_ORIG_PTR(alt);<br> > + updptr = (void *)origptr + update_offset;<br> > replptr = ALT_REPL_PTR(alt);<br> > <br> > nr_inst = alt->alt_len / sizeof(insn);<br> > @@ -125,7 +130,7 @@ static int __apply_alternatives(const struct alt_region *region)<br> > for ( i = 0; i < nr_inst; i++ )<br> > {<br> > insn = get_alt_insn(alt, origptr + i, replptr + i);<br> > - *(origptr + i) = cpu_to_le32(insn);<br> > + *(updptr + i) = cpu_to_le32(insn);<br> > }<br> > <br> > /* Ensure the new instructions reached the memory and nuke */<br> > @@ -162,9 +167,6 @@ static int __apply_alternatives_multi_stop(void *unused)<br> > paddr_t xen_size = _end - _start;<br> > unsigned int xen_order = get_order_from_bytes(xen_size);<br> > void *xenmap;<br> > - struct virtual_region patch_region = {<br> > - .list = LIST_HEAD_INIT(patch_region.list),<br> > - };<br> > <br> > BUG_ON(patched);<br> > <br> > @@ -178,30 +180,19 @@ static int __apply_alternatives_multi_stop(void *unused)<br> > BUG_ON(!xenmap);<br> > <br> > /*<br> > - * If we generate a new branch instruction, the target will be<br> > - * calculated in this re-mapped Xen region. So we have to register<br> > - * this re-mapped Xen region as a virtual region temporarily.<br> > - */<br> > - patch_region.start = xenmap;<br> > - patch_region.end = xenmap + xen_size;<br> > - register_virtual_region(&patch_region);<br> > -<br> > - /*<br> > * Find the virtual address of the alternative region in the new<br> > * mapping.<br> > * alt_instr contains relative offset, so the function<br> > * __apply_alternatives will patch in the re-mapped version of<br> > * Xen.<br> > */<br> > - region.begin = (void *)__alt_instructions - (void *)_start + xenmap;<br> > - region.end = (void *)__alt_instructions_end - (void *)_start + xenmap;<br> > + region.begin = __alt_instructions;<br> > + region.end = __alt_instructions_end;<br> > <br> > - ret = __apply_alternatives(&region);<br> > + ret = __apply_alternatives(&region, xenmap - (void *)_start);<br> > /* The patching is not expected to fail during boot. */<br> > BUG_ON(ret != 0);<br> > <br> > - unregister_virtual_region(&patch_region);<br> > -<br> > vunmap(xenmap);<br> > <br> > /* Barriers provided by the cache flushing */<br> > @@ -235,7 +226,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en<br> > .end = end,<br> > };<br> > <br> > - return __apply_alternatives(&region);<br> > + return __apply_alternatives(&region, 0);<br> > }<br> > <br> > /*<br> > -- <br> > 2.11.0<br> > <br> <br> _______________________________________________<br> Xen-devel mailing list<br> <a href="mailto:Xen-devel@lists.xenproject.org" target="_blank">Xen-devel@lists.xenproject.org</a><br> <a href="https://lists.xenproject.org/mailman/listinfo/xen-devel" rel="noreferrer" target="_blank">https://lists.xenproject.org/mailman/listinfo/xen-devel</a></blockquote></div>
On Fri, 25 May 2018, Julien Grall wrote: > On Fri, 25 May 2018, 22:54 Stefano Stabellini, <sstabellini@kernel.org> wrote: > You might want to CC Konrad next time on this patch > > > May I ask why? This code falls under Arm maintainership. I know, but I appreciate his input if he has time for it > > > On Tue, 22 May 2018, Julien Grall wrote: > > This is part of XSA-263. > > > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > > > --- > > I am aware of the missing commit message here. I wanted to send the > > series on the ML to get some feedback first. > > --- > > xen/arch/arm/alternative.c | 35 +++++++++++++---------------------- > > 1 file changed, 13 insertions(+), 22 deletions(-) > > > > diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c > > index 9ffdc475d6..bd62183def 100644 > > --- a/xen/arch/arm/alternative.c > > +++ b/xen/arch/arm/alternative.c > > @@ -97,12 +97,16 @@ static u32 get_alt_insn(const struct alt_instr *alt, > > /* > > * The region patched should be read-write to allow __apply_alternatives > > * to replacing the instructions when necessary. > > + * > > + * @update_offset: Offset between the region patched and the writable > > + * region for the update. 0 if the patched region is writable. > > */ > > -static int __apply_alternatives(const struct alt_region *region) > > +static int __apply_alternatives(const struct alt_region *region, > > + paddr_t update_offset) > > { > > const struct alt_instr *alt; > > - const u32 *replptr; > > - u32 *origptr; > > + const u32 *replptr, *origptr; > > + u32 *updptr; > > > > printk(XENLOG_INFO "alternatives: Patching with alt table %p -> %p\n", > > region->begin, region->end); > > @@ -118,6 +122,7 @@ static int __apply_alternatives(const struct alt_region *region) > > BUG_ON(alt->alt_len != alt->orig_len); > > > > origptr = ALT_ORIG_PTR(alt); > > + updptr = (void *)origptr + update_offset; > > replptr = ALT_REPL_PTR(alt); > > > > nr_inst = alt->alt_len / sizeof(insn); > > @@ -125,7 +130,7 @@ static int __apply_alternatives(const struct alt_region *region) > > for ( i = 0; i < nr_inst; i++ ) > > { > > insn = get_alt_insn(alt, origptr + i, replptr + i); > > - *(origptr + i) = cpu_to_le32(insn); > > + *(updptr + i) = cpu_to_le32(insn); > > } > > > > /* Ensure the new instructions reached the memory and nuke */ > > @@ -162,9 +167,6 @@ static int __apply_alternatives_multi_stop(void *unused) > > paddr_t xen_size = _end - _start; > > unsigned int xen_order = get_order_from_bytes(xen_size); > > void *xenmap; > > - struct virtual_region patch_region = { > > - .list = LIST_HEAD_INIT(patch_region.list), > > - }; > > > > BUG_ON(patched); > > > > @@ -178,30 +180,19 @@ static int __apply_alternatives_multi_stop(void *unused) > > BUG_ON(!xenmap); > > > > /* > > - * If we generate a new branch instruction, the target will be > > - * calculated in this re-mapped Xen region. So we have to register > > - * this re-mapped Xen region as a virtual region temporarily. > > - */ > > - patch_region.start = xenmap; > > - patch_region.end = xenmap + xen_size; > > - register_virtual_region(&patch_region); > > - > > - /* > > * Find the virtual address of the alternative region in the new > > * mapping. > > * alt_instr contains relative offset, so the function > > * __apply_alternatives will patch in the re-mapped version of > > * Xen. > > */ > > - region.begin = (void *)__alt_instructions - (void *)_start + xenmap; > > - region.end = (void *)__alt_instructions_end - (void *)_start + xenmap; > > + region.begin = __alt_instructions; > > + region.end = __alt_instructions_end; > > > > - ret = __apply_alternatives(®ion); > > + ret = __apply_alternatives(®ion, xenmap - (void *)_start); > > /* The patching is not expected to fail during boot. */ > > BUG_ON(ret != 0); > > > > - unregister_virtual_region(&patch_region); > > - > > vunmap(xenmap); > > > > /* Barriers provided by the cache flushing */ > > @@ -235,7 +226,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en > > .end = end, > > }; > > > > - return __apply_alternatives(®ion); > > + return __apply_alternatives(®ion, 0); > > } > > > > /* > > -- > > 2.11.0 > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xenproject.org > https://lists.xenproject.org/mailman/listinfo/xen-devel > > >
On 26/05/18 00:24, Stefano Stabellini wrote: > On Fri, 25 May 2018, Julien Grall wrote: >> On Fri, 25 May 2018, 22:54 Stefano Stabellini, <sstabellini@kernel.org> wrote: >> You might want to CC Konrad next time on this patch >> >> >> May I ask why? This code falls under Arm maintainership. > > I know, but I appreciate his input if he has time for it I will, but you could also have CCed him from your first e-mail to give him more time. Cheers,
diff --git a/xen/arch/arm/alternative.c b/xen/arch/arm/alternative.c index 9ffdc475d6..bd62183def 100644 --- a/xen/arch/arm/alternative.c +++ b/xen/arch/arm/alternative.c @@ -97,12 +97,16 @@ static u32 get_alt_insn(const struct alt_instr *alt, /* * The region patched should be read-write to allow __apply_alternatives * to replacing the instructions when necessary. + * + * @update_offset: Offset between the region patched and the writable + * region for the update. 0 if the patched region is writable. */ -static int __apply_alternatives(const struct alt_region *region) +static int __apply_alternatives(const struct alt_region *region, + paddr_t update_offset) { const struct alt_instr *alt; - const u32 *replptr; - u32 *origptr; + const u32 *replptr, *origptr; + u32 *updptr; printk(XENLOG_INFO "alternatives: Patching with alt table %p -> %p\n", region->begin, region->end); @@ -118,6 +122,7 @@ static int __apply_alternatives(const struct alt_region *region) BUG_ON(alt->alt_len != alt->orig_len); origptr = ALT_ORIG_PTR(alt); + updptr = (void *)origptr + update_offset; replptr = ALT_REPL_PTR(alt); nr_inst = alt->alt_len / sizeof(insn); @@ -125,7 +130,7 @@ static int __apply_alternatives(const struct alt_region *region) for ( i = 0; i < nr_inst; i++ ) { insn = get_alt_insn(alt, origptr + i, replptr + i); - *(origptr + i) = cpu_to_le32(insn); + *(updptr + i) = cpu_to_le32(insn); } /* Ensure the new instructions reached the memory and nuke */ @@ -162,9 +167,6 @@ static int __apply_alternatives_multi_stop(void *unused) paddr_t xen_size = _end - _start; unsigned int xen_order = get_order_from_bytes(xen_size); void *xenmap; - struct virtual_region patch_region = { - .list = LIST_HEAD_INIT(patch_region.list), - }; BUG_ON(patched); @@ -178,30 +180,19 @@ static int __apply_alternatives_multi_stop(void *unused) BUG_ON(!xenmap); /* - * If we generate a new branch instruction, the target will be - * calculated in this re-mapped Xen region. So we have to register - * this re-mapped Xen region as a virtual region temporarily. - */ - patch_region.start = xenmap; - patch_region.end = xenmap + xen_size; - register_virtual_region(&patch_region); - - /* * Find the virtual address of the alternative region in the new * mapping. * alt_instr contains relative offset, so the function * __apply_alternatives will patch in the re-mapped version of * Xen. */ - region.begin = (void *)__alt_instructions - (void *)_start + xenmap; - region.end = (void *)__alt_instructions_end - (void *)_start + xenmap; + region.begin = __alt_instructions; + region.end = __alt_instructions_end; - ret = __apply_alternatives(®ion); + ret = __apply_alternatives(®ion, xenmap - (void *)_start); /* The patching is not expected to fail during boot. */ BUG_ON(ret != 0); - unregister_virtual_region(&patch_region); - vunmap(xenmap); /* Barriers provided by the cache flushing */ @@ -235,7 +226,7 @@ int apply_alternatives(const struct alt_instr *start, const struct alt_instr *en .end = end, }; - return __apply_alternatives(®ion); + return __apply_alternatives(®ion, 0); } /*
This is part of XSA-263. Signed-off-by: Julien Grall <julien.grall@arm.com> --- I am aware of the missing commit message here. I wanted to send the series on the ML to get some feedback first. --- xen/arch/arm/alternative.c | 35 +++++++++++++---------------------- 1 file changed, 13 insertions(+), 22 deletions(-)