| Message ID | 20240131175043.1488886-1-alex.bennee@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFC] xen/arm: improve handling of load/store instruction decoding | expand |
Hi Alex, On 31/01/2024 17:50, Alex Bennée wrote: > While debugging VirtIO on Arm we ran into a warning due to memory > being memcpy'd across MMIO space. While the bug was in the mappings > the warning was a little confusing: > > (XEN) d47v2 Rn should not be equal to Rt except for r31 > (XEN) d47v2 unhandled Arm instruction 0x3d800000 > (XEN) d47v2 Unable to decode instruction > > The Rn == Rt warning is only applicable to single register load/stores > so add some verification steps before to weed out unexpected accesses. > > I updated the Arm ARM reference to the online instruction decoding > table which will hopefully be more stable than the Arm ARM section > numbers. I am not sure if the links to the Arm websites are stable. But from past, experience, URL tends to disappear after a while. This is why we went with the section + the Arm spec. This also has the advantage that we can check any differences between version. So my preference is to stick the Arm ARM reference. Bertrand, Michal, Stefano, any opinions? Anyway, the idea looks fine to me. I left mostly some style comments. > > Signed-off-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> > --- > xen/arch/arm/decode.c | 20 ++++++++++++++++++++ > xen/arch/arm/decode.h | 38 +++++++++++++++++++++++++++++++++++--- > 2 files changed, 55 insertions(+), 3 deletions(-) > > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > index 2537dbebc1..824025c24c 100644 > --- a/xen/arch/arm/decode.c > +++ b/xen/arch/arm/decode.c > @@ -87,6 +87,26 @@ static int decode_arm64(register_t pc, mmio_info_t *info) > return 1; > } > > + /* > + * Check this is a load/store of some sort > + */ Coding style: This is a single line comment, so the preferred format is: /* .... */ > + if ( (opcode.top_level.op1 & 0b0101) != 0b0100 ) NIT: We are trying to avoid opcoding value in Xen. Can you add some define? > + { > + gprintk(XENLOG_ERR, "Not a load/store instruction op1=%d", Does the value need to be signed? > + opcode.top_level.op1); > + goto bad_loadstore; > + } > + > + /* > + * We are only expecting single register load/stores > + */ Same here. > + if ( (opcode.ld_st.op0 & 0b0011) != 0b0011 ) > + { > + gprintk(XENLOG_ERR, "Not single register load/store op0=%d", Same remark as above. > + opcode.ld_st.op0); > + goto bad_loadstore; > + } > + > /* > * Refer Arm v8 ARM DDI 0487G.b, Page - C6-1107 > * "Shared decode for all encodings" (under ldr immediate) > diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h > index 13db8ac968..b1580178eb 100644 > --- a/xen/arch/arm/decode.h > +++ b/xen/arch/arm/decode.h > @@ -24,9 +24,27 @@ > #include <asm/processor.h> > > /* > - * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores > - * Page 318 specifies the following bit pattern for > - * "load/store register (immediate post-indexed)". > + * From: > + * https://developer.arm.com/documentation/ddi0602/2023-12/Index-by-Encoding > + * > + * Top level encoding: > + * > + * 31 30 29 28 25 24 0 > + * ___________________________________________________________________ > + * |op0 | x x | op1 | | > + * |____|______|______|_______________________________________________| > + * > + * op0 = 0 is reserved > + * op1 = x1x0 for Loads and Stores > + * > + * Loads and Stores > + * > + * 31 28 27 26 25 24 9 8 0 > + * ___________________________________________________________________ > + * | op0 | 1 | op1 | 0 | op2 | | > + * |________|___|_____|___|________________|__________________________| > + * > + * Load/store register (immediate post-indexed) > * > * 31 30 29 27 26 25 23 21 20 11 9 4 0 > * ___________________________________________________________________ > @@ -35,6 +53,20 @@ > */ > union instr { > uint32_t value; > + struct { > + unsigned int ign2:25; > + unsigned int op1:4; /* instruction class */ > + unsigned int ign1:2; > + unsigned int op0:1; /* value = 1b */ > + } top_level; > + struct { > + unsigned int ign1:9; > + unsigned int op2:15; > + unsigned int fixed1:1; /* value = 0b */ > + unsigned int op1:1; > + unsigned int fixed2:1; /* value = 1b */ > + unsigned int op0:4; > + } ld_st; > struct { > unsigned int rt:5; /* Rt register */ > unsigned int rn:5; /* Rn register */ Cheers,
Julien Grall <julien@xen.org> writes: > Hi Alex, > > On 31/01/2024 17:50, Alex Bennée wrote: >> While debugging VirtIO on Arm we ran into a warning due to memory >> being memcpy'd across MMIO space. While the bug was in the mappings >> the warning was a little confusing: >> (XEN) d47v2 Rn should not be equal to Rt except for r31 >> (XEN) d47v2 unhandled Arm instruction 0x3d800000 >> (XEN) d47v2 Unable to decode instruction >> The Rn == Rt warning is only applicable to single register >> load/stores >> so add some verification steps before to weed out unexpected accesses. >> I updated the Arm ARM reference to the online instruction decoding >> table which will hopefully be more stable than the Arm ARM section >> numbers. > > I am not sure if the links to the Arm websites are stable. But from > past, experience, URL tends to disappear after a while. This is why we > went with the section + the Arm spec. Depends if you can get the older spec. The section numbers unfortunately change between versions. We have the same problem in our TCG aarch64 backend unfortunately when we named the encoders after section numbers. > > This also has the advantage that we can check any differences between > version. So my preference is to stick the Arm ARM reference. Bertrand, > Michal, Stefano, any opinions? > > Anyway, the idea looks fine to me. I left mostly some style comments. > >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org> >> Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> >> --- >> xen/arch/arm/decode.c | 20 ++++++++++++++++++++ >> xen/arch/arm/decode.h | 38 +++++++++++++++++++++++++++++++++++--- >> 2 files changed, 55 insertions(+), 3 deletions(-) >> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c >> index 2537dbebc1..824025c24c 100644 >> --- a/xen/arch/arm/decode.c >> +++ b/xen/arch/arm/decode.c >> @@ -87,6 +87,26 @@ static int decode_arm64(register_t pc, mmio_info_t *info) >> return 1; >> } >> + /* >> + * Check this is a load/store of some sort >> + */ > > Coding style: This is a single line comment, so the preferred format is: > > /* .... */ > >> + if ( (opcode.top_level.op1 & 0b0101) != 0b0100 ) > > NIT: We are trying to avoid opcoding value in Xen. Can you add some define? > >> + { >> + gprintk(XENLOG_ERR, "Not a load/store instruction op1=%d", > > Does the value need to be signed? > >> + opcode.top_level.op1); >> + goto bad_loadstore; >> + } >> + >> + /* >> + * We are only expecting single register load/stores >> + */ > > Same here. > >> + if ( (opcode.ld_st.op0 & 0b0011) != 0b0011 ) >> + { >> + gprintk(XENLOG_ERR, "Not single register load/store op0=%d", > > Same remark as above. > >> + opcode.ld_st.op0); >> + goto bad_loadstore; >> + } >> + >> /* >> * Refer Arm v8 ARM DDI 0487G.b, Page - C6-1107 >> * "Shared decode for all encodings" (under ldr immediate) >> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h >> index 13db8ac968..b1580178eb 100644 >> --- a/xen/arch/arm/decode.h >> +++ b/xen/arch/arm/decode.h >> @@ -24,9 +24,27 @@ >> #include <asm/processor.h> >> /* >> - * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores >> - * Page 318 specifies the following bit pattern for >> - * "load/store register (immediate post-indexed)". >> + * From: >> + * https://developer.arm.com/documentation/ddi0602/2023-12/Index-by-Encoding >> + * >> + * Top level encoding: >> + * >> + * 31 30 29 28 25 24 0 >> + * ___________________________________________________________________ >> + * |op0 | x x | op1 | | >> + * |____|______|______|_______________________________________________| >> + * >> + * op0 = 0 is reserved >> + * op1 = x1x0 for Loads and Stores >> + * >> + * Loads and Stores >> + * >> + * 31 28 27 26 25 24 9 8 0 >> + * ___________________________________________________________________ >> + * | op0 | 1 | op1 | 0 | op2 | | >> + * |________|___|_____|___|________________|__________________________| >> + * >> + * Load/store register (immediate post-indexed) >> * >> * 31 30 29 27 26 25 23 21 20 11 9 4 0 >> * ___________________________________________________________________ >> @@ -35,6 +53,20 @@ >> */ >> union instr { >> uint32_t value; >> + struct { >> + unsigned int ign2:25; >> + unsigned int op1:4; /* instruction class */ >> + unsigned int ign1:2; >> + unsigned int op0:1; /* value = 1b */ >> + } top_level; >> + struct { >> + unsigned int ign1:9; >> + unsigned int op2:15; >> + unsigned int fixed1:1; /* value = 0b */ >> + unsigned int op1:1; >> + unsigned int fixed2:1; /* value = 1b */ >> + unsigned int op0:4; >> + } ld_st; >> struct { >> unsigned int rt:5; /* Rt register */ >> unsigned int rn:5; /* Rn register */ > > Cheers, Will fix up the comments and repost.
On 05/02/2024 11:42, Julien Grall wrote: > > > Hi Alex, > > On 31/01/2024 17:50, Alex Bennée wrote: >> While debugging VirtIO on Arm we ran into a warning due to memory >> being memcpy'd across MMIO space. While the bug was in the mappings >> the warning was a little confusing: >> >> (XEN) d47v2 Rn should not be equal to Rt except for r31 >> (XEN) d47v2 unhandled Arm instruction 0x3d800000 >> (XEN) d47v2 Unable to decode instruction >> >> The Rn == Rt warning is only applicable to single register load/stores >> so add some verification steps before to weed out unexpected accesses. >> >> I updated the Arm ARM reference to the online instruction decoding >> table which will hopefully be more stable than the Arm ARM section >> numbers. NIT: commit msg should be written in imperative mood. > > I am not sure if the links to the Arm websites are stable. But from > past, experience, URL tends to disappear after a while. This is why we > went with the section + the Arm spec. > > This also has the advantage that we can check any differences between > version. So my preference is to stick the Arm ARM reference. Bertrand, > Michal, Stefano, any opinions? I would prefer if we refer to Arm ARM instead of links to Arm websites. Also, looking at Arm ARM J.a C4.1.88 (Loads and Stores) the encoding is a bit different compared to that website and includes op3 and op4 (op2 is 24:23). ~Michal
diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c index 2537dbebc1..824025c24c 100644 --- a/xen/arch/arm/decode.c +++ b/xen/arch/arm/decode.c @@ -87,6 +87,26 @@ static int decode_arm64(register_t pc, mmio_info_t *info) return 1; } + /* + * Check this is a load/store of some sort + */ + if ( (opcode.top_level.op1 & 0b0101) != 0b0100 ) + { + gprintk(XENLOG_ERR, "Not a load/store instruction op1=%d", + opcode.top_level.op1); + goto bad_loadstore; + } + + /* + * We are only expecting single register load/stores + */ + if ( (opcode.ld_st.op0 & 0b0011) != 0b0011 ) + { + gprintk(XENLOG_ERR, "Not single register load/store op0=%d", + opcode.ld_st.op0); + goto bad_loadstore; + } + /* * Refer Arm v8 ARM DDI 0487G.b, Page - C6-1107 * "Shared decode for all encodings" (under ldr immediate) diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h index 13db8ac968..b1580178eb 100644 --- a/xen/arch/arm/decode.h +++ b/xen/arch/arm/decode.h @@ -24,9 +24,27 @@ #include <asm/processor.h> /* - * Refer to the ARMv8 ARM (DDI 0487G.b), Section C4.1.4 Loads and Stores - * Page 318 specifies the following bit pattern for - * "load/store register (immediate post-indexed)". + * From: + * https://developer.arm.com/documentation/ddi0602/2023-12/Index-by-Encoding + * + * Top level encoding: + * + * 31 30 29 28 25 24 0 + * ___________________________________________________________________ + * |op0 | x x | op1 | | + * |____|______|______|_______________________________________________| + * + * op0 = 0 is reserved + * op1 = x1x0 for Loads and Stores + * + * Loads and Stores + * + * 31 28 27 26 25 24 9 8 0 + * ___________________________________________________________________ + * | op0 | 1 | op1 | 0 | op2 | | + * |________|___|_____|___|________________|__________________________| + * + * Load/store register (immediate post-indexed) * * 31 30 29 27 26 25 23 21 20 11 9 4 0 * ___________________________________________________________________ @@ -35,6 +53,20 @@ */ union instr { uint32_t value; + struct { + unsigned int ign2:25; + unsigned int op1:4; /* instruction class */ + unsigned int ign1:2; + unsigned int op0:1; /* value = 1b */ + } top_level; + struct { + unsigned int ign1:9; + unsigned int op2:15; + unsigned int fixed1:1; /* value = 0b */ + unsigned int op1:1; + unsigned int fixed2:1; /* value = 1b */ + unsigned int op0:4; + } ld_st; struct { unsigned int rt:5; /* Rt register */ unsigned int rn:5; /* Rn register */
While debugging VirtIO on Arm we ran into a warning due to memory being memcpy'd across MMIO space. While the bug was in the mappings the warning was a little confusing: (XEN) d47v2 Rn should not be equal to Rt except for r31 (XEN) d47v2 unhandled Arm instruction 0x3d800000 (XEN) d47v2 Unable to decode instruction The Rn == Rt warning is only applicable to single register load/stores so add some verification steps before to weed out unexpected accesses. I updated the Arm ARM reference to the online instruction decoding table which will hopefully be more stable than the Arm ARM section numbers. Signed-off-by: Alex Bennée <alex.bennee@linaro.org> Cc: Manos Pitsidianakis <manos.pitsidianakis@linaro.org> --- xen/arch/arm/decode.c | 20 ++++++++++++++++++++ xen/arch/arm/decode.h | 38 +++++++++++++++++++++++++++++++++++--- 2 files changed, 55 insertions(+), 3 deletions(-)