Message ID | 1375282195-21653-2-git-send-email-julien.grall@linaro.org |
---|---|
State | Superseded, archived |
Headers | show |
On Wed, 2013-07-31 at 15:49 +0100, Julien Grall wrote: > Some erratas on ARM processor requires to decode the instruction. > The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt. > > For the moment, the decoder only supports: > - THUMB2 store instruction > - THUMB single load/store instruction > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > --- > xen/arch/arm/Makefile | 1 + > xen/arch/arm/decode.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++++ > xen/arch/arm/decode.h | 38 +++++++++++++ > 3 files changed, 182 insertions(+) > create mode 100644 xen/arch/arm/decode.c > create mode 100644 xen/arch/arm/decode.h > > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile > index 5ae5831..5c13a65 100644 > --- a/xen/arch/arm/Makefile > +++ b/xen/arch/arm/Makefile > @@ -30,6 +30,7 @@ obj-y += vtimer.o > obj-y += vpl011.o > obj-y += hvm.o > obj-y += device.o > +obj-y += decode.o > > #obj-bin-y += ....o > > diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c > new file mode 100644 > index 0000000..e432d2a > --- /dev/null > +++ b/xen/arch/arm/decode.c > @@ -0,0 +1,143 @@ > +/* > + * xen/arch/arm/decode.c > + * > + * Instruction decoder > + * > + * Julien Grall <julien.grall@linaro.org> > + * Copyright (C) 2013 Linaro Limited. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <xen/types.h> > +#include <xen/sched.h> > +#include <asm/current.h> > +#include <asm/guest_access.h> > +#include <xen/lib.h> > + > +#include "decode.h" > + > +/* TODO: Handle all THUMB2 instruction other than simple store */ I think we can take it as a given that things will be added as and when they are required, no need to mention it IMHO. > +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) > +{ > + uint16_t hw2; > + int rc; > + uint16_t op1, op2, op; > + > + rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2)); > + if ( rc ) > + return rc; > + > + /* See A6.3 of DDI 0406C.b */ > + op1 = (hw1 >> 11) & 0x3; > + op2 = (hw1 >> 4) & 0x7f; > + op = hw2 >> 15; > + > + if ( op1 == 3 && ((op2 & 0x71) == 0x00) ) Please can we do the op1 decode with a switch. I know there is only one case now but it will avoid the temptation to add a chain of elses. ` op2 decoding is a pain. oh well. > + { > + /* Store single data item */ > + dabt->reg = (hw2 >> 12) & 0x7; > + /* TODO: Handle access size */ Is this difficult to do? Perhaps you need to just decode op2 a bit further? Having done that it may turn out to be easier to decode op1 and op2 together as a single switch statement. > + return 0; > + } > + > + printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n", > + current->domain->domain_id, hw1, hw2); > + > + return 1; > +} > + > +/* TODO: Handle all THUMB instructions other than store */ > +static int decode_thumb(register_t pc, struct hsr_dabt *dabt) > +{ > + uint16_t instr; > + int rc; > + > + rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)); > + if ( rc ) > + return rc; > + > + switch ( instr >> 12 ) > + { > + case 5: > + { > + /* Load/Store register */ > + uint16_t opB = (instr >> 9) & 0x7; > + > + switch ( opB & 0x3 ) > + { > + case 0: > + dabt->size = 2; > + break; > + case 1: > + dabt->size = 1; ->sign is uninitialised for these two cases? Actually, for many of them I think? > + break; > + case 3: > + dabt->sign = 1; > + /* Fall-through */ > + case 2: > + dabt->size = 0; > + break; > + } > + > + dabt->reg = instr & 7; > + > + break; > + } > + case 6: > + /* Load/Store word immediate offset */ > + dabt->size = 2; > + dabt->reg = instr & 7; > + break; > + case 7: > + /* Load/Store byte immediate offset */ > + dabt->size = 0; > + dabt->reg = instr & 7; > + break; > + case 8: > + /* Load/Store halfword immediate offset */ > + dabt->size = 1; > + dabt->reg = instr & 7; > + break; > + case 9: > + /* Load/Store word sp offset */ > + dabt->size = 2; > + dabt->reg = (instr >> 8) & 7; > + break; > + case 14: > + if ( instr & (1 << 11) ) > + return decode_thumb2(pc, dabt, instr); > + goto bad_thumb; > + case 15: > + return decode_thumb2(pc, dabt, instr); > + default: > + goto bad_thumb; > + } > + > + return 0; > + > +bad_thumb: > + printk("DOM%u: unhandled THUMB instruction 0x%x\n", > + current->domain->domain_id, instr); > + return 1; > +} > + > +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt) > +{ > + /* XXX: zeroed ISS when decode will be fully implemented */ > + > + if ( regs->cpsr & PSR_THUMB ) > + return decode_thumb(regs->pc, dabt); Needs an is_pv32_domain too. > + > + /* TODO: Handle ARM instruction */ > + > + return 1; > +} > diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h > new file mode 100644 > index 0000000..dc130a3 > --- /dev/null > +++ b/xen/arch/arm/decode.h > @@ -0,0 +1,38 @@ > +/* > + * xen/arch/arm/decode.h > + * > + * Instruction decoder > + * > + * Julien Grall <julien.grall@linaro.org> > + * Copyright (C) 2013 Linaro Limited. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#ifndef __ARCH_ARM_DECODE_H_ > +#define __ARCH_ARM_DECODE_H_ > + > +#include <asm/regs.h> > +#include <asm/processor.h> > + > +int decode_instruction(const struct cpu_user_regs *regs, > + struct hsr_dabt *dabt); > + > +#endif /* __ARCH_ARM_DECODE_H_ */ > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * indent-tabs-mode: nil > + * End: > + */
On 07/31/2013 04:34 PM, Ian Campbell wrote: > On Wed, 2013-07-31 at 15:49 +0100, Julien Grall wrote: >> Some erratas on ARM processor requires to decode the instruction. >> The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt. >> >> For the moment, the decoder only supports: >> - THUMB2 store instruction >> - THUMB single load/store instruction >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> >> --- >> xen/arch/arm/Makefile | 1 + >> xen/arch/arm/decode.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++++ >> xen/arch/arm/decode.h | 38 +++++++++++++ >> 3 files changed, 182 insertions(+) >> create mode 100644 xen/arch/arm/decode.c >> create mode 100644 xen/arch/arm/decode.h >> >> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile >> index 5ae5831..5c13a65 100644 >> --- a/xen/arch/arm/Makefile >> +++ b/xen/arch/arm/Makefile >> @@ -30,6 +30,7 @@ obj-y += vtimer.o >> obj-y += vpl011.o >> obj-y += hvm.o >> obj-y += device.o >> +obj-y += decode.o >> >> #obj-bin-y += ....o >> >> diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c >> new file mode 100644 >> index 0000000..e432d2a >> --- /dev/null >> +++ b/xen/arch/arm/decode.c >> @@ -0,0 +1,143 @@ >> +/* >> + * xen/arch/arm/decode.c >> + * >> + * Instruction decoder >> + * >> + * Julien Grall <julien.grall@linaro.org> >> + * Copyright (C) 2013 Linaro Limited. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <xen/types.h> >> +#include <xen/sched.h> >> +#include <asm/current.h> >> +#include <asm/guest_access.h> >> +#include <xen/lib.h> >> + >> +#include "decode.h" >> + >> +/* TODO: Handle all THUMB2 instruction other than simple store */ > > I think we can take it as a given that things will be added as and when > they are required, no need to mention it IMHO. I will remove it. >> +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) >> +{ >> + uint16_t hw2; >> + int rc; >> + uint16_t op1, op2, op; >> + >> + rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2)); >> + if ( rc ) >> + return rc; >> + >> + /* See A6.3 of DDI 0406C.b */ >> + op1 = (hw1 >> 11) & 0x3; >> + op2 = (hw1 >> 4) & 0x7f; >> + op = hw2 >> 15; >> + >> + if ( op1 == 3 && ((op2 & 0x71) == 0x00) ) > > Please can we do the op1 decode with a switch. I know there is only one > case now but it will avoid the temptation to add a chain of elses. > op2 decoding is a pain. oh well. >> + { >> + /* Store single data item */ >> + dabt->reg = (hw2 >> 12) & 0x7; >> + /* TODO: Handle access size */ > > Is this difficult to do? Perhaps you need to just decode op2 a bit > further? It was for simplicity. I will add the support on the next patch series. > Having done that it may turn out to be easier to decode op1 and op2 > together as a single switch statement. Sounds good. I will give a try. >> + return 0; >> + } >> + >> + printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n", >> + current->domain->domain_id, hw1, hw2); >> + >> + return 1; >> +} >> + >> +/* TODO: Handle all THUMB instructions other than store */ >> +static int decode_thumb(register_t pc, struct hsr_dabt *dabt) >> +{ >> + uint16_t instr; >> + int rc; >> + >> + rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)); >> + if ( rc ) >> + return rc; >> + >> + switch ( instr >> 12 ) >> + { >> + case 5: >> + { >> + /* Load/Store register */ >> + uint16_t opB = (instr >> 9) & 0x7; >> + >> + switch ( opB & 0x3 ) >> + { >> + case 0: >> + dabt->size = 2; >> + break; >> + case 1: >> + dabt->size = 1; > > ->sign is uninitialised for these two cases? > > Actually, for many of them I think? I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in decode_instruction. Do I still need to set sign to 0? >> + break; >> + case 3: >> + dabt->sign = 1; >> + /* Fall-through */ >> + case 2: >> + dabt->size = 0; >> + break; >> + } >> + >> + dabt->reg = instr & 7; >> + >> + break; >> + } >> + case 6: >> + /* Load/Store word immediate offset */ >> + dabt->size = 2; >> + dabt->reg = instr & 7; >> + break; >> + case 7: >> + /* Load/Store byte immediate offset */ >> + dabt->size = 0; >> + dabt->reg = instr & 7; >> + break; >> + case 8: >> + /* Load/Store halfword immediate offset */ >> + dabt->size = 1; >> + dabt->reg = instr & 7; >> + break; >> + case 9: >> + /* Load/Store word sp offset */ >> + dabt->size = 2; >> + dabt->reg = (instr >> 8) & 7; >> + break; >> + case 14: >> + if ( instr & (1 << 11) ) >> + return decode_thumb2(pc, dabt, instr); >> + goto bad_thumb; >> + case 15: >> + return decode_thumb2(pc, dabt, instr); >> + default: >> + goto bad_thumb; >> + } >> + >> + return 0; >> + >> +bad_thumb: >> + printk("DOM%u: unhandled THUMB instruction 0x%x\n", >> + current->domain->domain_id, instr); >> + return 1; >> +} >> + >> +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt) >> +{ >> + /* XXX: zeroed ISS when decode will be fully implemented */ >> + >> + if ( regs->cpsr & PSR_THUMB ) >> + return decode_thumb(regs->pc, dabt); > > Needs an is_pv32_domain too. I will add in the next patch series. >> + >> + /* TODO: Handle ARM instruction */ >> + >> + return 1; >> +} >> diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h >> new file mode 100644 >> index 0000000..dc130a3 >> --- /dev/null >> +++ b/xen/arch/arm/decode.h >> @@ -0,0 +1,38 @@ >> +/* >> + * xen/arch/arm/decode.h >> + * >> + * Instruction decoder >> + * >> + * Julien Grall <julien.grall@linaro.org> >> + * Copyright (C) 2013 Linaro Limited. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#ifndef __ARCH_ARM_DECODE_H_ >> +#define __ARCH_ARM_DECODE_H_ >> + >> +#include <asm/regs.h> >> +#include <asm/processor.h> >> + >> +int decode_instruction(const struct cpu_user_regs *regs, >> + struct hsr_dabt *dabt); >> + >> +#endif /* __ARCH_ARM_DECODE_H_ */ >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ > >
On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote: > >> + switch ( opB & 0x3 ) > >> + { > >> + case 0: > >> + dabt->size = 2; > >> + break; > >> + case 1: > >> + dabt->size = 1; > > > > ->sign is uninitialised for these two cases? > > > > Actually, for many of them I think? > > I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in > decode_instruction. Do I still need to set sign to 0? Well, you need to at least do one or the other! Even if you are zeroing the iss be default I think it would be useful from a documentation/clarity perspective to set it explicitly to zero. Ian.
On 07/31/2013 05:26 PM, Ian Campbell wrote: > On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote: > > >>>> + switch ( opB & 0x3 ) >>>> + { >>>> + case 0: >>>> + dabt->size = 2; >>>> + break; >>>> + case 1: >>>> + dabt->size = 1; >>> >>> ->sign is uninitialised for these two cases? >>> >>> Actually, for many of them I think? >> >> I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in >> decode_instruction. Do I still need to set sign to 0? > > Well, you need to at least do one or the other! > > Even if you are zeroing the iss be default I think it would be useful > from a documentation/clarity perspective to set it explicitly to zero. There is only few functions which deal with signed-register. So if we add a line "dabt->sign = 0" it's less clear. What about a helper? update_dabt(struct hsr_dabt *dabt, uint16_t reg, uint8_t size, bool_t sign)
On Wed, 2013-07-31 at 17:39 +0100, Julien Grall wrote: > On 07/31/2013 05:26 PM, Ian Campbell wrote: > > On Wed, 2013-07-31 at 17:18 +0100, Julien Grall wrote: > > > > > >>>> + switch ( opB & 0x3 ) > >>>> + { > >>>> + case 0: > >>>> + dabt->size = 2; > >>>> + break; > >>>> + case 1: > >>>> + dabt->size = 1; > >>> > >>> ->sign is uninitialised for these two cases? > >>> > >>> Actually, for many of them I think? > >> > >> I plan to zeroed the ISS field (ie sign, reg...) by default. See TODO in > >> decode_instruction. Do I still need to set sign to 0? > > > > Well, you need to at least do one or the other! > > > > Even if you are zeroing the iss be default I think it would be useful > > from a documentation/clarity perspective to set it explicitly to zero. > > There is only few functions which deal with signed-register. So if we > add a line "dabt->sign = 0" it's less clear. Even if the function doesn't deal with a signed register ever the consumer of the decoded hsr doesn't know that and needs to know not to sign extend, so it needs to be cleared explicitly somewhere. > What about a helper? > > update_dabt(struct hsr_dabt *dabt, uint16_t reg, uint8_t size, > bool_t sign)
diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile index 5ae5831..5c13a65 100644 --- a/xen/arch/arm/Makefile +++ b/xen/arch/arm/Makefile @@ -30,6 +30,7 @@ obj-y += vtimer.o obj-y += vpl011.o obj-y += hvm.o obj-y += device.o +obj-y += decode.o #obj-bin-y += ....o diff --git a/xen/arch/arm/decode.c b/xen/arch/arm/decode.c new file mode 100644 index 0000000..e432d2a --- /dev/null +++ b/xen/arch/arm/decode.c @@ -0,0 +1,143 @@ +/* + * xen/arch/arm/decode.c + * + * Instruction decoder + * + * Julien Grall <julien.grall@linaro.org> + * Copyright (C) 2013 Linaro Limited. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <xen/types.h> +#include <xen/sched.h> +#include <asm/current.h> +#include <asm/guest_access.h> +#include <xen/lib.h> + +#include "decode.h" + +/* TODO: Handle all THUMB2 instruction other than simple store */ +static int decode_thumb2(register_t pc, struct hsr_dabt *dabt, uint16_t hw1) +{ + uint16_t hw2; + int rc; + uint16_t op1, op2, op; + + rc = raw_copy_from_guest(&hw2, (void *__user)(pc + 2), sizeof (hw2)); + if ( rc ) + return rc; + + /* See A6.3 of DDI 0406C.b */ + op1 = (hw1 >> 11) & 0x3; + op2 = (hw1 >> 4) & 0x7f; + op = hw2 >> 15; + + if ( op1 == 3 && ((op2 & 0x71) == 0x00) ) + { + /* Store single data item */ + dabt->reg = (hw2 >> 12) & 0x7; + /* TODO: Handle access size */ + return 0; + } + + printk("DOM%u: unhandled THUMB2 instruction 0x%x%x\n", + current->domain->domain_id, hw1, hw2); + + return 1; +} + +/* TODO: Handle all THUMB instructions other than store */ +static int decode_thumb(register_t pc, struct hsr_dabt *dabt) +{ + uint16_t instr; + int rc; + + rc = raw_copy_from_guest(&instr, (void * __user)pc, sizeof (instr)); + if ( rc ) + return rc; + + switch ( instr >> 12 ) + { + case 5: + { + /* Load/Store register */ + uint16_t opB = (instr >> 9) & 0x7; + + switch ( opB & 0x3 ) + { + case 0: + dabt->size = 2; + break; + case 1: + dabt->size = 1; + break; + case 3: + dabt->sign = 1; + /* Fall-through */ + case 2: + dabt->size = 0; + break; + } + + dabt->reg = instr & 7; + + break; + } + case 6: + /* Load/Store word immediate offset */ + dabt->size = 2; + dabt->reg = instr & 7; + break; + case 7: + /* Load/Store byte immediate offset */ + dabt->size = 0; + dabt->reg = instr & 7; + break; + case 8: + /* Load/Store halfword immediate offset */ + dabt->size = 1; + dabt->reg = instr & 7; + break; + case 9: + /* Load/Store word sp offset */ + dabt->size = 2; + dabt->reg = (instr >> 8) & 7; + break; + case 14: + if ( instr & (1 << 11) ) + return decode_thumb2(pc, dabt, instr); + goto bad_thumb; + case 15: + return decode_thumb2(pc, dabt, instr); + default: + goto bad_thumb; + } + + return 0; + +bad_thumb: + printk("DOM%u: unhandled THUMB instruction 0x%x\n", + current->domain->domain_id, instr); + return 1; +} + +int decode_instruction(const struct cpu_user_regs *regs, struct hsr_dabt *dabt) +{ + /* XXX: zeroed ISS when decode will be fully implemented */ + + if ( regs->cpsr & PSR_THUMB ) + return decode_thumb(regs->pc, dabt); + + /* TODO: Handle ARM instruction */ + + return 1; +} diff --git a/xen/arch/arm/decode.h b/xen/arch/arm/decode.h new file mode 100644 index 0000000..dc130a3 --- /dev/null +++ b/xen/arch/arm/decode.h @@ -0,0 +1,38 @@ +/* + * xen/arch/arm/decode.h + * + * Instruction decoder + * + * Julien Grall <julien.grall@linaro.org> + * Copyright (C) 2013 Linaro Limited. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#ifndef __ARCH_ARM_DECODE_H_ +#define __ARCH_ARM_DECODE_H_ + +#include <asm/regs.h> +#include <asm/processor.h> + +int decode_instruction(const struct cpu_user_regs *regs, + struct hsr_dabt *dabt); + +#endif /* __ARCH_ARM_DECODE_H_ */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */
Some erratas on ARM processor requires to decode the instruction. The decoder will, obviously, decode and fill the ISS fields of the hsr_dabt. For the moment, the decoder only supports: - THUMB2 store instruction - THUMB single load/store instruction Signed-off-by: Julien Grall <julien.grall@linaro.org> --- xen/arch/arm/Makefile | 1 + xen/arch/arm/decode.c | 143 +++++++++++++++++++++++++++++++++++++++++++++++++ xen/arch/arm/decode.h | 38 +++++++++++++ 3 files changed, 182 insertions(+) create mode 100644 xen/arch/arm/decode.c create mode 100644 xen/arch/arm/decode.h